rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.24k stars 1.6k forks source link

Auto-import of modules plus inability to order completions leads to pathalogical cases #17477

Open mqudsi opened 4 months ago

mqudsi commented 4 months ago

The inability to influence the order of completions combined with RA suggesting completions from as-of-yet un-imported traits can result in the completions being rendered virtually useless by the presence of certain dependencies that pollute the RA's suggested completions list and completely overwhelm the actually desirable results (which end up being displayed after or mixed in throughout).

Here's a screenshot showing how having a dependency on color_eyre results in dozens of unwanted completions overwhelming the completions list:

image

Is there any workaround to suppress the auto-import of a particular crate for the purposes of generating completions (i.e. never suggest anything from color_eyre unless I already have the trait imported)?

davidbarsky commented 3 months ago

There isn't, but I think a configuration option that takes a list of crates to exclude from the flyimport functionality could be a reasonable thing to have.

mqudsi commented 3 months ago

Thanks, that's good to hear. I don't mean to nitpick but I was thinking of blacklisting more on a namespace (module path) rather than crate basis. But I imagine that's what you ultimately envisioned? Then again, perhaps that doesn't work with how RA is currently architectured? Would doing it on a per-crate basis speed up completions overall (because RA doesn't have to walk that crate) or since this is being applied at the auto-import level (rather than lower than that) would it not make a difference? (I am thinking it terms of dual-usage here, if this can be used both to prevent completions pollutions and to speed up RA when there are dependencies it chokes on.)

How doable do you think this is for someone not at all familiar with the codebase to tackle as a PR? Because it sounds somewhat straightforward enough to me, but I've misled myself many a time before!

Veykril commented 3 months ago

If we blacklist per crate that would be a speed improvement as we can just skip searching for paths for items from blacklisted crates (but this speed up is strictly for completions). Blacklisting module segments instead would still be a speed up but significantly less so as we would merely be skipping looking at paths from blacklisted modules (that is we keep looking on alternative paths where as the other approach doesn't even try to find anything). The crate one is certainly simpler to implement and probably what I'd prefer.

https://github.com/rust-lang/rust-analyzer/blob/9fd6c695daa1be2f9588cb5cfefbbd2dabe7d887/crates/ide-completion/src/completions/flyimport.rs#L266 is the entry point for this in import completions. You need to drill through that a bit to get to the part that calculates applicable items, which happens here https://github.com/rust-lang/rust-analyzer/blob/9fd6c695daa1be2f9588cb5cfefbbd2dabe7d887/crates/ide-db/src/items_locator.rs#L17. So the callers of that function should filter out items from blacklisted crates in some manner

Veykril commented 3 months ago

It looks straight forward to me, just a bit annoying as the required change is several layers deep and you need to thread the config through there

lnicola commented 2 months ago

We could also offer just one completion, like yellow (+100 others) when the trait is not in scope.

BenjaminBrienen commented 2 months ago

Is there a way to make folders of sorts? Group them by crate, module, or trait and select the group to expand the subcontext menu.

lnicola commented 2 months ago

I don't think the LSP supports anything like that.

BenjaminBrienen commented 2 months ago

I thought not :(

ChayimFriedman2 commented 1 month ago

Turns out I'm working on this without intending :P and already close to an end.

So @rustbot claim

dtolnay commented 1 month ago

@ChayimFriedman2 it looks as though #18179 is based on IDE-side configuration, i.e. there would continue to be nothing that the maintainer of color_eyre or owo_colors could do to fix the problem reported in this issue. They would need to instruct all users that they need to set a particular configuration in their IDE when they work on a crate that has owo_colors in its transitive dependencies.

If that is accurate, I do not think #18179 should be marked as closing this issue.

ChayimFriedman2 commented 1 month ago

@dtolnay Yes, this is accurate description. Personally I think we should have both, since this is subjective but some crates may want to default to remove their traits. Implementing a hypothetic #[rust_analyzer::do_not_complete] should be even easier than that PR, and if there is desire (i.e. crate authors will use this) I'm willing to do that, although I'm not sure how to still allow users to opt-in for traits that crate authors opted-out for.