Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trait method impl restrictions #3678

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Aug 13, 2024

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:

@joshtriplett joshtriplett added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 13, 2024
@joshtriplett joshtriplett added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Aug 13, 2024
@burdges
Copy link

burdges commented Aug 13, 2024

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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member Author

@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
Copy link

burdges commented Aug 14, 2024

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
Copy link

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

@joshtriplett
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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

@traviscross
Copy link
Contributor

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
Copy link
Collaborator

rfcbot commented Aug 26, 2024

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

Concerns:

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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Aug 26, 2024
@scottmcm
Copy link
Member

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
Copy link

burdges commented Aug 28, 2024

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
Copy link
Contributor

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
Copy link
Contributor

traviscross commented Aug 28, 2024

  • 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

This comment was marked as duplicate.

@scottmcm
Copy link
Member

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

Avoid implying that this should always happen if possible. The compiler
may not always want to do this.
@joshtriplett
Copy link
Member Author

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

@scottmcm
Copy link
Member

@rfcbot resolve reference-text

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Sep 23, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 23, 2024

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

text/3678-final.md Outdated Show resolved Hide resolved
@traviscross
Copy link
Contributor

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

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Sep 25, 2024
@nikomatsakis
Copy link
Contributor

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
Copy link

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
Copy link

kpreid commented Sep 30, 2024

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.

@scottmcm
Copy link
Member

scottmcm commented Oct 1, 2024

Another idea of a use for final type:

trait From<T> {
    final type Source = T;
    fn from(x: Self::Source) -> Self;
}

Because whenever I have to repeat a very-long type in the from method that I just had to write in the impl header anyway, I get sad.

(Not to say that we'd necessarily actually do that in From, but this kind of associated convenience typedefs are common in C++ and can be really convenient.)

@scottmcm
Copy link
Member

scottmcm commented Oct 1, 2024

@PoignardAzur I do think that's interesting. It makes me think of how it'd be really nice to split out provided methods into various partial implementations that might differ for different trait bounds -- like having a provided Clone::clone for Self: Copy, or a provided PartialOrd::partial_cmp for Self: Ord.

That said, I actually like having the word "final" in there, whether as final or #[final], because I want a way to talk about them. If we're going to talk about (a future) Copy::copy as being "a final fn" -- like we say "a non-exhaustive struct" today -- then I think actually having that word in the source is a good thing. (Well, so long as it's not the default. I probably wouldn't want to have to write inherent impl every time.) If nothing else, it's way easier to search for "what does final fn mean?" than for "what does it mean when there's an impl for a trait in a different block?"

I wonder if that syntax should instead be sugar for an unnamed extension trait...

@PoignardAzur
Copy link

PoignardAzur commented Oct 2, 2024

You could add the keyword to those impl blocks.


#[final]
impl MyTrait {
    fn method(&self) {
        println!("MyTrait::method: {self}");
    }
}

// or

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

I wonder if that syntax should instead be sugar for an unnamed extension trait...

I mean, an unnamed extension trait implemented on all instances of the trait is basically indistinguishable from an impl block with final items, right?

@traviscross
Copy link
Contributor

I actually like having the word... because I want a way to talk about them

They'd just be inherent methods (on traits).

@slanterns
Copy link
Contributor

slanterns commented Oct 4, 2024

I have mixed feelings towards the newly proposed syntax, i.e. "just think it like inherent methods on types!" vs "all of the trait's contract should be visible from the trait definition itself!" :(

(For the latter, a final method, though cannot be overrided, is also a part of the trait's "contract" just like all other ordinary methods.)

@kpreid
Copy link

kpreid commented Oct 4, 2024

Regarding contracts, here is a hypothetical future feature that would build on final:

  1. In general, a trait serves two users: callers and implementors.

  2. Traits have properties which all uses of the traits should obey; these properties are currently only set down in signatures (checked) where possible, and documentation (not checked) otherwise.

  3. final fns allow traits to provide methods which are guaranteed to do validation in between the caller and the implementor, checking properties or enforcing them via transformation of the data/calls. Thus, the caller can rely on certain properties without relying on the implementor.

  4. However, there's a further missing piece: a caller can always call any trait function without such validation. We could imagine another feature which is functions which are

    • implementable (not final),
    • callable by code in the module defining the trait (or some other visibility scope; details could vary), and
    • not callable by other code (that is merely using the trait).

    Such functions would provide a guarantee that an implementor can rely on, that they are only called in accordance with the trait’s rules.

In this lens, the trait can end up having two different API surfaces: one for callers and one for implementors. When I started writing, I thought this was a good argument for final fn syntax, but now I think it isn't, really. But maybe it will inspire some more useful thoughts.

@bluebear94
Copy link

  • not callable by other code (that is merely using the trait).

If you wanted to restrict calling a trait method to the containing module, then you could make it take an instance of a type that can only be constructed in the module:

pub struct AToken(());

pub trait A {
    fn called_privately(&self, a: u32, b: &str, token: AToken) -> bool;
}

// somewhere later in self:
fn do_something(a: &impl A) {
    let _ = a.called_privately(5, "test", AToken(()));
}

@kpreid
Copy link

kpreid commented Oct 4, 2024

AToken constrains the maximum count of calls to called_privately() only; an implementor can then call another implementation’s called_privately() with arbitrary a and b. It also can’t enforce any rules about the pattern of calls to multiple methods unless the token gets involved in more complex ways. But let’s not go too far along the tangent of designing or debating the value of that feature; I only mentioned it to see if it provided any inspiration for the choice of syntax for final-or-whatever.

@traviscross
Copy link
Contributor

traviscross commented Oct 5, 2024

@kpreid raises what I also think is a key point here, which is that traits define both the implementable interface and part of the callable one. In my view, in Rust, defining the implementable interface is their unique role, because we have other ways to define callable interfaces. E.g., this of course works today:

trait Tr {
    // Nothing in the implementable interface...
}

impl dyn Tr + '_ {
    // ...but there's now something in the callable one when used
    // with `dyn`.
    fn f(&self) {}
}

fn g(x: &dyn Tr) {
    x.f();
}

If we could replace the above with:

trait Tr {
    // There's still nothing in the implementable interface.
}

impl Tr { // <--- Think of this like `impl (impl Tr) { .. }`.
    // There's now something in the callable interface.
    fn f(&self) {}
}

fn g(x: &dyn Tr) {
    x.f();
}

...such that it would also now allow:

// Hey, look at that, increased parity between 
// `dyn Trait` and `impl Trait`.
fn h(x: &impl Tr) {
    x.f();
}

That would make a lot of sense to me.

We made bare trait syntax into a hard error in Rust 2021 to recover that syntactic space in the language. Maybe it will have been for this. (Or maybe we work out some other syntax that achieves the key idea here.)

(There are interesting questions too about how to notate whether such methods should go in the vtable, as sometimes that is what is wanted and something it is not. I could see an argument that perhaps the trait's necessary role isn't necessarily to define either the implementable interface or the callable one, but instead to define the vtable contents. That'd be a coherent position, but it seems maybe too oriented around optimization rather than language semantics (especially as traits are often used statically without a vtable at all). Probably an attribute seems more natural to me for controlling this.)

@scottmcm
Copy link
Member

scottmcm commented Oct 5, 2024

(Aside, TC: it'd be nice to have your concerns in the thread here, rather than in an external document.)

When I look at

A trait isn't a base class, but that's kind of what my brain sees with final there.

I prefer to think about the intent someone has, rather than any particular language details.

(Niko's https://internals.rust-lang.org/t/bikeshed-rename-catch-blocks-to-fallible-blocks/7121/4?u=scottmcm post really changed how I think about a bunch of these things, where it's not quite the same as something else but it's close enough that reusing existing intuition is valuable.)

If I'm using a final method in Java or a final class in C#, for example, I'm expressing the same thing: I need to keep people from changing what this does because I need to be able to trust it's my implementation, not something else.

Whether that's Copy::copy in Rust -- where unsafe code wants to rely on it actually being a copy, and not say panicking -- whether that's String in C# -- where I need people to not be able to give be weird strings that bypass my auth checks -- or whatever, it's the same programmer intent, so I think it fits fine, and I still like final.

(Whether that's final or #[final] I don't think is all that important. We're pretty inconsistent about what should be an attribute vs "real" syntax, and sometimes say we need to urgently reserve a keyword for a design-incomplete feature and sometimes dump things in attributes to avoid reserving something even when they make a huge change to how things work. If we had k#final working well, for example, we might pick final even if it wasn't reserved already.)

@traviscross
Copy link
Contributor

traviscross commented Oct 6, 2024

This is where I point out what we're going to decide in...

...which is that this program will work and print "cat":

trait Mammal {
    //#[final] // <--- Write this to prevent the "override" below?
    fn frob(&self) { println!("mammal") }
}

trait Cat: Mammal { // "Cat extends Mammal..."
    fn frob(&self) { println!("cat") }
}

fn f(x: impl Cat) {
    x.frob() // Prints "cat".
}

That is, there already is this way of "extending" traits with subtraits, and there will soon be this other axis for the "overriding" of a method. This other axis is a better fit for final. Which isn't to say whether or not we'll ever want the knob on that axis, just that it makes it a poor fit here.

And given that we already support the conceptually similar:

trait Tr {}

impl dyn Tr + '_ {
    // This is an inherent method on
    // `dyn` instances of the trait.
    fn f(&self) {}
}

fn g(x: &dyn Tr) {
    x.f();
}

...what we're talking about in this RFC seems to me much more like "inherent" methods than "final" ones, whatever syntax we choose for that. To your point about considering intent, my intent here would be to add an inherent method, carrying over my intuitions about what that means elsewhere in Rust, such as in the above. That is, I don't think we need to import a concept here. We already have one!

@joshtriplett and I talked this through last week, and on the basis of these considerations, he's planning to update this RFC to change final to #[inherent] (with the same semantics as in the RFC), to add the inherent impl-style syntax above as a matter of future work, to add an open question of whether we want to do one, the other, or both prior to stabilization, and to restart the proposed FCP.

With that, I'll be happy to check the box here (or propose FCP merge myself), and we can dig into further discussion on the open item when time permits.

@cynecx

This comment was marked as resolved.

@traviscross

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.