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: delegation of implementation #1406

Closed
wants to merge 7 commits into from

Conversation

contactomorph
Copy link


## Associated types/constants

Unless explicitly set associated types and constants should default to the surrogate implementation value of the corresponding items.
Copy link
Contributor

Choose a reason for hiding this comment

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

How could a type explicitly set associated types different from the surrogate implementation without introducing a type error?

Copy link
Author

Choose a reason for hiding this comment

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

It will certainly introduce a type error. But if you need diverging associated types there's a high chance you want something more complex than delegation anyway. In that case you will certainly have to provide a specific implementation for most of your methods. The proposed feature is only worth it if the delegating type has a behaviour "similar to" the surrogate type. The more it diverges the less valid the entire approach becomes.

An alternative could be to allow additional delegating expressions for associated types. This would create a bridge between DelegatingType::AssociatedType and SurrogateType::AssociatedType and enable the compiler to transform one type into another where required. But in the end I felt that the situations where this would be the expected behaviour might be so specific that I didn't mention that possibility.

@dgrunwald
Copy link
Contributor

How does (partial) delegation interact with default impls?

trait T {
   fn f(&self);
   fn df(&self) { self.f(); }
}
impl T for i32 {
   fn f(&self) {
      println!("i32")
   }
}
struct MyType(i32);
impl T for i32 use self.0 {
   fn f(&self) {
      println!("MyType")
   }
}

If MyType::df() gets called, does it end up delegating to i32::df() (and thus i32::f()), or does it use the trait's default impl that calls MyType::f()?

Either choice leads to potentially confusing bugs; so maybe we shouldn't support partial delegation, but only full delegation?

Are there any other potentially problematic interactions with other features (e.g. specialization)?


Also, you should separate the possible extensions ("Inverse delegating expressions", "Combined delegation", "Function-based delegation", "More complex delegation") from the detailed design. I think most of those are not worth the additional complexity.

But the core feature idea, impl Trait for Type use Expr;, gets 👍 from me.

@leoyvens
Copy link

Thanks for writing this RFC, I agree with the concerns of inheritance overuse, this makes great use of the trait system 👍. The inverse expressions are more complex and it's not clear what super might be used for in the future. I prefer the "introduce a new trait" alternative to the function based one.

@burdges
Copy link

burdges commented Dec 13, 2015

Very nice.

I doubt partial delegation makes sense, as it gets you into inheritance issues, but maybe that's a use case for function delegation, assuming it makes sense. I donno if this ad hoc function delegation is wise either, but if if so maybe functions that could be delegated to could be marked as such, basically making them an anonymous trait. Or maybe this should be done with default impls anyways. Also, combined delegation looks harmless.

It's worth being more explicit that this deals with structs and tuple structs (newtypes), but apparently does not cover anonymous tuples, which it might, and maybe say why. Around enums, one might mention in Alternates that deriving mechanisms could potentially do enums, but they are likely heavier and remain an ongoing topic of research in the functional language community : https://stackoverflow.com/questions/3864647/how-does-deriving-work-in-haskell/3864801#3864801

@contactomorph
Copy link
Author

Also, you should separate the possible extensions ("Inverse delegating expressions", "Combined delegation", "Function-based delegation", "More complex delegation") from the detailed design.

Done.

How does (partial) delegation interact with default impls?

That's a good question. I think the most reasonable approach is to give delegation higher precedence over default impls and specialization in all situations. This rule is simple to understand and IMHO the most compatible with the principle of least astonishment.

I doubt partial delegation makes sense, as it gets you into inheritance issues.

To me it is the equivalent of the possibility to override a virtual method in a subclass in the OOP world so I'm not sure on which aspects it could be considered as an issue.

It's worth being more explicit that this deals with structs and tuple structs (newtypes), but apparently does not cover anonymous tuples, which it might, and maybe say why.

I focused on examples found in existing code which mainly deal with structs. I may miss something but I see no reason why this feature should be limited to a certain kind of type. Basically what you need is something that can support implementations (any type) and an expression to "convert" a type B into a type A. This does not impose any prerequisite on the nature of A and B. They can even be of heterogeneous kind.

@burdges
Copy link

burdges commented Dec 13, 2015

In that case, a delegation for an enum type might use a match statement? If I make a tagged union of a bunch of constructors with elements of distinct types implementing a trait T then potentially I can delegate through a match to each of them even though their implementations might be quite different?

@contactomorph
Copy link
Author

In that case, a delegation for an enum type might use a match statement?

Of course. That might not be a very good idea for readability reasons and a preferred approach would certainly be to encapsulate your match inside a function that would be called in the delegating expression. But it should be possible.

If I make a tagged union of a bunch of constructors with elements of distinct types implementing a trait T then potentially I can delegate through a match to each of them even though their implementations might be quite different?

I'm not sure exactly about the issue. If the variants of your enum are actually quite different, it naturally implies there is very little chance you can identify a single surrogate type that would make sense. So just don't use delegation at all in that case. However this is a consequence of the semantics of your objects (what your enum and your trait represent) not an issue with the compatibility of delegation and enums.

Now if you realize for example that a given enum B can be projected onto another enum A and that A already implements a trait that can meaningfully be imported by B, use delegation. Same if you can convert your struct field to a specific enum variant, or a tuple, or even a lambda ... As long as it makes sense according to the semantics of you domain I don't see why we should impose any a priori limitation on the kind of types that can delegate or be delegated to.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2015

The simple delegation case could be handled by the derive syntax. This would be much clearer imo. Usually impl notes that you are actually implementing something. derive on the other hand shows that you are simply basing the implementation of that trait on the implementation of the children of the type.

The derive syntax that could be used can be bikeshedded of course, but simply putting the expression after the type should be readable.

#[derive(PartialEq("self.1.abs()")]
struct A(i32, i64);

About partial delegation: that sounds more like you actually should split your trait into multiple traits.

@contactomorph
Copy link
Author

#[derive(PartialEq("self.1.abs()")]
struct A(i32, i64);

derive might be a better approach, however I'm definitely not in favor of a syntax where pieces of code are imbedded inside strings. This looks like a defeat of the type system to me.

About partial delegation: that sounds more like you actually should split your trait into multiple traits.

Your remark implies that you owns the trait and can modify it as you want. That's not necessarily true.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2015

however I'm definitely not in favor of a syntax where pieces of code are imbedded inside strings. This looks like a defeat of the type system to me.

neither am I, but that's currently the only available syntax. Caveat: you can do this in a crate instead of the standard library by using a different name than derive.

Your remark implies that you owns the trait and can modify it as you want. That's not necessarily true.

True, but my statement still stands, the owner should split it. Rust should not have syntax just for working around bugs in foreign code.

@Stebalien
Copy link
Contributor

@oli-obk,

The simple delegation case could be handled by the derive syntax. This would be much clearer imo. Usually impl notes that you are actually implementing something. derive on the other hand shows that you are simply basing the implementation of that trait on the implementation of the children of the type.

Not with the current macro system. As I stated earlier, you would need two macro expansion phases. derive works because the trait definition to be derived is hard-coded so derive doesn't need to look anything up. However, in this case, the trait to be derived could be anything and needs to be looked up and reflected at compile time.

@anmej
Copy link

anmej commented Dec 14, 2015

May I suggest using as instead of use? Impl Trait for Type as self.0 sounds almost human.

@Stebalien
Copy link
Contributor

@anmej How about via or by (like kotlin). as just doesn't sound right.

@anmej
Copy link

anmej commented Dec 14, 2015

I assume the devs don't want to add new keywords.

@blaenk
Copy link
Contributor

blaenk commented Dec 15, 2015

I was reminded of C++11's constructor inheriting.

impl<'a> Hash for H<'a> {
  use self.name impl;

  // can "replace"/override certain method impls
}

Or make it use impl self.name; if that is easier to parse.

By the way, what about enabling the adoption of all of the implementations, e.g. if self.name here can be compared to a Foo, a Bar, and other types, we could adopt them like this:

impl<Rhs> PartialOrd<Rhs> for Thing {
  use self.name impl;
}

If we only want certain ones then we can write them out explicitly (unless some shorter syntax is proposed, though I'm not necessarily eager to come up with one):

// adopt PartialOrd<Foo> and PartialOrd<Bar> implementations

impl PartialOrd<Foo> for Thing {
  use self.name impl;
}

impl PartialOrd<Bar> for Thing {
  use self.name impl;
}

@ahicks92
Copy link

As someone currently using C++ and considering Rust for future projects, this looks like it could solve almost all the places I'd actually want to use inheritance. For what it's worth, something like this would go a long way towards attracting me to the language. Definitely +1.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Dec 16, 2015
@contactomorph
Copy link
Author

May I suggest using as instead of use ? Impl Trait for Type as self.0

How about via or by (like kotlin). as just doesn't sound right.

Or make it use impl self.name; if that is easier to parse.

I have no strong opinion on the exact syntax as long it is short and consistent. I guess not introducing yet another keyword would be desirable.

As someone currently using C++ and considering Rust for future projects, this looks like it could solve almost all the places I'd actually want to use inheritance

This is good news. At the same time it shows there is a urgent need for better factoring features before the adoption of inheritance.

@Aatch
Copy link
Contributor

Aatch commented Dec 27, 2015

So my major concern is that it seems very difficult to produce useful error message. If the 300 issues tagged with A-diagnostics (the most-used tag) on rust-lang/rust are anything to go by, error messages are frequent source of confusion and problems. And this taking into account that many people praise the high quality of Rust's errors!

# Summary
[summary]: #summary

Provide a syntactic sugar to automatically implement a given trait `Tr` using a pre-existing type implementing `Tr`. The purpose is to improve code reuse in rust without damaging the orthogonality of already existing concepts or adding new ones.
Copy link
Member

Choose a reason for hiding this comment

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

s/rust/Rust/ (proper noun)

@shepmaster
Copy link
Member

This seems like a great step forward, and I am a big fan of encouraging composition over inheritance. Similar to the concerns about inheritance, I think that Deref / DerefMut are misused because they give the ability to reuse code without much interaction between a programmer and keyboard (cause we are lazy). The key, as you point out, is to make doing the right thing as easy (or easier) than the incorrect thing.

I've also started on a compiler plugin to do the same thing and quickly ran into similar issues. You simply don't know the relevant types at the right time of the process. I think that would be the same problem with a derive-based implementation proposed by @oli-obk.

@Aatch do you have any specific issues in mind? Conceptually, I'd hope that the errors would basically be the same as if we had a macro that expanded to the impl.

behnam added a commit to open-i18n/rust-unic that referenced this pull request Aug 8, 2017
Character Properties are of different kinds and shapes, and as UNIC
components grow, we need a better way to be able to categorize them by
their shape, and a way to make sure we have consistent, noncolliding
API for them.

This is the first step into building a CharProperty taxonomy, with as
little as possibly needed to provide the assurances desired.

We hope that the implementation can be improved over time with new
features added to the language. There's already some proposals in this
front. See these discussions for more details:

* [Traits as contract, without changes to call-sites](https://users.rust-lang.org/t/traits-as-contract-without-changes-to-call-sites/11938/11>)

* [RFC: delegation of implementation](rust-lang/rfcs#1406)
behnam added a commit to open-i18n/rust-unic that referenced this pull request Aug 8, 2017
Character Properties are of different kinds and shapes, and as UNIC
components grow, we need a better way to be able to categorize them by
their shape, and a way to make sure we have consistent, noncolliding
API for them.

This is the first step into building a CharProperty taxonomy, with as
little as possibly needed to provide the assurances desired.

We hope that the implementation can be improved over time with new
features added to the language. There's already some proposals in this
front. See these discussions for more details:

* [Traits as contract, without changes to call-sites](https://users.rust-lang.org/t/traits-as-contract-without-changes-to-call-sites/11938/11>)

* [RFC: delegation of implementation](rust-lang/rfcs#1406)
behnam added a commit to open-i18n/rust-unic that referenced this pull request Aug 9, 2017
Character Properties are of different kinds and shapes, and as UNIC
components grow, we need a better way to be able to categorize them by
their shape, and a way to make sure we have consistent, noncolliding
API for them.

This is the first step into building a CharProperty taxonomy, with as
little as possibly needed to provide the assurances desired.

We hope that the implementation can be improved over time with new
features added to the language. There's already some proposals in this
front. See these discussions for more details:

* [Traits as contract, without changes to call-sites](https://users.rust-lang.org/t/traits-as-contract-without-changes-to-call-sites/11938/11>)

* [RFC: delegation of implementation](rust-lang/rfcs#1406)
behnam added a commit to open-i18n/rust-unic that referenced this pull request Aug 9, 2017
Character Properties are of different kinds and shapes, and as UNIC
components grow, we need a better way to be able to categorize them by
their shape, and a way to make sure we have consistent, noncolliding
API for them.

This is the first step into building a CharProperty taxonomy, with as
little as possibly needed to provide the assurances desired.

We hope that the implementation can be improved over time with new
features added to the language. There's already some proposals in this
front. See these discussions for more details:

* [Traits as contract, without changes to call-sites](https://users.rust-lang.org/t/traits-as-contract-without-changes-to-call-sites/11938/11>)

* [RFC: delegation of implementation](rust-lang/rfcs#1406)
@contactomorph
Copy link
Author

contactomorph commented Aug 11, 2017

I guess I'm asking for it to be "resolved". The main goal of the RFC is to provide a syntactic niceness, so it seems important to build consensus around the final syntax.

@cramertj Fine. Then my opinion is that the syntax should support these features: 

  • associate an expression to a list of method names 
  • provide a shortcut to express the idea of delegating all methods from the delegated trait 
  • allow several expressions to be provided for different subsets of methods (mixed delegation) 

As usual a dedicated keyword is of course clearer (the delegate to possibility is nice) but that's not something that we can afford for a cheap price. Can we still consider this as a workable option? 
 
I used keyword "use" because it was free at this time in this context and because it sounded fairly consistent with the semantics of delegation. I have no problem with the fact it should keep its usual role because we would like to make that role valid in more places. 
 
As I already said I'm not a big fan of attributes in this context because:

  • you suddenly have to interpret string content as code (sure this is not a big problem for a single field but it is for a more general expression)
  • you are artifically giving the impression that you do not add new keywords whereas you are in fact creating a parallel category of keywords

May I suggest also impl MyTrait for MyType => self.0; (and similar operators)? An existing keyword might be hard to fit in there and doesn't guarantee clarity. For methods we can do even better, IMO:
impl MyTrait for MyType { fn method = self.field.method; }

@eddyb I would find it pretty obscure not to have similar syntaxes for two variations on the same idea. Additionally the second proposition does not extend very nicely to a list of methods. Why not

impl MyTrait for MyType { fn method1, method2 => self.field; }

@est31
Copy link
Member

est31 commented Aug 21, 2017

@contactomorph could you consider signing off to the MIT/Apache 2.0 licensing terms inside this issue: #2096 ? Thanks!

@contactomorph
Copy link
Author

@aturon @eddyb @nikomatsakis @nrc @pnkfelix @withoutboats would it be possible to add an "Ergonomics Initiative" lavel to this Pull request ? Thank you

@petrochenkov petrochenkov added the Ergonomics Initiative Part of the ergonomics initiative label Aug 24, 2017
@aturon
Copy link
Member

aturon commented Aug 31, 2017

I'm going to move to postpone this RFC. While this remains an important problem the lang team would like to tackle, there remain blocking issues with the RFC as proposed.

An effort was started to develop a new, more conservative design, and we'd very much love to see that come to fruition, but at this point it seems highly unlikely to land before the impl period.

Thanks, @contactomorph and many, many others for continuing to push on this.

@rfcbot fcp postpone

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Aug 31, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 31, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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 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 19, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 19, 2017

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

@aturon
Copy link
Member

aturon commented Sep 20, 2017

I'm going to go ahead and close this out as postponed, given that the impl period has started. I hope folks can pick this back up toward the end of the year!

@aturon aturon closed this Sep 20, 2017
@Kixunil
Copy link

Kixunil commented Sep 20, 2017

😢

@crlf0710
Copy link
Member

Today is the end of the year.

@WiSaGaN
Copy link
Contributor

WiSaGaN commented Jan 25, 2018

@aturon what is the criterion for reopening this issue to continue discussion? This feature is one of the few ergonomics changes that benefit both new users and existing users without adding much cognitive load. If we want to focus on polishing the language instead of opening other new RFCs this year, this is one of the good candidates.

@scottmcm
Copy link
Member

@aturon
Copy link
Member

aturon commented Jan 27, 2018

@WiSaGaN I completely agree, and would love to see more progress here. As @scottmcm pointed out, the linked thread is probably the best place to pick things back up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ergonomics Initiative Part of the ergonomics initiative 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.