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

rust: no details in completion popup #4591

Open
3 tasks done
ensc opened this issue Oct 22, 2024 · 20 comments
Open
3 tasks done

rust: no details in completion popup #4591

ensc opened this issue Oct 22, 2024 · 20 comments
Labels

Comments

@ensc
Copy link

ensc commented Oct 22, 2024

Thank you for the bug report

  • I am using the latest version of lsp-mode related packages.
  • I checked FAQ and Troubleshooting sections
  • You may also try reproduce the issue using clean environment using the following command: M-x lsp-start-plain

Bug description

With recent "rust-analyzer" versions, completion does not contain details anymore. E.g. with rust-analyzer 1.83.0- nightly (eb4e234 2024-10-09) and later I get

image

Previous versions (e.g. rust-analyzer 1.83.0-nightly (55a22d2 2024-10-06)) showed

image

Looking in the io log shows that "detail" and "documentation" attributes are missing with recent rust-analyzer.

See rust-lang/rust-analyzer#18351 too.

Steps to reproduce

Try to complete in rust sources.

Expected behavior

full function signature should be shown in the popup

Which Language Server did you use?

rust-analyzer

OS

Linux

Error callstack

No response

Anything else?

Removing "documentation" + "detail" from

lsp-mode/lsp-mode.el

Lines 3761 to 3762 in 27d6e79

. ((properties . ["documentation"
"detail"
seems to restore the wanted behaviour.

@ensc ensc added the bug label Oct 22, 2024
@jcs090218
Copy link
Member

Thanks for reporting this issue.

Is this the expected behavior from the upstream (language server)? 🤔

@ensc
Copy link
Author

ensc commented Oct 22, 2024

rust-analyzer developers say in rust-lang/rust-analyzer#18351 (comment)

That sounds like the client is telling r-a it supports lazily resolving those fields, so the client needs to also send a corresponding resolve request

This was added by rust-lang/rust-analyzer#18167

Recommendation sounds very expensive to me (but I do not have any idea about the LSP protocol) and disabling lazy-resolve support sounds better.

@kiennq
Copy link
Member

kiennq commented Oct 28, 2024

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?
lsp-mode does support lazy resolving for better perf due to less data passing and json processing. But it's triggered when item's document is requested, i.e, when the item is focused, company-mode will request the document for the focused item.
Probably that can be improved to show the detail for all displayed item instead

@ensc
Copy link
Author

ensc commented Oct 28, 2024

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?

does not have an effect; details are not shown

image

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

lsp-mode does support lazy resolving for better perf due to less data passing and json processing. But it's triggered when item's document is requested, i.e, when the item is focused

Does this really result in better performance? For me, this sounds very expensive. E.g. instead of parsing a "detail" (and perhaps "documentation") attribute in hundreds/thousands of completion candidates, you have to send hundreds/thousands resolve requests, wait for their responses and parse them.

@kiennq
Copy link
Member

kiennq commented Oct 29, 2024

For me, this sounds very expensive. E.g. instead of parsing a "detail" (and perhaps "documentation") attribute in hundreds/thousands of completion candidates

The point is server can opt-in into this if it makes sense for them. You can see, fetching documentation and details of hundreds/thousands of candidates can be very expensive as it hit disk to get the documentation. Moreover, that fetching happens on every single keystroke that most likely the user doesn't care about. Also, that make processing every keystroke become slower.

When lazy-resolving is supported, that means there's no waste to retrieve more information (and potentially disk reading) when the user is typing. Now, after the user has finish typing and starts looking at the candidates, we can start fetching candidates' detail when the user is interacting with normally a smaller number of candidates instead of all candidates. And they should be quicker than fetching the whole data at the beginning so the user would feel LSP run faster as well.

@ensc
Copy link
Author

ensc commented Oct 29, 2024

The point is server can opt-in into this if it makes sense for them

Sounds like a design flaw of the LSP protocol... Clients should be able to set this per request. E.g. minimal set of information for the per-keystroke action and detailed set for the dropdown list with completions.

Moreover, that fetching happens on every single keystroke that most likely the user doesn't care about.

I do not see this here: textDocument/completion request happens some time after I stop typing. Perhaps because I have (lsp-idle-delay 1) or so.

we can start fetching candidates' detail when the user is interacting with normally a smaller number of candidates instead of all candidates

For my use case I do not see a big reduction of candidates. E.g. often when I look in the completion dropdown list, I have only a raw idea about the method (naming, return type) and have to browse large parts of the list to find the correct one.

I am ok with fetching "documentation" lazily (large amount of text which should be displayed only for a selected item); but "detail" is an essential information which should be made available in the dropdown list immediately.

@kiennq
Copy link
Member

kiennq commented Oct 30, 2024

For my use case I do not see a big reduction of candidates. E.g. often when I look in the completion dropdown list, I have only a raw idea about the method (naming, return type) and have to browse large parts of the list to find the correct one.

Even in that case, you will have more time to look at each candidate than having to fetch everything at first.
LSP is designed for a general editor and not only Emacs with idle delay to fetch the candidates. The editor client (Emacs) in this case can opt-in into saying that they don't support lazy resolving as well. However, even with idle delay to fetch the candidates, processing more data makes Emacs slower and shows the candidates slower as well. Opt-in into lazy resolving allows Emacs to just process a piece of smaller information and display it faster, which can be significant if the language server is in remote (TRAMP) instead of local machine.
I think what we can improve here is to lazy resolve all of displayed candidates (9) asynchronously so that it doesn't affect the UX.

Btw, I've tried with the nightly build of Windows and it seems that the detail is still fetched and not lazy-resolved.

@ensc
Copy link
Author

ensc commented Oct 30, 2024

which can be significant if the language server is in remote (TRAMP) instead of local machine.

this seems to make lazy-resolving yet more slower... Hundreds of resolve requests to fill the completion dialog might be no network issue when sent locally. But over TRAMP?

What would be the effects when "detail" is removed from "resolveSupport"? Would this attribute be missing from the resolve answer? Or would it just reenable the field in the textDocument/completion response?

However, even with idle delay to fetch the candidates, processing more data makes Emacs slower

At least with rust, the "detail" property is so small compared with the rest of the response, that the difference is probably not noticable.

Btw, I've tried with the nightly build of Windows and it seems that the detail is still fetched and not lazy-resolved.

strange; here it is

$ rust-analyzer --version
rust-analyzer 1.84.0-nightly (1e4f10b 2024-10-29)

and I tested it with M-x lsp-start-plain.

@kiennq
Copy link
Member

kiennq commented Oct 30, 2024

which can be significant if the language server is in remote (TRAMP) instead of local machine.

this seems to make lazy-resolving yet more slower... Hundreds of resolve requests to fill the completion dialog might be no network issue when sent locally. But over TRAMP?

I think the push for RA to use lazy-resolving is because the language server can be on remote instead of local, at least that's what pointed out in the discussion.
And it's not hundreds of resolve requests to fill the completion dialog, only the visible one should be resolved, and at a time, it's a small number (9 candidates) instead of fetching thousands at the beginning and slow down the RA as well as client at the start.
The lazy-resolve request can be sent parallelly, unlike the completion request where we need to update the candidate list every time the user is typing (we have cache but some language server like clangd send their own list everytime), the lazy resolve is after the initial candidate list is showing, and we have more time to fetch the detail while not making it slow for the user to see the candidate list.

Also, the idle delay is controlled by the completion framework (company-mode in this case), it can send the request to fetch new candidates as soon as the user types on the keyboard and display the result later when the result comes, so we will need to handle that as well instead of lsp-mode idle delay.

strange; here it is

$ rust-analyzer --version
rust-analyzer 1.84.0-nightly (1e4f10b 2024-10-29)

and I tested it with M-x lsp-start-plain.

It's indeed very strange. I'm not sure why.

@traviscross
Copy link

It was thought that PR #4610 might solve this, but in testing with rust-analyzer at commit rust-lang/rust-analyzer@fc98e06 along with lsp-mode 20241113.743 (which contains PR #4610), the problem still reproduces.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

I don't believe #4610 is an incomplete fix for 2 reasons:

  1. This issue is probably a bug in rust-analyzer in that they've misinterpreted the LSP spec, I've explained the rationale here.

  2. completion: support async detail completion resolve and more lazy properties #4610 treats the completion items from textDocument/completion and completionItem/resolve the same, which causes UI problems for pretty much every other language servers and completion UIs.

@kiennq

only the visible one should be resolved, and at a time, it's a small number (9 candidates) instead of fetching thousands at the beginning and slow down the RA as well as client at the start.

This is indeed what #4610 tries to solve. The issue with company eagerly resolving the entire completion list is a little problematic, perhaps it should follow what #4625 and corfu-pixel-perfect has done, i.e. follow the VS Code model. CC @dgutov

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

BTW I can't reproduce this issue on rust-analyzer 1.83.0 (90b35a62 2024-11-26) anymore. @ensc Is this solved by rust-lang/rust-analyzer#18388 as stated here ?

@traviscross
Copy link

Probably this revert...

...would have made this unreproducible in r-a 1.83.0.

@ensc
Copy link
Author

ensc commented Dec 8, 2024

@ensc Is this solved by rust-lang/rust-analyzer#18388 as stated here ?

With lsp-mode 20241202.334 + rust-analyzer 1.85.0-nightly (9c707a8 2024-12-07) I still see the effect from the initial report (no details in popup).

Details appear after selecting another item in the list (which was added by #4610); while this is a huge improvement, I am still not used at it and miss the old behavior where details are visible immediately.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 8, 2024

@SomeoneToIgnore after our nice little discussion last night, is there a plan to roll back all remnants of rust-lang/rust#133476 ?

@wyuenho
Copy link
Contributor

wyuenho commented Dec 8, 2024

@ensc BTW, are you using company or corfu?

@dgutov
Copy link
Contributor

dgutov commented Dec 8, 2024

The issue with company eagerly resolving the entire completion list is a little problematic, perhaps it should follow what 4610 and corfu-pixel-perfect has done, i.e. follow the VS Code model

A more concrete proposal might help.

Offhand, I can say that company doesn't resolve the entire completion list - only the entries currently visible in the popup, as well as some annotations (for entries that are string-equal to each other). And the details of that really depend on the implementation in lsp-mode. But if a handful of "resolve" operations can be expensive, then I suppose that could still be suboptimal.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 8, 2024

Sorry I mean #4625 , I've tagged you over there, let's move our convo there.

@ensc
Copy link
Author

ensc commented Dec 9, 2024

@ensc BTW, are you using company or corfu?

company

@wyuenho
Copy link
Contributor

wyuenho commented Dec 12, 2024

Comfirmed this is still happening with rust-analyzer nightly. This is likely a bug in R-A, as suggested here. There's no reason R-A should skip the detail in the response of textDocument/completion. Let's file an issue over there instead.

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

Successfully merging a pull request may close this issue.

6 participants