Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deref and method shadowing #42021

Closed
gavento opened this issue May 16, 2017 · 2 comments
Closed

Deref and method shadowing #42021

gavento opened this issue May 16, 2017 · 2 comments

Comments

@gavento
Copy link
Contributor

gavento commented May 16, 2017

Types implementing Deref, e.g. Rc<T>, can shadow methods of their target types. This can happen in two ways:

  • either by the impl of both types defining the same method (case A)
  • or by the method being added to either of the types via a trait (cases B and C).

While this is according to the intended Deref design, it can easily lead to confusion or bugs. It is also one case where adding impls or importing traits can silently change behavior (see below) or break the code (see e.g. #41906 and the original #41865).

We should be able to detect it and at least issue a warning.

Details and illustration

To illustrate, consider this code excerpt (full playpen). (The types here are abstract on purpose, but see #41906 and case C below for examples with Borrow.)

#[derive(Default)]
struct A<T> { inner: T }

impl<T> Deref for A<T> {
    type Target = T;
    fn deref(&self) -> &T {
        &self.inner
    }
}

#[derive(Default)]
struct B;

impl B {
    fn method(&self) { println!("B::method"); }
}

fn main() {
    let a: A<B> = Default::default();
    a.method();
}

Case A is triggered by adding impl<T> A<T> { fn method(&self) { } }. While here the behavior is least confusing of the three, it still a bad design we might be able to detect and warn about. (This is the reason Deref types such as std::rc::Rc generally only have type-associated methods.)

Case B is triggered by implementing a trait with method on A<T> as below. This is harder to both avoid (trait methods are not associated) and notice.

trait Tr {
    fn method(&self) { println!("Tr::method"); }
}
impl<T> Tr for A<T> { }

Case C is triggered similarly to B, by the code below. The difference is that the trait is implemented in another module and only gets activated by an use.

mod sub {
    pub trait Hidden {
        fn method(&self) { println!("Hidden::method"); }
    }
    impl<T> Hidden for super::A<T> { }
}
use sub::Hidden;

For this last case, I have a slightly more realistic example with use std::borrow::Borrow; triggering a different behavior with no warnings. See the playpen.

Solution

On a method call, we could inspect the entire Deref chain of types for methods of the same name and issue a warning if we find a method of the same name.

Users could resolve this with:

  • Explicit deref and call: (*a).method()
  • Explicit call without deref: A::method(&a)
  • Some #[allow(deref_shadowing)] pragma

I would see this as an important warning: activating any two of the cases A, B or C at once is a compiler error (as it should be) and the Deref coercion is very close to that from user point of view but gives no warning.

This would also resolve #41906 in a general way (are there any other related issues?)

@Mark-Simulacrum
Copy link
Member

New lints (as this proposes) need to go through the RFC process. You can also suggest a lint to Clippy (https://github.com/Manishearth/rust-clippy). If you or someone else wants to pursue this, please follow the RFC process here (or go through clippy) https://github.com/rust-lang/rfcs#before-creating-an-rfc.

@gavento
Copy link
Contributor Author

gavento commented Jun 22, 2017

Thanks, I was thinking about going through Clippy (and will ask for directions in an issue there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants