Support implementing quickfixes via language server

[kdvolder]

As far as I could figure out, implementing a quickfix via a language server isn't really possible. I think this is a shortcoming of the protocol. Though I'm not really sure about that. It could just be I'm missing something.

I tried getting some answers here: http://stackoverflow.com/questions/43328582/how-to-implement-quickfix-via-a-language-server

But got no replies.

Anyhoo. The problem seems to be the following:

The closest thing we have to implementing quickfixes is via textDocument/codeAction.

But... this only allows the server to request that the client executes some command on the client-side to perform the 'quickfix'.

There seem to be a number of problems with this kind of an approach:

1. there is no way for the server to implement commands.
2. there is no standard list of commands that can be relied upon.
3. if we accept the fact that we have to implement commands on the client:

• we have to implement them for every client we want to support (so this is not portable)
• the client is the wrong place to implement code transformation that require understanding the document structure (the knowledge for doing such transformations lives in the server).

So my conclusion is, this is rather broken / impossible at the moment. Wonder if it could be fixed.

One idea is that protocol should offer some mechanic similar to how content-assist completions work. I.e. a 'code action' item should provide the means to be resolved to some document or workspace edits when the user invokes it.

PS: I did manage to implement a some quickfixes, following exactly the mechanism described in the SO question. I.e. I defined a client-side command that executes a quickfix by sending back the info attached to it back the the language server using some custom protocol. The server then responds with workspace / document edits to be applied, which the client-side custom command then applies to the workspace.

[alanz]

I am also trying to thread this particular needle, and have come to the conclusion that it is something to do with textDocument/executeCommand, but there is some kind of registration that needs to be done from the server to the client so that it knows the command is external.

I look forward to a comprehensive explanation.

[kdvolder]

Hmm... yeah textDocument/executeCommand seems promising. At the very least it will be able to remove some ugly pieces from my awkward solution. I.e. the 'custom protocol' extension is no longer needed (textDocument/executeCommand will do instead). Still not clear how we get the client to recognize the command and send it to the server.

[mickaelistria]

Quick fixes are currently textDocument/codeActions, taking the Diagnostic as context: https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#textDocument_codeAction . The codeAction rely on textDocument/commands, which happens to become client specific.
Some other reports relate to this issue of commands which are not very usable in the context of multiple clients: #178 , #61, #126 ,
FYI, in LSP4J, it turns out we decided to implement support for any command in a greedy way, only considering the arguments ( http://git.eclipse.org/c/lsp4e/lsp4e.git/tree/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/codeactions/CodeActionMarkerResolution.java#n52 ). Although it's not a nice strategy nor piece of code, it's currently the most pragmatic and working way we found to integrate properly with VSCode language servers.

[kdvolder]

Okay... I think the pieces of this puzzle finally are starting to click together in my head.

To indicate which commands to execute on the server I think we need to use client/registerCapability and provide it a ExecuteCommandRegistrationOptions , where we can list the command-ids that the client should send to server rather than execute locally.

I think that was the only piece of this puzzle that was still missing. I'll try this out tomorrow and see if I can make that work.

[kdvolder]

Hmmm... vscode sends a rather sparse looking 'capabilties' object upon initialization.

{
  "jsonrpc": "2.0",
  "id": 0,
  "method": "initialize",
  "params": {
    "processId": 1698,
    "rootPath": "/home/kdvolder/git/sts4\/concourse",
    "capabilities": {},
    "trace": "off"
  }
}

So I guess it doesn't provide support for this stuff yet. Which also means that I won't be able to keep good on my promise to try this out tomorrow and report back. Or would that support be available already on the 'insiders build'? Currently I'm just using the latest stable vscode release.

[kdvolder]

Never mind I am dumb. I needed to update the language-server-client module version in my package.json to get protocol version 3.0 support.

[alanz]

And does vscode then actually use the new features if you do that? Are you running a dev version of vscode?

[alanz]

Ok, I get it now. The package.json for my client extension.

[kdvolder]

@alanz I'm still trying to figure out exactly what versions I need for everything to work together. I just updated the client version but I'm getting some compile errors, which I gather is because it needs a more up-to-date vscode api to compile against. I'll post an update if I figure it all out. Right now I can't really answer your questions yet as until this compiles I can't really see it run and debug things.

[alanz]

FYI, I updated my extension package.json to be https://github.com/alanz/vscode-hie-server/blob/master/package.json, did an npm update in that dir and the newer protocol is now used for my extension.

Using the standard vscode, current version, installed from a deb.

[alanz]

And this is the initialize message I am getting from vscode with that config

{"jsonrpc":"2.0"
,"id":0
,"method":"initialize"
,"params":
  {"processId":5196
  ,"rootPath":"/home/alanz/tmp/haskell-hie-test-project"
  ,"rootUri":"file:///home/alanz/tmp/haskell-hie-test-project"
  ,"capabilities":
    {"workspace":
      {"applyEdit":true
      ,"workspaceEdit":{"documentChanges":true}
      ,"didChangeConfiguration":{"dynamicRegistration":false}
      ,"didChangeWatchedFiles":{"dynamicRegistration":false}
      ,"symbol":{"dynamicRegistration":true}
      ,"executeCommand":{"dynamicRegistration":true}
      }
    ,"textDocument":
      {"synchronization":
        {"dynamicRegistration":true
        ,"willSave":true
        ,"willSaveWaitUntil":true
        ,"didSave":true}
      ,"completion":
        {"dynamicRegistration":true
        ,"completionItem":{"snippetSupport":true}}
        ,"hover":{"dynamicRegistration":true}
        ,"signatureHelp":{"dynamicRegistration":true}
        ,"references":{"dynamicRegistration":true}
        ,"documentHighlight":{"dynamicRegistration":true}
        ,"documentSymbol":{"dynamicRegistration":true}
        ,"formatting":{"dynamicRegistration":true}
        ,"rangeFormatting":{"dynamicRegistration":true}
        ,"onTypeFormatting":{"dynamicRegistration":true}
        ,"definition":{"dynamicRegistration":true}
        ,"codeAction":{"dynamicRegistration":true}
        ,"codeLens":{"dynamicRegistration":true}
        ,"documentLink":{"dynamicRegistration":true}
        ,"rename":{"dynamicRegistration":true}
       }
     }
  ,"trace":"off"}
  }

[kdvolder]

So I made some progress. But hit a little snag. When I try to register the my quickfix commands via the protocol I'm getting an error like this:

[pool-2-thread-1] ERROR org.springframework.ide.vscode.commons.util.Log - Error
org.eclipse.lsp4j.jsonrpc.ResponseErrorException: Unhandled method client/registerCapability

Not really 100% sure of this, but poking around a bit with the debugger, I find that there is no handler for client/registerCapability however there is one called client/registerFeature. Also found in the source-code for the language-server client here:

https://github.com/Microsoft/vscode-languageserver-node/blob/143fb3928737db949bb800ba27d1c6580ad0c024/client/src/protocol.ts#L95

Is the reference implementation actually using a different name than the Spec?

[kdvolder]

Indeed that seems the case, after switching over to using registerFeature my quickfixes are working now. Raised issue here:

microsoft/vscode-languageserver-node#199

I think its clear now that the LSP protocol has everything it needs to handle quickfixes properly. So I'm closing this ticket.

[alanz]

@kdvolder Can you give me an example of the registration message you send that works?

[alanz]

nevermind, I got it going too, by making sure my extension code is up to date. See https://github.com/alanz/vscode-hie-server/tree/168ad64931af4ff700d4a62eff724d3af9a2987c

[kdvolder]

@alanz Maybe you don't need it anymore. But I'm sending something like this:

{
   "jsonrpc":"2.0",
   "id":"1",
   "method":"client/registerFeature",
   "params":{
      "registrations":[
         {
            "id":"4165c42a-1ced-49b0-b2fd-ad08bca1702c",
            "method":"workspace/executeCommand",
            "registerOptions":{
               "commands":[
                  "sts.vscode-concourse.codeAction"
               ]
            }
         }
      ]
   }
}

[kdvolder]

FYI: I have updated my SO question with an answer that provides a summary of what I have learned. Hopefully it can save someone else some time figuring this out :-)

[alanz]

What I have is the response to initialize as

{
  "result":{
    "capabilities":{
         "textDocumentSync":1,
         "hoverProvider":true,
         "codeActionProvider":true,
         "renameProvider":true,
         "executeCommandProvider":{
              "commands":["applyrefact:applyOne"]}}}
  ,"jsonrpc":"2.0"
  ,"id":0
}

The in the codeAction reply

{"result":[{"title":"apply hint:Redundant do","command":"applyrefact:applyOne"}],"jsonrpc":"2.0","id":4}

No parameters yet, but it arrives as an executeCommand request at the server

[kdvolder]

@alanz Ah that's interesting. I used the 'dynamic capability registration'. I didn't know you can also register your commands via the 'initialize response'. That's actually slighlty simpler if you don't need the commands to be registered dynamically (which I don't). Thanks for the info.

[alanz]

The big thing that is missing in this spec is scenario based flows, so you can see how it is intended to be used.

cc @dbaeumer