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: Implement parent items with child traits #2303

Closed
wants to merge 1 commit into from

Conversation

porky11
Copy link

@porky11 porky11 commented Jan 18, 2018

This RFC allows to implement functions, associated types and associated constants to be defined while implementing a child trait.
Therefore you have to define them in a special way, using the parent traits.

Rendered

This RFC allows to implement functions, associated types and associated constants to be defined while implementing a child trait.
Therefore you have to define them in a special way, using the parent traits.
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jan 18, 2018
```
But in order to allow this, trait `A` needs to be defined in a different way, like this:
```
trait A: use B+use C {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be cool if there was a way to specify that you want this behavior at the trait implementation, rather than the trait definition. This would allow you write these sorts of impls with existing traits.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think, it'd be useful at definition time.
What you think, would be something like that, I assume:

struct Number;
impl Add+Sub for Number {
    fn add(…) {…}
    fn sub(…) {…}
}

This may simplify implementation, but doesn't help with compatibility.

Maybe in combination with trait aliasses this would be useful.
When you want to split your trait A into B and C, like in the example from the RFC, you could write this:

trait B{…}
trait C{…}
trait A_old{…}
trait A = B+C+A_old;

This would be useful for backward compatibility too, but you would have to split it just into new traits without the ability to leave something inside the original trait. Also you probably couldn't create dynamic versions of this type then, anymore, which then would break it.

Copy link
Member

Choose a reason for hiding this comment

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

My immediate reaction was that this seemed like a natural and neat extension which would save me a lot of boilerplate. However, I also think that boilerplate would be significantly reduced by existing (unimplemented) features such as implied bounds.

Copy link
Author

Choose a reason for hiding this comment

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

I think implied bounds was the feature, that you don't have to write where clauses in every implementation, if you write it in the type definition itself, wasn't it?

Copy link
Member

Choose a reason for hiding this comment

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

@porky11 Yes.

[reference-level-explanation]: #reference-level-explanation

This will be implemented by recursively searching for all names of traits, which are used by the current trait.
If one of the names exists at least twice, this will be reported as an error when the trait is defined.
Copy link
Member

Choose a reason for hiding this comment

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

Currently, it's a backwards compatible change to add default methods to a trait. This would cause that to break, since trait Foo: use Bar { fn foo() { ... } } would break if Bar added a default method called foo.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, then this may be a bit more complicated, than I thought.
Then my other approach for using something as defined in Alternatives seems more useful:

trait Bar {
    fn bar();
    fn foo() {…}
}

trait Foo: Bar {
    use Bar::bar; //now bar is implementable when implementing `Foo`
    fn foo();
}

Then another problem occurs: It's not possible to use traits, when the user doesn't have to implement one of it's functions, but this was not possible anyway, according to my descriptions.
The child trait is required to use everything of the parent trait, that has no default value. This way, it will be ensured, that adding something with default impl won't be a breaking change now.

@petrochenkov
Copy link
Contributor

C++ allows a very similar thing and it's something that I personally don't like compared to Rust.
You have to mix all methods from several interfaces into a single pile and the only way to separate them is comments.

class Interface1 {
    virtual void interface1()  = 0;
};
class Interface2 {
    virtual void interface2() = 0;
};
class Interface3: Interface1, Interface2 {
    virtual void interface3() = 0;
};
class Derived: Interface3 {
    // Implement interface 1
    void interface1() override {}

    // Implement interface 2
    void interface2() override {}

    // Implement interface 3
    void interface3() override {}

    // Inherent methods
    void derived() {}
};

I think this is okay if Interface3 decides that Interface1 and Interface2 are its implementation details (for convenience or some other reasons), and Interface3 is the actual inseparable public interface, but it's better not allowed for impls for arbitrary trait hierarchies.

@Ixrec
Copy link
Contributor

Ixrec commented Jan 19, 2018

It seems like the strongest argument for this feature is that it (arguably, maybe) makes it backwards-compatible to "split" a trait into several smaller traits.

So my main question is: How often does this use case really come up in public APIs? I don't use Rust very often, but every time I've looked at a Rust crate's docs all the traits seemed to have only one or two or occasionally three required methods. Some of them do have a ton of defaulted methods, but I have a hard time imagining a scenario where you'd want to move defaulted methods to a separate smaller trait, and it wouldn't be obvious prior to a 1.0 release.

@burdges
Copy link

burdges commented Jan 19, 2018

I dislike this right now because it complicates finding the impl. Also I'd think subsuming impl's where clauses into struct, child trait, or even type aliases where clauses alleviates much boilerplate.

I do favor ensuring that child traits can provide specializations for parent traits under more scenarios. Right now, there are nasty cases where a child cannot provide a specializations for parent with an associated type or else doing it cause recursion overflows, but those are specialization bugs not new features.

@porky11
Copy link
Author

porky11 commented Jan 19, 2018

@Ixrec
That was also my arument for this.
The reason, why I thought, this would be useful, is, that they think about splitting some trait into multiple traits in num_tratis, but they don't want to break anything.

@cramertj
Copy link
Member

cramertj commented Jan 19, 2018

@Ixrec Actually just the other day I hit a case where I wanted to split a trait into two, though I don't think this RFC would solve the problem (it's sort of tangentially related, and not tied to backcompat). The situation was something like this:

trait Foo {
    type Arg;
    type Ret;
    fn bar(&self, x: Self::Arg) -> Self::Ret;
}

I wanted to change the trait so that bar could potentially accept multiple different types (rather than a single associated type). However, I couldn't just make Foo generic on Arg since I wanted Ret to be the same for all Arg types. In order to make this happen, I made the following split:

trait FooBase {
    type Ret;
}

trait Foo<Arg>: FooBase {
    fn bar(&self, x: Self::Arg) -> Self::Ret;
}

Now, however, types which implement Foo<Arg> for only one value of Arg need much more verbose impls, since they have to implement two traits instead of one. It'd be cool if you could just impl Foo<u8> and include the Ret type in the impl, which seems like something this feature (or one like it) could allow.

@porky11
Copy link
Author

porky11 commented Jan 19, 2018

@cramertj
This would support, what you want.

@aturon
Copy link
Member

aturon commented Feb 2, 2018

cc @withoutboats @nikomatsakis

@withoutboats
Copy link
Contributor

@porky11

The motivation section is rather brief and I'm not sure I understand it. I think I get the ergonomics point - if your type has a lot of parameters with bounds and such it can be quite annoying to reiterate this. However, "implied bounds" alleviates a lot of this pain.

This second motivation is intriguing, but I don't understand what you mean:

being able to change trait hierarchy by splitting types as a crate maintainer without breaking anything.

Can you elaborate on this, maybe show some examples?

@porky11
Copy link
Author

porky11 commented Feb 5, 2018

@withoutboats
My text already contains examples.
But maybe they are too abstract, so here another one:

Assuming, you define a trait for objects moving in space. You can access the position, the velocity and displace the object by some delta and accelerated by some force:

trait Moving<T> {
    fn pos(&self) -> T;
    fn displace(&self, delta: T);
    fn vel(&self) -> T;
    fn accelerate(&self, force: T);
}

Now you use this trait in many places, maybe even other people use this crate.
Then you see, other than you thought first, that it's useful to have a trait for some objects, that don't have a velocity, but have a position and can be displaced:

trait Positional<T> {
    fn pos(&self) -> T;
    fn displace(&self, delta: T);
}

But now you would also like to be able to use the functions, you define for your Positional trait for all objects, that are Moving.
You could define the function for all Movers:

impl<M,T> Positional<T> for M where M: Moving<T> {
    fn pos(&self) -> T {
        self.pos()
    }
    fn displace(&mut self, delta: T) {
        self.displace(delta);
    }
}

This also works for most cases. But it's not, what you would like to have. If it wouldn't break anything, you would have used different design, but when you don't want to break, you cannot change it anymore.
This design also has some more problems:
Assuming you use Positional and Moving in the same module:

mod test {
    use Mover;
    use Positional;
    fn test() {
        let p = MyPositional{…}
        let m = MyMover{…}
        println("{:?}, {:?}", p.pos(), m.pos());
    }
}

This would cause an error like that: error[E0034]: multiple applicable items in scope
This could not happen with the other approach.

Another example is, you have multiple traits, that you want to implement a new parent trait for, when you found, that multiple traits share some functions, here a Drawable, which also has a position and can be displaced. Probably a stupid example, but there may be other cases, where this could really happen.

impl<D,T> Positional<T> for D where D: Drawable<T> {
    fn pos(&self) -> T {
        self.pos()
    }
    fn displace(&mut self, delta: T) {
        self.displace(delta);
    }
}

This also is not possible.
Maybe also allowing this would be more useful, but I don't think, this will work soon, if at all.
And when you create the Drawable trait afterwards, it won't be a problem, since you can just make the Positional trait a parent of it.
I hope this helps to understand.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

I've definitely encountered the problem of wanting to split traits up. I've also, as @cramertj noted, encountered the problem on the other side: wanting to define traits that I can define "all at once" without distinct impls. I suppose though that often this could be defined at the trait level.

Regarding the semver questions, it seems like we would only allow one trait to "use" another if they are in the same crate.

All that said, I'm not sure about the keyword choice -- something is tickling me about this proposal, but I'm not sure what it is. Basically, we want to define a kind of "hierarchy of related traits" -- this feels like something we've wanted in other contexts too.

@porky11
Copy link
Author

porky11 commented Feb 12, 2018

@nikomatsakis
As I told here, this choice may be preferable. It also is more like the default syntax for using and will still work, if something like partial impls are supported.
Your suggestion to only allow this inside a single crate seems like a good restriction.

If restricted to a single crate, maybe it's worth thinking about implicitely allowing this, but this would probably require a new RFC.

@burdges
Copy link

burdges commented Feb 12, 2018

I could imagine being explicit impl<..> (TraitA<..>,TraitB<..>,TraitC<..>) for Type<..> where .. perhaps, with perhaps trait MyTraits<..> = (TraitA<..>,TraitB<..>,TraitC<..>); providing this.

@eddyb
Copy link
Member

eddyb commented Feb 12, 2018

@burdges Why (TraitA, TraitB) syntax instead of TraitA + TraitB?

@burdges
Copy link

burdges commented Feb 12, 2018

Just my mistake

@porky11
Copy link
Author

porky11 commented Feb 12, 2018

@burdges
This may be useful for ergonomics, but won't fix the main problem of splitting traits into multiple traits

@nox
Copy link
Contributor

nox commented Feb 25, 2018

While I understand the rationale behind the RFC, I don't think this helps understanding the code in any way, because now one must remember the super traits of all implemented traits at all time. I feel like this optimises for things that are just a bit painful to write when they are implemented the first time, but will hinder understanding those very same impls a few weeks later.

My experience implementing a massive amount of traits for a massive amount of data types in Servo and in particular Stylo makes me conclude that this is unneeded and potentially harmful.

@nikomatsakis
Copy link
Contributor

I was thinking about this the other day. I would like to propose an alternative. I think we should generalize the syntax of impl so that it takes a list of bounds, rather than a single trait ref:

impl<..> Trait1<P1..Pn> + Trait2<P1..Pn> for P0

This would be exactly equivalent to independent impls:

impl<..> Trait1<P1..Pn> for P0 { .. }
impl<..> Trait2<P1..Pn> for P0 { .. }

where the generic parameters and items are divided accordingly.

There are some things to work out:

  • What to do if both traits define the same item? (Easiest answer: error)
  • Can we cleanly divide up the trait parameters between the two? (I don't see an issue)

If we did this, and we combine with trait aliases, then I think we get a lot of the goals of this RFC. For example, we could (I think) backwards compatibly change

trait Foo {
   fn a();
   fn b();
}

into

trait Foo = Foo1 + Foo2;
trait Foo1 { fn a(); }
trait Foo2 { fn b(); }

@scottmcm
Copy link
Member

scottmcm commented Mar 15, 2018

Not having something in this space is why .Net has IList that doesn't implement IReadOnlyList, so I feel like having something allowing splits like that is valuable, though I don't really have an opinion on whether exactly what's proposed here is the way to do that.

For example, I wish we could generalize .clone_from() in some way (so you can clone_from a Vec from a slice, for example), which might need making Clone a combination of something else in some way.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp postpone

My feeling on this RFC is that it is tackling the right problem but the design is not quite there yet. Therefore, I'd prefer to postpone in favor of more discussion on internals or elsewhere. I do want to thank @porky11 for the interesting idea, though.

Some concerns about the specific form of this RFC:

  • Adding another knob to supertraits (trait Foo: use Bar) feels like it is a pretty heavy-weight change; we had better be sure it's worth it.
  • I'd like to give end users the choice to implement the component traits independently (per @petrochenkov's point).

Personally, I'm enthusiastic about this direction of being able to leverage trait aliases, but I'm open to other ideas. It certainly has its complications too: for example, I just realized that the trait alias approach runs into the semver concerns that @cramertj mentioned, which seem more problematic in that context.

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 15, 2018

Team member @nikomatsakis has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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!

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. 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 Mar 15, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 19, 2018

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

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 29, 2018

The final comment period is now complete.

@Centril
Copy link
Contributor

Centril commented Mar 29, 2018

Closing per completed FCP with a motion to postpone.

Thanks to @porky11 for the RFC =)

@Centril Centril closed this Mar 29, 2018
@Centril Centril added the postponed RFCs that have been postponed and may be revisited at a later time. label Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. postponed RFCs that have been postponed and may be revisited at a later time. 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.