resolution ambiguity between inherent and (prelude-provided) trait methods should have more guard rails
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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