From 67351e29a93159116290d8e45a1b97ad0818a92d Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 28 Aug 2017 21:25:34 +0300 Subject: [PATCH 1/6] Compiler-generated Clone impls for Arrays and Tuples --- text/0000-all-the-clones.md | 100 ++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 text/0000-all-the-clones.md diff --git a/text/0000-all-the-clones.md b/text/0000-all-the-clones.md new file mode 100644 index 00000000000..3a2eeaf0cb2 --- /dev/null +++ b/text/0000-all-the-clones.md @@ -0,0 +1,100 @@ +- Feature Name: all_the_clones +- Start Date: 2017-08-28 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +Add compiler-generated `Clone` implementations for tuples and arrays with `Clone` elements of all lengths. + +# Motivation +[motivation]: #motivation + +Currently, the `Clone` trait for arrays and tuples is implemented using a [macro] in libcore, for tuples of size 11 or less and for `Copy` arrays of size 32 or less. This breaks the uniformity of the language and annoys users. + +Also, the compiler already implements `Copy` for all arrays and tuples with all elements `Copy`, which forces the compiler to provide an implementation for `Copy`'s supertrait `Clone`. There is no reason the compiler couldn't provide `Clone` impls for all arrays and tuples. + +[macro]: https://github.com/rust-lang/rust/blob/f3d6973f41a7d1fb83029c9c0ceaf0f5d4fd7208/src/libcore/tuple.rs#L25 + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Arrays and tuples of `Clone` arrays are `Clone` themselves. Cloning them clones all of their elements. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +Make `clone` a lang-item, add the following trait rules to the compiler: + +``` +n number +T type +T: Clone +---------- +[T; n]: Clone + +T1,...,Tn types +T1: Clone, ..., Tn: Clone +---------- +(T1, ..., Tn): Clone +``` + +And add the obvious implementations of `Clone::clone` and `Clone::clone_from` as MIR shim implementations, in the same manner as `drop_in_place`. The implementations could also do a shallow copy if the type ends up being `Copy`. + +Remove the macro implementations in libcore. We still have macro implementations for other "derived" traits, such as `PartialEq`, `Hash`, etc. + +# Drawbacks +[drawbacks]: #drawbacks + +The MIR shims add complexity to the compiler. Along with the `derive(Clone)` implementation in `libsyntax`, we have 2 separate sets of implementations of `Clone`. + +Having `Copy` and `Clone` impls for all arrays and tuples, but not `PartialEq` etc. impls, could be confusing to users. + +# Rationale and Alternatives +[alternatives]: #alternatives + +Even with all proposed expansions to Rust's type-system, for consistency, the compiler needs to have at least *some* built-in `Clone` implementations: the type `for<'a> fn(Foo<'a>)` is `Copy` for all user-defined types `Foo`, but there is no way to implement `Clone`, which is a supertrait of `Copy`, for it (an `impl Clone for fn(T)` won't match against the higher-ranked type). + +The MIR shims for `Clone` of arrays and tuples are actually pretty simple and don't add much complexity after we have `drop_in_place` and shims for `Copy` types. + +## The array situation + +In Rust 1.19, arrays are `Clone` only if they are `Copy`. This code does not compile: +```Rust +fn main() { + let x = [Box::new(0)].clone(); //~ ERROR + println!("{:?}", x[0]); +} +``` + +The reason (I think) is that there is no good way to write a variable-length array expression in macros. This wouldn't be fixed by the first iteration of const generics. + +OTOH, this means that making non-`Copy` arrays `Clone` is less of a bugfix and more of a new feature. It's however a nice feature - `[Box; 1]` not being `Clone` is an annoying and seemingly-pointless edge case. + +## Implement `Clone` only for `Copy` types + +As of Rust 1.19, the compiler *does not* have the `Clone` implementations, which causes ICEs such as rust-lang/rust#25733 because `Clone` is a supertrait of `Copy`. + +One alternative, which would solve ICEs while being conservative, would be to have compiler implementations for `Clone` only for *`Copy`* tuples of size 12+ and arrays, and maintain the `libcore` macros for `Clone` of tuples (in Rust 1.19, arrays are only `Clone` if they are `Copy`). + +This would make the shims *trivial* (a `Clone` implementation for a `Copy` type is just a memcpy), and would not implement any features that are not needed. + +When we get variadic generics, we could make all tuples with `Clone` elements `Clone`. With the right trick, we could also make all arrays with `Clone` elements `Clone`. + +## Use a MIR implementation of `Clone` for all derived impls + +The implementation on the other end of the conservative-radical end would be to use the MIR shims for *all* `#[derive(Clone)]` implementations. This would increase uniformity by getting rid of the separate `libsyntax` derived implementation. However: + +1. We'll still need the `#[derive_Clone]` hook in libsyntax, which would presumably result in an attribute that trait selection can see. That's not a significant concern. +2. The more annoying issue is that, as a workaround to trait matching being inductive, derived implementations are imperfect - see rust-lang/rust#26925. This means that we either have to solve that issue for `Clone` (which is dedicatedly non-trivial) or have some sort of type-checking for the generated MIR shims, both annoying options. +3. A MIR shim implementation would also have to deal with edge cases such as `#[repr(packed)]`, which normal type-checking would handle for ordinary `derive`. I think drop glue already encounters all of these edge cases so we have to deal with them anyway. + +## `Copy` and `Clone` for closures + +We could also add implementations of `Copy` and `Clone` to closures. That is RFC #2132 and should be discussed there. + +# Unresolved questions +[unresolved]: #unresolved-questions + +See Alternatives. From e8bf76d197a4c2a100958f90da46b65be645e279 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 28 Aug 2017 21:55:52 +0300 Subject: [PATCH 2/6] We can actually implement Clone for all the things. --- text/0000-all-the-clones.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/text/0000-all-the-clones.md b/text/0000-all-the-clones.md index 3a2eeaf0cb2..6b3acd74aed 100644 --- a/text/0000-all-the-clones.md +++ b/text/0000-all-the-clones.md @@ -68,7 +68,20 @@ fn main() { } ``` -The reason (I think) is that there is no good way to write a variable-length array expression in macros. This wouldn't be fixed by the first iteration of const generics. +~~The reason (I think) is that there is no good way to write a variable-length array expression in macros. This wouldn't be fixed by the first iteration of const generics.~~ Actually, this can be done using a for-loop (`ArrayVec` is used here instead of a manual panic guard for simplicity, but it can be easily implemented given const generics). +```Rust +impl Clone for [T; n] { + fn clone(&self) -> Self { + unsafe { + let result : ArrayVec = ArrayVec::new(); + for elem in (self as &[T]) { + result.push(elem.clone()); + } + result.into_inner().unwrap() + } + } +} +``` OTOH, this means that making non-`Copy` arrays `Clone` is less of a bugfix and more of a new feature. It's however a nice feature - `[Box; 1]` not being `Clone` is an annoying and seemingly-pointless edge case. From c5ac20b5d7890194681c7d2e5fa74175ae962640 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 29 Aug 2017 11:56:03 +0300 Subject: [PATCH 3/6] add links to other documents Github markdown does not include auto-links to other issues like Github in issues does. --- text/0000-all-the-clones.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/text/0000-all-the-clones.md b/text/0000-all-the-clones.md index 6b3acd74aed..8b543a26f4f 100644 --- a/text/0000-all-the-clones.md +++ b/text/0000-all-the-clones.md @@ -87,7 +87,7 @@ OTOH, this means that making non-`Copy` arrays `Clone` is less of a bugfix and m ## Implement `Clone` only for `Copy` types -As of Rust 1.19, the compiler *does not* have the `Clone` implementations, which causes ICEs such as rust-lang/rust#25733 because `Clone` is a supertrait of `Copy`. +As of Rust 1.19, the compiler *does not* have the `Clone` implementations, which causes ICEs such as [rust-lang/rust#25733] because `Clone` is a supertrait of `Copy`. One alternative, which would solve ICEs while being conservative, would be to have compiler implementations for `Clone` only for *`Copy`* tuples of size 12+ and arrays, and maintain the `libcore` macros for `Clone` of tuples (in Rust 1.19, arrays are only `Clone` if they are `Copy`). @@ -100,14 +100,20 @@ When we get variadic generics, we could make all tuples with `Clone` elements `C The implementation on the other end of the conservative-radical end would be to use the MIR shims for *all* `#[derive(Clone)]` implementations. This would increase uniformity by getting rid of the separate `libsyntax` derived implementation. However: 1. We'll still need the `#[derive_Clone]` hook in libsyntax, which would presumably result in an attribute that trait selection can see. That's not a significant concern. -2. The more annoying issue is that, as a workaround to trait matching being inductive, derived implementations are imperfect - see rust-lang/rust#26925. This means that we either have to solve that issue for `Clone` (which is dedicatedly non-trivial) or have some sort of type-checking for the generated MIR shims, both annoying options. + +2. The more annoying issue is that, as a workaround to trait matching being inductive, derived implementations are imperfect - see [rust-lang/rust#26925]. This means that we either have to solve that issue for `Clone` (which is dedicatedly non-trivial) or have some sort of type-checking for the generated MIR shims, both annoying options. + 3. A MIR shim implementation would also have to deal with edge cases such as `#[repr(packed)]`, which normal type-checking would handle for ordinary `derive`. I think drop glue already encounters all of these edge cases so we have to deal with them anyway. ## `Copy` and `Clone` for closures -We could also add implementations of `Copy` and `Clone` to closures. That is RFC #2132 and should be discussed there. +We could also add implementations of `Copy` and `Clone` to closures. That is [RFC #2132] and should be discussed there. # Unresolved questions [unresolved]: #unresolved-questions See Alternatives. + +[RFC #2132]: https://github.com/rust-lang/rfcs/pull/2132 +[rust-lang/rust#25733]: https://github.com/rust-lang/rust/issues/25733 +[rust-lang/rust#26925]: https://github.com/rust-lang/rust/issues/26925 From 0ad5ce70e50d7e4f2b2429465b46ad344b88c141 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 29 Aug 2017 12:09:26 +0300 Subject: [PATCH 4/6] address review comments --- text/0000-all-the-clones.md | 38 +++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/text/0000-all-the-clones.md b/text/0000-all-the-clones.md index 8b543a26f4f..4166fd1bbb5 100644 --- a/text/0000-all-the-clones.md +++ b/text/0000-all-the-clones.md @@ -44,6 +44,40 @@ And add the obvious implementations of `Clone::clone` and `Clone::clone_from` as Remove the macro implementations in libcore. We still have macro implementations for other "derived" traits, such as `PartialEq`, `Hash`, etc. +Note that independently of this RFC, we're adding builtin `Clone` impls for all "scalar" types, most importantly fn pointer and fn item types (where manual impls are impossible in the foreseeable future because of HRTBs, e.g. `for<'a> fn(SomeLocalStruct<'a>)`), which are already `Copy`: +``` +T fn pointer type +---------- +T: Clone + +T fn item type +---------- +T: Clone + +And just for completeness (these are perfectly done by an impl in Rust 1.19): + +T int type | T uint type | T float type +---------- +T: Clone + +T type +---------- +*const T: Clone +*mut T: Clone + +T type +'a lifetime +---------- +&'a T: Clone + +---------- +bool: Clone +char: Clone +!: Clone +``` + +This was considered a bug-fix (these types are all `Copy`, so it's easy to witness that they are `Clone`). + # Drawbacks [drawbacks]: #drawbacks @@ -70,7 +104,7 @@ fn main() { ~~The reason (I think) is that there is no good way to write a variable-length array expression in macros. This wouldn't be fixed by the first iteration of const generics.~~ Actually, this can be done using a for-loop (`ArrayVec` is used here instead of a manual panic guard for simplicity, but it can be easily implemented given const generics). ```Rust -impl Clone for [T; n] { +impl Clone for [T; n] { fn clone(&self) -> Self { unsafe { let result : ArrayVec = ArrayVec::new(); @@ -93,7 +127,7 @@ One alternative, which would solve ICEs while being conservative, would be to ha This would make the shims *trivial* (a `Clone` implementation for a `Copy` type is just a memcpy), and would not implement any features that are not needed. -When we get variadic generics, we could make all tuples with `Clone` elements `Clone`. With the right trick, we could also make all arrays with `Clone` elements `Clone`. +When we get variadic generics, we could make all tuples with `Clone` elements `Clone`. When we get const generics, we could make all arrays with `Clone` elements `Clone`. ## Use a MIR implementation of `Clone` for all derived impls From c981ea1e8e8a552edfd6d76fbf258622945dc86d Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 29 Aug 2017 12:10:25 +0300 Subject: [PATCH 5/6] HRTB -> higher-ranked types --- text/0000-all-the-clones.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-all-the-clones.md b/text/0000-all-the-clones.md index 4166fd1bbb5..27e85f92d6f 100644 --- a/text/0000-all-the-clones.md +++ b/text/0000-all-the-clones.md @@ -44,7 +44,7 @@ And add the obvious implementations of `Clone::clone` and `Clone::clone_from` as Remove the macro implementations in libcore. We still have macro implementations for other "derived" traits, such as `PartialEq`, `Hash`, etc. -Note that independently of this RFC, we're adding builtin `Clone` impls for all "scalar" types, most importantly fn pointer and fn item types (where manual impls are impossible in the foreseeable future because of HRTBs, e.g. `for<'a> fn(SomeLocalStruct<'a>)`), which are already `Copy`: +Note that independently of this RFC, we're adding builtin `Clone` impls for all "scalar" types, most importantly fn pointer and fn item types (where manual impls are impossible in the foreseeable future because of higher-ranked types, e.g. `for<'a> fn(SomeLocalStruct<'a>)`), which are already `Copy`: ``` T fn pointer type ---------- From 4a4b3c5fe73702cd3d0fddefb85d44488ed6d198 Mon Sep 17 00:00:00 2001 From: Aaron Turon Date: Mon, 11 Sep 2017 09:22:18 -0700 Subject: [PATCH 6/6] RFC 2133: compiler-generated Clone impls for Arrays and Tuples --- text/{0000-all-the-clones.md => 2133-all-the-clones.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename text/{0000-all-the-clones.md => 2133-all-the-clones.md} (98%) diff --git a/text/0000-all-the-clones.md b/text/2133-all-the-clones.md similarity index 98% rename from text/0000-all-the-clones.md rename to text/2133-all-the-clones.md index 27e85f92d6f..1ef1870f29b 100644 --- a/text/0000-all-the-clones.md +++ b/text/2133-all-the-clones.md @@ -1,7 +1,7 @@ - Feature Name: all_the_clones - Start Date: 2017-08-28 -- RFC PR: (leave this empty) -- Rust Issue: (leave this empty) +- RFC PR: https://github.com/rust-lang/rfcs/pull/2133 +- Rust Issue: https://github.com/rust-lang/rust/issues/44496 # Summary [summary]: #summary