[stable(not yet) backport] Revert r-a completions breakage

[SomeoneToIgnore]

This PR revers recent completion-related changes in r-a, which caused nvim and helix to malfunction.
Changes reverted:

1. internal: Send less data during textDocument/completion if possible rust-analyzer#18167
2. lsp: fix completion_item something_to_resolve not being a latch to true rust-analyzer#18247
3. Use completion item indices instead of property matching when searching for the completion item to resolve rust-analyzer#18503

See rust-lang/rust-analyzer#18503 (comment) for more context

cc @BoxyUwU

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

⚠️ Warning ⚠️

• Pull requests are usually filed against the master branch for this repo, but this one is against stable. Please double check that you specified the right target!

[rustbot]

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

[BoxyUwU]

needs approval from r-a team before merging

[workingjubilee]

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

I believe it should be possible, in the rustc repo, to apply git revert to the relevant commitish for each patch? It may require doublechecking the commitish because subtrees can affect the commitish.

[SomeoneToIgnore]

So here's what I've tried: had taken a rust-lang/rust-analyzer@81636f1 commit of mine and tried to revert it:

~/work ❯ cd rust

~/work/rust stable:kb/stable ❯ git co origin/stable
Note: switching to 'origin/stable'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 8bfa98634a7 Auto merge of #133445 - BoxyUwU:stable, r=BoxyUwU

~/work/rust (8bfa986) HEAD ❯ git revert 81636f1fd119b253fedc46e65be3e373210f74dd
fatal: bad object 81636f1fd119b253fedc46e65be3e373210f74dd

~/work/rust (8bfa986) HEAD ❯ cd src/tools/rust-analyzer

src/tools/rust-analyzer (8bfa986) HEAD ❯ git revert 81636f1fd119b253fedc46e65be3e373210f74dd
fatal: bad object 81636f1fd119b253fedc46e65be3e373210f74dd

[davidbarsky]

@bors delegate=@BoxyUwU

(see rust-lang/rust-analyzer#18503 (comment))

[bors]

✌️ @BoxyUwU, you can now approve this pull request!

If @davidbarsky told you to "r=me" after making some further change, please make that change, then do @bors r=@davidbarsky

[workingjubilee]

@SomeoneToIgnore That is because, slightly confusingly, that is a different commitish:

$ git log --author "Kirill"
commit 7347beed8876e73de345dcf6c70360ee96ec3631
Author: Kirill Podoprigora <[email protected]>
Date:   Wed Nov 13 22:35:39 2024 +0200

    Add test cases

commit 81f61058519a408ca8d55e87435f092cc967de8b
Author: Kirill Podoprigora <[email protected]>
Date:   Wed Nov 13 15:31:07 2024 +0200

    Address review

commit 98a71766b8b6ece6fabb8429e5fe53805ba28b78
Author: Kirill Podoprigora <[email protected]>
Date:   Wed Nov 13 15:05:31 2024 +0200

    Add ``exact-llvm-major-version`` directive

commit e646263abc74ac6b1bba4cb5a1da99e101a59412
Author: Kirill Bulatov <[email protected]>
Date:   Mon Nov 11 16:06:55 2024 +0100

    Update the file hash

commit 572ae698be3bea00d8ed4ec927c7fa0220f9ceaf
Author: Kirill Bulatov <[email protected]>
Date:   Mon Nov 11 15:50:04 2024 +0100

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

[SomeoneToIgnore]

I've updated the hash in a separate commit: 64a8db6

The hash value is taken from the corresponding test in the r-a:

~/work/rust-analyzer master:origin/master*​ ❯ git diff
diff --git a/crates/rust-analyzer/src/lsp/ext.rs b/crates/rust-analyzer/src/lsp/ext.rs
index 6ddfe118d5..618481bbc6 100644
--- a/crates/rust-analyzer/src/lsp/ext.rs
+++ b/crates/rust-analyzer/src/lsp/ext.rs
@@ -825,8 +825,6 @@ pub struct CompletionResolveData {
     pub position: lsp_types::TextDocumentPositionParams,
     pub imports: Vec<CompletionImport>,
     pub version: Option<i32>,
-    pub trigger_character: Option<char>,
-    pub completion_item_index: usize,
 }

 #[derive(Debug, Serialize, Deserialize)]

~/work/rust-analyzer master:origin/master*​ ❯ cargo xtask tidy
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
     Running `target/debug/xtask tidy`
thread 'main' panicked at xtask/src/tidy.rs:41:9:

lsp/ext.rs was changed without touching lsp-extensions.md.

Expected hash: 6292ee8d88d4c9ec
Actual hash:   96f88b7a5d0080c6

Please adjust docs/dev/lsp-extensions.md.

stack backtrace:
   0: rust_begin_unwind
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:662:5
   1: core::panicking::panic_fmt
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:74:14
   2: xtask::tidy::check_lsp_extensions_docs
             at ./xtask/src/tidy.rs:41:9
   3: xtask::tidy::<impl xtask::flags::Tidy>::run
             at ./xtask/src/tidy.rs:15:9
   4: xtask::main
             at ./xtask/src/main.rs:56:39
   5: core::ops::function::FnOnce::call_once
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

~/work/rust-analyzer master:origin/master*​ ❯

[workingjubilee]

The revert of 572ae69 applies cleanly so we just need to refind the ishes.

[SomeoneToIgnore]

Thank you, I've found and reverted all commits properly + cherry-picked the hash change commit on top.
I have compared the diffs before force-pushing (with lease) and they seem to be identical; I've not squashed the commits to clearly show the entire process, but let me know if I should.

[workingjubilee]

I think unsquashed is fine. Perhaps reverting the merge commits would have been more to-the-point but I didn't explain how to find those (because it took a moment to remember how, honestly ^^;).

$ git log --merges --grep "SomeoneToIgnore"
commit 61dba0292dd2786ba4019090d26999d7afcfcd90
Merge: c2ffafdc9a7 e646263abc7
Author: Laurențiu Nicola <[email protected]>
Date:   Mon Nov 11 16:31:11 2024 +0000

    Merge pull request #18503 from SomeoneToIgnore/kb/better-resolve-indexing
    
    Use completion item indices instead of property matching when searching for the completion item to resolve

commit f075375e65a733ec23dd20ce8baed68059b5a713
Merge: 0790356a0ce 1a93651b8c5
Author: bors <[email protected]>
Date:   Sat Oct 19 04:55:39 2024 +0000

    Auto merge of #18335 - SomeoneToIgnore:editorconfig-glob, r=lnicola
    
    internal: Fix editorconfig glob
    
    Had been testing Zed's editorconfig branch on r-a and noticed that something was odd with yaml files.
    
    https://spec.editorconfig.org/#glob-expressions
    
    > {s1,s2,s3}
    
    > any of the strings given (separated by commas, can be nested) (But {s1} only matches {s1} literally.)

commit 3e42fc55962fa19557ee2a34de192adebce11c47
Merge: bd1c7e99d69 5544bf54e35
Author: bors <[email protected]>
Date:   Wed Oct 9 15:35:33 2024 +0000

    Auto merge of #18247 - jhgg:lsp/fix-something-to-resolve, r=Veykril
    
    lsp: fix completion_item something_to_resolve not being a latch to true
    
    while looking at #18245 i noticed that `something_to_resolve` could technically flap between true -> false if some subsequent fields that were requested to be resolved were empty.
    
    this fixes that by using `|=` instead of `=` when assigning to `something_to_resolve` which will prevent it from going back to false once set.
    
    although some cases it's simply assigning to `true` i opted to continue to use `|=` there for uniformity sake. but happy to change those back to `=`'s.
    
    cc `@SomeoneToIgnore`

commit 4800a0eef7547674b96c8462846aac4fb655c1d8
Merge: 6938084f7e7 536ac471c47
Author: bors <[email protected]>
Date:   Mon Sep 30 08:36:54 2024 +0000

    Auto merge of #18167 - SomeoneToIgnore:fat-completions, r=Veykril
    
    internal: Send less data during `textDocument/completion` if possible
    
    Similar to https://github.com/rust-lang/rust-analyzer/pull/15522, stops sending extra data during `textDocument/completion` if that data was set in the client completions resolve capabilities, and sends those only during `completionItem/resolve` requests.
    Currently, rust-analyzer sends back all fields (including potentially huge docs) for every completion item which might get large.
    
    Same as the other one, this PR aims to keep the changes minimal and does not remove extra computations for such fields — instead, it just filters them out before sending to the client.
    
    The PR omits primitive, boolean and integer, types such as `deprecated`, `preselect`, `insertTextFormat`, `insertTextMode`, etc.  AND `additionalTextEdits` — this one looks very dangerous to compute for each completion item (as the spec says we ought to if there's no corresponding resolve capabilities provided) due to the diff computations and the fact that this code had been in the resolution for some time.
    It would be good to resolve this lazily too, please let me know if it's ok to do.
    
    When tested with Zed which only defines `documentation` and `additionalTextEdits` in its client completion resolve capabilities, rust-analyzer starts to send almost 3 times less characters:
    
    Request:
    ```json
    {"jsonrpc":"2.0","id":104,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///Users/someonetoignore/work/rust-analyzer/crates/ide/src/inlay_hints.rs"},"position":{"line":90,"character":14},"context":{"triggerKind":1}}}
    ```
    
    <img width="1338" alt="image" src="https://github.com/user-attachments/assets/104f19b5-7095-4fc1-b008-5d829623b2e2">
    
    Before: 381944 characters
    [before.json](https://github.com/user-attachments/files/17092385/before.json)
    
    After: 140503 characters
    [after.json](https://github.com/user-attachments/files/17092386/after.json)
    
    After Zed's [patch](https://github.com/zed-industries/zed/pull/18212) to enable all resolving possible: 84452 characters
    [after-after.json](https://github.com/user-attachments/files/17092755/after-after.json)

[SomeoneToIgnore]

Sorry, I've had to push the correct hash and now ensured that cargo xtask tidy runs in rust-analyzer without issues.

cargo c still fails for me with the same error as on vanilla stable:

    Checking parser v0.0.0 (/Users/someonetoignore/work/rust/src/tools/rust-analyzer/crates/parser)
    Checking chalk-recursive v0.98.0
    Checking unicode-normalization v0.1.23
    Checking dirs-sys v0.4.1
    Checking unicase v2.7.0
    Checking anyhow v1.0.86
    Checking cargo-platform v0.1.8
    Checking filetime v0.2.24
    Checking fsevent-sys v4.1.0
    Checking lazy_static v1.5.0
    Checking unicode-ident v1.0.12
    Checking sharded-slab v0.1.7
    Checking protobuf-support v3.2.0
    Checking notify v6.1.1
    Checking pulldown-cmark v0.9.6
    Checking cargo_metadata v0.18.1
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 tracing-log v0.2.0
    Checking proc-macro2 v1.0.86
For more information about this error, try `rustc --explain E0599`.
    Checking toml_datetime v0.6.8
error: could not compile `parser` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

so I cannot be sure that things are correct.

[workingjubilee]

Trying the build (locally)

[workingjubilee]

@SomeoneToIgnore On my end, the following commands work and pass without issue:

• ./x.py build rust-analyzer
• ./x.py build rust-analyzer --stage 2
• ./x.py check rust-analyzer
• ./x.py test rust-analyzer

Although I didn't set the dist profile, maybe I should? But the release should be able to build it correctly. I'm assuming that the difference is a cfg being passed somewhere.

[SomeoneToIgnore]

Thank you, this way it all builds and the tests pass indeed.
Is there a way I can run the binary as something standalone?

~/work/rust stable:kb/stable ❯ ./build/aarch64-apple-darwin/stage2-tools-bin/rust-analyzer
dyld[81969]: Library not loaded: @rpath/librustc_driver-526d3d269d9ebf27.dylib
  Referenced from: <CB795F76-FF8B-3EAE-A18C-D7D5ABE7A4E5> /Users/someonetoignore/work/rust/build/aarch64-apple-darwin/stage2-tools-bin/rust-analyzer
  Reason: tried: '/Users/someonetoignore/work/rust/build/aarch64-apple-darwin/stage2-tools-bin/../lib/librustc_driver-526d3d269d9ebf27.dylib' (no such file), '/Users/someonetoignore/work/rust/build/aarch64-apple-darwin/stage2-tools-bin/../lib/librustc_driver-526d3d269d9ebf27.dylib' (no such file)
zsh: abort      ./build/aarch64-apple-darwin/stage2-tools-bin/rust-analyzer

[workingjubilee]

I think the answer is no, but how to set things up so that you are using that version so you can test the runtime interactions may be a question that @davidbarsky may be able to answer? I am not sure.

[cuviper]

cargo c still fails for me with the same error as on vanilla stable:

[...]
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`

The currently released stable does not have GuardedStrPrefix, but it is in the stable branch that will release as 1.83.0 on Thursday, via #123951.

[DianQK]

Does that make sense if we just change back to the following code?

    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);
    };

[SomeoneToIgnore]

This is exactly what I've proposed in rust-lang/rust-analyzer#18503 (comment)
so it sort of does to me, as feels simpler.

Even more sense would be to fix nvim and helix, but...

[BoxyUwU]

@bors r=BoxyUwU,davidbarsky rollup=never p=20

[bors]

📌 Commit f7c53a2 has been approved by BoxyUwU,davidbarsky

It is now in the queue for this repository.

[BoxyUwU]

@bors p=100

[bors]

⌛ Testing commit f7c53a2 with merge 90b35a6...

[bors]

☀️ Test successful - checks-actions
Approved by: BoxyUwU,davidbarsky
Pushing 90b35a6 to stable...