Prefer inherent over supertrait methods in trait object method resolution

[Jules-Bertholet]

View all comments

Partially addresses #51402. Inherent methods on trait objects now take precedence over methods from supertraits of the trait object's trait; 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.

[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.

[lcnr]

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:

Hmm, I don't consider this a strong argument against this change. Having weird priorities and import behavior for trait objects and generic parameters feels unrelated to whether a method is inherent: e.g. the following behaves in the same way

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

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

fn example<T: bar::Test>(d: &T) {
    T::test(d); // prints Test::test
    AnotherTest::test(d); // prints "AnotherTest"
}

fn main() {
    example(&());
}

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.

I did think about this when looking at this PR (and didn't actually mention that in my review 😅). I think having 3 preference levels is fine. What is concerning to me is that "dyn Trait trait method where dyn Trait: Trait doesn't hold" cannot be explicitly named if it overlaps with an inherent method and I didn't consider that earlier.

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.

one way to deal with the overlap of dyn Any I considered with @compiler-errors in the past was a way to get the vtable of a type and use that in the blanket impl of Any (and then disable the builtin impl in case there is overlap).

---

My vibe here is that the concern by Niko about future language work related to non dyn-compatible trait objects together with the fact that the reasoning to land this change being fairly weak means that I don't really see us merging this change.

The effort to judge whether this change negatively interacts with future work is larger than the benefit we get from landing it 🤔

On a more general note, I really dislike the current state of dyn trait being quite broken since 1.0 while never being quite important enough for somebody to sit down and spend a few months really diving into where to go long term. I don't even have anything actionable here. I expect that worst case I will look into this for #57893 once a bunch of the easier and "more core" unsoundnesses are fixed

[Jules-Bertholet]

My vibe here is that the concern by Niko about future language work related to non dyn-compatible trait objects together with the fact that the reasoning to land this change being fairly weak means that I don't really see us merging this change.

That's fair. I think it would be desirable to have a way to refer to the vtable method specifically, but makes sense to block until that is figured out.

Would you be receptive to a more restricted PR that only prefers inherent methods over methods from supertraits? This would be sufficient to fix the semver hazard issue, and wouldn't cause any issues for non dyn-compatible trait objects because you can always do an upcast.

[nikomatsakis]

@theemathas

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.

This is kind of my point. Normally we permit 'inherent' things only on "nominal types" like struct or enum that can't be "added". The "name" for a dyn trait is kind of... the combination of names of all the traits... it's just all a bit odd. I maintain this is because a dyn Trait is really an existential type like exists T: Trait. T, and when you phrase it like that, it's clear that Trait is not being used as the "name of a type" (which is the bound variable T) but rather as a bound. This is why I consider it something of a mistake to permit dyn Trait there, I think it leads to issues. I would rather that the set of methods, fields, and other properties on dyn $TRAITS is determined by the definition of $TRAITS and not augmented by the user.

---

@Jules-Bertholet

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

This is a good point, it does lead to a "fragile base class" type of problem, or it could anyway. That said, we can certainly permit inherent methods defined on dyn Sub even if there is a trait method defined on Super. It's a bit odd but it still gives a way to specify which one you want (Sub::foo(sub) and Super::foo(sub)) in an unambiguous way. @traviscross also pointed out that we have that same issue today but you get ambiguity errors, which is why we added the RFC to resolve in favor of subtraits, allowing for things like Itertools, so I guess that's useful precedent to lean on?

---

@lcnr

I don't consider this a strong argument against this change. Having weird priorities and import behavior for trait objects and generic parameters feels unrelated to whether a method is inherent: e.g. the following behaves in the same way

I wondered if you would bring it up. It's true that generic parameters also add the trait methods as "pseudo-inherent methods". I wouldn't mind reverting that change in a future edition, to be honest, although also it'd just be annoying for users for little real gain.

I feel differently about dyn. I guess it's because of the direction that I think we are going to make dyn Foo have (the dyn-compatible subset of) methods from Foo even when it may not implement Foo, which cannot be explained any other way. I don't like the idea of having to have a 3rd class of method, I was very happy when @compiler-errors pointed out that you could view these methods as inherent methods on the type, and I'd like to preserve that.

[Jules-Bertholet]

Would you be receptive to a more restricted PR that only prefers inherent methods over methods from supertraits? This would be sufficient to fix the semver hazard issue, and wouldn't cause any issues for non dyn-compatible trait objects because you can always do an upcast.

This is now implemented. (The Reference PR will still need updating.)