rust-lang / rust-lang/rust

resolution ambiguity between inherent and (prelude-provided) trait methods should have more guard rails

Open
#139,732 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-method-lookup A-trait-system C-bug T-compiler T-lang T-types
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

Here are two similar pieces of code.

version 1 (playground):

#[derive(Debug, PartialEq, Eq, PartialOrd)]
struct Xy { x: i32, y: i32 }

#[allow(dead_code)]
impl Xy {
    // imagine a slew of other methods here
    fn max(&self, other: Xy) -> Xy {
        use std::cmp::max;
        Xy { x: max(self.x, other.x), y: max(self.y, other.y) }
    }
    // and imagine a slew of other methods here as well
    // (thus the `allow(dead_code)` above).
}

fn main() {
    let unit_x = Xy { x: 1, y: 0 };
    let unit_y = Xy { x: 0, y: 1 };
    let sum = unit_x.max(unit_y);
    println!("{sum:?}");
}

version 2 (playground):

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
struct Xy { x: i32, y: i32 }

#[allow(dead_code)]
impl Xy {
    // imagine a slew of other methods here
    fn max(&self, other: Xy) -> Xy {
        use std::cmp::max;
        Xy { x: max(self.x, other.x), y: max(self.y, other.y) }
    }
    // and imagine a slew of other methods here as well
    // (thus the `allow(dead_code)` above).
}

fn main() {
    let unit_x = Xy { x: 1, y: 0 };
    let unit_y = Xy { x: 0, y: 1 };
    let sum = unit_x.max(unit_y);
    println!("{sum:?}");
}

version 1, when run, prints this:

Xy { x: 1, y: 1 }

version 2, when run, prints this:

Xy { x: 1, y: 0 }

(The only difference between the two blocks of code above is that one included Ord in the derive; the other did not.)

I expected to see this happen: The compiler should issue a warning at the point where fn max is defined, saying that the Ord trait, which is part of the prelude (and thus does not need to be explicitly imported at the call site to take precedence over fn max), is going to end up being called on any use of this method that doesn't explcitly disambiguate. In other words, the compiler should proactively check for resolution ambiguities based on what prelude-provided traits are implemented and what methods those traits provide.

A lint that would look something like:

You have defined the inherent method `fn max(&self, other: Xy) -> Xy` on the `Xy` type
which also implements the `Ord` trait. Any call of the form `xy.max(other)` will not call 
your inherent method. You may want to choose a different name for this method, or 
prominently indicate in its documentation that one must use the unambiguous call 
syntax `Xy::max(&xy, &other)` to invoke it.

Instead, this happened: The compiler only "warns" via the dead code lint.

I can understand an attitude that says "this is your own fault for ignoring the dead code warning", but I do not consider this to be quite the same as a dead code problem.

The ambiguity in method resolution here is dangerous, because its all too easy to overweight the immediately visible inherent definition of fn max(&self, ...), while overlooking the prelude's provision of Ord with its own definition of fn max(&self, ...) that will end up taking precedence here.

Basically, I am claiming that while the dead code lint can be correlated with more serious problems (and thus one can argue that users should be treating instances of the dead code lint as seriously as any other diagnostic issue), I think that in practice we can do a better job of identifying cases like this where the choice of method name is very likely to cause a resolution ambiguity for all reasonable uses of the method.

Meta

Rust versions I tested:

1.86.0; 1.88.0

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce both Rust Playground examples using the reported Rust versions and compare method resolution when Ord is derived. Then trace the compiler's method-resolution and linting behavior to determine where a diagnostic for the inherent method and prelude-provided Ord::max could be added; done means the ambiguity receives a clear warning without relying only on dead-code reporting.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.