rfcs Trait method impl restrictions

Support restricting implementation of individual methods within traits, using the already reserved final keyword.

This makes it possible to define a trait that any crate can implement, but disallow overriding one of the trait's methods or associated functions.

This was inspired in the course of writing another RFC defining a trait, which wanted precisely this feature of restricting overrides of the trait's method. I separated out this feature as its own RFC, since it's independently useful for various other purposes, and since it should be available to any crate and not just the standard library.

Rendered

Tracking:

https://github.com/rust-lang/rust/issues/131179

Aug 13 '24 19:08 joshtriplett

We've past proposals for inherent trait methods I think, mostly proposing a second inherent looking impl block, not sure that syntax matters, but this winds up functionally equivelent, yes?

There is however a question of default speed vs size optimizations in vtables, aka do these methods appear in the vtable, or do they have a generic implementation for every type? Also, how does one flag that these method should appear in a vtable, or should use a generic implementation for every type?

Aug 13 '24 20:08 burdges

It might be better to have this as an attribute rather than yet another potential keyword position for parsing to deal with. Otherwise, great.

Aug 13 '24 20:08 Lokathor

@Lokathor wrote:

It might be better to have this as an attribute rather than yet another potential keyword position for parsing to deal with. Otherwise, great.

I get that, but on the flip side, that'd be inconsistent with RFC 3323, and it seems awkward to use a keyword in one place and an attribute in another.

Aug 13 '24 22:08 joshtriplett

@burdges wrote:

We've past proposals for inherent trait methods I think, mostly proposing a second inherent looking impl block, not sure that syntax matters, but this winds up functionally equivelent, yes?

That might be equivalent to a subset of this, depending on the details. I haven't seen those proposals.

There is however a question of default speed vs size optimizations in vtables, aka do these methods appear in the vtable, or do they have a generic implementation for every type? Also, how does one flag that these method should appear in a vtable, or should use a generic implementation for every type?

In theory, if you have a method that can't be overridden outside of the crate/module, and nothing overrides it, you could optimize by omitting it from the vtable. I don't think that optimization should be mandatory, or required for initial implementation, though.

Aug 13 '24 22:08 joshtriplett

I'm more asking what's the best default. If the best default were to be in the vtable, then you can just do

trait Trait {
    #[inline(always)]
    impl(crate) fn f(&self) { f_inner(self) }
}
fn f_inner<T: Trait>(s: &T) { .. }

I'd expect f_inner gets only one copy for the trait obejct here.

If otoh the best default were not to be in the vtable, then rustc should do something more complex.

Anyways yeah maybe not relevant here.

Aug 14 '24 01:08 burdges

Another future possibility could be to add impl(unsafe) trait methods, which can only be (re)implemented by unsafe impls.

Aug 15 '24 18:08 bluebear94

@bluebear94 Interesting! So, rather than requiring unsafe impl Trait for Type, you could implement the trait safely, as long as you don't override the method? That's a novel idea.

Aug 15 '24 18:08 joshtriplett

this would be quite useful for stabilizing std::error::Error::type_id, which currently has several workarounds to prevent it from being overridden: https://doc.rust-lang.org/1.80.1/src/core/error.rs.html#88-100 cc rust-lang/rust#60784

Aug 20 '24 20:08 programmerjake

@programmerjake That's a great use case, thank you!

Aug 20 '24 22:08 joshtriplett

This seems like a reasonable and desirable extension to RFC 3323. So I propose...

@rfcbot fcp merge

Thanks @joshtriplett for writing this up.

I note that syntax was left as an open item on RFC 3323, and so we're implicitly carrying that open item here also.

Aug 26 '24 08:08 traviscross

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

[x] @joshtriplett
[ ] @nikomatsakis
[ ] @pnkfelix
[ ] @scottmcm
[ ] @tmandry
[x] @traviscross

Concerns:

visiblity-style-vs-final-style (https://github.com/rust-lang/rfcs/pull/3678#issuecomment--1979004716)

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.

Aug 26 '24 08:08 rfcbot

Big +1 to having something like this.

@rfcbot concern visiblity-style-vs-final-style

I think these are far more than a syntax difference, so I want to talk about it more.

As a couple of examples:

With #[final] we can allow it in #[marker] traits, but with impl(in self) we can't, because it could still be overridden in the module.
With #[final] we can have the MIR inliner inline the provided method into other provided methods or into generic code, but with impl(in self) we can't because it might be overridden somewhere.
With #[final] unsafe code can trust the implementation of the method, but arguably if it's overridable at all by anyone then to trust the implementation of the method it ought to be an unsafe trait.

So is the visibility phrasing here actually important? Do we actually need it? The cases I know of don't need it

the RFC mentions Error::type_id, which doesn't need it.
I'd like to add a Copy::copy method, which doesn't need it (and would really benefit from being trivially inlinable and such)
things like https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/visit/trait.MutVisitor.html don't need overriding the super_* methods to be impossible, but I think would be perfectly happy to just mark them #[final], and as far as I know has no need for impl(crate) or similar.

The provided implementation of a #[final] can always call another trait if needed, so I think that anyone who really needs such a thing could do it that way. And having the stronger rule gives us a bunch of advantages. And if we're going to give it semantic capabilities beyond just a visibility-like error, I don't think it should look like visibility -- my code should never fail to compile because I changed something from impl(self) to impl(crate) for example! (Going to pub might make things unsound, but doesn't make it stop compiling -- other than name resolution glob conflicts or something.)

Thus my inclination is that we should do the #[final] version of it instead of the impl (in …) version.

Aug 28 '24 18:08 scottmcm

I think #[final] is easier to explain too. As usages of the impl and use keywords multiply, we quickly lose our ability to explain them.

Aug 28 '24 20:08 burdges

If it is to be an attribute then I would prefer something like #[no_override] that says what it does, rather than taking a C++ keyword that imo isn't very descriptive. But +1 to this suggestion - I think it is immediately obvious what the effects are, and either being on or off is easier to follow than giving fine grained control. Agreeing with Scott, being able to e.g. override something in the module but not the rest of the crate doesn't seem like a common enough need to justify the complexity.

I assume the syntax was chosen for parity with RFC3323, but I think it is okay to deviate from this if it comes with a simplification.

Aug 28 '24 20:08 tgross35

With #[final] we can allow it in #[marker] traits, but with impl(in self) we can't, because it could still be overridden in the module.

With #[final] we can have the MIR inliner inline the provided method into other provided methods or into generic code, but with impl(in self) we can't because it might be overridden somewhere.

With #[final] unsafe code can trust the implementation of the method, but arguably if it's overridable at all by anyone then to trust the implementation of the method it ought to be an unsafe trait.

Note that the RFC allows for impl(), which would express the same thing as #[final]. From the RFC:

In addition to impl(visibility), a trait method or associated function can also use impl(), which does not permit overriding the method anywhere, even in the current module. In this case, all implementations will always use the default body.

This is something I specifically checked to ensure was there before proposing FCP merge, in part for the reasons you mention.

Aug 28 '24 23:08 traviscross

Random thing: I'd forgotten that we actually have final reserved as a keyword already, so final fn foo() { … } is also a possibility without needing an edition transition.

Duh, this is already mentioned in the RFC 🤦

Aug 29 '24 04:08 scottmcm

Note that the RFC allows for impl(), which would express the same thing as #[final].

I think that that just pushes me even more to skip the impl(in blah) part of the feature, because if what I want is already an unusual case that doesn't work with the 3323-style syntax, that says to me we should just skip that style syntax entirely.

If we need a special syntax, let's make it final or #[final]. That way we can keep the "this is only about visibility, not runtime semantics" property of pub(…) and impl(…). If we're not adding pub() fn foo() {} and struct Foo { mut() a: i32 }, then I think any superficial similarity here is more harmful than good. We can keep impl(…) on provided methods if people really want it -- though it would be good to have at least a single concrete example scenario in the RFC -- but I'm opposed to mixing the visibility restrictions with non-visibility ones.

(And, aesthetically, impl() looks weird to me.)

Aug 29 '24 05:08 scottmcm

@scottmcm

If we're not adding pub() fn foo() {} and struct Foo { mut() a: i32 }, then I think any superficial similarity here is more harmful than good.

I think we should add mut() or equivalent: "this field is not writable from anywhere". (Useful for an object ID or similar that can never change once constructed.)

I agree that pub() ("this can't be called from anywhere") isn't useful.

If we need a special syntax, let's make it final or #[final].

While I do personally think the RFC 3678 syntax has value, I'm not going to continue proposing that that syntax when we don't have a concrete use case for the cases other than impl(). I've changed the RFC to propose final, and moved the generalized RFC-3678-style syntax to future possibilities.

Given the size of this change, I'm going to cancel and restart the FCP so that anyone who previously checked their box can decide if they still wish to do so with the new version.

Sep 07 '24 00:09 joshtriplett

Canceling and restarting the FCP so that anyone who previously checked their box can decide if they still wish to do so with the new version that uses final.

@rfcbot cancel

@rfcbot merge

Sep 07 '24 00:09 joshtriplett

@joshtriplett proposal cancelled.

Sep 07 '24 00:09 rfcbot

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

[x] @joshtriplett
[x] @nikomatsakis
[ ] @pnkfelix
[x] @scottmcm
[x] @tmandry
[x] @traviscross

Concerns:

proposed FCP should be restarted with a new name (https://github.com/rust-lang/rfcs/pull/3678#issuecomment--1859283973)
~~reference-text~~ resolved by https://github.com/rust-lang/rfcs/pull/3678#issuecomment--1927559703
~~tc-wants-to-talk-through-it~~ resolved by https://github.com/rust-lang/rfcs/pull/3678#issuecomment--1862637958

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.

Sep 07 '24 00:09 rfcbot

Makes sense to me. Sometimes provided methods are awkward because the thing you really want is a final method (thinking about the super_* visitor methods in the compiler, which are called from overrideable methods). Thanks for proposing this.

@rfcbot reviewed

Sep 07 '24 01:09 tmandry

I think I'd like for us to talk this one through in a design meeting, so I propose:

https://github.com/rust-lang/lang-team/issues/288

Sep 07 '24 11:09 traviscross

I have wanted this feature many, many times over the years. It would also allow the methods defined (rather awkwardly) as inherent methods on dyn Any to be moved to the trait, allowing them to be called on subtraits of any.

@rfcbot reviewed

Sep 07 '24 14:09 nikomatsakis

That said, a design meeting plus some time for folks to comment both seem good.

One possible extension: final associated types, which I have also wanted. I have to go digging around to see why exactly:)

Sep 07 '24 14:09 nikomatsakis

One example:

It would be nice if IntoIterator were defined as

trait IntoIterator {
    final type Item = Self::Iter::Item;
    type Iter: Iterator;
    fn into_iter(self) -> Self::Iter;
}

Sep 07 '24 14:09 nikomatsakis

Associated constants could also be made final as a further possibility.

Sep 08 '24 05:09 bluebear94

Associated constants could also be made final as a further possibility.

imo final should be supported on any item you can put in a trait definition: associated types associated constants functions anything else we add in the future.

Sep 09 '24 16:09 programmerjake

@rfcbot concern reference-text

I'd like to see something like https://github.com/rust-lang/rfcs/pull/3678/files#r1768869462 to nail down a couple of the details as part of the text here

Sep 20 '24 16:09 scottmcm

That said, the current plan as described in the current version sounds great to me! I look forward to having it.

@rfcbot reviewed

Sep 20 '24 16:09 scottmcm

rfcs rfcs copied to clipboard

Trait method impl restrictions

rfcs
rfcs copied to clipboard