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

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. :)

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

I've read the spec about 10 times now and I don't see where it says the server must delay responding with all the properties in completionItem#resolveSupport. Both the paragraph and the interface comment in https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion says similar things:

By default the request can only delay the computation of the detail and documentation properties

Before version 3.16.0 only the predefined properties documentation and detail could be resolved lazily.

Rust-analyzer seems to be the only language server does not respond with details when it is listed in completionItem#resolveSupport.

The can and could should be interpreted like any other spec in that they are merely hints, as most language servers do respond with detail and optionally documentation in textDocument/completion when speed is not an issue. This is illustrated by this sentence:

A typical use case is for example: the textDocument/completion request doesn’t fill in the documentation property for returned completion items since it is expensive to compute.

I don't think speed is an issue for rust-analyzer. If it doesn't respond with detail when listed in completionItem#resolveSupport but speed is not a problem, I'll consider this a bug there instead of a bug for lsp-mode.

@SomeoneToIgnore
Copy link
Author

To me, the point is different, so I disagree.

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.

So, lsp-mode signals to r-a that it can (as a client) resolve certain properties lazily, an r-a now can follow the spec and omit certain fields which it does gladly.
But turns out that lsp-mode lies and actually cannot resolve those things lazily despite the declaration — that is a bug.

And if you want to compare with the others, then I'd say that the only two odd clients who cannot uphold their declared capabilities are lsp-mode and nvim's LSP, all the rest (VSCode, Kate, Zed, Helix, etc.) are perfectly fine if anything declared in their resolveSupport is resolved lazily.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

So, lsp-mode signals to r-a that it can (as a client) resolve certain properties lazily, an r-a now can follow the spec and omit certain fields which it does gladly.
But turns out that lsp-mode lies and actually cannot resolve those things lazily despite the declaration — that is a bug

There are two separate but related issues. First, rust-analyzer does not have to omit detail from the response of textDocument/completion. In fact, I'd argue it should not omit them as speed is not an issue. The fact it does omit detail is a misinterpretation of the spec. Second, before #4610, lsp-mode only resolved the completion item when the lsp-completion--exit-fn or lsp-completion--get-documentation was called (company-quickhelp and friends will call this), but the newly resolved detail was never prepended to the documentation like VS Code, so you never knew it was resolved.

What was missing, was the ability to prepend the detail to the documentation like VSCode, hence #4625.

@SomeoneToIgnore
Copy link
Author

SomeoneToIgnore commented Dec 7, 2024

First, rust-analyzer does not have to omit detail from the response of textDocument/completion

The problem of can in that spec place, that the other is correct too:
rust-analyzer does not have NOT to omit detail from the response of textDocument/completion

After it's stated in the resolveCapabilities, it's up to the server to decide what to do — it can omit, it can keep it.
But it's the client that gave him that capability, and the client is clearly not able to fulfill its part of the deal.

In fact, I'd argue it should not omit them as speed is not an issue

It's not just the speed but also the amount of JSON sent over between instances — for a remote (e.g. ssh) LSP server, that would be sending over the wire.

Cannot comment the rest of the internal impl details.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

After it's stated in the resolveCapabilities, it's up to the server to decide what to do — it can omit, it can keep it.
But it's the client that gave him that capability, and the client is clearly not able to fulfill its part of the deal.

We are in agreement until "the client is clearly not able to fulfill its part of the deal". It did resolve the completion item, you just didn't know it did because it was never shown to you unless you used a package that would popup the documentation for the selected completion item, then all of a sudden the detail will show up when the completion popup refreshes. That's how corfu works. For company, it's even sillier, it'll just do an N+1 call to resolve every completion item before the popup is shown, that's why it's slow as molasses.

It's not just the speed but also the amount of JSON sent over between instances — for a remote (e.g. ssh) LSP server, that would be sending over the wire.

Then rust-analyzer should omit detail when the client is connecting over a TCP connection, there's no reason why it should omit them when connecting via stdio.

@SomeoneToIgnore
Copy link
Author

when the client is connecting over a TCP connection

Those are some odd fantasies, as the transport level is not specified (you may want to read the spec again).

I'm somewhat surprised there's so much blah-blah over this PR, what is the problem with removing details from the completion resolveCapabilities?
Then, by the spec, the server must include the details into every response and the client can be happy?

Alternatively, you can follow rust-lang/rust-analyzer#18630 and add another exception of your proud client there (given it's well with spec compatibility and actually sends its clientInfo properly filled?)

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

Those are some odd fantasies, as the transport level is not specified (you may want to read the spec again)

You may want to look at how every other language server is implemented instead of rediscovering all the problems every other language server implementation has discovered before you. I don't care how you implement network connections, the important thing is, you must be able to distinguish a network connection from a stdio connection, most other language servers provide TCP connections and stdio connections, which is also what lsp-mode supports. If you can't, that's another bug in rust-analyzer.

I'm somewhat surprised there's so much blah-blah over this PR, what is the problem with removing details from the completion resolveCapabilities?
Then, by the spec, the server must include the details into every response and the client can be happy?

To someone who does not understand emac-lisp or the implementation of lsp-mode or how it cooperates with other Emacs packages to resolve the completion item, or understand what "can" and "could" mean in a spec, you sure have a lot of strong opinions.

lsp-mode supports literally 100+ of LSP servers, rust-analyzer is the odd one out that misinterprets the spec, does not or refuse to distinguish connection types when deciding what to return in textDocument/completion, why should lsp-mode omit detail and documentation in resolveSupport just because some random dude who works on rust-analyzer says so? AFAIK, the vast majority of language servers either do not have any "detail" to respond in textDocument/completion, or there's no problem returning them because speed is not an issue, these are sensible defaults to tell the language servers what lsp-mode can resolve lazily. detail in resolveSupport is largely ignored by language servers, and documentation also, as they tend to be long so the language servers never send them even when omitted from resolveSupport. Everyone else when faced with the same problem inevitably coalesced around the same solution and rust-analyzer is the only one doing all kinds of weird things and demanding everyone else to special-case rust-analyzer, or in this case, change the default for everyone else to cater to rust-analyzer. Why? What's so special about rust-analyzer?

@SomeoneToIgnore
Copy link
Author

some random dude who works on rust-analyzer says so?

One note I'd like to state here, is that I do not really work on rust-analyzer.

For the rest of the things, seems that you've also misread the fact that the only two special oddballs are lsp-mode and nvim, and the rest of the editors work just fine.
What is so special about lsp-mode that it cannot try and understand things about can in a similar way others do?

Thinking on it more I'd really want to cut another blag blerp storm from a zealot, and call it quits: after all, you're the master here, who am I to come and try to fix things.

Thank you for helping me to realize this quite soon.

@SomeoneToIgnore SomeoneToIgnore deleted the more-lsp-spec-compliance branch December 7, 2024 15:59
@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

What is so special about lsp-mode that it cannot try and understand things about can in a similar way others do?

Nothing. lsp-mode is doing the correct thing here. I've said it a couple of times already and you still don't understand when and how lsp-mode resolves the completion item.

Given your revert, it doesn't seem to matter anymore. I wonder how many editors you've broken in the past few months TBH. Please stop.

@SomeoneToIgnore
Copy link
Author

Not in your bigot powers to ask me to stop on anything: in fact, rust-analyzer repo still contains the changes, so it's going to move on with this in rustc repo too, sooner or later.

In fact, I'd love you to stop instead, as you cannot really add any technical details to the topic, but still add your strongly opinionated blah for some reason.

What's upsetting, it's not even on the point, as

or understand what "can" and "could" mean in a spec

is a straight up lie due to #4609 (comment)

We are in agreement until "the client is clearly not able to fulfill its part of the deal".

which meant that first you've agreed on the can interpretation and then something else jumped in your head to provoke that lying commentary.
Maybe, you should stop and re-read the thread a few times, before posting another blurp?

The most interesting question is this

I'm somewhat surprised there's so much blah-blah over this PR, what is the problem with removing details from the completion resolveCapabilities?
Then, by the spec, the server must include the details into every response and the client can be happy?

But even here you continued spilling the letters without answering, so I assume you have no good clue on that too (as anything else, including the spec).

Overall, this pointing and telling what to do without an attempt to explain that will only cause retaliation, but as a small god in that repo, not like you care it seems.


So, yeah, unless you have anything clever to say, let's stop, both of us.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

Ok, let me draw this out for you.

lsp-mode/lsp-completion.el

Lines 457 to 463 in c3be413

(defun lsp-completion--get-documentation (item)
"Get doc comment for completion ITEM."
(-some->> item
(lsp-completion--resolve)
(get-text-property 0 'lsp-completion-item)
(lsp:completion-item-documentation?)
(lsp--render-element)))

lsp-mode/lsp-completion.el

Lines 611 to 690 in c3be413

(defun lsp-completion--exit-fn (candidate _status &optional candidates)
"Exit function of `completion-at-point'.
CANDIDATE is the selected completion item.
Others: CANDIDATES"
(unwind-protect
(-let* ((candidate (if (plist-member (text-properties-at 0 candidate)
'lsp-completion-item)
candidate
(cl-find candidate (funcall candidates) :test #'equal)))
(candidate
;; see #3498 typescript-language-server does not provide the
;; proper insertText without resolving.
(if (lsp-completion--find-workspace 'ts-ls)
(lsp-completion--resolve candidate)
candidate))
((&plist 'lsp-completion-item item
'lsp-completion-start-point start-point
'lsp-completion-markers markers
'lsp-completion-resolved resolved
'lsp-completion-prefix prefix)
(text-properties-at 0 candidate))
((&CompletionItem? :label :insert-text? :text-edit? :insert-text-format?
:additional-text-edits? :insert-text-mode? :command?)
item))
(cond
(text-edit?
(apply #'delete-region markers)
(insert prefix)
(pcase text-edit?
((lsp-interface TextEdit) (lsp--apply-text-edit text-edit?))
((lsp-interface InsertReplaceEdit :insert :replace :new-text)
(lsp--apply-text-edit
(lsp-make-text-edit
:new-text new-text
:range (if (or (and current-prefix-arg (eq lsp-completion-default-behaviour :replace))
(and (not current-prefix-arg) (eq lsp-completion-default-behaviour :insert)))
insert
replace))))))
((or (unless (lsp-falsy? insert-text?) insert-text?) label)
(apply #'delete-region markers)
(insert prefix)
(delete-region start-point (point))
(insert (or (unless (lsp-falsy? insert-text?) insert-text?) label))))
(lsp--indent-lines start-point (point) insert-text-mode?)
(when (equal insert-text-format? lsp/insert-text-format-snippet)
(lsp--expand-snippet (buffer-substring start-point (point))
start-point
(point)))
(when lsp-completion-enable-additional-text-edit
(if (or resolved
(not (seq-empty-p additional-text-edits?)))
(lsp--apply-text-edits additional-text-edits? 'completion)
(-let [(callback cleanup-fn) (lsp--create-apply-text-edits-handlers)]
(lsp-completion--resolve-async
item
(-compose callback #'lsp:completion-item-additional-text-edits?)
cleanup-fn))))
(if (or resolved command?)
(when command? (lsp--execute-command command?))
(lsp-completion--resolve-async
item
(-lambda ((&CompletionItem? :command?))
(when command? (lsp--execute-command command?)))))
(when (and (or
(equal lsp-signature-auto-activate t)
(memq :after-completion lsp-signature-auto-activate)
(and (memq :on-trigger-char lsp-signature-auto-activate)
(-when-let ((&SignatureHelpOptions? :trigger-characters?)
(lsp--capability :signatureHelpProvider))
(lsp-completion--looking-back-trigger-characterp
trigger-characters?))))
(lsp-feature? "textDocument/signatureHelp"))
(lsp-signature-activate))
(setq-local lsp-inhibit-lsp-hooks nil))
(lsp-completion--clear-cache)))

This is how lsp-mode resolves the completion item on demand. This is the 4th time I've reiterated myself. I'm not going to repeat myself again and you just have to believe me.

If you are to proceed with omitting detail by default, I beg you to consider that not every client can issue N+1 requests to resolve a page of completion items. VS Code can support on demand or an optional itemResolveCount in vscode.executeCompletionItemProvider to eagerly resolve a list of items, but not Emacs. Emacs doesn't even have any meaningful concurrency support. Up to now, pretty much every language server ignores detail in resolveSupport and sends down the details in textDocument/completion when it deems suitable. Rust's types aren't that long, just send them by default. Even if Emacs had great concurrency, by omitting detail in the response, rust-analyzer is still being a special snowflake that forces everyone to issue inefficient N+1 requests to resolve a page of items. This is in direct contradiction to your claim of network efficiency here.

In addition, the spec does not say the detail for a completion item has to be the same in the response of textDocument/completion and completionItem/resolve, as you have no doubt discovered already. In fact, that's how the typescript language server works. By omitting detail by default, you are precluding rust-analyzer the opportunity of this mode of operation unnecessarily.

What I have problem with you is your tone, and your arrogant assumption that when detail and documentation are omitted, all the servers in the world must send them in the response of textDocument/completion. No, the spec in no place specifies any "must". In fact, most servers ignore detail and documentation in resolveSupport because these author know what the speed trade-offs are. rust-analyzer is the only server where there's an attempt to defy this convention that plays well with pretty much every editor out there.

Does this make sense?

@SomeoneToIgnore
Copy link
Author

SomeoneToIgnore commented Dec 7, 2024

Thank you for the links and more context on the spec interpretation, now your walls of text start to look like something I wanted to see from the start.

In addition, the spec does not say the detail for a completion item has to be the same in the response of textDocument/completion and completionItem/resolve, as you have no doubt discovered already.

No, the spec in no place specifies any "must".

That is not something I understand at all: the spec that you've linked in #4609 (comment) it says the following:

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.

Which is complete opposite of what's said?
I read it as "If a client specifies completionItem#resolveSupport without the detail, it has to be provided in the textDocument/completion response and must not be changed during resolve".
What is your reading of this abstract you've re-read 10 times?

And your point is that there's an Unwritten Convention above the LSP that was upheld by "pretty much every language server" before this time and that's incorrect, even though the spec, outside these excerpts, specified that the server

By default the request can only delay the computation of the detail and documentation properties.

I find this claim enforcement odd as the whole agreement, there are outliers like vtsls which can not send detail for every completion item in a reasonable time frame, so some exceptions were existing before r-a change, and you either had to deal with that detail being resolved later before, or work around it.

After all, you say it yourself:

pretty much every language server

so there has to be something like that already.

Looking at the code snippets more, you're clearly capable of sending a completionItem/resolve request and processing its results, why cannot you either

  • take the detail out of there along the way, and use it for the particular item resolved
  • or, send a different set of client capabilities to the rust-analyzer server, that forces it to include the detail from the start?

What is the problem with following either of the examples, given that there are servers that should force you to do that?
Not knowing about them or not working with them does not deny the fact that those servers are spec-compliant in the first place, and whatever Unwritten Convention there is, it's not in the spec.

and your arrogant assumption that whendetail and documentation are omitted, all the servers in the world must send them in the response...

So that is the culprit? Sure, I have no good understanding of the code base and that what I would expect to see in the code review.
For sure, let's keep the existing servers doing what they do, it's a certain amount of them that follow the spec more rigorously that has to be worked around it seems.

your claim of network efficiency here.

Funny again, but my point is still about the other thing (which you could've asked about, instead of proposing some odd things)

First, Rust types ARE relatively long, given that r-a has to specify a fully qualified name for most of them.
Second, of I type let a = vec! where v e and c trigger a completion list query + filtering, I will get a 827 (!!!) entries with 24658 characters (!!!) returned back, in detail field alone.

This is the inefficiency I mean, and I do not think it's anyhow good to send this for nothing, if can be avoided.


What I have problem with you is your tone

Likewise!

Jumping in here, contradicting everyone above and keeping your rich context to yourself, while throwing things like "this is a bug in the server", diverting the discussion away from the spec and its interpretation towards some network craze is nothing a person would enjoy.

I would expect such complaints to appear in the first message instead of all what followed.
The fact that it took a few hours of slapping your pride in the comments, whilst you've maintained the same craze of a tone, just makes it even more hilarious: you're no better in this situation, so address part of that feedback into the mirror.

But I'm more than happy to drop that when the things are back on track as now, do you like the way it's worded above the separator? Are you capable of keeping the same constructiveness in this discussion for a bit longer, and toning down yourself?

If so, let's continue as I'm curious to see your elaborations on points 1, 2 and 3.
4 can be left alone, as that part of the discussion went to worse really fast and I do not think we should stray away from the spec interpretation issues for now, at least.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

Which is complete opposite of what's said? I read it as "If a client specifies completionItem#resolveSupport without the detail, it has to be provided in the textDocument/completion response and must not be changed during resolve". What is your reading of this abstract you've re-read 10 times?

My reading is the "(usually sortText, filterText, insertText and textEdit)" part. The author of the spec clearly is aware of the fact that the major servers for the most used languages do not honor detail and documentation. There's simply no reasonable way for them to do so. For example, for Python and Ruby, if the language servers had to compute the types for a page of completions just because the client omits detail in resolveSupport, even with optional types, every textDocument/completion request will take seconds to respond.

An additional hint for the correct reading is, both detail and documentation of a completion item model are optional, which implies they are not required, for the simple fact that there could be any language server for anything like restructured text that just don't have anything smart to put into the detail under any circumstances.

And your point is that there's an Unwritten Convention above the LSP that was upheld by "pretty much every language server" before this time and that's incorrect, even though the spec, outside these excerpts, specified that the server

By default the request can only delay the computation of the detail and documentation properties.

I find this claim enforcement odd as the whole agreement, there are outliers like vtsls which can not send detail for every completion item in a reasonable time frame, so some exceptions were existing before r-a change, and you either had to deal with that detail being resolved later before, or work around it.

Yes, which is to resolve on demand as pointed out in the code snippets. Works for every language server. Many language servers don't even return detail or documentation during a resolve, how do you think we deal with it?

why cannot you either

  • take the detail out of there along the way, and use it for the particular item resolved

That's exactly what master does and causes the issue I want to solve with #4625.

  • or, send a different set of client capabilities to the rust-analyzer server, that forces it to include the detail from the start?

Because there has never been a server that refuses to send down detail in textDocument/completion when it can. And usually when the server cannot, resolve on demand will return a detail. In the case of both not returning detail, there's nothing anyone can do.

Some additional context for you is, lsp-mode does not work by itself. There's an additional autocompletion UI package called company. If a user has installed it, it will blast out N+1 requests to resolve the items, which makes it incredibly slow. Which means, if you omit detail when detail is in resolveSupport, you actually triggered a much slower code path.

Not knowing about them or not working with them does not deny the fact that those servers are spec-compliant in the first place, and whatever Unwritten Convention there is, it's not in the spec.

Like browsers, if every web browsers followed the W3C standards back in the days, every webpage would have been broken. There's specification, and then there's reality. LSP is an intentionally ill-defined spec, lots of things are not specified to allow for interpretation, trade-offs and experimentation. When the community has settled, then the consensus go into the spec. This is how standards should be formed.

First, Rust types ARE relatively long, given that r-a has to specify a fully qualified name for most of them. Second, of I type let a = vec! where v e and c trigger a completion list query + filtering, I will get a 827 (!!!) entries with 24658 characters (!!!) returned back, in detail field alone.

This is clearly a strawman. No autocompletion UI is stupid enough to cause a massive completion candidate computation on every keystroke. All autocompletion UI deals with this problem by either debouncing or limiting the minimum number of characters before asking for a list of completion candidates. If you want to handle ill-behaved clients on the server, just implement a timeout.

Jumping in here, contradicting everyone above and keeping your rich context to yourself, while throwing things like "this is a bug in the server", diverting the discussion away from the spec and its interpretation towards some network craze is nothing a person would enjoy.

I jumped in here because it was claimed that rust-analyzer will skip detail in my PR, despite it being irrelevant, I got curious. However, I actually did my research before commenting, whereas you seem not to have looked at other prior-arts before breaking rust-analyzer for a number of editors for a whole month.

I would expect such complaints to appear in the first message instead of all what followed. The fact that it took a few hours of slapping your pride in the comments, whilst you've maintained the same craze of a tone, just makes it even more hilarious: you're no better in this situation, so address part of that feedback into the mirror.

You didn't slap my pride, I just have a lot of time to argue on the internet during a stormy Saturday night ROFL.

@SomeoneToIgnore
Copy link
Author

Because there has never been a server that refuses to send down detail in textDocument/completion when it can.

How is that even supposed to correspond to #4625 's description?

When using typescript-language-server, the initial call to textDocument/completion does not return any detail or documentation for any of the completion items.

To me, this seems to be exactly the same case as with r-a, so great that you're on top of it, fixing things.
And judging from the fact that emacs is able to use details from the resolution, seems that at least it's a known issue and had been worked on before.

I'd rather not spend your time on the spec interpretation as clearly this turns into some zealous personal opinions again:

The author of the spec clearly is aware of the fact that the major servers for the most used languages do not honor detail and documentation.

There's specification, and then there's reality.

To me, the reality seems to be different, where all but 2 editors act up and try to dictate things unnecessarily.
Again, great to see that they start to change to match the reality, and good luck with that.

@wyuenho
Copy link
Contributor

wyuenho commented Dec 7, 2024

How is that even supposed to correspond to #4625 's description?

That's a mistake of which I've correct myself here. typescript language server does return detail from textDocument/completion when it can, and the resolved completion item does not even contain the same detail, even when it originally had some detail already.

judging from the fact that emacs is able to use details from the resolution, seems that at least it's a known issue and had been worked on before

Yes, but unsatisfactorily I'm afraid. For years I had been really put off by the resolved detail suddenly popping up and messing with the completion popup dialog. I've finally found time to fix it.

To me, the reality seems to be different, where all but 2 editors act up and try to dictate things unnecessarily. Again, great to see that they start to change to match the reality, and good luck with that.

I'm curious what Helix and Neovim do that's different, but thanks.

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
5 participants