Respect vim.lsp.commands, client.commands, and server capabilities when executing CompletionItem.command

[rjooske]

Currently, when cmp-nvim-lsp receives a CompletionItem with a Command, it executes the command by directly making a workspace/executeCommand request.
This behavior fails to respect the user-defined command handlers in vim.lsp.commands and client.commands, as well as whether the language server is capable of handling the command.
This PR addresses this issue by using the client:_exec_cmd() method, which does respect vim.lsp.commands, client.commands, and server capabilities.

I am aware that client:_exec_cmd() is probably meant to be a private method and that relying on it might become a problem in the future.
However, I'm not sure if copying the whole method over to this plugin would be any better, and a quick search reveals that there are some plugins that do rely on client:_exec_cmd().
Please let me know if you have a better idea.

[rjooske]

I just noticed that cmp behaves weird in certain situations with this change applied. I'll keep this PR as a draft until I figure out why.

[rjooske]

Turns out that I actually had to copy and paste the logic inside the client:_exec_cmd() since there was no way to tell if the command was handled by a user-defined handler or the language server, making it not possible to call the callback argument correctly.
The weird behavior I mentioned was because callback was not being called correctly and is fixed now.

[rjooske]

I think it's also worth talking about how I got here for the future googlers.

The first symptom of this bug that this PR fixes was rust-analyzer flooding the LSP log file with this error message:

[ERROR][2024-10-11 02:08:36] .../vim/lsp/rpc.lua:770	"rpc"	"/Users/rjooske/.cargo/bin/rust-analyzer"	"stderr"	'2024-10-10T17:08:36.571952Z ERROR rust_analyzer::dispatch: unknown request: Request { id: RequestId(I32(16)), method: "workspace/executeCommand", params: Object {"command": String("rust-analyzer.triggerParameterHints"), "title": String("triggerParameterHints")} }\n'

(and the manually formatted version for legibility:)

rust_analyzer::dispatch: unknown request: Request {
    id: RequestId(I32(16)),
    method: "workspace/executeCommand",
    params: Object {
        "command": String("rust-analyzer.triggerParameterHints"),
        "title": String("triggerParameterHints")
    }
}

I found out that the rust-analyzer.triggerParameterHints command in CompletionItem.commands was being sent to rust-analyzer as workspace/executeCommand requests, which rust-analyzer was rejecting since it doesn't have such a command.
Trying to fix this, I tried to handle the commands by using vim.lsp.commands and client.commands as documented here, but nothing I did worked and the commands were still being sent to rust-analyzer.
Eventually I found out that it was cmp-nvim-lsp that failed to account for vim.lsp.commands and client.commands, as well as the server capabilities.

Also, after this PR is applied, you'll probably see this warning when accepting a completion item from rust-analyzer (assuming you don't have the config discussed below), which I'm guessing will be googled in the future.

cmp_nvim_lsp: Language server `rust_analyzer` does not support command `rust-analyzer.triggerParameterHints`. This command may require a client extension.

This is because the commands from completion items are now being handled properly and by default there's no handlers on the client (neovim) side for the rust-analyzer.triggerParameterHints command.
You can put this in your config to show the parameter hints in neovim after accepting a completion item, which is the intended behavior of rust-analyzer's clients, and to make the warning go away.

vim.lsp.commands["rust-analyzer.triggerParameterHints"] = function()
  vim.lsp.buf.signature_help()
end

Alternatively, you can just do nothing and only get rid of the warning if you find the parameter hints annoying.

vim.lsp.commands["rust-analyzer.triggerParameterHints"] = function() end

If you don't want to modify the global command handlers, you can also put the handler in client.commands (documented here), which has a higher precedence.

[hrsh7th]

I understand your PR, but I'm not sure if the language server is returning server_capabilities.executeCommandProvider properly.

If the language server sends a supported command without returning server_capabilities.executeCommandProvider, won't this process cause an error?

[hrsh7th]

IMO, we should check client command first.

[seblyng]

It seems like exec_cmd is no longer private in 0.11 after this: neovim/neovim#30932

I have this issue in roslyn.nvim which I maintain for C#, and there was recently a PR into neovim that fixes this issue for the built in client. However, this issue still remains for nvim-cmp (and blink.cmp for that matters).

See this and linked issue for more details: neovim/neovim#32445

What about for example potentially just check if exec_cmd exists, and use that if it's available?

[rjooske]

I'm sorry that it took me this long to reply...

To @hrsh7th

I understand your PR, but I'm not sure if the language server is returning server_capabilities.executeCommandProvider properly.

Do you mean rust analyzer may not be returning executeCommandProvider properly? If so I don't think you're correct. executeCommandProvider is a list of command names that can be executed on the language server via workspace/executeCommand, and rust-analyzer.triggerParameterHints is a command that rust analyzer expects the LSP client to run. So rust-analyzer.triggerParameterHints not being included in executeCommandProvider is totally normal.

If the language server sends a supported command without returning server_capabilities.executeCommandProvider, won't this process cause an error?

Yes it does cause an error in that case, but I'd say that's the language server's fault and there's not much we can do about this.

IMO, we should check client command first.

I'm not sure what exactly is your problem with my changes? My code checks if either self.client.commands or vim.lsp.commands can handle the given command, and that's in line with how these kinds of "follow-up commands" like CompletionItem.command are handled by the neovim LSP client.

To @seblyng
Thanks for letting us know about the exec_cmd updates!

What about for example potentially just check if exec_cmd exists, and use that if it's available?

Sure, and it looks like that's how blink.cmp implements this part of their plugins, but that actually doesn't work at least in cmp-nvim-lsp. (or maybe also in blink.cmp and it's an overlooked bug? I've never used blink so don't count on me though)
The third argument of exec_cmd, which is a callback, is called only when exec_cmd ends up sending workspace/executeCommand to the language server. In cmp-nvim-lsp however, there's a callback we need to call regardless of whether workspace/executeCommand was sent or not, which you can find here. Since there is currently no way for us to know whether exec_cmd actually sent workspace/executeCommand, we can't use exec_cmd (or the older _exec_cmd for the same reason), which is why I ended up copy-pasting the code from exec_cmd to properly call the callback when needed.

We can probably request a change to the upstream like making exec_cmd return true when it sends workspace/executeCommand, but in the meantime this PR should do the job just fine.