codeAction and executeCommand

[willt]

I'm trying to figure out how textDocument/codeAction and workspace/executeCommand
are supposed to work together.
I think what should happen when code is selected and a command is ran like extractMethod is as follows:

1. Client sends a codeAction request to the server when text is selected. It contains the start/end range of the code selected. (I believe the server needs to store this information somewhere and update it whenever changed.)
2. The users uses a command like extractMethod. The client would send a executeCommand to the server that contains the command name and some arguments. (Still haven't figured out if you can add your own arguments to this request)
3. The server would run the command and send a workspace/applyEdit to the client to finalize the changes.

Is this correct so far? Does anything further need to happen? Document Syncing or anything ?
Thanks

[rictic]

This is a tricky part of the API right now. I documented my own adventures understanding it here: #144 (comment)

It sounds like you're on the right track. You can use WorkspaceEdit.documentChanges (in supporting clients) to send over the intended starting versions of the documents in your change.

[willt]

@rictic Cool ill check that out. Its a little weird I think that codeAction sends over all available commands each time. I would have thought these would be pre-registered somewhere.
I tried looking at other language servers for examples like the python one for example. I don't think it behaves this way at all.
Another confusing thing is, Having codeActions causes the lightbulb icon to appear on text selection. I haven't dug enough into that yet to see if there is a way to filter it or not.

[rcjsuen]

@willt What do you mean by "pre-registered"? The server should only send the commands that are relevant to the selection back to the client. The server does however need to state all the commands that it supports in its ExecuteCommandOptions when responding to an initialize request.

What you described is exactly how it's done in my Dockerfile language server. Feel free to take a look at the server's code and the actual implementation of the analysis of the diagnostics and the commands. I don't currently support versioned edits via WorkspaceEdit's documentChanges though.

The light bulb issue has been brought up before. Please see microsoft/vscode#33241, microsoft/vscode#33459, and microsoft/vscode#33555. The final comment is probably what's most relevant as it is the latest update on the issue. Please also see #389 regarding the necessary work to incorporate it into the language server protocol's specification.

[willt]

@rcjsuen Thanks for a link to your code. I will check it out. Okay so are you saying when the server receives a codeAction request it would evaluate what text is selected and then decide what commands to send back to the client?

If that's correct, do you know if there is a way of dynamically enabling commands in the context menu of vscode ? I know when you setup the commands in package.json you can do "when": "editorHasSelection && editorLangId == python" but I don't think you can dynamically change that can you ?

[puremourning]

What I don't understand is how the client is supposed to know when the "command" has finished executing. Like what if the user changes the document between "applyEdit" commands from the server? It just seems too racy.

[rcjsuen]

@willt Yes, the CodeActionParams includes a Range as well as a context with a Diagnostic[] array so you should only send back what is relevant instead of all your commands.

And no, I don't know the answer to your second question. I haven't dabbled in the art of writing VS Code extensions myself.

@puremourning Right now the response to a workspace/executeCommand is either any | null so you are correct that it is ambiguous as to when it has truly finished. Regarding changes to the document "in between" things happening you should version your edits via the method @rictic described in his earlier comment.

[puremourning]

Versioned edits don’t help from the user perspective. What should we do? Just refuse to apply the codeaction of it changed? What if it is partially applied? Would we even know that given the other problem of the separate channel for the actions vs the request.

Ideally the API would be that the apply command request returns an array of actions to perform. That solves all the problems.

[rictic]

What should we do? Just refuse to apply the codeaction of it changed? What if it is partially applied?

Yes. The client should never partially apply an edit. It should be atomic.

Would we even know that given the other problem of the separate channel for the actions vs the request

workspace/executeCommand is a request, not a notification, so the server can report errors in running the command.

It would be a nice feature if getCodeActions could return workspace/applyEdit rather than sending down a command that is then passed right back to the server. I believe one argument for the current workflow is that the command the server sends down could be much terser than a workspace/applyEdit, which can get pretty big.

[rcjsuen]

Also see #178 which suggests augmenting the textDocument/codeAction request to return a Command[] or a WorkspaceEdit though I realize some of the people here are already aware of that issue.

[puremourning]

What should we do? Just refuse to apply the codeaction of it changed? What if it is partially applied?
Yes. The client should never partially apply an edit. It should be atomic.

if we don't know when the full set of edits has been delivered for a command, how is this possible?

let's say the server issues two edit requests, with an arbitrary delay between them.

when the client receives edit 2, let's say something has changed, but edit 1 has already been applied. Thus you have a torn command with no way for the client to protect it.

I believe one argument for the current workflow is that the command the server sends down could be much terser than a workspace/applyEdit, which can get pretty big.

If executeCommand returned an array of WorkspaceEdit, or something, rather than issuing a completely unrelated set of messages with no restriction on when or how they are delivered, this would all be trivial and there would be no issue. As it stands, unless i'm (still!) missing something important, there's just a huge race condition with the current protocol.

[rictic]

if we don't know when the full set of edits has been delivered for a command, how is this possible?

let's say the server issues two edit requests, with an arbitrary delay between them.

when the client receives edit 2, let's say something has changed, but edit 1 has already been applied. Thus you have a torn command with no way for the client to protect it.

If you have a set of edits that must all be applied atomically then they must be sent down as a single workspace/applyEdit command, not two separate ones.

If executeCommand returned an array of WorkspaceEdit, or something, rather than issuing a completely unrelated set of messages with no restriction on when or how they are delivered, this would all be trivial and there would be no issue. As it stands, unless i'm (still!) missing something important, there's just a huge race condition with the current protocol.

A single WorkspaceEdit should be enough. Maybe that's the missing piece? A WorkspaceEdit contains any number of changes to make to any number of files, so there is no need to return an array of WorkspaceEdits.

(Ok, that's not entirely true. I've lobbied in the past for the option for workspace/applyEdit to take an array of workspace edits, where each edit is considered atomic but the client is free to apply only a subset of the edits in case of conflict, but I believe that's unrelated to the discussion here)

[puremourning]

Sure. If the server is required to produce a single WorkspaceEdit and nothing else, then why can’t that be the response to the executeCommand request? Otherwise Cthulhu.

[dbaeumer]

See also #389 and #432