Add CodeActions/Command structure

[rockbruno]

This PR adds the skeleton of the Code Actions request, used to provide actions to the user based on a selection range. Actions are used for refactoring and to provide quick fixes to the user when errors are diagnosed. Part one to make #70 easier to integrate.

Because CodeActions have several kinds, the idea was to create the concept of CodeActionProviders and tie them to their specific kinds, allowing the main codeActions function to determine which providers to trigger for a given request. The results are added and returned to the client.

Command

In the LSP, commands are just an identifier and an [Any]? argument list. Because they need to be sent to the client and then parsed back again, that makes them a bit difficult to recognize in the server. To remedy this, I wrapped the Command Structure around a CommandDataType protocol to make the contents predictable, but this required playing around with Codable. It works, but it's not very pretty so I'm ready to listen to better options.

PS: I've left WorkspaceEdit out for now - I think the PR that implements ApplyEdit will be better suited for it.

[danielmartin]

Thanks for working on this! I've added some small suggestions. I think that adding a test would be great, but we could also add it later when when have the first code action provider implemented. Let's wait for @benlangmuir's opinion, who has a better context than me as the owner of the project.

[danielmartin]

Should this be optional? A command can have no arguments.

[danielmartin]

Also, could we keep the name arguments for this field, just like in the LSP spec?

[rockbruno]

Thanks for the comments Daniel! I was concerned that directly naming it arguments would be misleading since it works differently from the specs, but I agree that it sounds better.

I made it non-optional because the command identifier comes from the protocol, but I don't remember why I did that. I think making it optional will be better indeed 👍

[rockbruno]

I've been applying the suggestions and I now remember why I did that - it's because SourceKit-LSP requires all requests to have a textDocument and ExecuteCommand isn't a TextDocumentRequest. I forced the argument to always contain it in order to circumvent it because I assumed that making the requests not need it would be a massive change, if possible at all since it uses that to determine which language server to trigger. I'm not sure what's the best approach here now. What do you think?

[benlangmuir]

Can you expand on this? We should not require a textDocument in general. If the Command is something that will be executed by sourcekitd/clangd then sure, we need to know how to dispatch it, but that doesn't need to be a document name. We can embed whatever information (e.g. toolchain service) we need into arguments and then look it up if we are asked to execute the command.

[rockbruno]

Ah, I was under the impression that we couldn't support argument-less commands in the code as we somehow have to explicitly add the "this is Swift" info in the command's arguments (and a document was the easier way since the requests are built on top of them), but after some thinking I reworked the structure to make Command's Codable inject/retrieve the textDocument in/from the first position of the array automatically. The command's JSON itself will never have empty arguments, but the type inside the code will now be able to.

This is going to do the trick while we don't have a command without a document related to it, but I see now that adding a type of request that doesn't need a document would be straight-forward if we end up getting to that point.

[danielmartin]

I think it's more accurate to describe this as "A CommandDataType represents the arguments required to execute a Command."

Also, If you agree with my suggestion to keep "arguments", I'll rename it as CommandArgsType.

[danielmartin]

I don't know whether requiring all these three properties may be very restrictive to the model or not. I imagine a hypothetic future "Organize imports" command action that would only require a reference to the textDocument to do its job. Also, I'd copy here the property docs you have in SemanticRefactorCommandDataType.

[danielmartin]

Nit: Should default arguments come last in an argument list? @benlangmuir

[benlangmuir]

All else being equal, yes, but here it's probably best to match the order of the fields. Maybe only should have default of nil?

[rockbruno]

I changed Command to be an enum instead of protocol/struct combos to make Codable/Hashable conformance easier. Ready for review again!

[rockbruno]

@benlangmuir Is these a plan for these old PRs?

[benlangmuir]

Sorry, I'd like to get back to these soon, but I've been busy and I want to do a detailed review. I definitely haven't forgotten them!

[danielmartin]

I'll rebase/rework #19 on top of this once the commands structure is approved.

[benlangmuir]

@rockbruno hey, sorry again for the long delay. The way this patch represents the Command structure and requests could probably work if we controlled all the pieces, but because we are also supporting clangd I think we need more flexibility. Specifically, we don't know what clangd will want to put in these Command structures, but we will need to be able to pass them through.

Here's what I suggest; let me know if you think this makes sense:

• We add an AnyJSON type to hold the arbitrary arguments data that can round-trip
• The LanguageServerProtocol library doesn't know anything about the specific content
• The swift implementation learns to convert whatever model objects it wants to put in the Command arguments to and from the AnyJSON type
• You don't need to implement clangd support, but we should at least make sure we can round trip some arbitrary data through this type

In a similar vein, the result type of the CodeAction request needs to be something like a CommandOrCodeAction - for backwards compatibility, we're supposed to check the client capabilities before using the newer CodeAction structure.

[rockbruno]

No worries! Sounds reasonable. The tough part is how we're getting AnyJSON to conform to Codable/Hashable as the arguments can be anything, but I think I have something in mind.

[rockbruno]

Apologies for the delay, I was short on time these last few weeks.

When I did this for the first time I didn't noticed it had support for different return types - so I took a proper look now and reworked the structure. I wrapped the capabilities and the response into custom types that take the client's capabilities to determine what to encode/decode.

I also got rid of the implemented Commands to make things simpler, so it looks just like the LSP counterpart now. For clang I type erased the arguments, but it's still unclear to me how it will interact with all of this so if that's not enough to support it let me know!

[rockbruno]

Thanks for the tips! I made the server capabilities into an enum as well as it represents the same idea.

[benlangmuir]

@swift-ci please test

[benlangmuir]

Thanks for your patience working through all these review comments! Looks great!

[rockbruno]

Ah thanks for your patience when looking at my mistakes! I rebased #70, but I think I'll close it and open a new PR as most of the points I mentioned there are no longer relevant.