Use completion item indices instead of property matching when searching for the completion item to resolve

[SomeoneToIgnore]

Part of #18363
Closes #18501

Previously, completions for resolution were re-queried with the same completion parameters, and then picked by matching their properties as

let Some(mut resolved_completion) = resolved_completions.into_iter().find(|completion| {
        completion.label == original_completion.label
            && completion.kind == original_completion.kind
            && completion.deprecated == original_completion.deprecated
            && completion.preselect == original_completion.preselect
            && completion.sort_text == original_completion.sort_text
    }) else {
        return Ok(original_completion);
    };

this is incorrect, as

1. completion.deprecated == original_completion.deprecated check may never success due to the property being unresolved initially (other properties are fine and are not resolved)
2. Equal labels and other parameters can happen for various traits with the same name, that might have different imports associated with them.

So, instead, follow a less fragile approach: since we resolve completions for the same document state and same query, we can rely on the Vec<CompletionItem> order to return the same set of the completion items, in the same order.

This is also fragile, but seems to work and definitely works better than the previous version.

---

It would be great to see if we can derive a better "id" for such items, but I'm clueless here.
If this approach does not seem to be good, we'd better follow #18501 path and revert this entirely (bringing back tons of useless json in responses though)

cc @traviscross

[lnicola]

We could also include detail in the comparison, right? But I hope the completion order is deterministic, let's give this a try and see if it breaks horribly.

[SomeoneToIgnore]

We could also include detail in the comparison, right?

Alas, not really, as we do resolve detail if asked:

rust-analyzer/crates/rust-analyzer/src/lsp/to_proto.rs

Lines 339 to 344 in aabab29

	let detail = if fields_to_resolve.resolve_detail {
	something_to_resolve |= item.detail.is_some();
	None
	} else {
	item.detail.clone()
	};

which means that the "original item" will always have None in this case, and the non-original one will have Some(...).

Due to the same idea, && completion.deprecated == original_completion.deprecated now fails for e.g. Emacs + lsp_mode

[traviscross]

We'll want to backport this commit to beta.

[DianQK]

I encountered an autocompletion error in Neovim and bisected to this PR. In the following function, when I type "ma" and select max(...), the completion result ends up being clamp. Can anyone else reproduce this?

fn hello(s: String) {
  s. // type ma
}

[lnicola]

I don't have a working nvim setup, but I couldn't reproduce this either of Code, Helix, or Zed.

[SomeoneToIgnore]

@DianQK does hrsh7th/cmp-nvim-lsp#75 fix things for you?

[DianQK]

@DianQK does hrsh7th/cmp-nvim-lsp#75 fix things for you?

I can confirm that it doesn't resolve this, but it has resolved another problem where function parameters were not being auto-completed. It changes s.clamp to s.clamp(min, max) that "min" and "max" are placeholders.

[SomeoneToIgnore]

I have no clue what Nvim does so special, alas, but good that it fixes some things.

From my side, can also add that emacs + lsp-mode worked what I was testing this PR, and Zed works.

[DianQK]

I don’t fully understand the code or know exactly where the issue lies. However, I believe that the root cause is not in this PR.

From the logs, I observed that when Nvim switches between completion candidates, it sends a textDocument/didChange request. Afterward, handle_completion_resolve returns different resolved_completions results. I noticed that the value of CompletionAnalysis::NameRef changes from 35..36 to 35..48.

I maybe have resolved the issue by modifying cmp.select_next_item() to cmp.select_next_item({ behavior = cmp.SelectBehavior.Select }). Hope this will be helpful in addressing the problem with either nvim or rust-analyzer.

For reference: https://github.com/hrsh7th/nvim-cmp/blob/f17d9b4394027ff4442b298398dfcaab97e40c4f/lua/cmp/view/native_entries_view.lua#L130.

[SomeoneToIgnore]

I observed that when Nvim switches between completion candidates, it sends a textDocument/didChange request

So, every time you scroll through the completions, a new "edit" request is issued?
Seems very odd to me (but hard to say without knowing the nvim reasoning for this), I would expect that simple completion items scrolling has to only resolve items, and not "change" the text document.

rust-analyzer completion resolution relies on the fact that completions being resolved are from the same, unchanged document as the original, unresolved completions came.

This is according to the example from the LSP spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion

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. When the item is selected in the user interface then a ‘completionItem/resolve’ request is sent with the selected completion item as a parameter. The returned completion item should have the documentation property filled in.

I have a few questions on top:

• what is the reason for sending that textDocument/didChange between selections?
• if it sounds odd to you too, can you open the PR/discussion with all that in the cmp-nvim-lsp repo, as I lack the knowledge on how to use and configure nvim to repro the issue?
• what's going on with cmp-nvim-lsp and its maintainers in general? Seems that things were broken around a month and no one even bothers to fix that or merge the Make LSP completions resolve capabilities more spec-compliant hrsh7th/cmp-nvim-lsp#75 PR??

[lnicola]

@DianQK can you take a look at #18536? I'm not sure what I'm supposed to see there, but it sounds a little similar.

[DianQK]

if it sounds odd to you too, can you open the PR/discussion with all that in the cmp-nvim-lsp repo, as I lack the knowledge on how to use and configure nvim to repro the issue?

I don't know about the other questions. Since this is an option, I don't think this is an issue. I could create a discussion about this.

[DianQK]

@DianQK can you take a look at #18536? I'm not sure what I'm supposed to see there, but it sounds a little similar.

Looks the same. I can reproduce it on println!.

[tgross35]

I think some swift action is needed here - this was backported to 1.83 which is due for release in three days (@traviscross @cuviper), but it seems to have introduced a pretty severe bug:

• Autocompletion values change as you hover over them #18536
• [bug] LSP autocompletes with completely different completion from what was selected #18558
• Autocomplete suggestion changes to something random once selected #18547

[cuviper]

@BoxyUwU is handling the final release process this time. The stable candidate was already prepared in rust-lang/rust#133445, but it should be possible to respin that, if the rust-analyzer team figures out what they want soon.

[ChayimFriedman2]

I think 3 days is definitely not enough to cook a proper fix. We should revert #18167 and try again after stable (at least, revert for stable). @lnicola @SomeoneToIgnore your thoughts?

[lnicola]

I didn't really understand the new issue (the screencaps look fine to me?). Is it worse than #18363?

[ChayimFriedman2]

From what I understand the completions users see is not what they get when they accept the completion, which seems pretty severe to me.

Also I don't want to revert #18503, but the original PR, #18167 (temporarily).

[flodiebold]

This looks like what is arguably a client issue to me. Neovim and Helix seem to edit the text document when selecting completions and afterwards try to resolve the previous completions; at least that's what I suspect. The spec doesn't outright say this is forbidden, but I don't think it makes sense for completion items to still be valid after modifying the document.

Still, reverting the PR on stable might be a good idea.

[ChayimFriedman2]

I think given how little time we have, reverting the PR is the safest option. For the next release we can decide whether it's client's fault and we want to re-introduce the changes as-is, or whether we want to do something to help them.

[SomeoneToIgnore]

Do note that you'll have to revert both related PRs then, as they cause different kinds of troubles.

nvim has a way to fix this (according to #18503 (comment)) but no one on the nvim side seems interested, as even hrsh7th/cmp-nvim-lsp#75 is not merged yet, and this is a fix of a clear violation of the LSP spec on the nvim side.

I suspect the current editing situation is closer to the client error too, as all other editors including Kate get it right.

[workingjubilee]

@SomeoneToIgnore As you seem to know what the correct reverts are to post, can you PR them to rust-lang/rust:stable and ping @BoxyUwU?

[SomeoneToIgnore]

I can sure try in a few hours when back to the keyboard.

Another (IMO, better) proposal could be a forward-fix, that forces r-a to resolve all 3 problematic, completion-related, properties always (as it was doing before along with all other properties), leaving the rest bloated ones like docs to be resolvable still.

Would that be a feasible middle-ground and something people are willing to try?

[workingjubilee]

No, there will be no "forward-fixes" right before rolling the stable release.

[workingjubilee]

To be clear, if the revert is accepted, it will not be with 3 days on the clock. It will be with essentially negative time on the clock: the process of generating stable release artifacts is already underway. So any revert lands only at Boxy's discretion.

• The ideal time to post a revert would have been around Nov 17-Nov 20, against the beta branch, when the problem was identified and before the stable branch was cleaved off.
• A to-beta fix should usually be a minimal fix, to minimize collateral damage.
  • This means it should usually be a revert.
  • There are, however, cases where the revert is significantly larger or cannot be applied.
  • There are also cases where a large diff is a simpler logical change.
  • Exercise your judgement here.
• This does imply nightly and beta fixes can be split between forward-fixes and reverts.

[BoxyUwU]

If the r-a team thinks a revert would be a good idea and a PR to stable is opened very quickly then it's definitely doable. There is really not much time though since we have to have time to rebuild the artifacts which only really leaves a day or so to decide if this is actually something the r-a team would want to backport and then actually get the PR opened.

I would not want to land anything other than a revert of the involved PRs though as if there is anything wrong with the backport it would be not feasible to backport another thing.

[BoxyUwU]

cc @rust-lang/rust-analyzer if someone could make an explicit decisive decision as to whether a revert should be backported that'd be great :3

[SomeoneToIgnore]

TIL that rust repo does not seem to use r-a as a submodule, so I had to create reverse-patches manually, and manually apply them.

Here's the PR: rust-lang/rust#133476
I could not build it on regular stable due to a compilation error, so I cannot verify that my patches had applied correctly: I have a suspicion that the lsp_ext.rs-related test will fail as the file hash is not correct.

The compilation error:

src/tools/rust-analyzer (8bfa986) HEAD ❯ git co origin/stable
HEAD is now at 8bfa98634a7 Auto merge of #133445 - BoxyUwU:stable, r=BoxyUwU

src/tools/rust-analyzer (8bfa986) HEAD ❯ pwd
/Users/someonetoignore/work/rust/src/tools/rust-analyzer

src/tools/rust-analyzer (8bfa986) HEAD ❯ cargo c
    Checking parser v0.0.0 (/Users/someonetoignore/work/rust/src/tools/rust-analyzer/crates/parser)
    Checking tt v0.0.0 (/Users/someonetoignore/work/rust/src/tools/rust-analyzer/crates/tt)
    Checking chalk-solve v0.98.0
    Checking profile v0.0.0 (/Users/someonetoignore/work/rust/src/tools/rust-analyzer/crates/profile)
    Checking rayon-core v1.12.1
    Checking ra-ap-rustc_pattern_analysis v0.68.0
    Checking typed-arena v2.0.2
    Checking scoped-tls v1.0.1
    Checking line-index v0.1.1
    Checking thiserror v1.0.63
    Checking tinyvec_macros v0.1.1
    Checking unicode-bidi v0.3.15
    Checking option-ext v0.2.0
    Checking percent-encoding v2.3.1
    Checking same-file v1.0.6
error[E0599]: no variant or associated item named `GuardedStrPrefix` found for enum `TokenKind` in the current scope
   --> crates/parser/src/lexed_str.rs:191:41
    |
191 |                 rustc_lexer::TokenKind::GuardedStrPrefix => {
    |                                         ^^^^^^^^^^^^^^^^ variant or associated item not found in `TokenKind`

    Checking tinyvec v1.8.0
    Checking cfg v0.0.0 (/Users/someonetoignore/work/rust/src/tools/rust-analyzer/crates/cfg)
    Checking home v0.5.9
    Checking form_urlencoded v1.2.1
    Checking rayon v1.10.0
    Checking walkdir v2.5.0
    Checking toolchain v0.0.0 (/Users/someonetoignore/work/rust/src/tools/rust-analyzer/crates/toolchain)
For more information about this error, try `rustc --explain E0599`.
error: could not compile `parser` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

I'm attaching the patches I've used, and if I was mistaken along the way, I'd be very grateful for a helping hand.
At worst, now there's a list of all related PRs to revert.

1.patch
2.patch
3.patch

[davidbarsky]

@BoxyUwU revert away. I'll bors r+ rust-lang/rust#133476.