Trait auto import may mistake inherent method calls for trait method calls

[kotauskas]

A realistic use case which sometimes reproduces the issue:

use std::{rc::Rc, cell::RefCell};

let my_thing = Rc::new(RefCell::new(None));
send_it_somewhere(Rc::clone(&my_thing));
*my_thing.borrow_mut() = Some("Sample data!");

Here, typing out the .borrow_mut() will import std::borrow::BorrowMut, which will try to use the implementation of BorrowMut on Rc<RefCell<Option<&'static str>>> because it'd take priority over deref coercion of Rc down to the RefCell with its own .borrow_mut(), thus mutably borrowing the variable storing the Rc rather than its content. Assigning to that fails with a type mismatch (expects you to put a new Rc there but finds an Option<&'static str> which should itself be wrapped in an Rc).

This does not reliably reproduce if you encounter the issue and remove the import (typing out the same code won't trigger the BorrowMut auto import again), but one thing I noticed is that this happens only when hovering over the .borrow_mut() you just typed out shows the documentation from the BorrowMut trait rather than the method. However, even when you remove the import it added and it doesn't add it again (the bug essentially disappears), hovering over .borrow_mut() will still show the BorrowMut documentation, even though BorrowMut isn't in scope and the method call refers to RefCell::borrow_mut().

I'm using the VSCode extension (matklad.rust-analyzer, not the official one) if that matters at all.

[ogoffart]

I find this issue very frustrating as I work in a codebase that uses lots of Rc<RefCell> and somehow, vscode often select the borrow() that uses the std::borrow::Borrow by defaut

On this screenshot, I'm just typing my_thing.bor and it preselect the wrong one.

So if we're note very careful, then rust analyzer will add the use std::borrow::Borrow and suddenly there are lots of confusing errors in the module for all the usages of borrow or borrow_mut.

[Veykril]

I assume your editor of choice is VSCode here.
What is your editor.suggestSelection setting set to? Given the ordering shown here I assume it is set to recentlyUsed or recentlyUsedByPrefix as otherwise it would always pick the first one. Interestingly enough I do not get this behaviour, even if I explicitly picked the use variant first, I always get the non-used one preselected first.

[Veykril]

Also as for the original issue here, I don't think what the issue describes is actually still happening for us(we now show both completions separately with a (use TraitPath) label).

[ogoffart]

What is your editor.suggestSelection setting set to?

Right, it is recentlyUsed.
And I guess that's why I was able to reproduce it so easily after it hapenned last time.

So I think the problem is that borrow() is always there even if rust-analyzer don't know the exact type.
And it is probably the reason why I somehow end up often getting the wrong borrow().

I think it is unlikely that someone want to access these function, so if the type is not known, perhaps leave them out if the trait is not yet in scope?

[Veykril]

Ah ye that one is #10454 I believe.

[Veykril]

Closing as the original issue seems to have disappeared and the issue in the comment being tracked inother issue.

[kornelski]

I've ran into the exact same issue, and it's been bewildering.

I don't think it should simply be closed in favor of #10454, because the problem here is auto-import of traits, not method suggestion. Suggesting borrow_mut() on an "unknown" RefCell is perfectly fine — it will compile and work. But auto-import of BorrowMut unexpectedly breaks RefCells, and that is problematic in its own way.

[flodiebold]

Hmm @kornelski I think it would be helpful if you could provide a simple reproduction of the problem.

[kornelski]

I've had a struct like:

struct Foo {
   cell: Rc<RefCell<i32>>,
}

and I wrote:

self.cell.borrow_mu<tab>

rust-analyzer auto-completed borrow_mut(), but not did not understand it as RefCell::borrow_mut(), but as BorrowMut::borrow_mut() and inserted use std::borrow::BorrowMut; automatically for me at the top of the file (which I didn't notice).

Instead of RefMut<i32> I've ended up with &mut Rc<RefCell> and was totally confused why RefCell didn't seem to do anything.

[mejrs]

Can this issue be reopened? I'm very active teaching people rust, and this pain point comes up almost daily. It has not been solved.

[Veykril]

Do you have a minimal reproduction? It looks fixed to me

[mejrs]

Do you have a minimal reproduction? It looks fixed to me

That is the reproduction. People click on the second option, and then they get weird errors everywhere.

I think we just have a difference of opinion about what "fixed" means. What I see is that people (especially newcomers) still get confused by accidentally importing BorrowMut. You can see this for yourself: go to the rust community discord and search for "BorrowMut". You'll find about a hundred results, many of which are something like "Remove the use std::borrow::BorrowMut; you have at the top of your file.`

To me the correct fix seems to be to special-case Rc<RefCell<..>> to not suggest using BorrowMut.

[Veykril]

That does not sound like a fix to me, special casing one specific case here that is. There are probably more occasions where this could occur, BorrowMut is just one of the more common ones. I'm open for suggestions on how to prevent people from making this mistake or making it less likely for them, but I am not too happy about special casing like this (also in some world there is gonna be a certain someone who will complain about exactly this completion missing for some odd reason).

(On an unrelated note, I am also surprised as to why especially newcomers jump the gun with Rc and RefCell 😕)

[mejrs]

borrow_mut seems to be uniquely confusing like this. I can't off the top of my head recall any other cases where someone got confused by trait vs inherent methods.

[Veykril]

Well, why do people only get confused here? It's in no way special in regards to other inherent vs trait method collisions really.

One thing that came to mind right now though, since I assume part of the confusion for newcomers at least could come from them not understanding what the (use path::to::Trait) part means. We have plans to do a pop up on install of the extension, we could probably explain the few completion gimmicks there. Would that help?

[kornelski]

People won't read a random popup.

Installation time is wrong time to explain a problem users haven't run into yet, without relevant context. Even if you make users read it, there's very little chance they will recall this when they run into the problem.

You could show the warning when users select the BorrowMut trait for the first time, but it's not as good as never suggesting the trap in the first place.

[Veykril]

Well from the looks of it people either don't read what completions say either or they don't know what the (use path::to::Trait) part in completions mean. I am not saying we should describe the concrete issue in that page, I am saying we should describe what the different completions mean. I mean there has to be a reason why people pick the wrong one here right?

[mejrs]

We have plans to do a pop up on install of the extension, we could probably explain the few completion gimmicks there. Would that help?

I agree with @kornelski here; a popup listing some gotchas is a bad idea, especially when you can avoid having that gotcha in the first place.

I mean there has to be a reason why people pick the wrong one here right?

The main reason, I think, is that in general people tend to not read things.

[tvallotton]

Well, if you think about it, the BorrowMut auto-completion is always wrong, it instead should use universal function call syntax. So I think there are two reasonable options: remove the suggestion, or refactor the code to use UFCS. It would look something like this:

shared_cell.borrow_m<tab>

And it'd be transformed into

BorrowMut::borrow_mut(shared_cell)

Which would probably be annoying, but it would prevent those confusing ambiguity errors.

[Veykril]

BorrowMut::borrow_mut(shared_cell) would imply that BorrowMut is in scope which is the cause of the problem here so that wouldn't work.

The main reason, I think, is that in general people tend to not read things.

Fwiw this seems really bad? I would expect people read what they are about to complete since it can be something completely different.

[Veykril]

Let's move the discussions over to #13786, this issue wasn't about the confusion per se (the issue was about the inherent completion not appearing at all). Meant to open a new issue for this earlier but I got sidetracked, sorry. Though I am likely to yield now to special case and remove this collision, clearly issue here is with rust having made these methods opposed to just functions on the trait.