Prefer inherent methods in trait object method resolution

[Jules-Bertholet]

Fixes #51402. Inherent methods on trait objects now take precedence over methods from the trait object's trait, but these last continue to take precedence over other trait impls as before.

I don't know whether this should be feature gated or insta-stable FCP, will let T-types decide that.

r? types

@rustbot label T-types A-dyn-trait

[Jules-Bertholet]

This part is not so nice… ideally this would use the inherent impl too.

View changes since the review

[Jules-Bertholet]

I looked into making this work, but it seems non-trivial. The inherent_impls query is based on DefIds, so information about auto traits is lost

[oli-obk]

@bors try @rust-timer queue

and cratering it when the try job finishes, impl lgtm

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8a2f6fe (8a2f6fe57adadb246778b8d41925d3dc46d468cb)
Base parent: f28ac76 (f28ac764c36004fa6a6e098d15b4016a838c13c6)

[rust-timer]

Finished benchmarking commit (8a2f6fe): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -5.9%, secondary -6.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.9%	[-5.9%, -5.9%]	1
Improvements ✅ (secondary)	-6.5%	[-10.5%, -2.6%]	2
All ❌✅ (primary)	-5.9%	[-5.9%, -5.9%]	1

Cycles

Results (secondary 3.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 510.446s -> 505.256s (-1.02%)
Artifact size: 353.05 MiB -> 353.51 MiB (0.13%)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-158320 created and queued.
🤖 Automatically detected try build 8a2f6fe
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-158320 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-158320 is completed!
📊 0 regressed and 0 fixed (995573 total)
📊 4655 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-158320/retry-regressed-list.txt ↩

[theemathas]

The reference has a warning about this that needs to be corrected if this PR were to go through.

[Jules-Bertholet]

Stabilization report

This PR allows calling an inherent method on a trait object that shares a name with the method's primary trait. Up to now, this has always failed with an ambiguity error; after this PR, the inherent method shadows the trait method.

Reference PR: rust-lang/reference#2308

What is stabilized

trait Trait {
    fn foo(&self) -> i32 {
        1
    }
}

impl Trait for i32 {}

impl<'a> dyn Trait + 'a {
    fn foo(&self) -> i32 {
        2
    }
}

fn main() {
    let a: &i32 = &3;
    assert_eq!(a.foo(), 1);

    let obj: &(dyn Trait) = a;
    assert_eq!(obj.foo(), 2); // would previously give ambiguity error
    assert_eq!(Trait::foo(obj), 1);
}

What isn't stabilized

Inherent method lookup continues to not peel off auto traits on trait objects.

Continuing our previous example:

let obj2: &(dyn Trait + Send) = a;
assert_eq!(obj2.foo(), 1); // uses trait method

There is a risk that merging this PR now could make this more difficult to fix in the future, because doing so would change the behavior of code like the example above.

[oli-obk]

@rfcbot fcp merge types

[rust-rfcbot]

@oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[lcnr]

Was there any specific reason you've working on this. I agree this feels vaguely desirable, but don't have a concrete use case in mind right now.

There are a few interesting cases here 🤔

Which method to use already depends on the self type

trait Trait<T> {
    fn func(&mut self) {
        println!("trait");
    }
}
impl<T> Trait<T> for i32 {}

impl dyn Trait<u32> {
    fn func(&self) {
        println!("inherent");
    }
}

fn foo(mut x: Box<dyn Trait<u32>>) {
    x.func(); // inherent
    (&mut *x).func(); // trait
}

fn main() {
    foo(Box::new(1));
}

This candidate preference guides inference

trait Trait<T> {
    fn func(&self) {
        println!("trait");
    }
}

impl<T> Trait<T> for i32 {}
impl dyn Trait<u32> {
    fn func(&self) {
        println!("inherent");
    }
}

fn foo() {
    let x: Box<dyn Trait<_>> = Box::new(1);
    x.func(); // ambiguity error -> now constrains the infer var to `u32`
}

fn main() {
    foo();
}

Both of these feel acceptable to me and aren't really new issues

@rfcbot reviewed

Also, while I consider this (and want it to be) a bit more Types than Lang, this has definitely a stronger Lang overlap @rust-lang/lang