Automatically derive {Clone,PartialEq,PartialOrd} when {Copy,Eq,Ord} are derived

[bstrie]

Rendered

[nikomatsakis]

I'm a fan. This feels like an annoying papercut we should fix.

[erickt]

Thanks for writing this up! One thing to note is that as of right now generic types like Option<T> cannot use the short form

impl<T: Clone> Clone for Option<T> {
    fn clone(&self) -> Self { *self }
}

Because it may wrap non-Copy types. We would need to have specialization to get that fast path working.

[nikomatsakis]

I imagine that if we introduce specialization, we will add a single impl:

#[low_priority] // or whatever
impl<T: Copy> Clone for T {
    fn clone(&self) -> Self { *self }
}

and then just modify #[derive(Copy)] to no longer generate a Clone impl.

(Same for the other traits.)

[alexcrichton]

Cargo has a few examples of deriving Eq with a manual implementation of PartialEq. I'd be fine just moving some words from here to the "Drawbacks" section, though!

[glaebhoerl]

Why not "automatically derive supertraits of derived traits" in general? So #[derive(Ord)] (for example) also derives PartialEq, Eq, and PartialOrd, not just the latter.

[nikomatsakis]

@glaebhoerl hmm, good point. #[derive(Ord)] probably ought to imply #[derive(Eq)] too.

[alexcrichton]

One other drawback this may wish to mention is that [T; N] is a type that can implement Copy for all N but not Clone. For example in landing rust-lang/rust#23860 it was discovered that many FFI types could derive Copy but not Clone because they had a field that was along the lines of [u8; 100].

It's quite easy, however, to add a Clone implementation for a type that is Copy, so the drawback would just be that today's #[derive(Copy)] + manual Clone would have to switch to a manual Copy as well (slightly more verbose).

[nikomatsakis]

Note that @erickt's PR includes a specialized variant of Clone for the case where Copy is also mentioned and the types are not generic, which would have addressed this problem for FFI types at least. (Also this comment applies.)

[alexcrichton]

Aha, never mind then!

[nikomatsakis]

I spiced up the title to make it clearer what is being requested. @bstrie, hope you don't mind.

[arthurprs]

@glaebhoerl seems like a great idea

[mdinger]

This should fix #441 if subtraits are all automatically derived. I think that's where @glaebhoerl originally suggested it.

[Kintaro]

I'm all for it. We still have time for breaking changes now before the final, so if we'd do it, now's the time.

[arthurprs]

@Kintaro This isn't a breaking change.

[seanmonstar]

@arthurprs it is. See http://internals.rust-lang.org/t/feedback-requested-on-rfc-1028-potential-breaking-change/1847

[glaebhoerl]

@arthurprs It is, see this post by @nikomatsakis on Discourse. (Edit: Jynx.)

I think the customary approach in cases like this has been to gather some statistics on how much code would actually break? I suspect that it's not very much at all, and that this is worth doing.

[diwic]

Is it not possible to be intelligent enough to do: "Okay, we found a derive(Copy). Let's go look for a manual impl of Clone, and if we find one, use it, otherwise we autogenerate one".

With that logic, would that make this RFC a non-breaking change?

[sfackler]

We can't avoid generating the impl entirely, but we could tag it with an attribute that would tell later stages to strip it if it would conflict with another impl.

[seanmonstar]

@diwic It might be possible, and if it is, it should definitely be done. However, it may be that expansion of item decorators happens before the session has collected all trait implementations, in which case it couldn't be done.

[nagisa]

I prefer it being explicit. Sure, in this case implicit might be ergonomically better here, but it doesn’t match the style of the language, which is explicit.

[nikomatsakis]

@sfackler yes, that's specialization. If we were to add specialization in the future, we could add this backwards compatibilty.

[seanmonstar]

@nikomatsakis I think he means adding a #[implicitly_derived] attribute, and then if there's a conflict in later stages, removing the conflicting implementation that has said attribute.

[sfackler]

It wouldn't necessarily be specialization in this case, though, since the manual impl and the derived impl could be defined on the exact same bounds:

#[derive(Copy)]
struct Thing(i32);

impl Clone for Thing {
    fn clone(&self) -> Thing {
        *self
    }
}

So we'd need a separate concept of a "low priority" or "weak" implementation or something that'll be discarded if there's any conflict whatsoever.

[pythonesque]

I'm torn on this one. Overall, I do think I'm in favor, especially if we can add specialization in the future.

[glaebhoerl]

I think "check if there's already a manual impl, and only derive one if there isn't" versus "always derive an impl, but throw it away if it turns out there's a manual one" is just an implementation detail - the two are observationally equivalent, and there's no need to expose the underlying mechanism to end-users directly. (If I'm not missing something.)

[nstoddard]

I totally support this. I've always found it annoying how I have to derive both Eq and PartialEq. I don't mind the breakage, especially since very little code will probably be broken (does anyone have statistics on how much would actually break?).

[bluss]

We don't need a breaking change, can't we expand deriving in a non-breaking way? That way we have time to do it during a normal release cycle too and not do it in a rush.

For example, what about #[derive(Ord="full")] to derive the whole shebang, or some similar backwards compatible expansion of the current feature.

I think we need to calmly look at nice generically useful solutions just like we used to. Always deriving multiple traits is less flexible, and useful in less situations, even though it is easier when it is applicable. Even just giving the trait group a new derive name is a simpler and more broadly useful solution: #[derive(TotalOrdering)].

[nikomatsakis]

@seanmonstar @sfackler

The fact that it's a more limited, special-case of specialization doesn't really make me feel better about it, to be honest. I'm definitely opposed to adding some last-minute magic into the trait system without a very strong justification. I want people to be able to understand deriving as simple shorthands for things they could have typed themselves. I don't think the breakage will be that bad if we just do this. In any case, we're trying to gather some numbers based on @brson's tool and PR rust-lang/rust#23905.

[comex]

In the space of non-breaking solutions, along with the proposed aliases that would cover the supertrait use cases here, I propose one that covers at least Clone, {Partial,}Eq, {Partial,}Ord, Debug, and Hash - i.e. the set of derivable traits that would be useful to implement (especially for the sake of users of a library type, or at worst harmless) for most simple structs with a few integer or string fields. What I'd call 'data-like' (as opposed to 'object-like') structs. I'd personally find this more useful than being able to omit supertraits but still having to write out several different traits for everything.

(edited for clarity since I'm tired)

[nikomatsakis]

@bluss @comex yes, it's true that shorthand names are a reasonable compromise -- and maybe a better solution. The last time we went through the exercise, we failed to come to much consensus about what those shorthands ought to be, but perhaps we're in a better position now. Things are more stable, and there are a lot more crates we could grep through to try to figure out the most common combinations.

[vadimcn]

Another option would be to add #[derive_explicit(...)] for cases when you don't want auto-derived base traits.
As for the breaking change, can libsyntax detect common cases like #[derive(Copy, Clone)] in existing code and issue a deprecation warning instead of an error?

[nikomatsakis]

@vadimcn #[derive(Copy, Clone)] doesn't break. It's only if the Clone impl is manually implemented, but Copy is derived.

[bvssvni]

If specialization is planned at some point, I don't mind waiting for it, since that would be a backward compatible change.

[jpernst]

Speaking as a user who is generally in favor of inference, I have to say I'm against this. At least so long as manually implementing any trait that is also implicitly derived results in an error. Manually implementing one trait should not bar you from deriving another. If this problem can be solved more generally in the future, then this would be a great addition at that time.

[tedsta]

I am for this as long as the specialization gets implemented eventually. If there will never be a way to manually implement PartialEq, for example, and derive Eq, I am against this.

[nikomatsakis]

I think having thought this through I'm now feeling that it's not worth making a breaking change for this. Depending how things work out, we can either have some kind of shorthand notation or perhaps we'll be able to rely on specialization (we'd have to see).

[SiegeLord]

I'd rather have a shorthand than this RFC. My code would be broken by it, and I don't see the benefit given that there is a lint (at least for Copy + Clone).

[norcalli]

Logically, if I see something that says #[derive(Copy)] or #[derive(Eq)], I would assume it would also imply a partial comparison or clone, so I don't see that as a big problem. And that way, seeing the word Partial would indicate to me that the type doesn't have a full version faster and clearer than if you had to always specify both. And I can see how if specialization comes in, it would be able to be dropped in, so that's nice. So I'd be tentatively in support of this.

[iopq]

What's the point of a breaking change to save a few characters? Why not implement it in a non-breaking way (if you have a Clone implementation, let it be used in the Copy implementation) instead of breaking it?

[Gankro]

I'm in favour of this. It's weird that we allow the user to even ask for non-sensical derivations (e.g. "only Ord"). I'm sympathetic to manual implementations becoming a bit more verbose, but those are the exception, and we should optimize for the common case. This change makes Rust "just work" better. Also it allows us to make stronger assumptions about derived implementations: if they derive(Ord) we know exactly how Ord is implemented. If we decide to introduce UnsafeOrd or whatever then we can silently upgrade all derive(Ord)'s to derive(UnsafeOrd)'s (iff they contain only UnsafeOrd's).

The fact that it saves key-strokes is simply gravy, but not the main point IMHO.

[nikomatsakis]

I think I prefer the shorthand idea. @aturon suggested something like #[derive(Ord*)], meaning "Ord and all of its supertraits". This is not legal syntax today, but I've been wanting to expand the "grammar" of attributes to permit arbitrary token trees for a while now...anyway, something like that seems like a simple and consistent rule we could easily add later, and it would keep the workings of derive pretty straightforward.

[trws]

One option I haven't seen mentioned, forgive me if I missed it, would be to allow a user to specify which supertraits they wish to implement manually as part of the syntax. Something along the lines of #[derive(Copy, -Clone)] would make it possible to explicitly supply a manual implementation of the supertrait by masking off the derived version. It would remain breaking, but avoids forcing a manual implementation of the subtrait where a supertrait needs specialization.

[nikomatsakis]

After much deliberation, we've decided not to pursue this avenue, because it doesn't seem worth making a breaking change -- even a small one. Moreover, this change is not universally popular (e.g., 1, 2, 3) and may be pedagogically challenging, as it breaks the correspondence where deriving a trait is exactly equivalent to implement it (since implementing a trait does not automatically implement its supertraits). The alternative of creating shorthands seems to be a promising alternative, and might also go further in reducing verbosity by permitting groups of common traits to be group together. Therefore, I'm going to close this RFC as postponed under issue #441. @bstrie, thanks for writing this up and @erickt, thanks for the preliminary implementation. Thanks everybody else for the comments and thoughts.