Trait method impl restrictions

[joshtriplett]

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

[burdges]

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?

[Lokathor]

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

[joshtriplett]

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

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

[burdges]

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.

[bluebear94]

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

[joshtriplett]

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

[programmerjake]

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

[joshtriplett]

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

[traviscross]

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.

[rfcbot]

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.

[scottmcm]

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.

[burdges]

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.

[tgross35]

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.

[traviscross]

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

[scottmcm]

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 🤦

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

[joshtriplett]

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

[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

[rfcbot]

@joshtriplett proposal cancelled.

[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
• [ ] @traviscross

Concerns:

• reference-text resolved by https://github.com/rust-lang/rfcs/pull/3678#issuecomment--1927559703
• tc-wants-to-talk-through-it (https://github.com/rust-lang/rfcs/pull/3678#issuecomment--1920446560)

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.

[tmandry]

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

[traviscross]

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

[nikomatsakis]

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

[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:)

[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;
}

[bluebear94]

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

[programmerjake]

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.

[scottmcm]

@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

[scottmcm]

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

@rfcbot reviewed

[jhpratt]

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.

Counterpoint: the syntax in the restrictions RFC is an unresolved question. My personal preference has grown towards a #[restrict(impl(crate))] sort of syntax, which solves other issues re. pre-existing macros as well.

[joshtriplett]

@jhpratt

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.

Counterpoint: the syntax in the restrictions RFC is an unresolved question. My personal preference has grown towards a #[restrict(impl(crate))] sort of syntax, which solves other issues re. pre-existing macros as well.

Since my original comment here, I've switched to no longer attempting to mirror the RFC 3323 syntax.

[joshtriplett]

@scottmcm I believe I've now addressed your concern.

[scottmcm]

@rfcbot resolve reference-text

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[traviscross]

@rfcbot concern tc-wants-to-talk-through-it

As mentioned above, and in the meeting today, I definitely want something like this, but there are some things I'd like to talk through here first. We didn't quite get to that in the meeting today, so let's file a concern to leave space for it.

[nikomatsakis]

Something I realized:

@joshtriplett it's worth mentioning, I think, that we could allow marker traits to have final methods (but not other methods).

[PoignardAzur]

Maybe a little late, but I'd like to suggest another syntax:

trait MyTrait: Display {
    // stuff
}

impl MyTrait {
    fn method(&self) {
        println!("MyTrait::method: {self}");
    }
}

This syntax would basically be an inherent impl block, for traits. It would have the same properties as final in this RFC.

[kpreid]

Interesting. impl MyTrait {} used to be valid syntax, with the meaning that is today given to impl dyn MyTrait {}, before dyn was mandatory for trait object types. This argues both against and in favor of it:

• Against: It might be confusing that an old syntax has been given a new but different meaning.
• For: The new meaning is one that I recall a lot of beginners wanting, and they had to be told that their program didn't do what they hoped (that the methods would be found only if a dyn type was involved and not otherwise).

Other arguments:

• For: This makes it very straightforward that such a method is not something a trait impl can override because it's not in the trait declaration — it uses an existing principle about what goes in an impl, rather than adding a new kind of access restriction.
• For: No new grammar whatsoever (impl MyType {} and impl MyTrait {} are the same at the syntactic level).
• For: This adds all the code-organization power of impl blocks:
  • Putting the blocks anywhere you want instead of embedding all the final methods in the trait declaration itself
  • Declaring bounds on the block instead of the individual method or the trait
• Against: Now it's harder to see what methods importing a trait introduces (when reading source code, not documentation).

Personally, I don't care very much — I'm looking forward to being able to use finalness regardless of what syntax it has. Both seem elegant enough.