rust-lang / rust-analyzer

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

Borrow/BorrowMut auto importing completions are confusing for newcomers #13786

Open Veykril opened 1 year ago

Veykril commented 1 year ago

People tend to use the autoimporting trait method completion for borrow/borrow_mut when they meant to use the inherent method completion for RefCell causing confusion as the method now changes meaning.

See https://github.com/rust-lang/rust-analyzer/issues/8468#issuecomment-1289571080 for previous discussions

flodiebold commented 1 year ago

Making it harder to accidentally autoimport BorrowMut or other traits shadowing inherent methods is one thing (I think we should complete a qualified method call with the full qualified path in such cases, and maybe even more importantly downrank that completion heavily). Maybe we can also do something for the case where someone already imported BorrowMut though? At least we should, when completing an inherent method that's shadowed by some trait method, complete the qualified method call. And maybe we could have a note diagnostic with an assist that suggests to remove the import and replace the qualified method call by a normal one.

flodiebold commented 1 year ago

So to clarify, I would see the following possible action items:

leddoo commented 3 months ago

suggestion: rank all completions that do require an import after the ones that don't. imo, that's the common case, reusing something that's already available.

this would resolve the borrow issue, as the borrow method on RefCell is already accessible. using Borrow::borrow would require an import. (NB: if Borrow is also in scope, you could consider the "inherent methods over trait methods" completion ordering, but that's not necessary to resolve this specific issue, as we're talking about confusing auto-imports)

it would also resolve the std::intrinstics::unreachable issue, where that unsafe function is imported (in an unsafe block afaict) instead of suggesting the already available unreachable!() macro.

implementation wise, i don't know how the ranking algorithm works, but i suspect these two borrow functions have the same priority. in which case, "doesn't require an import" could be used as a tie breaker.