Keep dyn-compatible final methods in the vtable

[mu001999]

View all comments

Fixes #153649

Current implementation will exclude final methods from the trait's vtable, this PR will keep those dyn-compatible ones in the trait's vtable.

[rustbot]

r? @dingxiangfei2009

rustbot has assigned @dingxiangfei2009.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 16 candidates

[eggyal]

I don't think this is sound. If the final method is not dyn compatible, then surely it should definitely NOT appear in the vtable and should also not be available on dyn Trait (if the trait can itself be dyn compatible at all)?

[mu001999]

RFC has the statement says:

A final fn never prevents a trait from having dyn-compatibility; the trait can remain dyn-compatible as long as all non-final methods support dyn-compatibility. This also means that a final fn can always be called on a dyn Trait, even if the same method as a non-final fn would not have been dyn-compatible.

And with allowing final methods not dyn compatible to be available on dyn Trait can allow the following usage:

#![feature(final_associated_functions)]

trait Trait {
    fn foo(&self);

    final fn bar<T>(&self, x: T) -> T {
        self.foo(); // Call dyn-compatible methods
        x
    }
}

struct Foo;
impl Trait for Foo {
    fn foo(&self) {
        println!("Foo");
    }
}

struct Bar;
impl Trait for Bar {
    fn foo(&self) {
        println!("Bar");
    }
}

fn foo(t: &dyn Trait) {
    let _ = t.bar(0i32);
}

fn main() {
    foo(&Foo);
    foo(&Bar);
}

[QuineDot]

Repeating a comment from IRLO:

[The cited issue] is related to #57893; namely, always preferring the user defined method in cases of overlap breaks invocations of .type_id() on dyn Any (preferring the built-in implementation in that case is load bearing). Which is considered unfortunate as preferring the user implementation is the best fix from a soundness perspective, as I understand it. (See this example of preferring the built-in implementation causing unsoundness.)

[...]

If final fn is always not in the vtable, it would basically act like preferring the user implementation. Corollary: Any::type_id could not be a final fn.

---

The cited issue shows that this applies to custom downcasting impls, not just dyn Any. An alternative to this PR is to just not make the method final. There is probably a larger lang/types decision to be made here wrt. final methods and vtables.

[mu001999]

r? compiler

[nnethercote]

@fee1-dead: I will pass this over to you, because it's stuff I know nothing about and you reviewed #151783. Thanks!

r? fee1-dead

[rustbot]

fee1-dead is not on the review rotation at the moment.
They may take a while to respond.

[fee1-dead]

See #153649 (comment)

Marking as blocked, if you want to push this you should probably ask for t-lang discussion.

[mu001999]

if you want to push this you should probably ask for t-lang discussion.

Considering that we cannot use final to impl Error::type_id (i.e., #153598) without this PR, which is one motivation of RFC-3678.

Let me nominate this.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

(Speaking for me with my lang hat on, but not for the team) I think we should do this.

One of the things where this would be useful is https://doc.rust-lang.org/std/slice/fn.range.html, which is currently only module-scoped because putting it on a safe trait like RangeBounds would be unsound.

But if you're using dyn RangeBounds<usize>, it would be much better for a final fn slice_range on that to be in the vtable, because if it's RangeFull you don't want to run two matches on two dyn calls for start_bound+end_bound -- you want to make one dyn call to slice_range that's trivial under the hood.

You can always define something inherent on dyn Whatever if you want a method that isn't in the vtable.

[joshtriplett]

I am very confused about this.

Considering that we cannot use final to impl Error::type_id (i.e., #153598) without this PR, which is one motivation of RFC-3678.

This seems like it might be something unique to the peculiarities of type_id, rather than necessarily being something that generalizes to other final fn.

I think there are tradeoffs about whether a dyn-compatible final fn goes in the vtable or not, as @scottmcm identified. But I don't think we should be deciding that based on type_id. There are cases that would be optimized by putting them in the vtable, and cases that would be pessimized. I would expect that to be an orthogonal mechanism from final.

I added that mention of type_id to the RFC as an example based on folks mentioning that final would potentially help there. It shouldn't be taken as "if final doesn't work for type_id then it must be changed so that it can be".

[joshtriplett]

@scottmcm wrote:

You can always define something inherent on dyn Whatever if you want a method that isn't in the vtable.

That seems a lot more complicated and less usable, and would require a duplicate implementation if you also have a definition in the trait. I don't see how that would be compatible with writing the method only once.

[Jules-Bertholet]

I don't think we should do this, at least not in a semantically relevant way. #57893 and the dyn Any situation is bad enough; we should not be extending that mess any further. In fact, I think it would be best if Error::type_id were an inherent method on dyn Error, with any method on the trait itself needed to support it being an implementation detail invisible to user code.

(In cases where it's a pure performance optimization, like the slice_range example cited by @scottmcm above, my concern doesn't apply.)

[Jules-Bertholet]

In fact, I think it would be best if Error::type_id were an inherent method on dyn Error, with any method on the trait itself needed to support it being an implementation detail invisible to user code.

Proposed in #156252

[mu001999]

Based on the discussion above, I’ll close this PR for now, since

It shouldn't be taken as "if final doesn't work for type_id then it must be changed so that it can be".

And personally, I also think this change would make the behavior of final methods harder to predict (#153649 (comment)).