Prefer inherent methods in trait object method resolution

[Jules-Bertholet]

View all comments

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

[traviscross]

Thanks for the ping. For my part, I agree this is desirable. On process, as @lcnr suggested, the lang overlap here feels meaningful, and I would have preferred a dual FCP.

Cc @dtolnay: This affects question 10 on the quiz.

Nominating to discuss for visibility.

[nikomatsakis]

So, in the lang meeting, I expressed some skepticism of this change. (Also, I agree this is at the intersection of lang + types.)

My sense is that the language already says that dyn Trait has all the methods from Trait as inherent methods, for better or worse, as demonstrated in this example:

mod bar {
    pub trait Test {
        fn test(&self);
    }
    
    impl Test for () {
        fn test(&self) {
            println!("Test");
        }
    }
}

pub trait AnotherTest {
    fn test(&self);
}

impl<T: ?Sized> AnotherTest for T {
    fn test(&self) {
        println!("AnotherTest");
    }
}

fn example(d: &dyn bar::Test) {
    // First, `Test` is not imported, so this trait method is not callable, but second, we wind up `d.test()`
    // not `AnotherTest::test`
    d.test();

    AnotherTest::test(d); // prints "AnotherTest"
}

fn main() {
    example(&()); // prints Test::test
}

I think the bug is that we permit something like this in the first place:

impl dyn Foo {
    fn test(&self);
}

In general, I regret permitting inherent methods on dyn types entirely, as I do not consider them "nominal types". We basically treat them as "nominal types according to their primary trait", but it leads to weirdness like methods defined on Foo + Send, and I think it's not obvious that Foo is a better "nominal type" for dyn Foo + Send than "Send" (and you couldn't define an inherent method on Send). This impacts the ability to eventually do dyn Foo + Bar, etc. So I'm a bit reluctant to double-down on inherent methods here unless we have a good motivation.

This also impacts the project for for async fn in dyn trait -- cc @jackh726 and @spastorino -- in that part of the plan there is take advantage of the fact that Trait methods are inherent methods on dyn Trait by making them present even if dyn Trait: Trait doesn't hold. This would allow you to invoke them even if the trait is not entirely dyn-compatible.

So my proposal would be:

Let's make it a hard error to define an inherent method on dyn Foo that has the same name as a trait method from Foo. However, i'm not sure what the impact would be, are there ways that this pattern can be useful? @joshtriplett mentioned something about dyn safety, so maybe.

[theemathas]

@nikomatsakis I don't understand what you're saying about "primary traits". When we define a method on dyn Foo, we can't automatically call it on a dyn Foo + Send.

[joshtriplett]

@nikomatsakis One pattern that's useful is that people sometimes define an inherent method with impl dyn MyTrait that has the same name as a method from MyTrait that isn't in the vtable and isn't available on dyn MyTrait. So if we're going to set this restriction (and I think the restriction is a good idea!), then we should only treat it as a conflict if the method is one that's actually available on dyn MyTrait.

[Jules-Bertholet]

Let's make it a hard error to define an inherent method on dyn Foo that has the same name as a trait method from Foo.

There's one case this doesn't address: what if the trait method is on Foo's supertrait?

trait Super {
    fn method(&self) {}
}

trait Sub: Super {}

impl<'a> dyn Sub +'a {
    fn method(&self) {}
}

fn foo(a: &dyn Sub) {
    a.method();
}

Currently, this gives an error. After this PR, it compiles, with the method call resolving to the inherent method. And we want it to compile, because Super and Sub could be in separate crates, and adding a defaulted method to Super shouldn't be a breaking change.

[oli-obk]

@rfcbot fcp cancel

[rust-rfcbot]

@oli-obk proposal cancelled.

[oli-obk]

@rfcbot fcp merge lang,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
• @joshtriplett
• @lcnr
• @nikomatsakis
• @oli-obk
• @scottmcm
• @spastorino
• @tmandry
• @traviscross

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[Jules-Bertholet]

As for my original motivation for working on this: I had a forlorn (and now dashed) hope that this might help fix #57893. TL;DR of that issue:

The compiler currently doesn't enforce coherence between an explicit impl <T: ?Sized> Trait for T and the implicit impl for dyn Trait. This broken behavior leads to all sorts of nasty unsoundness. Unfortunately, it's also relied upon by Any, which needs <dyn Any>::type_id() to use the vtable instead of returning TypeId::of::<dyn Any>().

---

My forlorn hope was that maybe we could make <dyn Any>::type_id() an inherent method, shadowing the type_id() from the trait, and this would allow us to fix coherence to always prefer the blanket impl without breaking too much code. But unfortunately, this doesn't actually work—too much of the ecosystem uses code like <dyn Any as Any>::type_id().

---

While the Any fix didn't work out, I think the existence of dyn Any and its many inherent methods demonstrates that it is valuable to have methods on dyn that don't just delegate directly to a vtable. In the shiny future, I think we should have a syntax to explicitly refer to a vtable trait method, and then inherent methods can build on top of that to expose whatever custom API they want.