Make LSP completions resolve capabilities more spec-compliant

[SomeoneToIgnore]

Closes #72
Part of rust-lang/rust-analyzer#18504

rust-analyzer started to be more spec-compliant when it comes to completion resolve:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion

Since 3.16.0 the client can signal that it can resolve more properties lazily. This is done using the completionItem#resolveSupport client capability which lists all properties that can be filled in during a ‘completionItem/resolve’ request. All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and must not be changed during resolve.

So, if the editor declares a completion resolve support for the documentation, rust-analyzer as a server can omit it in the initial response and now it does so.

[SomeoneToIgnore]

To be more precise, textEdits is something we need to remove to fix #72

but given the overall situation, any "fancy" visualization of completion items needs detail and any hover request would also need documentation.

I have zero knowledge of the language and the editor, so feel free to update the properties correspondingly, if you're sure things are working with these capabilities.

[AppleSheeple]

@SomeoneToIgnore

Reason for closing would be appreciated.

The situation (from rust-analyzer's side at least) seems to be too fluid, with a mix of new features, changes, possible reverts, and possible fixes all being involved.

I applied your change here when this was submitted and stuck with a working rust-nightly and neovim-git versions until things move into a known good state.

[SomeoneToIgnore]

The main reason to close this is 0 activity on this PR for around a month — either I did something wrong and no one wants to comment this, or it's not needed to the repository.

Otherwise, rust-analyzer's source tree in rust project has a full revert, so things should work as they did before, without any extended resolve capabilities.

In rust-analyzer source tree, nothing is reverted still and bugs are being gradually fixed when more clarifications appear.
rust-lang/rust-analyzer#18630 makes an attempt to guess nvim-lsp's client metadata and use the old way of resolving things for it, so is the metadata is correct it can be also considered "fixed" at this point.

Apart from that, rust-lang/rust-analyzer#18653 should be fixing the last of the issues related to the new resolve approach, but time will tell.

[hrsh7th]

I'm really sorry. I completely overlooked this.

By the way, I don't understand this whole issue at all.
Is there a specification incompatibility in cmp-nvim-lsp (and nvim-cmp)?

In any case, I've never heard of this issue outside of rust-analyzer, so I'm hesitant to implement this fix.

[SomeoneToIgnore]

It's more a spec interpretation thing, I've quoted the excerpt in the OP: when certain item is added to completionItem#resolveSupport, the LSP server can (but not should) omit it in the initial response.

Apparently, no other server omits textEdit despite the corresponding capability given, but r-a does now.
I'm not quite sure why would a client give such a capability at all, so seemed logical to remove such thing — I think it still makes sense to do, at least for r-a, but it's your call.

r-a could return it back but turns out all other editors are managing their resolve capabilities better and are either able to resolve textEdit or, at least, do not specify that field in the resolveSupport, ergo the server must send it during the initial completions response.
nvim here is one of the two clients that both give the capability and fail to resolve it — after learning that, rust-lang/rust-analyzer#18630 tried to send back fields as before for nvim.

@hrsh7th , can you confirm that this LSP client does send ClientInfo in a form of "clientInfo":{"name":"Neovim","version":"0.8.0"}? (taken from #36 (comment)) If that's true and the client name is Neovim still, r-a has the fix on its side.

[hrsh7th]

My understanding

• cmp-nvim-lsp also specifies properties that would be problematic if resolved lazily in resolveSupport.properties
• General language servers do not omit textEdit, but rust-analyzer follows resolveSupport.properties
• For some reason, textEdit works even if it is resolved lazily in VSCode

Is this understanding correct?

[SomeoneToIgnore]

All but the last bullet seems correct: VSCode does not specify such property and not sure if it can resolve lazily correctly.

[hrsh7th]

Yes. I understand now. Your PR is correct.

But we should follow VSCode so we should specify only documentation, detail, additionalTextEdits, wdyt?

https://github.com/microsoft/vscode-languageserver-node/blob/906f5fb306e1f6059cbdcb1efd962647222b5867/client/src/common/completion.ts#L85

[SomeoneToIgnore]

Here, my tiny knowledge of nvim is over and I'm not sure what is a good advice: VSCode's list seems reasonable, I'd also add command there which almost no client seems to care about.

The only concern I have is about resolving detail: that might be somewhat dangerous if nvim prefers to display "rich" completion labels based on the information from it.
For example, r-a returns completion lists like [... "label":"PhantomData","labelDetails":{"detail":" (use std::marker::PhantomData)","description":"PhantomData<{unknown}>"}, ...]
If the client uses label as a fallback to the completion item display, and tries to render rich labels with the details by default, resolving this property will lead to certain visual "jumpiness".
Here's an example of vtsls server that ignores the spec and always resolves detail and Zed client that wants detail in the initial response and does not include it into the capabilities:

completions.mov

Adding detail into the list will make r-a behave in a similar way.
So, if nvim relies on detail when rendering the completion items, it might be better to omit it from the capabilities (which, unfortunately, won't guarantee the fact that it will still be forced to resolve items to get this property, see vtsls)

[hrsh7th]

In fact, it seems that we should specify properties that are not used until the item is selected or confirmed.

In other words, it seems dangerous to specify sortText or filterText. (This is because they are used to filter and sort all items in the list.)

Could you remove filterText and sortText from original PR? I'll merge it after it.

Ah, sorry. Please restore documentation field.

[SomeoneToIgnore]

I cannot reopen the PR unfortunately, but will create another one with the notes, thank you for the discussion.

[SomeoneToIgnore]

#77