-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
Make LSP completions resolve capabilities more spec-compliant #4609
Conversation
What would be a way to do it ? How does VS Code extension for rust analyzer implement it ? |
Unfortunately, I have no good pointers at how VSCode does this, but in case of Zed we are doing a mixture of two approaches:
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. 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.
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. 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. |
Thanks for the detailed explanation!
I guess with your change (in lsp-mode) we are still not resolving additional text edits ?
They are just Emacs snapshot tests that are failing (which is expected). @jcs090218 Probably worth disabling tests in snapshot ? |
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. |
We already support resolving additional text edits and also documentation property as well. Also, instead of disabling detail resolve, we can actually resolve it asynchronously without affecting the latency for candidate list to show up. |
It's still quite confusing, but we have the experimental flag on (see below). lsp-mode/.github/workflows/test.yml Lines 21 to 29 in b3edf86
FYI, @kiennq is working on fixing in #4612. And I will push more fixes when I have time. :) |
# Conflicts: # lsp-mode.el
b4eabdd
to
12f08d1
Compare
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
Rust-analyzer seems to be the only language server does not respond with details when it is listed in The can and could should be interpreted like any other spec in that they are merely hints, as most language servers do respond with
I don't think speed is an issue for rust-analyzer. If it doesn't respond with |
To me, the point is different, so I disagree.
So, lsp-mode signals to r-a that it 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. |
There are two separate but related issues. First, rust-analyzer does not have to omit What was missing, was the ability to prepend the |
The problem of 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.
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. |
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.
Then rust-analyzer should omit |
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 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 |
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.
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 |
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. 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. |
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. |
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
is a straight up lie due to #4609 (comment)
which meant that first you've agreed on the The most interesting question is this
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. |
Ok, let me draw this out for you. Lines 457 to 463 in c3be413
Lines 611 to 690 in c3be413
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 In addition, the spec does not say the detail for a completion item has to be the same in the response of What I have problem with you is your tone, and your arrogant assumption that when Does this make sense? |
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.
That is not something I understand at all: the spec that you've linked in #4609 (comment) it says the following:
Which is complete opposite of what's said? 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
I find this claim enforcement odd as the whole agreement, there are outliers like After all, you say it yourself:
so there has to be something like that already. Looking at the code snippets more, you're clearly capable of sending a
What is the problem with following either of the examples, given that there are servers that should force you to do that?
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.
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. This is the inefficiency I mean, and I do not think it's anyhow good to send this for nothing, if can be avoided.
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. 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 |
My reading is the "(usually 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.
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?
That's exactly what master does and causes the issue I want to solve with #4625.
Because there has never been a server that refuses to send down 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
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.
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.
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.
You didn't slap my pride, I just have a lot of time to argue on the internet during a stormy Saturday night ROFL. |
How is that even supposed to correspond to #4625 's description?
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. I'd rather not spend your time on the spec interpretation as clearly this turns into some zealous personal opinions again:
To me, the reality seems to be different, where all but 2 editors act up and try to dictate things unnecessarily. |
That's a mistake of which I've correct myself here. typescript language server does return detail from
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.
I'm curious what Helix and Neovim do that's different, but thanks. |
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
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
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.