-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Automatically derive {Clone,PartialEq,PartialOrd} when {Copy,Eq,Ord} are derived #1028
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
- Feature Name: derive-implied-traits | ||
- Start Date: 2015-03-26 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
This RFC proposes three related changes: | ||
|
||
1. When `#[derive(Copy)]` is placed on a type, that type automatically recieves a `#[derive(Clone)]` attribute. | ||
|
||
2. When `#[derive(Eq)]` is placed on a type, that type automatically receives a `#[derive(PartialEq)]` attribute. | ||
|
||
3. When `#[derive(Ord)]` is placed on a type, that type automatically recieves a `#[derive(PartialOrd)]` attribute. | ||
|
||
# Motivation | ||
|
||
In each of these three cases, one of each pair of traits is more "fundamental" than the other, such that it is reasonable to assume that no user wants to derive one without also deriving the other. | ||
|
||
It is evident and uncontroversial that `Copy` should imply `Clone` (https://github.com/rust-lang/rust/pull/23860). In the case of `Copy`, it is a common pitfall that library authors will derive `Copy` for their types and yet forget to derive `Clone` as well. When this happens it is a strict loss of expressiveness which makes the type in question ineligible to participate in generic programming that involves cloning, despite the fact that `Copy` effectively denotes types for whom a clone is but a `memcpy`. Past remedies to this problem have suggested a blanket impl of `Clone` for all `T: Copy` (https://github.com/rust-lang/rust/issues/17884), however modern consensus seems to believe that this approach is not feasible. Instead, we propose that any derived implementation of `Copy` automatically receive an autogenerated implementation of `Clone` in order to free library authors from this all-too-easy oversight. Finally, if the compiler knows that a type implements `Copy`, then there exist opportunities for the compiler to insert an optimized `Clone` implementation that consists entirely of `*self`. | ||
|
||
In the case of `Eq` and `Ord`, the motivation is different. Implementors of either of these traits must first implement their partial versions, which means that no user can accidentally omit them. However, it is our subjective experience that no user who derives `Eq` will have manually implemented `PartialEq`; likewise for `Ord` and `PartialOrd`. Therefore this aspect of this RFC is solely to improve ergonomics, and should be considered secondary in importance to the issue of `Copy` and `Clone`. | ||
|
||
# Detailed design | ||
|
||
The `derive` syntax extension must be modified to accomodate these hardcoded rules. The compiler must be able to accept both `#[derive(Copy, Clone)]` and `#[derive(Copy)]`, the latter of which will implicitly insert a derived implementation of `Clone`. Like wise the compiler must accept both `#[derive(Eq)]` and `#[derive(PartialEq, Eq)]`, as well as both `#[derive(Ord)]` and `#[derive(PartialOrd)]`. Despite not appearing in the source, the automatically inserted implementations must be visible to rustdoc. | ||
|
||
An implementation of this RFC can be seen here: https://github.com/rust-lang/rust/pull/23905 | ||
|
||
# Drawbacks | ||
|
||
Makes certain derived traits "special", forming a pseudo-inheritance hierarchy of sorts. | ||
|
||
Rustdoc will show implementations of traits with no direct origin in the source. | ||
|
||
# Alternatives | ||
|
||
The most straightforward alternative would be to accept only the `Copy`/`Clone` aspect of this RFC, as it is concerned with library correctness (in the "const correctness" sense) whereas the `Ord`/`PartialOrd` and `Eq`/`PartialEq` cases are merely for ergonomics. | ||
|
||
If the above alternative is desired, then it may make sense, for consistency's sake, to cause the compiler to return an error when it encounters `#[derive(Copy)]`, with a message demanding that the user modify the attribute to `#[derive(Copy, Clone)]`. This would address the drawback of there being an unclear origin for the `Clone` trait when viewed from rustdoc. It would also have symmetry with the demand for `#[derive(PartialOrd, Ord)]`, again assuming that the alternative from the prior paragraph is desired. | ||
|
||
# Unresolved questions | ||
|
||
Is it reasonable to assume that no user who automatically derives `Eq`/`Ord` has manually implemented `PartialEq`/`PartialOrd`? If this assumption fails to hold, what behavior should result? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cargo has a few examples of deriving |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other drawback this may wish to mention is that
[T; N]
is a type that can implementCopy
for allN
but notClone
. For example in landing rust-lang/rust#23860 it was discovered that many FFI types could deriveCopy
but notClone
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 isCopy
, so the drawback would just be that today's#[derive(Copy)]
+ manualClone
would have to switch to a manualCopy
as well (slightly more verbose).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that @erickt's PR includes a specialized variant of
Clone
for the case whereCopy
is also mentioned and the types are not generic, which would have addressed this problem for FFI types at least. (Also this comment applies.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, never mind then!