Autoimport completion doesn't suggest std::env::var

[flodiebold]

Not sure why. This is in the rust-analyzer source e.g. in main, just typing var; std::env::vars_os and std::env::set_var are suggested, but not std::env::var. Typing var and using the autoimport code action works.

[SomeoneToIgnore]

Hmm, seems to work for me on a dummy project:

gif

As an idea, this can be caused by overwhelming number of various modules returned from import_map search if you try this on a project with a big enough amount of dependencies.

Currently, import_map::Query has a limited set of capabilities to filter the results: https://github.com/rust-analyzer/rust-analyzer/blob/410996893489f6c64b472e6128f099f1de229806/crates/hir_def/src/import_map.rs#L248-L262
We cannot apply case_sensitive for a fuzzy search, neither can we apply anchor_end: if we input va, std::env::var will be ignored, since it does not end with va, but rather with var.

So, what we end up currently after a fuzzy search are all results that contain the letters v, a, r in any place of their module path.
Currently we mitigate this by querying a bigger amount of entities and filtering out the modules:

https://github.com/rust-analyzer/rust-analyzer/blob/410996893489f6c64b472e6128f099f1de229806/crates/completion/src/completions/unqualified_path.rs#L83-L87

But for some noisy cases it's not enough.
Autoimport code action works, because it has a better input for search: it considers all input as non-fuzzy, so we can apply anchor_end and get a better results.

A assume it might be relatively straightforward to add a filter that considers the result type, since import map already stores this information in ItemInNs:
https://github.com/rust-analyzer/rust-analyzer/blob/410996893489f6c64b472e6128f099f1de229806/crates/hir_def/src/import_map.rs#L55-L58

[SomeoneToIgnore]

Now when we filter out the modules eagerly, did it become better for you?

[SomeoneToIgnore]

I've noticed the in the rust-analyzer source words only now, sorry 😞

I've run a few tests and looks like after I've excluded the modues from the search results, we're still not showing the std::env::var.

Current reasons are the limits we put: for rust-analyzer, there are 45 index entries for the word env now that we can find in the completion::unqualified_path module.

First, we limit the search results in the query itself, with 50 entries:
https://github.com/rust-analyzer/rust-analyzer/blob/b7ece77af49ce59762fc3246a4c721411efe637e/crates/completion/src/completions/unqualified_path.rs#L88

and then attempt to speed up the things even more and limit them with 20 entries:

https://github.com/rust-analyzer/rust-analyzer/blob/b7ece77af49ce59762fc3246a4c721411efe637e/crates/completion/src/completions/unqualified_path.rs#L102

and there we loose the required method.

I've looked at the way it's done in intellij-rust and looks like they don't really have the limits at all: #6614 (comment)

IMO we can safely remove the first one (50), but the second one is not that simple: rendering the import edits is our current performance bottleneck: #6633 (comment)
it is proportional to the file size and might take up to hundreds of milliseconds.

The resolve approach masks the issue for the majority of the users (since our client supports the resolve capability):
#6706
but it might not be enough.

I see the following alternatives:

• check for client capabilities and apply .take(20) only if there are no resolve completion capabilities present
• tune the algorithm to remove the noise (for instance, we can start respecting the input's case)
• add some sorting heuristics to actually take the "most important" options into the completion proposals
• just remove the .take(20) and force everybody to use LSP >= 3.16 or turn the feature off until the critical section's performance is improved

I'm leaning towards the hybrid of the first and the last approaches, but please share your thoughts, if you have any.

[flodiebold]

Why do you think the resolve approach might not be enough? Just because resolve isn't supported by all editors? I think we could maybe disable autoimport completions by default if resolve isn't supported.

[SomeoneToIgnore]

Yes, due to the other editor implementations.
The way I see the current client resolve capabilities function, it's pretty untrivial for people to guess that you have to put the additionalTextEdits capability (case matters) in a deep nested path here: https://github.com/rust-analyzer/rust-analyzer/pull/6706/files#diff-e7680026ec72b75f26636791eeec3ad9236eba32132206b7f1f11eab6f57cacaR107-R129

I fear our client easily can be the only one doing that and we will just "break" this completion for the current users.

Sure, we can disable the completions completely on such case, but then we need to desice what to do with the feature toggle: #6631
Afaik, there are no dynamic settings changes that we can do, are there?

[kjeremy]

Why do you think the resolve approach might not be enough? Just because resolve isn't supported by all editors? I think we could maybe disable autoimport completions by default if resolve isn't supported.

I think this is the right approach for things that are expensive to compute.

[SomeoneToIgnore]

🤔 But we can invert the setting and call it "Disable autoimporst on completion" or whatever and keep it false by default.
I'll think on that a bit more and add it to #6706 if it's not merged by that time or into a new PR.

[flodiebold]

Yeah, I don't know if we can dynamically change the default. I think it'd also be fine to completely disable the feature if resolve isn't supported. I feel like there's some precedent for this in the fact that some assists are disabled if there is no snippet support. It's admittedly not very discoverable, but we could at least document it, and we should push the clients to implement resolve support anyway.

As for the feature toggle, I personally think we should just have a completion.autoimport.enable setting for people who want to disable it for other reasons.