Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LSP completions resolve capabilities more spec-compliant #4609

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SomeoneToIgnore
Copy link

@SomeoneToIgnore SomeoneToIgnore commented Nov 12, 2024

Closes #4591
Closes #4607
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.

According to #4591

  1. rust: no details in completion popup #4591 (comment)

Can you try to move to the next item in the completion list to see if the function detail will be displayed for the previously selected item?

I do not see any lsp communication in the (lsp-workspace-show-log) buffer when selecting another element.

  1. rust: no details in completion popup #4591 (comment)

Removing "documentation" + "detail" from ... seems to restore the wanted behaviour.


Before rust-analyzer's change, it sent back every possible property.
Now, it starts to send back only things that are not declared in the completion resolve capabilities, as by the spec, the rest should be possible to get resolved later by the editor.

Apparently, despite the resolve capabilities declaration, Emacs lsp-mode client cannot properly resolve these, hence the PR restores the status-quo by making rust-analyzer to return these properties immediately, as it had been before.

Later, a better resolve mechanism may be implemented to resolve these, if needed.

@psibi
Copy link
Member

psibi commented Nov 12, 2024

Later, a better resolve mechanism may be implemented to resolve these, if needed.

What would be a way to do it ? How does VS Code extension for rust analyzer implement it ?

@SomeoneToIgnore
Copy link
Author

SomeoneToIgnore commented Nov 12, 2024

Unfortunately, I have no good pointers at how VSCode does this, but in case of Zed we are doing a mixture of two approaches:

  • first, we have tried to resolve all that's possible, spawning an async resolve task for all visible menu entries and whatever got selected later, if the user moves along the completion menu entries
  • but it turned out that this either introduced certain latency or extra code complexity

I believe VSCode follows the complexity path, by trying to cache as many menu items as possible while the user types, predicting what has to be edited: e.g. Ha and HaMa might both replace the input with HashMap (+ add some global import edits, etc.) but it's the LSP which says that to replace (Ha and HaMa) to HashMap so it's some sort of predicting the LSP response.
This way, the resolve requests are relatively ready by the time the user asks for autocomplete.

We went with a simpler thing by re-querying the completion lists always, which lead us to excessive resolve queries — and even for the simple cases those were not instant, so there were noticed situations when a user may type and ask for autocomplete, which will take extra, very visible, fractions of second to execute due to resolve request + potential docs parsing took a while.

  • resolving everything also causes visual flicker, as details may contain rich text for the completion label, while regular, unresolved completion just has its plaintext label.
    If we were to resolve the entire item menu (menu item height number of resolve requests to make each time), that would look somewhat slow to appear.
    If we were to show the menu instantly, it will "jump" into colors some time later.

So, after trying various resolves, we've ended up using VSCode's resolve set in Zed: we force LSP servers to send us everything needed for the menu to be colored and ready to complete something useful at the caret position instantly.
This means resolving docs, detail and additional text edits only.

The server is faster to compute these properties in bulk (instead of menu item height number of resolve requests to make each time) and the editor can complete things almost instantly, with the additional text edits applied in the background, asynchronously. Those are usually imports, so at least for Rust this works quite well.


Sorry, I'm not a Lisp person and do not understand what is wrong with the tests.
In fact, I cannot even compile the project on my vacation device right now, so I will not be able to fix them this week at least and appreciate some help.

@psibi
Copy link
Member

psibi commented Nov 12, 2024

Thanks for the detailed explanation!

This means resolving docs, detail and additional text edits only.

I guess with your change (in lsp-mode) we are still not resolving additional text edits ?

do not understand what is wrong with the tests.

They are just Emacs snapshot tests that are failing (which is expected). @jcs090218 Probably worth disabling tests in snapshot ?

@SomeoneToIgnore
Copy link
Author

I guess with your change (in lsp-mode) we are still not resolving additional text edits ?

I suspect some odd things may be there based on the issue comments, but I'm not familiar with the codebase enough to say that.

It seems though, that r-a works in lsp-mode in cases when imports are added through autocompletion.
With the broken version, now, when you complete the HashMap, nothing happens, but after this fix imports appear again.

@kiennq
Copy link
Member

kiennq commented Nov 12, 2024

I guess with your change (in lsp-mode) we are still not resolving additional text edits ?

We already support resolving additional text edits and also documentation property as well.
The only one that isn't supported now is detail. Actually if you enable company-auto-update-doc, you can observe that after moving the selection, the detail will show up on the previously selected item.

Also, instead of disabling detail resolve, we can actually resolve it asynchronously without affecting the latency for candidate list to show up.
See #4610

@jcs090218
Copy link
Member

They are just Emacs snapshot tests that are failing (which is expected). @jcs090218 Probably worth disabling tests in snapshot ?

It's still quite confusing, but we have the experimental flag on (see below).

experimental: [false]
include:
- os: ubuntu-latest
emacs-version: snapshot
experimental: true
- os: macos-latest
emacs-version: snapshot
experimental: true
- os: windows-latest

FYI, @kiennq is working on fixing in #4612. And I will push more fixes when I have time. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Completion resolve support with textEdit rust: no details in completion popup
4 participants