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

RFC: calling default trait methods from overriding impls #3329

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

Conversation

adamcrume
Copy link

@adamcrume adamcrume commented Oct 15, 2022

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Oct 15, 2022
@programmerjake
Copy link
Member

since you're not getting the super-type of MyStruct -- rather than calling the default method like:

<MyStruct::super as Trait>::method()

imho having the super attached to the method is better:

<MyStruct as Trait>::super::method()

Co-authored-by: campersau <buchholz.bastian@googlemail.com>
@ChayimFriedman2
Copy link

It may be worth to explain the interaction with specialization, where you can have multiple default implementations (it can be as simple as "always calls the default of the trait declaration).

@adamcrume
Copy link
Author

That's a good point; it needs to explain how it interacts with specialization. I don't think it's quite as simple as "always calls the default of the trait declaration", though, because https://rust-lang.github.io/rfcs/1210-impl-specialization.html explicitly says:

Defaulted items in traits are just sugar for a default blanket impl:

trait Iterator {
    type Item;
    fn next(&mut self) -> Option<Self::Item>;

    fn size_hint(&self) -> (usize, Option<usize>) {
        (0, None)
    }
    // ...
}

// desugars to:

trait Iterator {
    type Item;
    fn next(&mut self) -> Option<Self::Item>;
    fn size_hint(&self) -> (usize, Option<usize>);
    // ...
}

default impl<T> Iterator for T {
    fn size_hint(&self) -> (usize, Option<usize>) {
        (0, None)
    }
    // ...
}

so we don't want to treat those differently.

We could say something like "super calls always call the method which would have been called if the overriding implementation (and anything that overrides them) were not present". Or a more conservative approach would be to say "super always calls the default of the trait declaration or the default unconstrained impl".

That also brings up the question of whether/how to allow calling specialized defaults, but since impls don't have names, that would require something like <MyStruct as MyTrait where MyStruct: Clone>::super::method() (to match impl<T: Clone> MyTrait for T, exact syntax TBD), which is unwieldy. Maybe that's the tradeoff for doing something so niche, though. I'm also happy to leave calling specialized default methods as future work.

If we don't (immediately) allow calling specialized default methods with something like UFCS, then that probably forces us to use "super always calls the default of the trait declaration or the default unconstrained impl", because otherwise otherwise, in ambiguous situations, there would be no way to disambiguate.

@the8472
Copy link
Member

the8472 commented Oct 19, 2022

In the standard library there are a bunch of specializations where either would be useful.

E.g. this case boils down to "if X then call the default specialization impl for this type". And here is a case where we'd want "call the trait's default impl"

@sdroege
Copy link

sdroege commented Oct 23, 2022

In gtk-rs we would also be able to make use of this new feature.

Currently we work around this by having the default implementations of traits (e.g. ObjectImpl::constructed) simply call into an extension trait (e.g. ObjectImplExt::parent_constructed). That works but is a bit verbose.

In addition to this specific feature, it would also be great if it would be possible to (reliably) detect (at runtime is sufficient) whether a trait implementation overrides a specific method implementation or if it kept it at the default.

@faptc
Copy link

faptc commented Oct 30, 2022

We could workaround this issue by using a blacket type that implements that default trait.

impl Foo for S {
    fn foo(&self) {
        struct U;
        impl Foo for U;
        U::foo();
    }
}

@adamcrume adamcrume changed the title Add RFC for calling default trait methods RFC: calling default trait methods from overriding impls Oct 30, 2022
@adamcrume
Copy link
Author

I added detail for how this interacts with specialization. An expansion to universal function call syntax seems necessary to support calling specific impls.

I also replaced <MyStruct::super as Trait>::method() with <MyStruct as Trait>::super::method(), the latter being preferable to a few people but also being more consistent with the expanded UFCS.

Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting here from the perspective that I could have used this in a specialization, if it existed.


Then within a `Vec<String>` impl:

- These evaluate to "Vec<String>":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't render properly due to the angle brackets being interpreted as HTML. Put them in backticks instead.

- `self.name()`
- `<Vec<String> as Trait>::name(self)`
- `<<T=String> Trait for T>::name(self)`
- These evaluate to "Vec<Display>":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

- `<<T=Vec<String>> Trait for T where T: Display>::name(self)`
- These evaluate to "Trait":
- `self.super.super.super.name()`
- `<Vec<String> as Trait>::super::super::super::name(self)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the super::super::... chaining a forward compatibility hazard in case another specialization of intermediate specificity is inserted somewhere in the chain?

I think super shouldn't be seen as a way of traversal but as a namespace disambiguator.
UFCS normally accesses the most-specific-impl namespace. By adding the keyword we signify we want exactly the impl on the named type, not a subtype.

Are there cases where it would be necessary to use super to vaguely point in a direction because one cannot name the exact supertype?

- These evaluate to "Trait":
- `self.super.super.super.name()`
- `<Vec<String> as Trait>::super::super::super::name(self)`
- `<<T=Vec<String>> Trait for T>::name(self)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like this syntax. <<T=Struct> Trait for T: Display>::name looks like a convoluted way to spell <Struct as Trait>::name, which would call the most specific method, not the trait-level default.

Maybe the impl keyword could be used to signify that we're calling an impl-provided default (which can only happen via specialization) instead of the trait-provided one?

Then it could be <Self as impl<T: Display> Trait for T>::default::name().

Other syntax was considered, such as:

- `super.method()`: This implicitly refers to `self` while not naming it, which could be surprising. It also makes it difficult to call default implementations on other values of the same type.
- `<super::Trait>::method(self)`: Using `super::` as a prefix could conflict with the existing semantics of `super`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using default instead super as keyword might work? It's about calling default impls after all.

@SOF3
Copy link

SOF3 commented Jun 27, 2023

If invocation of the default method impl is expected in all impls, why doesn't the crate providing the trait just offer the default method as a separate function? Or, why not just extract the default impl into another method and make the caller cal both traits every time?

imo the requirement to call the default impl is an antipattern because we cannot ensure that the user always calls the default impl anyway, which causes statically unchecked bugs.

That said, I had a similar requirement in xylem, where the Xylem trait implementors are expected to override Xylem::convert_impl but delegate to Xylem::convert instead, which just starts/ends its scope. But in this case I would rather separate convert and convert_impl into two methods (convert should have been a normal function instead of a default impl though) instead of explicitly requiring implementations to call start_scope/end_scope every time.

@Pzixel
Copy link

Pzixel commented Oct 5, 2023

Do you think default impl is the way to go here? I would rather expect compiler to just emit a function for each default impl in trait. It can translate code in Rust 1.0 compatible code with no reliences on experimental features:

trait Iterator {
    type Item;
    fn next(&mut self) -> Option<Self::Item>;

    fn size_hint(&self) -> (usize, Option<usize>) {
        (0, None)
    }
    // ...
}

=>

trait Iterator {
    type Item;
    fn next(&mut self) -> Option<Self::Item>;

    fn size_hint(&self) -> (usize, Option<usize>) {
        iterator_size_hint_default(self)
    }
    // ...
}

pub fn iterator_size_hint_default<T: Iterator>(this: T) -> (usize, Option<usize>) {
         (0, None)
}

@adamcrume
Copy link
Author

Pzixel: That is the same point made by SOF3. As described in the RFC: "This [delegating to a free function] requires the trait author to expose the default implementations intentionally." In other words, if the trait author didn't think about it, the user can't call the default implementation. The RFC also reduces boilerplate and namespace pollution.

@Pzixel
Copy link

Pzixel commented Oct 6, 2023

@adamcrume I'm not saying that "this RFC is not needed because you can write this", I'm writing this how it can be desugared to not rely on other unstable features like default implementations.

@SOF3
Copy link

SOF3 commented Oct 8, 2023

Another scenario I am implementing VisitMut to replace nested syn::Type::References to syn::Type:Path. But that means, when I override visit_type_mut, I have to handle all other variants, without the ability to call the parent (which simply calls syn::gen::visit_type_mut internally).

The intuitive approach here seems to be the ability to call the parent implementation, but it seems syn might as well just expose syn::gen::visit_type_mut directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

10 participants