Support for custom invocation of code actions' Commands

[deankevorkian]

Currently, the default client's flow of using code action works as follows -

1. Client requests server for available code actions according to file & location
2. Client receives the actions and shows them (and every action is associated with a Command object)
3. Client chooses one of the code actions in the UI
4. Client calls workspace/executeCommand for the Command associated with the code action.

IMO the LSP specs state pretty clearly here that the response for code actions should return an array of Commands that the Server is capable of running - the client should contact the server again with the associated Command to executeCommand.

However, not all servers work that way - for example eclipses' java server returns Commands it expects the calling client to understand, analyze and act to. (So obviously, non-standard/custom commands).

Since in our default implementation, in step 4 mentioned above, If I understand correctly, executeCommand is called automatically for Commands the server can't handle,' there's a need to override that behavior (which includes copying redundant code to packages, like ide-java, to implement a custom behavior).

Can the atom language client provide some hooks to registering custom commands (some Map object that maps a Command to a callback function can do) for ide-* packages? the way I see it, the client can expose functions to register a custom Command callback (without exposing the Map itself) and everytime a CodeAction is executed, it will first check if the Map contains custom implementation, and if it doesn't - it'll call workspace/executeCommand on the server.

thoughts? is this something you think we should support? (if so, i'd like to start working on a PR for this matter)

[deankevorkian]

@damieng could you please take a look at this only to tell if that is something you'd want? if so i'll submit a pr regarding this.

[Arcanemagus]

This sounds like a bug in Eclipses Java server. As you already state the spec clearly says that the server should respond to textDocument/codeAction with a list of Command literals that the server understands, not that the client should implement.

If Eclipse's Java server is responding with Commands that it doesn't understand how to handle, that's a pretty big bug on the part of the server, not something that should be worked around in the clients, after all that's what a standard is for 😉.

[henryju]

I think the confusion come from VSCode :) On VSCode, if by chance the command id returned by textDocument/codeAction matches a command already declared on VSCode side, it will directly invoke it, skipping the workspace/executeCommand. Even if this is convenient in some situations, I agree this is not what the standard says, and so I think you should keep the current behavior.
FYI, more discussions on this topic:
microsoft/language-server-protocol#394
microsoft/language-server-protocol#144

[deankevorkian]

That is correct, but only correct to this point of time. Back on v2 of the LSP specs it wasn't as clear and explicit as it is now on the recent specs.
If I understand correctly from the V2 specs there's no clear way of handling the Commands returned by the codeAction request, which is why this situation was created in the first place. So some language servers decided (back at the time) to handle this issue in different ways - some decided to go with the current standard, which is returning Commands the server acknowledges and some decided to return an action the client should figure out what to do with..

@henryju I don't think the confusion comes from VSCode (although when people complain about the JDT LS implementation, the JDT devs usually refer those people to the VSCode extension). It comes from the specs' lack of stating how to handle the returned Commands.

Question is, do we want to be "backward compatible" with the older version of the specs, or just expect the language servers to break their older implementation to fit the new, "standard" one.

[kdvolder]

Maybe I'm missing something, but it feels 'logical' to me how vscode handles this. I.e. commands somehow 'exist' in the client side. There isn't a list of 'standardized' commands that any client should provide, and this is, for sure, a huge gap in the spec. But I never found that it was unclear that's how it was intended to work is that, somehow, the commands where things a client should know how to execute. The only 'gap' I saw in the spec was that there was no 'standardized' list of commands.

Then a mechanic was provided via registerCapability for server to register itself as a handler for specific commands, thereby providing a mechanism for servers to become command implementor's too. But there's nothing saying that this means now all of a sudden servers are the only ones that can implement commands. It seemed clear to me from the get go that 'commands' are really client-side things that a client knows somehow how to handle. Providing an extra mechanism to make servers request a client delegates the execution of a specific subset of commands to the server... doesn't really imply to me that this means servers are the only ones that can implement commands.

Anyhoo... the issue is probably, the spec is a bit vague and I'm reading more between the lines than other folks are. And it just so happens my intuition 'accidentally' corresponds more or less with what vscode does (and i.m.o probably what the spec intended, even if the wording isn't explicit enough in spelling out all those details).

[damieng]

The spec is created by the VSCode people so you might see earlier iterations happen there as the spec tends to be driven by what they've done. Given that I think if they had intended on having this behavior become part of the spec they would have called it out. Instead if feels like they tried the client-knows-commands approach first and gradually came around to a different way of thinking which is what they documented. Without more clarification from the LSP spec we won't know either way.

[kdvolder]

@Arcanemagus

This sounds like a bug in Eclipses Java server. As you already state the spec clearly says that the server should respond to textDocument/codeAction with a list of Command literals that the server understands, not that the client should implement.

Can you point me to where it says that ? I.e. where does it say that this should be limited to command literals that the server understands. I searched but couldn't find it.

As far as I could find after searching a bit, there is no wording anywhere in the current incarnation of the spec that re-enforces the interpretation that commands are only things that are defined on a/the server.

In fact in the 'Command' definition it still has wordings that imply they are mostly client-side things with some hints that maybe in the future there might be a well-defined list of 'standardized' commands.

I.e. it says this:

Commands are identified by a string identifier. The protocol currently doesn’t specify a set of well-known commands. So executing a command requires some tool extension code.

[hansonw]

FWIW the current published spec also says:

The code action request is sent from the client to the server to compute commands for a given text document and range. These commands are typically code fixes to either fix problems or to beautify/refactor code. The result of a textDocument/codeAction request is an array of Command literals which are typically presented in the user interface. When the command is selected the server should be contacted again (via the workspace/executeCommand) request to execute the command.

Given that VSCode's implementation allows the extension to register client-side code for their server commands (and that many existing servers operate under that assumption), IMO it seems pretty reasonable to provide an easy way of overriding the behavior of executing particular commands in atom-languageclient as well.

[kdvolder]

@hansonw Thanks for the pointer. Indeed that does sound pretty strongly like these commands should be recognized by the same server. So it seems to contradict the snippet I quoted.

I think we need some more clarity, right now we are all reading more into the spec than it really says. So I've just raised a ticket microsoft/language-server-protocol#432.

We can't really have different folks pulling in different directions like this. As server implementor I'd want my server to work on atom and vscode both. Not choose between them.

[deankevorkian]

I agree. This is also what I stated in the beginning - current specs tell you should recontact the server with the chosen Command. But since there are already language servers who behave like this, and vscodes implementation let's them do so, question (at least for Atom now) is whether or not we want to treat this as an "acceptable behavior" (not standard but acceptable, unless that changes in the future and becomes part of the specs). If so, Atom and other editors should provide the core for package authors to "register" new commands, or override existing commands, for every package corresponding to a language server. So ide-java registers commands for eclipse's JDT LS, ide-rust for rust's and so on. That is the question I'm raising. Even if in the future this will not become a standard flow, there will already be language servers continuing to behave this way, and migrating might not be easy. It's about finding the balance of having all language servers possible to run on Atom, and not allowing too much unsure or undocumented behavior.

[kdvolder]

whether or not we want to treat this as an "acceptable behavior"

The pragmatist in me says that you should. The way I see it you loose nothing by allowing this behavior. The alternative is that you explicitly check whether the server has declared itself as a handler of the command and throw an error if it hasn't.

So this seems to be strictly a choice between allowing or explicitly blocking a useful behavior by deliberately checking for it and forcing people to stop doing it by breaking their servers.

There is an advantage to allowing it (i.e. stuff that people have implemented in their servers/extensions actually works). I don't see any advantage to blocking this behavior.

[laughedelic]

There is an advantage to allowing it (i.e. stuff that people have implemented in their servers/extensions actually works). I don't see any advantage to blocking this behavior.

There is an advantage in following a standardized protocol. If some servers rely on an undocumented or deprecated API or implement a feature outside of the declared protocol, it doesn't mean every client should follow the lead of VSCode and implement that just to support already existing functionality. What will happen in the future if LSP explicitly prohibits it or the server changes implementation? Things get easily broken in such cases. IMO

1. if it can be implemented in the atom-ide-* package or changed in the corresponding language server 👍 problem is solved (should be solved on that end)
2. atom-languageclient should follow LSP spec (yet to be clarified). Supporting any extra functionality for specific language servers is a burden on the maintainers.

[deankevorkian]

@laughedelic there is no way at the moment for atom-ide-* package authors to support such behavior because the atom language client immediately calls exexuteCommand with the Command object received from the code action. The only options are the 2nd part of option one (changing the corresponding servers implementation) and option two - not supporting this at all. And language server authors aren't on a hurry to change this behavior... See the Eclipses Java LS as an example (which made me bring up this issue).

[damieng]

So what's the short term solution here? It would seem to me that it would be to provide a hook where ide-x packages can register custom commands and we compare the command locally to this list first and execute one there - if it fails then call back to the server.

[Arcanemagus]

@kdvolder I'll be honest: I didn't actually read into what a Command was defined as, since as @hansonw already pointed out the definition of textDocument/codeAction says they are explicitly server side things that the client is requesting the server to do.

The fact that the definition of Command says "So executing a command requires some tool extension code." is... disconcerting and I can definitely see where the confusion is between the two!

[kdvolder]

@Arcanemagus well, so you didn't read the text in 'Command' and I somehow missed the text in textDocument/codeAction. The spec is big enough now its easy to miss pieces if your searching for something in particular. So hopefully we can both be excused for that :-)

[kdvolder]

@damieng I think providing a 'hook' client-side where commands can be registered by one extension / or language server so it can be called by another sounds good to me.

However... I would suggest a subtle change around one small detail:

we compare the command locally to this list first and execute one there - if it fails then call back to the server.

It seems to me that since there is already an LSP mechanism via a registerCapability which lets a server declare explicitly which commands it is capable to handle... It seems more logical to me that you should use that to decide whether or not you send a command to the server (versus trying to handle it some other way). I.e. why send a request to handle some command X to a server that didn't declare it is actually capable to handle that command X?

So to summarize. I think it should work like this:

• have common place where commands are registered on the client-side (this is a 'shared' registry between different language-servers / atom packages)
• if a server says it handles a command via registerCapability add a handler that forwards command to that server.

This means that a server who didn't say it can handle X will not be called on to handle X under any circumstances. I know this seems to fly directly in the face of the snippet @hansonw quoted. But I'd argue that this snippet is / should probable be changed since what it says doesn't really fit with what I think the writers of the spec intended. (I.e. I think its sloppy wording and the word "should" doesn't belong there). We will likely have to wait for clarification to be sure, but consider this...

If indeed it is the intention that all commands should be handled by the server, then why is there a mechanism via registerCapability for a server to declare precisely which commands it is capable of handling? It doesn't make much sense to me to have added such a complex machinery to the spec to allow a server to claim 'ownership' of a specific set of command ids, if there is already an assumption that the server is supposed to handle all commands anyway.

[kdvolder]

@laughedelic

1. if it can be implemented in the atom-ide-* package or changed in the corresponding language server +1 problem is solved (should be solved on that end)

I think we all agree on that. But... the point is as you say this is "if it can be implemented in the atom-ide-* package or changed in the corresponding language server".

But, what do you do if it is not possible? I think that's what we are talking about here.

[laughedelic]

But, what do you do if it is not possible? I think that's what we are talking about here.

@kdvolder @deankevorkian explained why it's not possible to do in atom-ide-*, but I don't see why it isn't possible to fix on the language server side.

[damieng]

I think we can put something into atom-languageclient to at least let ide-x packages get the first chance to respond to commands.

[kdvolder]

but I don't see why it isn't possible to fix on the language server side.

@laughedelic

I don't see how it is possible to fix there. Maybe I'm missing something. But if the lsp client forwards all commands to the server it got them from, then I don't see how this can ever result in a command execution of an action defined by another atom package.

So, some way or another something has to give and assume that there will be some commands that are executed in some other way. @damieng suggestion will probably work for atom-ide-xxx packages by making them somehow a special case. (Though I already explained that I'd prefer it to work slightly differently from what he's suggesting, and why).

I admit I have a stake in this discussion as I'm trying to implement a mechanism so that spring-boot language server can get classpath updates from jdt.ls (see eclipse-jdtls/eclipse.jdt.ls#595). And the mechanic I have in mind will only work if commands registered by one server can be called from another server. This implies that commands registered by one server / extension must be accessible / visible to other extensions / servers. @damieng suggestion won't quite work for us (since it just sounds like its making a special case out of the ide-xx packages, which doesn't cover our language server... since, well, that's not us, I guess).

Though my classpath updates prototype already works in vscode... and since there seems to be no suitable mechanic in atom for us to make it work there too... that essentially means... the choices being made here right now might force us to disable a bunch of functionality on atom for lack of a mechanism that we can actually use to make it work there (But it will work on vscode).

[UziTech]

Development of atom-languageclient has officially moved to https://github.com/atom-ide-community/atom-languageclient 🎉

If this is still an issue please consider opening an issue on that repo.