CodeActions to return Command[]|WorkspaceEdit

[mickaelistria]

In the vast majority of the use-cases of CodeActions, the action will be an edition. That would work for quick fix, code hints, generate related tests... Forcing the Command[] pattern here kinds of break the goal of being client-independent: the command are client-specific and it's the goal of the protocol to avoid such client-specific features.
So I would suggest that rather than returning a Command[], the code actions should be allowed to also return a WorkspaceEdit. That would cover much more nicely a lot of use-cases in a client-independent way.

[dbaeumer]

@mickaelistria yes and no. The reason why we have commands is that at a specific position (problem) there can be more than one possible code action (e..g quick fix, ....). So you can provide command and postpone the actual computation of the edit until later.

However I think there is also value to return a WorkspaceEdit. But that would need a UI piece as well.

[mickaelistria]

Ok, so it would be more something like (Command|WorkspaceEdit)[] that would match? In the Java/LSP4J world List<Either<Command, WorkspaceEdit>>?

[dbaeumer]

Exactly, plus a way to name the WorkspaceEdit to present a label in the user interface.

[mickaelistria]

Like completion, the WorkspaceEdit from CodeActions should allow some placeholders for variable substitutions/linked edit when applied.
That would allow to easily implement refactorings like "extract method", "extract local variable"... purely with LSP objects, in a portable way that's supposed to work with any client and server.
I believe this would be made easier when #236 is fixed so we can easily add "a posteriori" parameter capturing in any WorkspaceEdit.

[mickaelistria]

Exactly, plus a way to name the WorkspaceEdit to present a label in the user interface.

This is #243

[mickaelistria]

FWIW, at the moment, I believe this is the biggest limitation for the protocol, which prevents some of the common refactorings to be easily provided by language servers without tool-specific effort. I'd be delighted to know this gets considered for v4.

[dbaeumer]

There is actually an easy workaround for this with the applyEdit request that can be send form the server to the client. The only downside is one more server roundtrip. So I don't think that this 'blocks' something. I am even tending towards closing this unless we figure out the applyEdit is not sufficient.

[rictic]

Agreed that this isn't necessary. I think this is because the workflow for using applyEdit to implement a code action is a bit tricky, and could be solved with a bit of documentation or some sugar in LSP libraries.

For others coming to this bug, here's how to apply a workspace edit in response to a code action. I can confirm that this is working with both vscode and atom-languageclient with no tool-specific code. The conversation between server and client looks as follows:

  <- publishDiagnostics
  -> getCodeActions  returns Command(myAppName/applyEdit)
  -> executeCommand(myAppName/applyEdit)
  <- workspace/applyEdit

And here are links within the commit that landed this into our server.

Declare executeCommand capabilities:
Polymer/polymer-editor-service@a2f77a5#diff-bd6d4fe0d7e96ac0e708e87e1617eab6R130

Return a Command with the method name (myApp/applyEdits) that you registered the capability to execute, and your WorkspaceEdit as a parameter: Polymer/polymer-editor-service@a2f77a5#diff-bd6d4fe0d7e96ac0e708e87e1617eab6R245

Execute that command by doing an ApplyWorkspaceEditRequest:
Polymer/polymer-editor-service@a2f77a5#diff-bd6d4fe0d7e96ac0e708e87e1617eab6R236

[mickaelistria]

The workflow with applyEdit seems overkill to me in most use-cases. I agree there are workarounds, but I think we'd all prefer the LSP to be as simple and powerful as possible. My proosal IMO matches those 2 goals better than chaining several operations.

…

-- Mickael Istria Eclipse IDE <https://www.eclipse.org/downloads/eclipse-packages/> developer, at Red Hat Developers <https://developers.redhat.com/> community Elected Committer Representative at the Eclipse Foundation <https://www.eclipse.org/org/> board of directors

[puremourning]

ApplyEdit as a server to client notification is horrible because it is so stateful and asynchronous. Just send the edit on the command reply and editors can just apply it synchronously. I agree with @mickaelistria , returning a workspace edit which is he 99% case makes these commands much easier to implement in LSP clients.

[dbaeumer]

I agree that applying edits should be simple but the API shouldn't foster wrong coding patterns. Computing these edits (for example for refactorings) is very expensive and if a code action request returns 3 possible refactorings then it would be wrong to compute the edits upfront. They should be computed as late as possible not as early as possible.

Adding another roundtrip to this doesn't make it more async. So instead of returning the edit the server could instruct the command if the client should auto register for the command execution so that the flow gets easier to be implemented.

This kind of flow is even used in IDEs where the refactoring are not provided using a server. Usually the ide populates a menu (commands received via code actions). If a menu item is selected to calls into the language API to compute a workspace operation (execute command) and then applies theses workspace operations.

[puremourning]

As I understand it there is no limit to the number of unsolicited things the server can do in response to the execute command request. Is that correct? If it were a request/reply to execute command and it returned a workspace edit, then ok sure, but as it is an announced set of a sync notifications the editor has to know hat it is in the state of waiting for that to happen

[puremourning]

Announced should say ‘unbounded’

[mickaelistria]

@dbaeumer As per discussion about naming workspaceEdits at #243, would it make sense to rename this ticket as "CodeActions to return Command[]|ApplyWorkspaceEditParams" instead?

[mickaelistria]

Computing these edits is very expensive

I think this should rather say "can be very expensive" as many actions can return very small edits that can easily be computed quickly at the same time as the parser/analyzer detects an issue. For example, an extra closing parenthesis, when detected by the parser, can immediately return the right text edit to remove it. It's not always an expensive operation.
It can also happen, in case for example of a remote LS, that the cost of a ping is too long and that it would perform much better to includes the edits directly with the codeactions instead of exchanging more messages.
I think the LSP should acknowledge that there are simple cases for code actions, and then allows for code actions to also return edits directly instead of forcing back and forth communication when there is no need for it.

As per discussion about naming workspaceEdits at #243, would it make sense to rename this ticket as "CodeActions to return Command[]|ApplyWorkspaceEditParams" instead?

Note that this change is backward compatible. I could make a PR for that. If so, should it include a client capability such as codeactions.allowsDirectEdits ?

[dbaeumer]

@mickaelistria I agree that these can be cheap to compute.

A PR is very welcome here. In VS Code we added a type CodeAction as a possible result of a codeAction provider. May be we can take this as a blue print for the LSP changes.

And yes we would need to add a client capability since clients might not support this. Something like TextDocumentClientCapabilities.codeAction.codeActionSupport?: boolean

[mickaelistria]

The initial request seems well covered by #389 . Closing.