Collision of Borrow::borrow() and RefCell::borrow()

[gavento]

The trait std::borrow::Borrow and type std::rc::RefCell both have a method borrow(). This leads to ambiguity, possibly hard-to-understand errors and most importantly breaking unrelated code by adding use std::borrow::Borrow;.

Consider the following code:

use std::rc::Rc;
use std::cell::RefCell;
// Uncommenting causes compile error
//use std::borrow::Borrow;

pub struct S {
	flag : bool
}

type SCell = Rc<RefCell<S>>;

fn main() {
    // Type annotations just for clarity
    let s : SCell = Rc::new(RefCell::new(S {flag: false}));
    let sb : &S = &s.borrow();
    println!("{:?}", sb.flag);
}

(Rust playground link)

This code compiles and works as intended. But when you add use std::borrow::Borrow;, the compiler complains:

rustc 1.17.0 (56124baa9 2017-04-24)
error[E0277]: the trait bound `std::rc::Rc<std::cell::RefCell<S>>: std::borrow::Borrow<S>` is not satisfied
  --> <anon>:13:21
   |
13 |     let sb: &S = &s.borrow();
   |                     ^^^^^^ the trait `std::borrow::Borrow<S>` is not implemented for `std::rc::Rc<std::cell::RefCell<S>>`
   |
   = help: the following implementations were found:
             <std::rc::Rc<T> as std::borrow::Borrow<T>>

The problem is that the method call s.borrow() is applied to Rc which is an instance of Borrow rather than to the contained RefCell as intended (and as working in the example above), and the error just complains that this borrow is not possible (which is fine).

This is mentioned by @spirali in related #41865, but this issue deals with a different part of the problem. (Namely that if you remove the type annotation from let sb : &S = ..., the error message will complain about ambiguity of the Borrow::borrow() return type in a confusing way.)

I would propose to change the method name for RefCell::borrow to something else (a painful change, but leaving such bugs around things implementing the Deref trait might be even worse for the language long-term) and avoid this naming collisions whenever possible (is there a guideline for that?). Any other solutions?

[cuviper]

It's an ugly problem with auto-deref, but you can still write it manually as (*s).borrow() or RefCell::borrow(&*s). Are there cases where accidentally calling <Rc<T> as Borrow>::borrow() would still compile and give unintended results?

FWIW, this conflict is also why Rc's own methods are all associated rather than on self, so they don't shadow the inner type.

[gavento]

@cuviper Yeah, (*s).borrow() is how we solved it, and you are right that I cannot think of a practical example where s.borrow() would silently compile in both cases (depending on Borrow being in scope or not).

While solvable with the current syntax, I would say (from my own experience) that it is rather confusing, especially since it does apply to a common construct (e.g. Rc<RefCell<X>> and borrow() as above).

Any thoughts on adding a hint for this kind of error for Borrow (and perhaps related traits)? I would be happy to implement it, but I would appreciate some guidance, or at least pointers. (I am not familiar with rustc development but would love to help.)

[cuviper]

Adding a hint sounds great, but I can't guide you as I haven't yet learned how to do this myself. :)

[durka]

We can add a #[rustc_on_unimplemented] hint to std::borrow::Borrow. Concise wording will be tricky though.

[gavento]

@durka Thanks for the hint and a pointer, but I think this is too general: whenever Borrow is used, every type implements Borrow<T> for T and so #[rustc_on_unimplemented] would get triggered on every .borrow() call missing the appropriate impl Borrow<Target> for T and not just on this particular Deref-caused method shadowing.

Any other ideas? Perhaps some hint message would help even in the more general cases but I do not see the right wording.

[Mark-Simulacrum]

I have relabeled this as a diagnostics bug -- I wonder if it could make sense to just lint on the use std::borrow::Borrow import since that's always(?) not what you want.

[ta32]

IDE's like the clion rust plugin can automatically import std::borrow::Borrow when you are using the auto-complete leading to confusion when code suddenly stops compiling when it looks the same. Its not easy to notice an extra import was added.