Add a lint for calling methods which have namesakes along `Deref` chains when using the `.` operator

[CloudiDust]

Currently, Rust’s . operator auto-derefs its left hand side to search for methods. However, this may lead to problems. Consider the following snippet:

Snippet 1:

use std::rc::Rc;

#[derive(Show, Clone)]
struct Answer(i32);

impl Answer {
    fn answer(&self) {
        println!("The answer of life, universe and everything is {}!", self.0);
    }
}

fn main() {
   let rc_answer = Rc::new(Answer(42));
   // Calls Answer::answer.
   rc_answer.answer();
   // Call <Rc<Answer> as Clone>::clone, but <Answer as Clone>::clone is a thing too!
   let cloned_something = rc_answer.clone();
   // Call with UFCS on an Answer with implicit dispatching.
   let cloned_answer = Clone::clone(&*rc_answer);
   // Call with UFCS on an Rc with explicit dispatching.
   let cloned_rc = <Rc<Answer> as Clone>::clone(&rc_answer);
   println!("cloned_something: {:?}.\ncloned_answer: {:?}.\ncloned_rc: {:?}.",
       cloned_something, cloned_answer, cloned_rc);
}

Which prints:

The answer of life, universe and everything is 42!
cloned_something: Rc(Answer(42i32)).
cloned_answer: Answer(42i32).
cloned_rc: Rc(Answer(42i32)).

Both Answer and Rc<Answer> has a clone method,and when Rust searchs for a clone to call for rc_answer.clone(), it chooses the one on Rc. However:

1. rc_answer.answer() calls a method on a value of type Answer, so it is natural for one to assume rc_answer.clone() would call clone on that Answer too. But the actual behaviour is surprising.

2. Rust prefers to be fairly explicit when it comes to solving ambiguities with method dispatching, It refuses to make a guess in the following snippet:

Snippet 2:

trait Foo { fn names_the_same(&self); }
trait Bar { fn names_the_same(&self); }

struct FooBar(i32);
impl Foo for FooBar { fn names_the_same(&self) {} }
impl Bar for FooBar { fn names_the_same(&self) {} }

fn main () {
   FooBar(42).names_the_same();
}

The compiler complains:

error: multiple applicable methods in scope [E0034]

So it seems that happily accepting rc_answer.clone() in Snippet 1 is not consistent with the behaviour in Snippet 2.

The solution: Forbid . and mandate UFCS in such cases.

Pros: Consistency and less surprises. Cons: Breaking change and a bit more typing.

EDIT: As discussed below, forbidding . here can be too heavy handed. So instead, adding a lint is a good alternative solution.

So what do you think?

[naicode]

I completely understand you concern but at last to me the current behavior feels quite natural, this might be because I used a lot of prototyping based programming recently, where it is normal to first look at the called objects scope for a fitting method and then iterate down the prototyping chain (in this case the Deref chain).

The current behavior is also similar to a OOP inheritance chains and let’s you create OOP like structures with overriteing/shadowing e.g. this gist.

Still even through I prefer it this way it can be confusing, maybe some kind of feature gate would make sense?

[kennytm]

Alternatively: Turn this into a lint.

[CloudiDust]

@naicode, comparing this with a prototype-chain (or a child-parent class chain in class based OOP languages) is a great way to explain this behaviour.

I think in practice, the problem with the current behaviour is that Deref is often implemented on “wrappers” or “pointers”, which are mostly “transparent” when using the . operator, so people are more likely to forget that they are still not the wrapped values themselves. In the unusual occasions that this distinction matters, mistakes are likely to happen.

But yeah, that may not matter much if we know there is a chain involved. And a chain is obviously directed, which is unlike the satuation in Snippet 2, where Foo and Bar are not related, so implicitly choosing the names_the_same method of either would be questionable.

Also, I think feature gates are not for satuations like this one.

[CloudiDust]

@kennytm, yes, a lint is a good alternative. All we need is reminding people that there is a chain in case they forget, and forbidding . here can be too heavy-handed.

[aidancully]

+1 for the lint. One of my challenges in learning Rust has been figuring out when something is used via a pointer vs. by value (these details matter to me), and the language “helping out” by automatically trying to dereference increases my confusion. OTOH, I know I’ve got particular biases here, that others won’t share. A lint lets me express my code my way, letting others express it their way.

Still, I’d argue that the lint should default to warning.

[tbu]

Please not again a noisy lint like #![warn(unstable)], people will just disable it, adding boilerplate to every project.

EDIT: It’ll be unavoidably noisy due to built-in things like clone etc.

[naicode]

+1 for a lint, but I agree with @tbu that it should not be warning due too the resulting noise when using clone on a cell. Is there currently a level like weak warning witch we e.g. in the guide could recommend to inexperienced developers for learning purpose?

[aidancully]

If I’m not mistaken, any warning generated by this lint could be quieted by adding an explicit deref operation, is that right? (If not, what’s an example of some code that couldn’t be quieted?) This is not that similar #![warn(unstable)], where there often wouldn’t be a way to avoid using the interface, and you just have to live with the compilation noise.

[naicode]

@aidancully: In the cases where a warning would be generated using a explicit deref would lead to different behaviour. This is because the function call witch will generate the warning is on the outer struct but using explicit deref means working with the inner struct.

A possible solution already shown in the original post is using the traits method explicitly. E.g.

let x = //her something implementing clone
let y = Clone::clone(&x);

[aidancully]

Right, resolve the warning by either explicit deref or UFCS, when there’s an ambiguity. I should have understood that better, sorry.

In that case, I’d like to throw out another idea: #[lang(no_implicit_deref)], which would do the obvious thing, and prevent implicit deref at all, removing the possibility of ambiguity. In that case, the original code would fail to compile, as you’d need to call (*rc_answer).answer. It’d become obvious that different operations are acting on different structures. n/m, this looks like it’s a can of worms i don’t want to get into.

[netvl]

I don’t see much of a point of this given that autoderefs are used pervasively, for example, to provide &str methods on Strings.

[gavento]

Hi there,

I would like to resurrect this topic and reconsider what can be done about method shadowing. I apologize for the long text, but I want to show some examples and propose a solution for discussion, so please bear with me.

My motivation is that I recently encountered an issue Collision of Borrow::borrow() and RefCell::borrow() where a working code using Rc<RefCell<T>> is broken by adding use std::borrow::Borrow; to the module. The problem is not so much with Borrow but with shadowing .borrow(). Even worse, I have a (synthetic) example in an open Issue where using a module changes the behavior without any error or warning.

What @tbu pointed out is a valid concern: Virtually all wrapper types with Deref implement Clone and Borrow<Self>, and some also operators such as PartialEq. So warning the user on shadowing in these cases would cause \infty false positives.

The main issue in the examples above, however, arises from different types or traits having a method of the same name, and there is also the biggest danger of different behavior (in the undetected case) or even just compile-error confusion (as seen by @spirali here).

Note that while method shadowing/overriding is common in OOP, it arises from derived children classes rather than from unrelated traits of types linked by an automatic Deref.

Proposal: Issue a warning or a lint when, in the Deref chain, a method from a trait or type is shadowed by a method from another trait or type. This will likely apply to very few cases in the existing code, perhaps only to the (unfortunate) borrow() method.

What do you think? I would very much like to participate in resolving the issue, so any guidance is welcome!

[oli-obk]

This kind of lint can be tried out in clippy. Getting it into the compiler will be easier with an existing implementation. Feel free to open an issue in the clippy repository so we can give you some hints on where to look for information in the compiler and where to start writing the lint.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.