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

completion: support async detail completion resolve and more lazy properties #4610

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

kiennq
Copy link
Member

@kiennq kiennq commented Nov 12, 2024

Until now the even though we marked as supporting detail property of CompletionItem, we don't actually doing completionItem/resolve for that property but instead relied on another event (doc updated) to update that.
This change fixes that and allows us to query the completion item detail asynchronously.

9f759b82-51fc-4f38-817c-1a005db84821

@kiennq kiennq changed the title lsp-completion--annotate: support async completion resolve completion: support async detail completion resolve and more lazy properties Nov 12, 2024
@kiennq
Copy link
Member Author

kiennq commented Nov 13, 2024

This should also fix #4591 and #4607

@kiennq kiennq merged commit 5991c60 into emacs-lsp:master Nov 13, 2024
11 of 13 checks passed
@traviscross
Copy link

I've tested this with the latest nightly r-a and it unfortunately does not fix #4591.

That is, in my testing with lsp-mode-20241113.743, which I manually verified contains in the code in this PR, the method signature details are still not displayed.

@kiennq
Copy link
Member Author

kiennq commented Nov 14, 2024

I've tested this with the latest nightly r-a and it unfortunately does not fix #4591.

That is, in my testing with lsp-mode-20241113.743, which I manually verified contains in the code in this PR, the method signature details are still not displayed.

Interesting, can you repro that with lsp-start-plain?
I've tried with the same latest nightly r-a, the signature details will be shown after you change the focus (tab) to the next suggested candidate. The GIF in the description is also showing that.

Also, if you can take a trace log (lsp-toggle-trace-io) I can take a further look at it.

@kiennq kiennq deleted the fix/resolve-detail branch November 14, 2024 09:01
@traviscross
Copy link

traviscross commented Nov 14, 2024

the signature details will be shown after you change the focus (tab) to the next suggested candidate.

Ah, yes, that is what I see also. In my testing, I just flagged it as a failure when they didn't show initially and never selected the next candidate.

Is this the intended final state, or is there more to do? Prior to the r-a change in rust-lang/rust-analyzer#18167, the signature details were shown initially, before switching between candidates, and in testing other editors, including e.g. Emacs with eglot, they show the signature details initially also.

@kiennq
Copy link
Member Author

kiennq commented Nov 15, 2024

Is this the intended final state, or is there more to do? Prior to the r-a change in rust-lang/rust-analyzer#18167, the signature details were shown initially, before switching between candidates, and in testing other editors, including e.g. Emacs with eglot, they show the signature details initially also

Yes.
Prior to the r-a change, the signature details are always sent when we request for the suggestion candidates so that's why it shows immediately. With the new change in r-a, the detail and other information can be requested lazily and can make the candidates for r-a be able to show earlier. The lsp-mode change is to make us to fetch the annotations for some of the candidates (not all) asynchronously after the list has been displayed.
I think we should keep this behavior as it's good for language servers that support resolve candidates lazily and can send a lot of candidates after each keystroke.

To improve the current display issue, we can probably force refresh company-mode display list when the detail fetching is completed. But, since the user will need to interact with the candidates anyway, the current behavior (update when selected) might be also okay.

@traviscross
Copy link

traviscross commented Nov 15, 2024

Interesting, thanks for that context. Much appreciated, as is your work on lsp-mode and your quick response on making the PR here.

To improve the current display issue, we can probably force refresh company-mode display list when the detail fetching is completed.

Makes sense.

But, since the user will need to interact with the candidates anyway, the current behavior (update when selected) might be also okay.

For me, personally, I look at the list and decide which candidate to select before interacting with it, so that doesn't really work out. More generally, I'm probably just a bit concerned about people perhaps thinking things are broken when upgrading to Rust 1.83. Hate to ask anything of a fellow maintainer, of course, as all are overworked, but if there's something that can be done straightforwardly, it may be worth doing.

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

Successfully merging this pull request may close these issues.

4 participants