emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.81k stars 892 forks source link

rust: no details in completion popup #4591

Open ensc opened 1 month ago

ensc commented 1 month ago

Thank you for the bug report

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 https://github.com/rust-lang/rust-analyzer/issues/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 https://github.com/emacs-lsp/lsp-mode/blob/27d6e795610a7685304e57a52937dff38968c877/lsp-mode.el#L3761-L3762 seems to restore the wanted behaviour.

jcs090218 commented 1 month ago

Thanks for reporting this issue.

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

ensc commented 1 month ago

rust-analyzer developers say in https://github.com/rust-lang/rust-analyzer/issues/18351#issuecomment-2428295678

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 https://github.com/rust-lang/rust-analyzer/pull/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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 1 week ago

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