HACK: Support eclipe.jdt.ls's break with the LSP spec

[UziTech]

moved from atom/atom-languageclient#268

see issue atom/atom-languageclient#183

[deankevorkian]

This is obviously good specifically for Java's eclipse LSP but we should find a more generic way like proposed in this issue that allows package authors to "register" handlers for custom (or even generic) commands, and allow them to run a callback when such command is chosen by the user.
Are we sure we want to merge code like this?

[UziTech]

we should find a more generic way like proposed in this issue that allows package authors to "register" handlers for custom (or even generic) commands, and allow them to run a callback when such command is chosen by the user.

Is there somewhere in the spec that allows for registering commands? or should we start there and update the spec first?

I don't want to deviate from the spec too much.

Are we sure we want to merge code like this?

No, which is why it is still a draft.

[deankevorkian]

Is there somewhere in the spec that allows for registering commands? or should we start there and update the spec first?

I'm going to ignore the discussion that went on the bug I referenced, and the bugs referenced to it on the language-server-protocol repository, because things have changed and the specs look clearer now, and state this -

Command
Represents a reference to a command. Provides a title which will be used to represent a command in the UI. Commands are identified by a string identifier. The recommended way to handle commands is to implement their execution on the server side if the client and server provides the corresponding capabilities. Alternatively the tool extension code could handle the command. The protocol currently doesn’t specify a set of well-known commands.

I'll take some time to research this and come back with conclusions and examples.

[deankevorkian]

After having done research (and in particular going over the progression made with what started all of this - the Eclipse language server) it seems that in many cases where the non-standard message made sense in the standard it was suggested to it and was often added, and in other cases we should allow it. For example - in the vscode java extension, the way they get project settings is by invoking executeCommand to their language server with their custom command (java.project.getSettings) and then use the return value (which is also a custom non-standard object).

I think it's fine for language servers to offer extra capabilities that enhance the experience of the developer.
The real issue is when they break the behavior of the protocol, on standard protocol actions.

What happened in the ide-java case referenced here is that Eclipse returned a Code Action Command that it expected the client to implement - while the specs specify that the Commands returned to the client from requesting Code Actions should be handled by the server itself.
So, for example, if I stand on an unreferenced variable in a Java file and one of the code actions returned suggests I remove that variable - when I click the suggestion it should be handled by the server (i.e we send the server a WorkSpaceEdit command if I'm not mistaken). But in the ide-java case the client needed to handle that special message.

I think the most ideal thing we can do - and that should in turn help us handle many cases of having to override behaviors - is let the extension developer set a sort of "middleware" or register his own callback to any executeCommand that happens.
That API should feel like this -
A developer can register a callback that happens when an executeCommand is fired for DoBlaBlaJavaThingyTypeBeat. If a command was not registered with a callback by the developer - let that Command reach the server. Otherwise - fire the callback that was registered by the developer.

Some possible use cases -

1. Intervene when a CodeAction is clicked, and a Command needs to be executed.
2. Register all custom commands callbacks that might be needed for things like CodeAction - where the extension doesn't directly communicate the LSP via a custom button or action of it's own, but rather as part of an automatic flow (or a flow determined by the basic atom-languageserver package).

Makes sense? I feel this will bring full control for extension developers on how Commands are executed...

[UziTech]

I like it. Can you give me an example of what that would look like for a client?

[deankevorkian]

My proposal looks like this.
In atom-languageclient we have adapters before certain capabilities or functions of the language server are executed.
It's a convention of "wrappers" we have that abstracts some of the implementation details (As much as possible, since it's after all a spec that needs to be followed all the way down), like CodeActionAdapter and RenameAdapter.

I propose adding a CommandExecutionAdapter class that "wraps" the call of executeCommand directly in the LanguageClientConnection (that, in hand, invokes a workspace/executeCommand request to the language server).
In that class we'll have a logic that lets package authors write a custom callback for an executed command, that should act globally like we said (meaning - not only in CodeActions, but in every place that uses executeCommand).
After we allow authors hook a custom callback for commands, we also expose an actual executeCommand in that very same Adapter that should take care for executing commands (as a convention, and we have to ensure this, no one should directly call executeCommand - but rather go through that adapter. Even other adapters). It will test if a given Command has a custom callback register for, and invoke it if it does, otherwise - it will simply call the regular executeCommand in LanguageClientConnection.

A sample / gist of all of this -

import { Command, ExecuteCommandParams } from "../languageclient";
import { LanguageClientConnection } from "../main";

export type CommandCustomCallbackFunction = (command: ExecuteCommandParams) => Promise<void>;

export default class CommandExecutionAdapter {
    private static commandsCustomCallbacks: Map<string, CommandCustomCallbackFunction> = new Map<string, CommandCustomCallbackFunction>();

    public static registerCustomCallbackForCommand(command: string, callback: CommandCustomCallbackFunction) {
        this.commandsCustomCallbacks.set(command, callback);
    }

    public static async executeCommand(connection: LanguageClientConnection, command: string, commandArgs?: any[] | undefined): Promise<any | void> {
        const executeCommandParams = CommandExecutionAdapter.createExecuteCommandParams(command, commandArgs);
        const commandCustomCallback = this.commandsCustomCallbacks.get(command);

        return commandCustomCallback != null ? await commandCustomCallback(executeCommandParams) : await connection.executeCommand(executeCommandParams);
    }

    public static createExecuteCommandParams(command: string, commandArgs?: any[] | undefined): ExecuteCommandParams {
        return {
            command: command,
            arguments: commandArgs
        };
    }
}

I think it is concise and rather clear, and also doesn't add a new convention rather than following the existing one where Adapters are being used. Lemme know what you think, in the meantime I'll have to go ahead and start referencing executeCommand invocations to go through the Adapter

[UziTech]

So the java client would look something like the following code instead of this PR?

class JavaLanguageClient extends AutoLanguageClient {
  ...
  constructor () {
    ...
    this.registerCustomCallbackForCommand('java.apply.workspaceEdit', (command) => {
      if (command.arguments) {
        // Guaranteed only ever one element in the arguments array
        const edit: WorkspaceEdit = command.arguments[0];
        CodeActionAdapter.applyWorkspaceEdit(edit);
      }
    })
  }
  ...
}

[deankevorkian]

Precisely. :)

EDIT: executeCommand is async all the way, so if it runs asynchronously it would have awaits in it and return Promise, otherwise it would have to return an empty Promise. Promise.resolve() for say

[UziTech]

LGTM Can you create a PR for that?

[deankevorkian]

done, added some tests, still gotta see this works well with ide-java as a testbed

[UziTech]

closing in favor of #117