-
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
split the Copy
trait in two traits: Pod
and Copy
#936
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,189 @@ | ||
- Feature Name: pod | ||
- Start Date: 2015-03-04 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Split the `Copy` trait in two traits: `Pod` and `Copy`. `Pod` indicates that a | ||
type can be cloned by just copying its bits ("`memcpy`"), and the new `Copy` | ||
indicates that the type should be implicitly copied instead of moved. | ||
|
||
# Motivation | ||
|
||
### `Range` is not `Copy` (or deriving `Copy` is hard) | ||
|
||
We have a guideline that says that iterators should not be implicitly copyable | ||
(i.e. they shouldn't implement `Copy`). The `Range` struct, which is what | ||
`a..b` desugars to, is an iterator, and under this guideline it shouldn't be | ||
`Copy`, but this hurts ergonomics in other cases where it's not used as an | ||
iterator, for example a struct that contains `Range` can't be marked as `Copy`, | ||
because `Range` is not `Copy`. | ||
|
||
This is a more general problem, if upstream decides that some type shouldn't be | ||
`Copy`, even if it could, then downstream can't make `Copy`able structs/enums | ||
that contain said type. | ||
|
||
### `Cell` only admits `Copy` data | ||
|
||
This is unnecessarily restrictive because the data doesn't need to be | ||
implicitly copyable, the actual requirement is that the data should be | ||
cloneable by just `memcpy`ing it. | ||
|
||
### How splitting `Copy` helps with this? | ||
|
||
Implementing `Pod` for a type is much less controversial than implementing | ||
`Copy` on it. This should result in more types marked as `Pod` in libraries, | ||
which in turn gives downstream users more control about what can be `Copy` as | ||
the only requirement is that the fields must be `Pod`. | ||
|
||
`Cell` can be modified to have a `Pod` bound instead of a `Copy` one, this | ||
would allow more types, like iterators, to be stored in a `Cell`. | ||
|
||
# Detailed design | ||
|
||
### The traits | ||
|
||
A `Pod` marker trait will be added to the stdlib, and the `Copy` trait will be | ||
updated as follows: | ||
|
||
``` | ||
/// Indicates that this type can be cloned by just copying its bits (`memcpy`) | ||
#[lang = "pod"] | ||
trait Pod: MarkerTrait {} | ||
|
||
/// Indicates that this type will be implicitly copied instead of moved | ||
#[lang = "copy"] | ||
trait Copy: Pod {} | ||
``` | ||
|
||
### Compiler rules | ||
|
||
Update the compiler to enforce the following rules: | ||
|
||
- `Pod` can only be implemented by structs/enums whose fields are also `Pod`. | ||
- Built-in/primitive types that were automatically considered `Copy` by the | ||
compiler (`i8`, `&T`, `(A, B)`, `[T; N]`, etc), will now be considered to be | ||
both `Pod` and `Copy`. | ||
- `Copy` restrictions will carry over to `Pod`, e.g. types with destructors | ||
can't implement `Pod`, nor `Copy` because of the `trait Copy: Pod` | ||
requirement. | ||
|
||
### `#[derive]` syntax extensions | ||
|
||
To avoid breaking the world, the `#[derive(Copy)]` syntax extension will be | ||
updated to derive *both* `Copy` and `Pod`: | ||
|
||
``` rust | ||
#[derive(Copy)] | ||
struct Foo<T>(T); | ||
|
||
// expands into | ||
impl<T: Pod> Pod for Foo<T> {} | ||
impl<T: Pod> Copy for Foo<T> {} | ||
//^ note that the bound is `T: Pod` (not `T: Copy`!) | ||
``` | ||
|
||
Also add a `#[derive(Pod)]` extension to derive only `Pod`. | ||
|
||
### `Cell` | ||
|
||
`Cell` will be change its `T: Copy` bound to `T: Pod`. | ||
|
||
# Drawbacks | ||
|
||
Cognitive load. Currently you have to think whether a type should be `Clone`, | ||
`Copy+Clone` or neither (affine type). Now you have to choose between `Clone`, | ||
`Pod+Clone`, `Pod+Clone+Copy` or neither. (There are other combinations like | ||
e.g `Pod+Copy`, but you almost always want to add `Clone` in there as well) | ||
|
||
# Alternatives | ||
|
||
Don't do this, figure out how some other way to solve the `Range` problem | ||
(should it be `Copy`?). And, don't let non-implictly copyable structs (like | ||
most iterators) be stored in `Cell`s. | ||
|
||
### More magic `#[derive]`s | ||
|
||
To reduce the burden of having to write `#[derive(Pod, Clone)]` and | ||
`#[derive(Copy, Clone)]`, the `#[derive]` syntax extensions could be modified | ||
as follows: | ||
|
||
- `#[derive(Clone)]` derives `Clone` | ||
- `#[derive(Pod)]` derives `Pod` and `Clone` | ||
- `#[derive(Copy)]` derives `Pod`, `Clone` and `Copy` | ||
|
||
This means that for any type you only have to pick *one* of these `#[derive]`s | ||
or neither. | ||
|
||
The author would prefer to obtain the same behavior not with syntax extensions | ||
but with a blanket implementation: | ||
|
||
``` rust | ||
impl Clone for T where T: Pod { | ||
fn clone(&self) -> T { | ||
unsafe { | ||
mem::transmute_copy(self) | ||
} | ||
} | ||
} | ||
``` | ||
|
||
However, this is not actionable without negative bounds because of coherence | ||
issues (overlapping implementations). | ||
|
||
### OIBIT `Pod` | ||
|
||
Another option is to define the `Pod` trait as an OIBIT. `Pod` would then have | ||
a default implementation, and negative implementations for mutable references | ||
and for types with destructors | ||
|
||
``` rust | ||
unsafe trait Pod: MarkerTrait {} | ||
unsafe impl Pod for .. {} | ||
impl<T: Drop> !Pod for T {} | ||
impl<'a, T: ?Sized> !Pod for &'a mut T {} | ||
``` | ||
|
||
The resulting behavior matches the built-in (compiler) rules of the current | ||
`Copy` trait. | ||
|
||
As in the original proposal, the new `Copy` trait would become a supertrait | ||
over `Pod`: | ||
|
||
``` rust | ||
trait Copy: Pod {} | ||
``` | ||
|
||
This approach has the advantage that is no longer necessary to manually | ||
implement `Pod` as the trait will be implicitly derived whenever the type can | ||
be `Pod`. And, when a type can't be `Pod`, because e.g. one of its fields is | ||
not `Pod`, then it won't implicitly implement `Pod`, however this can be | ||
overridden with a manual `unsafe impl Pod for Ty`. | ||
|
||
The cases where one must explicitly opt-out of `Pod` are rare, and involve | ||
affine types without destructors. An example is iOS' `OsRng`: | ||
|
||
``` rust | ||
// not implictly copyable, doesn't implement `Drop` | ||
#[cfg(target_os = "ios")] | ||
struct OsRng { | ||
_dummy: (), | ||
} | ||
|
||
// with this alternative, a negative implementation is required to maintain the | ||
// current semantics | ||
impl !Pod for OsRng {} | ||
``` | ||
|
||
The downside of this approach is that by changing the fields of a struct the | ||
`Pod`ness of the type can be inadvertently (without compiler warning/error) | ||
changed as well. | ||
|
||
# Unresolved questions | ||
|
||
- Is there a name that better describes what `Pod` is? | ||
- Should we bring back the "you could implement `Pod` for type" lint? The | ||
author thinks that you almost always want to mark as many things as possible | ||
as `Pod` - that's not the case with the current `Copy` (e.g. iterators). | ||
- Should the `Pod` trait be in the prelude? |
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.
(note that with an OIBIT
Pod
this comment is largely irrelevant, just wanted to jot it down)I have personally been very worried about
derive
expanding to multiple traits in the past. I believe it will force our hand in quickly developing a customization syntax of a method to deriveCopy
but notPod
. I worry about the cliff of#[derive]
where once you can't use it's you fall into the pit of despair of writing everything out manually (which can be unpleasant, but may not be so bad withCopy
andPod
).I am basically worried about the precedent that this sets for future
derive
modes. For example I don't think thatEq
auto-derivingPartialEq
is necessarily what we would want because you often want to customize thePartialEq
implementation to just a few fields. I suspect that at least one case will have to avoidderive(Copy)
due to the desire to customize the implementation ofPod
even thoughderive(Copy)
might work today.This may be a bit hand-wavy in the specific case of
Pod
andCopy
(as they're somewhat special marker traits), so we may not be so bad off taking this route. Plus we could always roll this out a bit and see if you ever really do want to customizePod
but notCopy
(it may not happen in this specific case). Just want to register my concern for adding a little more magic to derive!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.
In the case of
Eq
, if we make it anunsafe
trait (why haven't we done it already?), thederive
implementation ofEq
would be incorrect unlessderive
was also supplying thePartialEq
implementation - the same goes forOrd
andPartialOrd
, as well.