Lint against inherent methods on types implementing `Receiver` and `Deref`

[BennoLossin]

When inherent methods are added to types that implement Receiver or Deref today, they take precedence over methods with the same name on the type they receive for or deref to. This means that it becomes impossible to call those with the method calling syntax, resulting in poorer ergonomics for the type that implements Receiver or Deref. For example:

pub struct MyBox<T>(Box<T>);

impl<T> Deref for MyBox<T> {
    type Target = T;

    fn deref(&self) -> &T {
        &*self.0
    }
}

impl<T> MyBox<T> {
    pub fn method(&self) -> u32 {
        42
    }
}

// ---

struct Struct;

impl Struct {
    fn method(&self) -> i32 {
        -42
    }
}

fn main() {
    let s = MyBox(Box::new(Struct));
    
    let x: i32 = s.method(); //~ ERROR: mismatched types
}

Since Receiver exists purely for these ergonomic reasons -- allowing methods to be called using the method syntax with custom receiver types -- this goes directly against the spirit of that trait. In the standard library it is already standard practice to not add inherent methods to types like Box and Arc and instead make them associated functions.

For this reason, there should be a lint that triggers on inherent methods on types that implement Receiver or Deref.

In the example above, the lint could look like this:

warning: inherent methods on types implementing `Deref` shadow methods on the target
  --> src/main.rs:14:4
   |
5  | impl<T> Deref for MyBox<T> {
   | ---------------------------- `Deref` implemented for `MyBox` here
...
13 | impl<T> MyBox<T> {
   | ------------------ inherent method in this `impl` block
...
14 |     pub fn method(&self) -> u32 {
   |     ^^^           ^^^^^ publicly accessible inherent method declared here
help: consider making `method` an associated function:
   |
14 |     pub fn method(this: &Self) -> u32 {
   |                   +++++++++++

The lint should only fire on methods that are pub (or the maximal publicity of the type, so a pub(crate) type would also trigger on pub(crate) functions).

Another important consideration is that Deref is often used for the subtyping pattern or for dereferencing to a "morally contained type". In this case, the target type is not a generic, but a concrete type or a rigid type containing a generic (for example [T]). For this reason the lint for Deref should not trigger in those cases. The rationale is that since one knows the concrete type, avoiding name collisions is much easier. It also avoids false positives, since in the subtyping pattern, both types want to have inherent methods. In particular this will ensure that the Deref implementation for Vec will not trigger the lint.

Zulip discussion

[BennoLossin]

@rustbot label +C-feature-request +A-lints +A-lang

[rustbot]

Error: Unknown labels: A-lang

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[BennoLossin]

@rustbot label +C-feature-request +A-lints +T-lang

[workingjubilee]

My memory is that this pattern is quite deliberately used in many types in the standard library, to provide better implementations on the wrapping type than the deref'd-to type has access to. This lint does not make sense if my memory is correct, because it would be advising people to not reproduce patterns we make great use of.

[BennoLossin]

I quickly grepped through the code and found the following functions where I think the lint would trigger:

• std::pin::Pin
  • as_ref
  • as_mut
  • as_deref_mut
  • set
• std::collections::binary_heap::PeekMut::refresh (unstable)

Have I missed any?

During this search, I found many functions written using the associated style fn _(this: &mut Self, ...) (for example the stable binary_heap::PeekMut::pop(this: Self)). So it seems like sometimes the rationale is to avoid shadowing? Is there an official t-libs policy for this somewhere?

[workingjubilee]

Vec::is_empty?

Extremely trivial example, I know, my point is, I guess, that there is a distinct difference between types that deref to T and types that deref to [T].

[workingjubilee]

I'm not aware of an official policy.

I believe we also implement as_mut_ptr and as_ptr directly on Vec for safety reasons.

[BennoLossin]

Maybe I didn't write the rule with the best words possible, but my intention was that Vec's Deref impl is covered by it:

Another important consideration is that Deref is often used for the subtyping pattern. In this case, the target type is not a generic, but a concrete subtype. For this reason the lint for Deref should not trigger in that case. The rationale is that since one knows the concrete type, avoiding name collisions is much easier. It also avoids false positives, since in the subtyping pattern, both types want to have inherent methods.

So since <Vec<T> as Deref>::Target is [T], which is not a generic, but a concrete type, we don't lint on it. We essentially obtain the rigid type of the target and if it is a generic, we apply the lint.

[workingjubilee]

Ah, I see. I wasn't entirely sure what you meant there and should have asked, but it was because I usually don't refer to things with unbound type variables in them as concrete types. That is an ambiguous usage when compared against a "fully concrete" type (shallow versus deep normalization).

I am still unsure if this is a good lint but I am now more ambivalent then since it is not whatever I thought it was.