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: Implied #[derive(SuperTrait)] #2385

Closed
wants to merge 4 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Apr 2, 2018

🖼️ Rendered

📝 Summary

When deriving standard library such as Copy, the transitive closure of all super traits will also be implicitly derived. That is, it is sufficient to #[derive(Copy)] to get the Clone impl as well.

💖 Thanks

To @aturon, @nikomatsakis, and discussions at Rust All Hands for the idea. See the internals thread for some discussion.

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Apr 2, 2018
Copy link
Contributor

@sgrif sgrif left a comment

Choose a reason for hiding this comment

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

I do not think that this is anywhere near important enough that we should be considering breaking changes. Given that this RFC intentionally limits the scope to built-in derives, the compiler should have more than enough information to do this without breaking if a manual impl is present.


### Custom derive

+ This RFC does **not** affect the behavior of `#[derive(..)]` for traits
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate, but does answer the question I was going to ask about how trait resolution comes into play

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also the section "Future work" wrt. #[proc_macro_derive(Sub, implies(Super))].


## Breakage

This RFC will break some code. Specifically, if a user has already manually
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think that this should be considered acceptable. You've used Clone as your example, in which case, sure -- That's not realistic breakage that is going to happen. However, it is perfectly reasonable to have a manual impl of PartialEq, which is represents full equality, and just want to #[derive(Eq)]. I do not think it is worth breaking the stability guarantees that Rust has promised because it leads to other inconveniences.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, it is perfectly reasonable to have a manual impl of PartialEq, which is represents full equality, and just want to #[derive(Eq)].

That's exactly what I wanted to say as well.
Since Eq is just a marker, deriving it is common while manual implementation of PartialEq is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a perfectly reasonable objection. However, I think that the scenario you mention will be unlikely, and easily mitigated with just invoking rustfix and then it becomes #[derive(only(Eq))] which is also more clear wrt. intent. On balance, I think the net win is significant enough to warrant the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like seeing rustfix mentioned as an argument for breaking changes. a) It's alpha, we cannot rely on it being ready and working perfectly well with the edition, b) it still introduces aggrevation, and rustfix is only a stopgap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is (at least to me) an argument for edition breaking changes having a less severe impact. How much of a mitigating factor rustfix should be considered as is of course subjective.

Regarding your points:
a) We have until August? rustfix will have to be sufficiently well functioning for other changes such as those for modules. Furthermore, the analysis around #[derive(..)] should not be very complicated because we are dealing with a fixed and very limited set of traits where the transitive closures have low cardinality.

b) I can't quantify aggrevation so I am not sure how I should reason about this.

Copy link
Member

Choose a reason for hiding this comment

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

The idea of rustfix is to allow people to deal with breakage in an easier way. It is not meant to allow or support breakage in the first place. Rustfix or not, breakage is still bad and should be avoided if possible. Here it is definitely possible and the arguments brought in favour of breakage don't really seem convincing to me.

Copy link

Choose a reason for hiding this comment

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

Additionally, while rustfix might be able to take over the "change code" phase of a migration, in many if not most cases you will still want or need to review all changes. This becomes more work the more things rustfix fixes. And at some point there will be situations where you'll want to move a 10 year old code base to current edition, and you'll have to work through the accumulated changes in 3 editions.


### Mitigating factors

1. We can do this breakage as part of edition 2018.
Copy link
Contributor

Choose a reason for hiding this comment

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

"can" != "should"

I really do not want to see "there's a new edition" be used as an excuse to make breaking changes that would not otherwise be considered. If we go down that path, IMO Rust has failed at its stability promises.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree wrt. breaking stability promises. This, or similar changes wrt. breakage, will not break code written for edition 2015. Only code that opts in to edition 2018 will see any breakage and so no code that worked before will break when upgrading your version of rustc unless you make changes and opt into the new edition.

Further, rustfix can easily help people migrate semi-automatically without doing any more work than just accepting all the fixes. In particular, if we permit #[derive(only(Foo))], then rustfix can output a very small diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really do not want to see "there's a new edition" be used as an excuse to make breaking changes that would not otherwise be considered.

I'm not sure what you mean by "would not otherwise be considered". This is a change we've wanted to make for a long time -- dating from #1028 -- but we backed off last time because of stability concerns. Part of the point of an edition (not the whole point, but an important point) was to let us revisit questions like this, where the expected impact is low (and can of course be mitigated via lints and the usual transition story). Anyway, I don't like arguing in GH line comments, so I'll leave it at that, but perhaps below I'll try to make the 'positive case' for this change a bit stronger (i.e., why it's valuable from an ergonomics and learnability POV).

Copy link
Contributor

Choose a reason for hiding this comment

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

But how does your comment from back then not apply anymore?

3. It is expected that the breakage will be relatively small because situations
where `Copy` is derived but `Clone` is implemented is rare.
Furthermore, it `Ord` it could be downright risky to derive `Ord` but
manually implement `PartialEq`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean PartialOrd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.


3. It is expected that the breakage will be relatively small because situations
where `Copy` is derived but `Clone` is implemented is rare.
Furthermore, it `Ord` it could be downright risky to derive `Ord` but
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? If I have a struct with some super internal private field which should not be taken into account either for ordering or equality, why is this dangerous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementations of PartialEq, PartialOrd, and Ord must agree with each other. It's easy to accidentally make them disagree by deriving some of the traits and manually implementing others.

https://doc.rust-lang.org/nightly/std/cmp/trait.PartialOrd.html#how-can-i-implement-partialord

It occurs to me that in the case the super internal private field which should not be accounted either for ordering or equality, you will manually impl all the above traits and you should, because the behavior becomes much more clear then.

Also, cc @nikomatsakis on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Deriving PartialOrd or Ord (but not both) is risky because they must match or things will go awry; if you (e.g.) implement PartialOrd manually to ignore a field, but you derive Ord, then Ord will not ignore the field. This is why clippy, for example, has a lint against this -- or I think it does? At least there is an issue requesting one rust-lang/rust-clippy#1621 =)

Choose a reason for hiding this comment

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

@nikomatsakis, for the orderings, once specialization (rust-lang/rust#31844) works, it would be much better to provide the blanket implementations impl<T: Ord> PartialOrd for T, impl<T: Eq> PartialEq for T, impl<T: PartialOrd> PartialEq for T and the disambiguating impl<T: Eq + PartialOrd> PartialEq for T. That would automatically produce consistent set by defining just Ord, PartialOrd or Eq as desired while specialization should still just take the explicit implementations where provided (I realize someone might have some other complex blanket impls that may not be clearly specializations, but for the epoch I would consider it worthy as it makes defining them much, much easier).

Similar treatment should be possible for Clone, i.e. impl<T: Copy> Clone for T for the same effect as this RFC. And of the usually derived set, I don't think anything else would need this treatment. That is, @Centril, I am proposing this (adding blanket definitions once specialization works) as an alternative.

Note: also the non-derived FnOnce, FnMut and Fn would benefit from similar treatment. It would defining function objects by hand actually practical.

However, with the future work outlined in the subsection [rustdoc improvements],
this drawback can be mitigated.

## Surprising for Haskell developers
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need to be called out as a drawback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A drawback however insignificant should be mentioned. I always try to write exhaustive RFCs.

Copy link
Contributor

@sgrif sgrif Apr 2, 2018

Choose a reason for hiding this comment

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

In that case, should we call out Ruby modules being able to include/extend/prepend other modules in the prior art section? There are likely several other languages that deserve consideration here as well

Copy link
Contributor Author

@Centril Centril Apr 3, 2018

Choose a reason for hiding this comment

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

Please link the relevant documentation or create a PR against my branch and I will certainly include it in the prior art.

Haskell was specifically mentioned because my familiarity with it and because our deriving system is based on Haskell.

@Centril
Copy link
Contributor Author

Centril commented Apr 3, 2018

Thank you for the elaborate review @sgrif. I do think the breakage is an important point you make and I've written my thoughts in replies above.

Given that this RFC intentionally limits the scope to built-in derives, the compiler should have more than enough information to do this without breaking if a manual impl is present.

For now yes, that is the intent. However, my preference would be to extend this as much as possible to custom derive macros in the future. A consistent experience in the ecosystem is important to me.

I think you are more familiar with the compiler internals than me, so forgive me, and please point out if I am wrong, but changing the behavior here as you propose would require delaying #[derive(..)] of libstd traits until after resolution of impls, and that might not be easy at all?

@est31
Copy link
Member

est31 commented Apr 3, 2018

I myself am sometimes annoyed when I have to type #[derive(PartialEq, Eq)], so generally I'm positive about the change.

The point about not doing breaking changes light heartedly by @sgrif is legitimate though... What about keeping #[derive] the way it is and introducing #[derive_recursive(Eq)] with the semantics this RFC talks about?

@Centril
Copy link
Contributor Author

Centril commented Apr 3, 2018

@est31 If we'd do that, and we could, I'd name the attribute #[derive_transitive(Eq)] instead.

However; I think derive_transitive would be less ergonomic. It would also hurt teachability due to having two separate concepts derive and transitive, and so it becomes less clear to the user what they should do. Of course, we could lint in favor of derive_transitive. The new attribute is somewhat more readable, which is a plus.

@est31
Copy link
Member

est31 commented Apr 3, 2018

It would also hurt teachability due to having two separate concepts

If you added #[derive(only(Eq))] you'd have two separate concepts as well. I just suggest different naming. The benefit is that we won't be forced to do a breaking change. I don't agree that derive_transitive is less ergonomic but even if it is... is it really worth a breaking change?

@squishy-clouds
Copy link

We could have the transitive derives be opt-in with something like #[derive(transitive(Ord))]. It doesn't introduce a new attribute like #[derive_transitive], and also, as far as I know, doesn't introduce any breaking changes.


2. We can give good error messages and help users to migrate with `rustfix`.

3. It is expected that the breakage will be relatively small because situations
Copy link
Member

Choose a reason for hiding this comment

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

Note that servo has code that #[derive(Eq)] but implements PartialEq manually.

https://github.com/servo/servo/blob/122bfa03e1fba01dc7d52e63268a83492d994b53/components/servo_arc/lib.rs#L771-L815

There were also tons of code which #[derive(Copy)] but implements Clone themselves because [T; n] did not implement Clone for n > 32.

https://github.com/sfackler/rust-openssl/blob/f63b9f05a3b1114dc058e461da24a3c7d867f207/openssl/src/hash.rs#L226-L241

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks; I'll make notes of this in the RFC text.

@Centril
Copy link
Contributor Author

Centril commented Apr 3, 2018

@est31

If you added #[derive(only(Eq))] you'd have two separate concepts as well.

Granted; however - the separate concept feels "smaller" from a teachability perspective to me and something that can be more easily relegated to an advanced section. We also have older teaching material on the internet to consider.

I don't agree that derive_transitive is less ergonomic

I'd like to highlight two separate concepts here which often get intermingled in discussion to a single one:

  • Ergonomics - is the degree to which it is ergonomic to write and develop code. This is mainly about the ease to write code.

  • Readability - is not the same thing as Ergonomics but rather the ease with which you can read code. This is arguably more important than ergonomics.

With these definitions of the concepts, it seems to me that derive_transitive adds _transitive and is therefore marginally less ergonomic. At the same time, derive_transitive is more clear on intent than derive which makes it arguably more readable.

is it really worth a breaking change?

I think this is the hard decision we face in this RFC; I think many will agree that the transitive behavior is what they want, but the breakage is not enticing. It is certainly a judgement call, and I was also initially skeptical. What tipped the scale for me was mainly the ease with which you can rustfix away the breakage and get a small and clean diff that simply changes #[derive(Copy)] to #[derive(only(Copy))].

@squishy-clouds

We could have the transitive derives be opt-in with something like #[derive(transitive(Ord))]. It doesn't introduce a new attribute like #[derive_transitive], and also, as far as I know, doesn't introduce any breaking changes.

In isolation this is fine; but it does unfortunately not scale:

#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
#[derive(transitive Copy, transitive Ord, Debug, Hash)]
#[derive_transitive(Copy, Ord, Debug, Hash)]
#[derive(Copy, Ord, Debug, Hash)]
pub enum Option<T> { None, Some(T), }

between transitive(Ord) and derive_transitive I'd go with the latter because the reduced repetition.

In any case, and for fairness, I will write down transitive(Foo) and derive_transitive as alternatives.

@burdges
Copy link

burdges commented Apr 3, 2018

I agree this sounds unimportant, so not worth any headaches.

If this happens, then I'd think #[derived_required] sounds more intuitive than `#[drive_transative]. Also, one might avoid arguments like

#[derive(Copy)]
#[derive_required] // derive Clone because this followsCopy
#[derive(Eq)] // Do not derive PartialEq because no #[derive_required] follows this
struct Foo{ ... }

Imho, we should explore more aggressive usage of associated constants and const generics before encouraging trait forests like PartialEq, etc., ala

pub trait Eq<Rhs: ?Sized = Self> {
    const TOTAL : bool = true;

    fn eq(&self, other: &Rhs) -> bool;
    fn ne(&self, other: &Rhs) -> bool { !self.eq(other) }
}

You might want TOTAL not to exist if Rhs != Self here, but one could do that with associated types and specialization.

reach from `x` in one or more steps.

- **super trait** - For a trait `Copy`, defined as `trait Copy : Clone {}`,
the trait `Clone` is a super trait of `Clone`. We also say that if `T: Copy`,

Choose a reason for hiding this comment

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

Should one of the Clones be a Copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it should, thanks.

@Ixrec
Copy link
Contributor

Ixrec commented Apr 3, 2018

I'm not sure that we want or need a fully general solution, as opposed to tweaks targeted specifically at "the standard traits".

How often does this pattern arise in crates other than std? Every time I've this ergonomic hiccup it's because the author wanted to derive "all the standard traits", and maybe De/Serialize, and that's it.

In the past there've been some comments about the type safety of PartialEq/Ord not being worth their ergonomic cost; did that ever go anywhere?

Maybe we could make this behavior a per-trait opt-in thing. Copy, Clone seems like by far the most compelling example, so we could add a #[derive_includes_supertraits] attribute on Copy first and see how that goes.

@Centril
Copy link
Contributor Author

Centril commented Apr 3, 2018

@burdges

If this happens, then I'd think #[derived_required] sounds more intuitive than #[drive_transative]. Also, one might avoid arguments like

I assume you meant recursive and not required here. Could you please elaborate on why you think recursive is more intuitive than transitive? Transitive here refers to the transitive closure property wrt. super-traits. The transitive closure R+ of a relation R can be defined recursively but this seems far fetched instead of just directly referring to the transitive closure.

I do think however that #[derive_transitive] as a modifier without arguments seems less ergonomic and direct than simply #[derive_transitive(Copy, Eq)].

There's also the problem that we can't guarantee that custom derive macros would honor the transitive closure property of #[derive_transitive] as done for libstd traits, so naming the attribute like so could be misleading.

Imho, we should explore more aggressive usage of associated constants and const generics before encouraging trait forests like PartialEq, etc., ala

That does seem neat, but I'm not sure such a large change can be done now. It seems a bit too late for such a design. Tho one should consider such hierarchies for new traits in the future.

@Ixrec

I'm not sure that we want or need a fully general solution, as opposed to tweaks targeted specifically at "the standard traits".

This RFC proscribes tweaks to the libstd traits tho. The RFC does not propose a fully general solution in reality. However, the Future work section does hint at possible roads ahead for per-macro opt-in generality.

How often does this pattern arise in crates other than std? Every time I've this ergonomic hiccup it's because the author wanted to derive "all the standard traits", and maybe De/Serialize, and that's it.

Consider a hierarchy such as with monoids, semigroups, etc. It would be, at least to me, quite desirable to derive all super traits implicitly when you derive a trait up the hierarchy.

In the past there've been some comments about the type safety of PartialEq/Ord not being worth their ergonomic cost; did that ever go anywhere?

Not sure what this is referring to. Links?

Maybe we could make this behavior a per-trait opt-in thing. Copy, Clone seems like by far the most compelling example, so we could add a #[derive_includes_supertraits] attribute on Copy first and see how that goes.

This would also be an edition-break. However, to me, the opt-in behavior is a property of the deriving macro, not of the trait, and as such, the proc macro should be annotated, as hinted at in the Future work section, and not the trait.

@Ixrec
Copy link
Contributor

Ixrec commented Apr 3, 2018

In the past there've been some comments about the type safety of PartialEq/Ord not being worth their ergonomic cost; did that ever go anywhere?

Not sure what this is referring to. Links?

rust-lang/rust-roadmap-2017#17 (comment)

Come to think of it, some of the discussion on that issue is basically about the mechanism this RFC proposes.

Maybe we could make this behavior a per-trait opt-in thing. Copy, Clone seems like by far the most compelling example, so we could add a #[derive_includes_supertraits] attribute on Copy first and see how that goes.

This would also be an edition-break. However, to me, the opt-in behavior is a property of the deriving macro, not of the trait, and as such, the proc macro should be annotated, as hinted at in the Future work section, and not the trait.

Wow, somehow I completely missed that part of the Future work section.

So, my poorly-expressed intent was that the supertrait would get auto-derived only if no manual impl existed (I think someone suggested that in an earlier comment but I can't see it now), which I assumed would be a slightly magical behavior only feasible for std traits. Hence the implications this is more "targeted" than the RFC's mechanism (in the sense that it could not be extended to non-std traits), and would not be a breaking change (even in the sense allowed by editions).

Consider a hierarchy such as with monoids, semigroups, etc. It would be, at least to me, quite desirable to derive all super traits implicitly when you derive a trait up the hierarchy.

Although this is a massive tangent I don't want to get too deep into, I'm very skeptical that higher-order categorical traits like these (what is the proper term?) are particularly valuable in a language that's not deeply pure and immutable the way Haskell is, especially in Rust since we can't even write the "building block" higher-order functions like map() without some kind of mutability/ownership polymorphism that afaik no one's proposing. So this ergonomic issue with derives still feels like a "std traits problem" to me.

@Centril
Copy link
Contributor Author

Centril commented Apr 4, 2018

@Ixrec

rust-lang/rust-roadmap-2017#17 (comment)

Right; so @aturon's point there on the papercut of #[derive(PartialOrd, Ord, PartialEq, Eq)] is what is being solved with this RFC.

So, my poorly-expressed intent was that the supertrait would get auto-derived only if no manual impl existed

I refer you to @sgrif's review here and my reply, here.

Although this is a massive tangent I don't want to get too deep into, [..]

Although I don't agree with you on the usefulness of algebra traits, they were just an example -- where there are trait hierarchies, which can be entirely domain specific, transitive deriving reduces boilerplate and improves ergonomics.

@est31
Copy link
Member

est31 commented Apr 4, 2018

@Centril

If you added #[derive(only(Eq))] you'd have two separate concepts as well.

Granted; however - the separate concept feels "smaller" from a teachability perspective to me and something that can be more easily relegated to an advanced section. We also have older teaching material on the internet to consider.

Breaking changes are absolutely toxic for teaching material. It is one thing if the code you paste is giving you a warning or slightly less idiomatic, but a completely different thing if it doesn't even compile.

Another teachability downside of your proposal is the inconsistency with the library situation:
Here, #[derive] would still only derive single traits and not recursively. So if there are more complex trait structures, users might be surprised that derive doesn't derive the supertraits automatically for them.

is it really worth a breaking change?

I think this is the hard decision we face in this RFC; I think many will agree that the transitive behavior is what they want, but the breakage is not enticing.

You have summarized this really well 👍

What tipped the scale for me was mainly the ease with which you can rustfix away the breakage and get a small and clean diff that simply changes #[derive(Copy)] to #[derive(only(Copy))].

I feel that such a change would be quite spammy... It would be better if rustfix only changed the #[derive(PartialEq, PartialOrd, Eq, Ord)] into #[derive(transitive(Eq, Ord))]. Then its changes would always make code look nicer, not make it look less nice.

It making breaking changes easy should not be a reason to make breaking changes light heartedly. I don't want to be required to use rustfix, and when I use it I want to have small diffs, I don't want every single line in my program to change.

We could have the transitive derives be opt-in with something like #[derive(transitive(Ord))]. It doesn't introduce a new attribute like #[derive_transitive], and also, as far as I know, doesn't introduce any breaking changes.

In isolation this is fine; but it does unfortunately not scale

What about #[derive(transitive(Copy, Ord), Debug, Hash)]?

@comex
Copy link

comex commented Apr 4, 2018

I think you are more familiar with the compiler internals than me, so forgive me, and please point out if I am wrong, but changing the behavior here as you propose would require delaying #[derive(..)] of libstd traits until after resolution of impls, and that might not be easy at all?

I think the compiler could 'just' say:

  • If I'm about to produce a "conflicting implementations of trait" error, and
  • one of the impls has a special attribute which #[derive(..)] would attach to implicit supertrait impls,
  • then just delete that impl entirely and keep going.

Alternately, you can get 99% of the way there after only performing name resolution, by looking for impls of the syntactic form, e.g.,

impl<…> Clone for MyStruct<…> where …

The autogenerated impl can't conflict with any valid impl that's not of this form – the only possibility would be a blanket impl of Clone for all T, but that would be a coherence violation. And it should always conflict with an impl that is of this form, because even if the manual impl names specific types as parameters to MyStruct, that would still conflict with the autogenerated generic impl (...right?). In particular, the autogenerated impl does not use default fn, so there's no way to avoid conflict even with specialization.

There might be a question about impls of the form

impl<…> Clone for <A as B>::C where …

where A is a concrete type, and <A as B>::C happens to evaluate to MyStruct<something>. These impls are not currently allowed by the compiler at all - it doesn't resolve associated types eagerly enough - but you'd have to figure out what to do if they became allowed in the future.

PartialEq is a bit harder, because given an impl like:

impl<…> PartialEq<…> for MyStruct<…> where …

…you have to determine whether the parameter to PartialEq is equivalent to the self type (or generic), which is where there would be a conflict. It seems that rustc does allow associated type projections in generic parameters, e.g.

impl PartialEq<<A as B>::C> for A

…where <A as B>::C may or may not resolve to A.

I don't know much about compiler internals, so I have no idea whether this would actually be hard to determine. But even if it were, it's also probably not a big deal if the 2018 edition broke code using this very specific weird pattern.

@Centril
Copy link
Contributor Author

Centril commented Apr 4, 2018

@est31

Breaking changes are absolutely toxic for teaching material.

I must unfortunately absolutely 👍 with you here.
However, from a teachability POV, the default should also be the unannotated version, so if we'd restart the clock, then I'd pick only over transitive. What weights more on scale? Hard to say.

Another teachability downside of your proposal is the inconsistency with the library situation:

Not sure I understand -- what is the "library situation"? Does it refer to custom derive macros?

I feel that such a change would be quite spammy... It would be better if rustfix only changed the #[derive(PartialEq, PartialOrd, Eq, Ord)] into #[derive(transitive(Eq, Ord))]. Then its changes would always make code look nicer, not make it look less nice.

Certainly rustfix could transform #[derive(PartialEq, PartialOrd, Eq, Ord)] into just #[derive(Ord)]. The only cases where you'd need only would be when you have the sub-trait derived but not the super traits.

I don't want to be required to use rustfix, and when I use it I want to have small diffs, I don't want every single line in my program to change.

Ideally, in a lot of cases, it would / should be as easy as:

<change the edition + version manually>
$ rustfix && cargo test &&
  git add . && git commit -m "I did rustfix" && git push &&
  cargo publish

What about #[derive(transitive(Copy, Ord), Debug, Hash)]?

That's not so bad 👍
Ideally, there would be something shorter than transitive but just as clear... ideas?

@comex

I think the compiler could 'just' say:

  • If I'm about to produce a "conflicting implementations of trait" error, and
  • one of the impls has a special attribute which #[derive(..)] would attach to implicit supertrait impls,
  • then just delete that impl entirely and keep going.

My initial reaction to this is that it would be an ingenious construction.
However, I have some concerns that you can hopefully address:

  • How bad will the effect be on compile times?
  • What is the interaction with specialization?
    Specialization (in the general sense and not necessarily always-applicable.
    Does coherence save us here thanks to "the only possibility would be a blanket impl<T> of Clone for T, but that would be a coherence violation"?

Alternately, you can get 99% of the way there after only performing name resolution, by looking for impls of the syntactic form, e.g.,

This is pretty interesting idea!

In particular, the autogenerated impl does not use default fn, so there's no way to avoid conflict even with specialization.

Is that not desirable tho for a derived impl to use default fn so that it can be overridden?

But even if it were, it's also probably not a big deal if the 2018 edition broke code using this very specific weird pattern.

Indeed it is quite weird. I'd say that the assertion that this would break approximately 0 code would not be bold, and probably true.

@phaylon
Copy link

phaylon commented Apr 4, 2018

With regard to rustfix, can it ever be 100%? What about things generated by macros, or via build.rs? These could mean that to get your code to build, you might have to fix dependencies.

@squishy-clouds
Copy link

squishy-clouds commented Apr 4, 2018

Ideally, there would be something shorter than transitive but just as clear... ideas?

#[derive(super(Copy, Ord), Debug, Hash)]?

or maybe #[derive(trans(Copy, Ord), Debug, Hash)]

@clarfonthey
Copy link
Contributor

Annotating transitivity for built-in derives begs the question of how you'd do that for proc macros, which already can transitively derive things without having to annotate it.

So, I'd rather allow it for builtins without an annotation.

@sgrif
Copy link
Contributor

sgrif commented Apr 4, 2018

Right; so @aturon's point there on the papercut of #[derive(PartialOrd, Ord, PartialEq, Eq)] is what is being solved with this RFC.

The point is that the traits themselves are not worth it. That has very little to do with the derive. The actual item on there is:

Potentially deprecate PartialEq/PartialOrd (@aturon)

The ergonomic hit comes from more than just the additional derives.

@kornelski
Copy link
Contributor

kornelski commented Apr 5, 2018

+1 to the problem and proposed syntax
-1 to proposed implementation

This:

#[derive(Copy)]
struct Foo;

impl Clone for Foo

should continue to work with this change. I'd expect the implied #[derive(Clone)] to be created only if there is no manual implementation. Given Rust's whole-crate compilation model I expect it to be clever about such things and search the whole create for possible implementations to avoid making a conflicting one.

So instead of #[derive(Copy)] unconditionally expanding into #[derive(Copy, Clone)] and causing breakage, it should be #[derive(Copy)] if there's impl Clone, and #[derive(Clone)] being used only as a fallback in case there's no user impl.

#[derive_transitive(Copy)]/#[derive(only(Copy))] look to me like spoonfeeding compiler redundant information that it could figure itself out.

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2018

What would it take to get blanket impls for this stuff working? I'd love to just have

bikeshed impl<T: Copy> Clone for T {
    fn clone(&self) -> Self { *self }
}
bikeshed impl<T: Ord> PartialOrd for T {
    fn partial_cmp(&self, other: &Rhs) -> Option<Ordering> { Some(self.cmp(other)) }
}
// etc

in core and be done with it, no special derive syntax needed at all --- just #[derive(Copy)] would get you Clone for free (as would impl Copy for YourTypeHere {}).

(Edit: bikeshed there might be default, but it might not be. It might be #[rustc_never_stable_magic_ignore_on_conflict] like would be nice on impl<T> From<!> for T too.)

@Centril
Copy link
Contributor Author

Centril commented Apr 5, 2018

Summarizing discussions with @scottmcm on IRC:

If bikeshed = default, then my understanding of that is that default impl<T: Copy> Clone for T { .. } does not actually create a blanket impl but rather only leaves a blueprint which you can then use.

With respect to specialization and the always applicable rule, the above impl would iirc translate into the following chalk query:

forall<A> {
    if (WellFormed(A)) {
        exists<T> {
            T = A,
            T: Copy
        }
    }
}

However, it does not flow from WellFormed(A) that T: Copy, and so the impl would be rejected under the always-applicable rule because it fails the "it relies only on implied bounds" test.

@jan-hudec
Copy link

jan-hudec commented Apr 11, 2018

@gavento,

Perhaps RFC 2010 (trait impl specialization) would help here

It would have the huge advantage of actually working for the manual implementations too. impl Ord for ... would give you all four and impl PartialOrd for ... would give you also PartialEq (but not Eq), so you wouldn't actually have to care about it even when you start manually providing the implementations.

And it's even more important then, because then you can't use the other derives now even if the other implementations are trivial.

@scottmcm
Copy link
Member

scottmcm commented Apr 13, 2018

Thank you, boats, for that helpful re-framing of the discussion. It makes me, overall, see this discussion as a mirror to a bunch of the discussions in C++ land about Concepts design. For example,

A good useful concept supports a fundamental concept (pun intended) by supplying the set of
properties – such as operations and member types – that a domain expert would expect. The
mistake made for Drawable and Addable was to use the language features naively without regard
to design principles. ~ http://www.stroustrup.com/good_concepts.pdf

The intent of concepts is to model semantic categories (Number, Range, RegularFunction) rather than syntactic restrictions (HasPlus, Array). According to ISO C++ core guideline T.20, "The ability to specify a meaningful semantics is a defining characteristic of a true concept, as opposed to a syntactic constraint." ~ http://en.cppreference.com/w/cpp/language/constraints

I wonder if a better answer than supertraits is defining a Rust version of what Fundamentals of Generic Programming (Dehnert and Stepanov, '98) calls a Regular Type. We're already doing better than C++ here—we don't need assignment (well, so long as we don't have !Move...) and don't need to explicitly include both == and != as separate things—but I think the idea translates well.

That is, redefine the problem from subtrait/supertrait to needing to know the list of traits at all.

1st Draft: #[derive(Regular)] provides Default + Clone + PartialEq + Eq + Hash + Debug.

The thought process for that list is basically "anything where there's not really any interesting choices or downsides for a boring type, so the derived one is what you want and not a future hazard".

(Regular could, I suppose, even be a trait alias, as things like String and HashMap meet it fine.)


Digression: The careful reader may have noticed that I didn't mention PartialOrd even though Fig. 1 of the Stepanov paper includes "Ordering". My main objection there is that the derived orderings are the only way safe code can tell the declaration order of braced-struct fields and of enum variants. (mem::Discriminant is Eq but not PartialOrd.) Thus the current derive leads to things like the confusingly-different behaviours of Ord::min and Ord::max, problems like seen in rust-lang/rust#49499, and in general belies the expectations of a nominal system.

@gavento
Copy link

gavento commented Apr 13, 2018

I would like to propose a way to use RFC 2010 (trait impl specialization again) to address this, including the Eq case.

If I get it right, a universal default impl should be doable with RFC 2010 for Ord=>PartialOrd (directly with an expect) and Copy => Clone (e.g. via std::intrinsics::copy). For Eq => PartialEq we would need some equivalent of eq in Eq.

Consider adding Eq::eq (and possibly Eq::ne similarly) with the same semantics as PartialEq::eq just with added reflexivity assumption. == would still be an alias for PartialEq::eq. However, this change alone would raise ERROR: multiple 'eq' found on every usage of a.eq(b), which is a breaking change. (playground example)

For this particular case of multiple eq in the global namespace, we could add a new internal attribute #[hidden_by_default] (bikesheddable) and apply it on Eq::eq. This attribute on a trait member function T::f would not include T::f in the searched methods on t_var.f() even when T is imported, but still accessible under T::f(&t_var).

This still needs a new mechanism in the compiler, but I would see it as more natural, understandable and general (even though we would likely keep it internal). Also I would hope it would be relatively easy to implement, filtering the candidate method list for this attribute on resolution. Perhaps we already have it in some form? I would be happy to draft an RFC for the attribute if you think it is a good idea, and the current RFC could then be updated to use that and default impls (with specialization).

If that works out, would that not resolve the issue entirely? By that I mean that both manual and derived impls of either just the subtraits (Eq, ...), supertraits (PartialEq, ...) or both are allowed, subtraits imply supertraits, and it also works for user-defined trait hierarchies (e.g. the num-traits crate?). What do you think, @withoutboats, @sgrif, @Centril and others?

@Manishearth
Copy link
Member

FWIW I discussed this a bit with eddyb and it is technically feasible to have such a feature imply a "weak" impl that can be overridden by an explicit impl. There are some hairy issues around generics, but it's not too bad.

@est31
Copy link
Member

est31 commented Apr 25, 2018

Weak impls are like all kind of overloading absolutely toxic to maintainability of code. you'll never know whether somewhere in some module of the crate, an impl is hiding that overrides the weak impl you are currently trying to read or trying to change.

As for compile times it is probably not helpful if the compiler has to process a ton of code only to find out that it is dead. If it can perform that check during macro expansion, we'd save some cycles.

As a positive point, weak impls move this RFC out of the breaking change category, but #[derive_transitive] does this as well.

@kornelski
Copy link
Contributor

kornelski commented Apr 26, 2018

The problems this RFC is trying to solve are "you have to type more" and "you have to know a quirk about derives". derive_transitive as a solution to this is underwhelming, since for one trait it isn't any less to type:

#[derive(Clone, Copy)]

vs

#[derive_transitive(Copy)]

And it doesn't reduce mental burden, just changes one thing you need to know into another thing you need to know (instead of knowing that for Copy you also need Clone, you have to know there are two kinds of derive directives, and when to use which.)

@squishy-clouds
Copy link

An alternative to a new derive attribute like #[derive_transtive(...)] was brought up earlier in the discussion:

To write out #[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)], you would write #[derive(Debug, Default, transitive(Ord, Copy))] or #[derive(Debug, Default, super(Ord, Copy))], which doesn't involve another attribute but you do still have to do something new different to tell it to derive the supertraits.

@est31
Copy link
Member

est31 commented Apr 26, 2018

it doesn't reduce mental burden, just changes one thing you need to know into another thing you need to know (instead of knowing that for Copy you also need Clone, you have to know there are two kinds of derive directives, and when to use which.)

Now you'll need to know which traits will be derived recursively and which won't... both during reading AND during writing. And yes you'll still need to know that there are two kinds of derive macros, ones which output implementations of the supertraits and others which only output the given trait precisely.

As for the "less typing" argument, you will the more traits you are deriving recursively, the more you will save and after a given number of traits, you'll break even.

@kornelski
Copy link
Contributor

kornelski commented Apr 28, 2018

There's an issue about #[derive] lacking type information to generate correct bounds: rust-lang/rust#26925

That reminds me of this issue which for the (suggested) implied behavior would need delay generation of an implementation until it's certain that there isn't a manual implementation.

So perhaps derive needs some magic behavior anyway? That #[derive(…)] wouldn't literally immediately generate syntax for the code it derives, but first magically pretend to the type system that the implementation exists, and generate a specific implementation later if necessary.

@Centril
Copy link
Contributor Author

Centril commented Apr 28, 2018

@kornelski Those things are fixed by #2353.

@kornelski
Copy link
Contributor

Oh no, more versions of derive :(

@Centril
Copy link
Contributor Author

Centril commented Apr 28, 2018

@kornelski well; not more versions, but modifiers on the deriving mechanism we already have.

@kornelski
Copy link
Contributor

kornelski commented Apr 28, 2018

I'm worried that this RFC plus the other RFC will cause a new "Which kind of Derive do you need?" chapter to be added to Rust teaching materials.

Instead of:

You need to choose between #[derive(Trait)] vs #[derive_transitive(Trait)] vs #[derive_no_bound(Trait)] vs #[derive_field_bound(Trait)] and read the next four pages of the manual explaining edge cases caused by macros lacking type information…

I'd really like:

There's only #[derive(Trait)] and it does what you want.

I get that the latter is hard to implement and runs into chicken-egg problems, but from user perspective it's so much nicer.

@gavento
Copy link

gavento commented Apr 28, 2018

@Manishearth Thanks for the discussion on the actual feasibility of the weak impl - I was not sure there myself. I am glad to see that this could work.

@est31 I see your concern, but if you are against weak impls and specialization in general, that should be discussed in the other RFC. I propose to add few very carefully selected impls from stronger traits to their weaker counterparts with the same obvious semantics (so Ord->PartialIrd, not Ord->Eq).

@kornelski I would hope for the same: a #derive that works as expected in the obvious common cases and follows some simple and tractable rules in the complex cases (which are generally unavoidable). I would be also happy with leaving things as they are, but if we want to change things to be easier, we should make them more complex.

@aturon
Copy link
Member

aturon commented May 17, 2018

At this point, it's clear that this proposal is not viable for the 2018 Edition; there's just too much else in flight, we haven't reached a clear consensus here, and there are plausible routes to addressing the motivation without tying it to the edition.

Thus, I'm moving to postpone further discussion on this topic:

@rfcbot fcp postpone

@rfcbot
Copy link
Collaborator

rfcbot commented May 17, 2018

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 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. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels May 17, 2018
@Centril
Copy link
Contributor Author

Centril commented May 17, 2018

I fully support the decision to postpone!

To continue the conversation re. solving the problem in a different way, there are some interesting other designs to consider that don't require breaking changes such as ideas mentioned in
https://internals.rust-lang.org/t/pre-rfc-cfg-attribute-alias/7404/

@rfcbot
Copy link
Collaborator

rfcbot commented May 26, 2018

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

@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 May 26, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 5, 2018

The final comment period, with a disposition to postpone, as per the review above, is now complete.

By the power vested in me by Rust, I hereby postpone this RFC.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Jun 5, 2018
@rfcbot rfcbot closed this Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. 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.

None yet