BorrowMut can cause confusion with Rc<RefCell<X>>

[mitsuhiko]

When BorrowMut is in scope and one has a rc: Rc<RefCell<X>> a call to rc.borrow_mut() does not deref into the inner RefCell but uses the generic call returning a &mut Rc<...>.

Something like this:

   = note: expected type `&mut std::rc::Rc<std::cell::RefCell<X>>`
   = note:    found type `std::option::Option<_>`

The compiler should ideally give feedback that the trait was used instead of the deref and how to fix this issue.

[shiMusa]

I just encountered the same issue.
Any ideas on giving feedback from the compiler?
I'm on 1.25.0-nightly (bd98fe0 2018-02-06)

[Mark-Simulacrum]

@estebank @GuillaumeGomez Any ideas on what we could do to improve the situation here? It feels somewhat hard to detect...

[GuillaumeGomez]

A bit yes...

[kristate]

Ran into this one.
rust version 1.26.0-nightly (259e4a6 2018-03-04)

[estebank]

I feel that we should just make it so that typeck resolves to the RefCell, but that would probably be a (slight) breaking change. cc @nikomatsakis what do you think about this?

The diagnostic should still be improved regardless. @mitsuhiko could we get a minimized repro case?

[mitsuhiko]

Minimized repro case:

Works:

use std::cell::RefCell;
use std::rc::Rc;

fn main() {
    let rc = Rc::new(RefCell::new(true));
    *rc.borrow_mut() = false;
}

Fails:

use std::borrow::BorrowMut;
use std::cell::RefCell;
use std::rc::Rc;

fn main() {
    let rc = Rc::new(RefCell::new(true));
    *rc.borrow_mut() = false;
}

Error:

  |
7 |     *rc.borrow_mut() = false;
  |                        ^^^^^ expected struct `std::rc::Rc`, found bool
  |
  = note: expected type `std::rc::Rc<std::cell::RefCell<bool>>`
             found type `bool`

[nikomatsakis]

I feel that we should just make it so that typeck resolves to the RefCell, but that would probably be a (slight) breaking change.

That'd be nice, but I fear that the impact might be larger than you are anticipating. I don't think that such a change is really on the table.

Possibly the BorrowMut trait should not have permitted . notation (i.e., by avoiding &mut self shorthand), but that's water under the bridge. (Though -- plausibly -- we could do something around the epoch here, where BorrowMut kind of "opts out" of being used with . notation in the newer epoch.)

I'm trying to decide how a suggestion here ought to work: there are definitely a few cases where method calls resolve to surprising places and the resulting errors can be confused. I wonder if we can find some way to capture those too.

For example, I am frequently hit by this problem, caused by a missing Clone:

fn foo<T>(x: &T) {
    let y = x.clone(); // results in `Clone` being invoked on the `&T`, rather than *failing*
}

In that case -- as here -- method resolution succeeds, but the error comes much later. It feels like we would need some sort of "tracer" that we can apply to the type so that we can see that it was derived via a method dispatch, and then trace back to see if that method dispatch may have had other possibilities.

I've been wanting to change how our type checker works -- it wouldn't necessarily have to be a massive change, basically just avoiding the refresh_type_vars_if_possible helper -- to enable this sort of tracking. Maybe a good project.

[shiMusa]

I'm not a pro on the interior workings of rust, but it seems to me to be an issue of function-overloading.

Why not use the as keyword to specify the origin of the function, e.g.
(x as Clone).clone(); or *(rc as RefCell).borrow_mut()?

PS: this would also help enormously with overloading in derived traits btw.

[nikomatsakis]

@shiMusa we have an existing notation for that, you can write (for example) Clone::clone(&x), but it's tedious.

[shiMusa]

Thanks @nikomatsakis , I forgot about that. Yeah, it's not something you'd easily think of...

[mitsuhiko]

I think the fact that a trait can shadow the methods of a type are particularly weird when working with trait objects. I came across this again in a different form when working with failure. rust-lang-deprecated/failure#211

failure implements a method on a trait with the same name as the default trait impl. I cannot find a way to disambiguate the call since I can't take the trait out of scope.

use std::fmt::Debug;

trait Foo: Debug + 'static {
    fn debug(&self) -> String where Self: Sized { format!("{:?}", self) }
}

impl Foo {
    fn debug(&self) -> String { format!("{:?}", self) }
}

trait AsFoo {
    fn as_foo(&self) -> &Foo;
}
impl<T: Bar> AsFoo for T {
    fn as_foo(&self) -> &Foo { self }
}

trait Bar: Foo + AsFoo {}
impl Bar {
    fn debug_via_bar(&self) { println!("debug: {}", self.as_foo().debug()) }
}

impl Foo for i32 {}
impl Bar for i32 {}

fn main() {
    let x: &Bar = &42i32;
    x.debug_via_bar();
}

[1414C]

Encountered the same issue as OP.
rust version stable-x86_64-apple-darwin unchanged - rustc 1.34.0 (91856ed 2019-04-10)

works:

use std::collections::HashMap;
use std::cell::RefCell;
use std::rc::Rc;

let mut hm: HashMap<&str, i32> = HashMap::new();
hm.insert("myKey1", 100);
hm.insert("myKey2", 200);

let rc = Rc::new(RefCell::new(hm));
let v1 = rc.borrow_mut().get("myKey1").unwrap().clone();
println!("read {} into v1!", v1);

error:

use std::borrow::BorrowMut;
use std::collections::HashMap;
use std::cell::RefCell;
use std::rc::Rc;

let mut hm: HashMap<&str, i32> = HashMap::new();
hm.insert("myKey1", 100);
hm.insert("myKey2", 200);

let rc = Rc::new(RefCell::new(hm));
let v1 = rc.borrow_mut().get("myKey1").unwrap().clone();
println!("read {} into v1!", v1);

error[E0599]: no method named `get` found for type `&mut std::rc::Rc<std::cell::RefCell<std::collections::HashMap<&str, i32>>>` in the current scope
  --> src/main.rs:34:30
   |
34 |     let v1 = rc.borrow_mut().get("myKey1").unwrap().clone();
   |                              ^^^

As a newcomer to Rust working through the book, this error puzzled me for a few minutes. My efforts to code a Rc<RefCell<T>> vs Rc<Box<T>> example had resulted in the addition of use std::borrow_BorrowMut to the scope of the Rc<RefCell<T>> code leading to the same problem described by the OP.

[runiq]

I've just been bitten by this. Given the proposed change in method resolution for arrays and into_iter() in the 2021 edition, I'd like to note @nikomatsakis' proposed fix for this issue in #39232 (comment):

Possibly the BorrowMut trait should not have permitted . notation (i.e., by avoiding &mut self shorthand), but that's water under the bridge. (Though -- plausibly -- we could do something around the epoch here, where BorrowMut kind of "opts out" of being used with . notation in the newer epoch.)

(Emphasis mine.) I'm reasonably certain 2021 edition is pretty much set in stone, but would this be in scope?

[oli3k]

I've also encountered this issue while learning Rust.
What can make it especially confusing is that use std::borrow::BorrowMut; will often by automatically added (by IDE plugins etc) when borrow_mut is called on the wrong thing, e.g. an Option.
I had a hell of a time figuring out why different borrow_mut calls were giving me different returns.

[Allen-Webb]

One ugly workaround I have used for this scenario is to not "use std::borrow::BorrowMut" and write the fully qualified name when BorrowMut is needed.

[noprofessional]

use

try_borrow_mut().unwrap()

instead of

borrow_mut()

can be a work around of this problem

since Rc does not implement try_borrow_mut()

[estebank]

Looking at this again, I think we should teach the type error to detect this case in particular and suggest writing *std::cell::RefCell::borrow_mut(&rc) = false; instead. In general, I think we could be able to show that multiple borrow_mut methods exist on the LHS and suggest the ones that would make the assignment typeck. That would work for any clash, even those introduced by crates.

[noprofessional]

use

try_borrow_mut().unwrap()

instead of

borrow_mut()

can be a work around of this problem

since Rc does not implement try_borrow_mut()

on second look, seem like rust-analyze automaticlly add use std::borrow::BorrowMut at begining, just remove it will solve the problem.

[arctic-penguin]

I bit my teeth out on that problem for an hour. Any indication if this is going to be fixed?

[javidcf]

Just started with Rust and got hit by this, I didn't even realise I had BorrowMut in scope, the IDE added it automatically for some reason (probably I tried to use .borrow_mut() in the wrong place first). My workaround (found by trial and error, as I didn't really understand the problem) was to just deref with *, so it would look like (*rc).borrow_mut(). But if you have an &Rc<_> you have to do (**rc).borrow_mut() etc., which starts getting confusing.

[joeyame]

Not necessarily a newcomer - I've probably been learning Rust for 6 months or so. (and >10 years of coding experience before this) I have given up on 3 projects where I encountered a need to use Rc<RefCell> and would visit countless websites to figure out how to work with X and thought I was doing the exact same thing as all the examples I saw.

Stumbled upon this issue and sure enough BorrowMut was at the top. Don't know how it got there. I'm gonna complain to rust-analyzer about that. Deleted it and now everything in the world makes sense again. I'm just gonna go around and find all the guides I was looking at and share a link to this issue because that was flabbergasting.

[nikomatsakis]

cc @rust-lang/wg-diagnostics

I am wondering if, given the new developments in the on_unimplemented hook, it would be easy to issue a warning here?

Wearing my lang hat, I also wonder about the suggestion I made a few years ago (which I had totally forgotten about), that we might allow some kind of edition-relation annotation so that the BorrowMut trait moves from &mut self to this: &mut Foo, and thus doesn't interfere with . notation dispatch.

[estebank]

#106150 provides the following suggestion:

error[E0308]: mismatched types
 --> src/test/ui/suggestions/shadowed-lplace-method.rs:9:24
  |
9 |     *rc.borrow_mut() = false; //~ ERROR E0308
  |     ----------------   ^^^^^ expected struct `Rc`, found `bool`
  |     |
  |     expected due to the type of this binding
  |
  = note: expected struct `Rc<RefCell<bool>>`
               found type `bool`
note: there are multiple methods with the same name, `borrow_mut` refers to `std::borrow::BorrowMut::borrow_mut` in the method call
 --> src/test/ui/suggestions/shadowed-lplace-method.rs:9:9
  |
9 |     *rc.borrow_mut() = false; //~ ERROR E0308
  |         ^^^^^^^^^^ refers to `std::borrow::BorrowMut::borrow_mut`
help: you might have meant to invoke a different method, you can use the fully-qualified path
  |
9 |     *std::cell::RefCell::<_>::borrow_mut(&rc) = false; //~ ERROR E0308
  |      +++++++++++++++++++++++++++++++++++++  ~

[mitsuhiko]

Another suggestion that it could spit out is to not bring the conflicting trait into the scope at all. For instance instead of importing BorrowMut one could suggest not importing it. The core issue however remains which is that code that previously worked starts breaking the moment the trait appears.

[estebank]

@mitsuhiko part of that can be added as an extra check (by looking to see if there are many methods with the same name in scope, instead of globally), but I would likely only change the wording of the suggestion at most, because I can't in this stage of the compiler verify whether the imported trait is used or not. The unused trait lint would complain by the time the code compiles, and then you're left with working but verbose code, which I feel isn't the worst outcome.

[mitsuhiko]

I’m guessing that BorrowMut is more likely in scope because someone implements it and not because someone calls it.

[estebank]

@mitsuhiko what do you think of the following?

https://github.com/rust-lang/rust/blob/2d02345b549384e66db54bc1efcb130ee9895dfd/src/test/ui/suggestions/shadowed-lplace-method.stderr#L11-L22

(Feel free to add comments to the PR so we don't spam everyone here.)

[estebank]

Closing as per #106150. I don't think we'll provide a suggestion to remove the use, but we give enough context for someone to try that out, and once the call is changed to be fully qualified, the trait would be marked as unused.