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.

Sign up for GitHub

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

Copy clone semantics #1521

Merged
merged 6 commits into from
May 4, 2016
+66 −0
Merged

Copy clone semantics #1521

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d69cd0a
First commit
ubsan Mar 1, 2016
5542c80
Add some modifications suggested by @durka
ubsan Mar 1, 2016
91629f9
Fix some language for @aatch
ubsan Mar 1, 2016
0feac23
A poorly defined clone shan't result in UB
ubsan Mar 1, 2016
df87d91
Add more motivation
ubsan Mar 2, 2016
b9b7854
Add a date
ubsan Mar 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions text/0000-copy-clone-semantics.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
- Feature Name: N/A
- Start Date: 01 March, 2016
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary
[summary]: #summary

With specialization on the way, we need to talk about the semantics of
`<T as Clone>::clone() where T: Copy`.

It's generally been an unspoken rule of Rust that a `clone` of a `Copy` type is
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to cite rust-lang/rust#23790, which initially proposed making Clone a supertrait of Copy, with the description:

"Copy is a refinement of Clone where memcpy suffices"

The assumption that Copy and Clone would be compatible was also implicitly used in accepting RFC 839, as a premise thereof was the iea that it would be compatible to relax a T: Copy bound to a T: Clone bound later on.

In other words, it's been more than an unspoken rule that clone and copy must be compatible, even if the rule seems not to have made its way onto the documentation of the Clone trait itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

(The example of specializing #[derive(Copy, Clone)] that was brought up elsewhere in the thread also seems relevant.)

equivalent to a `memcpy` of that type; however, that fact is not documented
anywhere. This fact should be in the documentation for the `Clone` trait, just
like the fact that `T: Eq` should implement `a == b == c == a` rules.

# Motivation
[motivation]: #motivation

Currently, `Vec::clone()` is implemented by creating a new `Vec`, and then
cloning all of the elements from one into the other. This is slow in debug mode,
and may not always be optimized (although it often will be). Specialization
would allow us to simply `memcpy` the values from the old `Vec` to the new
`Vec` in the case of `T: Copy`. However, if we don't specify this, we will not
be able to, and we will be stuck looping over every value.
Copy link
Member

Choose a reason for hiding this comment

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

We will be stuck relying on the optimizer to figure it out. Which it can do and insert memcpy, typically for Copy values of 8 bytes or smaller per element, it looks like.

Copy link
Author

Choose a reason for hiding this comment

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

@bluss exactly.


It's always been the intention that `Clone::clone == ptr::read for T: Copy`; see
[issue #23790][issue-copy]: "It really makes sense for `Clone` to be a
supertrait of `Copy` -- `Copy` is a refinement of `Clone` where `memcpy`
suffices, basically." This idea was also implicit in accepting
[rfc #0839][rfc-extend] where "[B]ecause Copy: Clone, it would be backwards
compatible to upgrade to Clone in the future if demand is high enough."

# Detailed design
[design]: #detailed-design

Specify that `<T as Clone>::clone(t)` shall be equivalent to `ptr::read(t)`
where `T: Copy, t: &T`. An implementation that does not uphold this *shall not*
result in undefined behavior; `Clone` is not an `unsafe trait`.

Also add something like the following sentence to the documentation for the
`Clone` trait:

"If `T: Copy`, `x: T`, and `y: &T`, then `let x = y.clone();` is equivalent to
`let x = *y;`. Manual implementations must be careful to uphold this."

# Drawbacks
[drawbacks]: #drawbacks

This is a breaking change, technically, although it breaks code that was
malformed in the first place.

# Alternatives
[alternatives]: #alternatives

The alternative is that, for each type and function we would like to specialize
in this way, we document this separately. This is how we started off with
`clone_from_slice`.

# Unresolved questions
[unresolved]: #unresolved-questions

What the exact wording should be.
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative wording: "When Copy type has user-defined implementation of Clone it's unspecified whether clone or memcpy is called when cloning a value of this type." or (shamelessly stolen from somewhere) "<...if Copy...> An implementation is allowed to omit the call to clone even if is has side effects. Such elision of calls to clone is called clone elision."

Copy link
Author

Choose a reason for hiding this comment

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

@petrochenkov it's not that it's unspecified whether clone or ptr::read is called when you actually call .clone(). It's just that libraries (including the standard library) are able to assume that Clone::clone == ptr::read for T: Copy; and, if that requirement is not met, libraries are allowed to exhibit unexpected (but defined) behavior. (Just like if an Ord type doesn't actually have an absolute ordering, or if an Eq type doesn't actually have transitive equality).

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not that it's unspecified whether clone or ptr::read is called when you actually call .clone()

Why not? If we go this route anyway, let's give the compiler/libraries all the freedom.
I'd even write "user-defined implementations of Clone for Copy types are ignored", but I'm not sure about generics:

fn f<T: Clone>(arg: T) -> T { arg.clone() } // It's not known if `T` is `Copy` or not and I'm not sure it's convenient to turn `clone`s into `memcpy`s during monomorphisation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just that libraries (including the standard library) are able to assume that Clone::clone == ptr::read for T: Copy

derive is not a library, so the language already can omit clones as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, "clone is always ignored" is better than "clone is sometimes ignored" from pedagogical point of view.

Copy link
Author

Choose a reason for hiding this comment

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

hmm...

At first I thought this was a bad idea, but then I started thinking. Not "always ignored", but if your clone has different semantics from a ptr::read then it should be implementation defined whether it is called.

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 the "optimizer magic overriding of clone".

Copy link
Author

Choose a reason for hiding this comment

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

@arielb1 Yeah, I guess. I do think it would be better if it's just kept to libraries and language constructs like #[derive]


[issue-copy]: https://github.com/rust-lang/rust/issues/23790
[rfc-extend]: https://github.com/rust-lang/rfcs/blob/master/text/0839-embrace-extend-extinguish.md