Skip to content

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

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

Closed
mqudsi opened this issue Jun 22, 2024 · 19 comments · Fixed by #19375
Closed

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

mqudsi opened this issue Jun 22, 2024 · 19 comments · Fixed by #19375
Labels
A-completion autocompletion C-bug Category: bug

Comments

@mqudsi
Copy link

mqudsi commented Jun 22, 2024

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)?

@mqudsi mqudsi added the C-bug Category: bug label Jun 22, 2024
@davidbarsky
Copy link
Contributor

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
Copy link
Author

mqudsi commented Jul 19, 2024

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
Copy link
Member

Veykril commented Jul 19, 2024

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.

.search_for_imports(&ctx.sema, import_cfg, ctx.config.insert_use.prefix_kind)
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
pub fn items_with_name<'a>(
. So the callers of that function should filter out items from blacklisted crates in some manner

@Veykril
Copy link
Member

Veykril commented Jul 19, 2024

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
Copy link
Member

lnicola commented Aug 18, 2024

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

@BenjaminBrienen
Copy link
Contributor

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
Copy link
Member

lnicola commented Aug 18, 2024

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

@BenjaminBrienen
Copy link
Contributor

I thought not :(

@ChayimFriedman2
Copy link
Contributor

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

So
@rustbot claim

@dtolnay
Copy link
Member

dtolnay commented Sep 26, 2024

@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
Copy link
Contributor

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

@Veykril
Copy link
Member

Veykril commented Jan 1, 2025

#18179 is now merged

@mqudsi
Copy link
Author

mqudsi commented Mar 31, 2025

If I'm understanding #19375 correctly, it requires crate authors to manually opt in to this for completions they'd like to filter out but doesn't expose any client/dev override to do so on their end?

If so, might I suggest re-opening this until a setting or other knob is added that ties a client-side config option to the backend work of #19375?

EDIT

I'm sorry, I missed that #18179 already provided that part. Thank you very much to all that made this possible!

@sunshowers
Copy link

Thanks for implementing this! As an owo-colors maintainer, should I go ahead and annotate the OwoColorize methods with ignore_flyimport? See owo-colors/owo-colors#141 for my proposed PR.

While testing it out, I noticed that even Ctrl-Space (in Zed, if it matters) didn't import the OwoColorize trait. I think it's fine for the methods to not be autocompleted, but it's a bit surprising and awkward for Ctrl-Space to not suggest importing the trait (it means that you'd have to manually write use owo_colors::OwoColorize). Not sure whether the LSP protocol supports differentiating between the two, though.

@ChayimFriedman2
Copy link
Contributor

While testing it out, I noticed that even Ctrl-Space (in Zed, if it matters) didn't import the OwoColorize trait. I think it's fine for the methods to not be autocompleted, but it's a bit surprising and awkward for Ctrl-Space to not suggest importing the trait (it means that you'd have to manually write use owo_colors::OwoColorize). Not sure whether the LSP protocol supports differentiating between the two, though.

Ctrl+Space just invokes completion, and we have no way to tell it is different from normal typing. You don't have to type the import manually, though; the quickfix (Ctrl+. usually) will still work.

@sunshowers
Copy link

sunshowers commented Apr 5, 2025

Thanks Chayim! I'll test it out and add a note to the documentation — we'll see how people react.

If, say, someone wanted to add an indication to the protocol about whether the autocomplete is "passive" (just typing in stuff) or "active" (pressing ctrl-space), how would one go about that? From a UX perspective it seems like a meaningful distinction to me, but I could be pretty off base here

@Veykril
Copy link
Member

Veykril commented Apr 5, 2025

Ctrl+Space just invokes completion, and we have no way to tell it is different from normal typing.

The LSP actual has a different completion trigger for this scenario, so we can in fact differentiate it if there is need to. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionTriggerKind

@sunshowers
Copy link

sunshowers commented Apr 6, 2025

Gotcha! I would definitely consider that FWIW, I think that would strike the right balance for owo-colors.

edit: though looking at the link it seems like maybe typing in identifiers is the same as ctrl-space?

@ChayimFriedman2
Copy link
Contributor

Ctrl+Space just invokes completion, and we have no way to tell it is different from normal typing.

The LSP actual has a different completion trigger for this scenario, so we can in fact differentiate it if there is need to. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionTriggerKind

This is not enough IMO; Manual invocation (Ctrl+Space) is also often done when completion is too slow to be automatically invoked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion autocompletion C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants