Generate builtin impls for `Clone` #43690

scalexm · Aug 5, 2017

This fixes a long-standing ICE and limitation where some builtin types implement Copy but not Clone (whereas Clone is a super trait of Copy).

However, this PR has a few side-effects:

Clone is now marked as a lang item.
[T; N] is now Clone if T: Clone (currently, only if T: Copy and for N <= 32).
fn foo<'a>() where &'a mut (): Clone { } won't compile anymore because of how bounds for builtin traits are handled (e.g. same thing currently if you replace Clone by Copy in this example). Of course this function is unusable anyway, an error would pop as soon as it is called.

Hence, I'm wondering wether this PR would need an RFC...
Also, cc-ing @nikomatsakis, @arielb1.

Related issues: #28229, #24000.

rust-highfive · Aug 5, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · Aug 5, 2017

IMO it does need an RFC and I am against this specific implementation atm, although I am pleasantly surprised it's possible at all. There might be some value to moving all builtin auto-derives to MIR.

cc @rust-lang/lang @rust-lang/compiler

arielb1 · Aug 6, 2017

@eddyb

What's your problem with the implementation? I think it's pretty clean.

However, we are probably not going to merge this at least before @nikomatsakis returns from vacation next week.

arielb1 · Aug 6, 2017

arielb1 reviewed Aug 6, 2017

View changes

arielb1 · Aug 6, 2017

src/librustc_trans/monomorphize.rs

@@ -143,6 +143,12 @@ fn resolve_associated_item<'a, 'tcx>(
                substs: rcvr_substs
            }
        }
+        traits::VtableBuiltin(ref data) => {
+            Instance {
+                def: ty::InstanceDef::BuiltinShim(def_id, data.ty),


You don't need to store ty anywhere - it is available as trait_ref.self_ty().

eddyb · Aug 6, 2017

@arielb1 My problem is not how it's implemented, but using this method only for some Clone impls. I'd be far more comfortable if all built-in derive impls were moved to MIR shims.

arielb1 · Aug 6, 2017

@eddyb

Because that would mean the code is "tested" more? I'm not so sure on that - it e.g. means we'll have to re-implement error reporting on #[repr(packed)] types, and who knows what else. Also, Clone is still an inductive trait (and must be, because it has a supertrait), so we'll have to do something about the bounds.

arielb1 · Aug 7, 2017

arielb1 reviewed Aug 7, 2017

View changes

arielb1 · Aug 7, 2017

src/librustc/ty/instance.rs

    DropGlue(DefId, Option<Ty<'tcx>>),
+
+    /// Builtin method implementation, e.g. `Clone::clone`.
+    BuiltinShim(DefId, Ty<'tcx>),


Can't this be CloneShim? In other places I have a separate shim class for each trait.

Seems fine, I'll change that.

arielb1 · Aug 7, 2017

arielb1 reviewed Aug 7, 2017

View changes

arielb1 · Aug 7, 2017

src/librustc_mir/shim.rs

+/// Build a `Clone::clone` shim for `recvr_ty`. Here, `def_id` is `Clone::clone`.
+fn build_clone_shim<'a, 'tcx>(tcx: ty::TyCtxt<'a, 'tcx, 'tcx>,
+                              def_id: DefId,
+                              rcvr_ty: ty::Ty<'tcx>)


don't call this rcvr_ty - ty is fine. The receiver is the first argument of the method, which has type &Self (e.g. &[Vec<u32>; 2] if the impl is for <[Vec<u32>; 2] as Clone>. Also fix the comment.

arielb1 · Aug 7, 2017

arielb1 reviewed Aug 7, 2017

View changes

arielb1 · Aug 7, 2017

src/librustc_mir/shim.rs

+/// Build a `Clone::clone` shim for `recvr_ty`. Here, `def_id` is `Clone::clone`.
+fn build_clone_shim<'a, 'tcx>(tcx: ty::TyCtxt<'a, 'tcx, 'tcx>,
+                              def_id: DefId,
+                              rcvr_ty: ty::Ty<'tcx>)


don't call this rcvr_ty - ty is fine. The receiver is the first argument of the method, which has type &Self (e.g. &[Vec<u32>; 2] if the impl is for <[Vec<u32>; 2] as Clone>. Also fix the comment above.

Actually there are too many variables named ty appearing in the code, and I ended up confusing two of them (and having a wrong code generation), that's why I renamed it but I agree rcvr_ty is not ideal, I'll find something else.

Is self_ty fine?

Yup self_ty would be fine.

arielb1 · Aug 7, 2017

arielb1 reviewed Aug 7, 2017

View changes

arielb1 · Aug 7, 2017

src/librustc_mir/shim.rs

+                              rcvr_ty: ty::Ty<'tcx>)
+                              -> Mir<'tcx>
+{
+    let sig = tcx.fn_sig(def_id);


Small note: this is a "fully generic" shim, so it knows the concrete receiver type. You could check if the source type is Copy and emit an "optimized" impl that just copies the result. This will speed up cloning [u8; 1048576] on unoptimized builds and is generally a good idea.

Also, as @eddyb says, it might be a smart idea to plumb #[derive(Clone)] impls through this to avoid implementing the logic twice and to potentially speed up compilation - maybe have #[derive(Clone)] add a #[is_builtin_clone] attribute to the method? I'm not sure.

@arielb1 how would we handle bounds checking? #[derive(Clone)] currently uses local variables of type AssertParamIsClone<T: Clone + ?Sized> for each field type T in order to deal with recursive types. This would not be applicable here.

Actually since we are "fully generic" here as you said, we would could indeed check by hand that the bounds hold for each field type when building the shim.

About plumbing #[derive(Clone)] - we want to give an error on the generic types, before monomorphization, so we can't use this function. So we need a separate checking function - maybe that is not a good idea after all.

arielb1 · Aug 7, 2017

arielb1 reviewed Aug 7, 2017

View changes

arielb1 · Aug 7, 2017

src/librustc_mir/shim.rs

+
+    match rcvr_ty.sty {
+        ty::TyArray(ty, len) => {
+            let mut returns = Vec::new();


I'm worried this might create code explosion for large arrays (think of <[Box<u8>; 1048576]>::clone). Can you try to use a loop and let LLVM unroll it? You're late MIR, so I don't think you have to care about pre-initializing the array.

There is also the possibility to only build a Clone shim for [T; N] where T: Copy, as it's the case already.

I would just write a loop.

arielb1 · Aug 7, 2017

arielb1 reviewed Aug 7, 2017

View changes

arielb1 · Aug 7, 2017

src/librustc_mir/shim.rs

+            func,
+            args: vec![Operand::Consume(ref_loc)],
+            destination: Some((loc.clone(), BasicBlock::new(i + 1))),
+            cleanup: None,


This is wrong - if the drop of a field panics, you need to drop all the previous fields.

It sounds like we have way too many places that try to create their own drop ladders, and we need some sort of unified abstraction.

Right, I was going to ask you about this anyway.

I'll need to write a drop ladder abstraction to help clean this up.

aturon · Aug 8, 2017

From a lang team perspective, I am on board with this change. I don't have an opinion on the compiler side of things.

scalexm · Aug 9, 2017

@arielb1 should be good now

bors · Aug 11, 2017

☔️ The latest upstream changes (presumably #43748) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · Aug 21, 2017

I like the approach. =)

@bors r+

bors · Aug 21, 2017

📌 Commit d58b40e has been approved by nikomatsakis

bors · Aug 21, 2017

⌛️ Testing commit d58b40e with merge 31c5918...

bors · Aug 21, 2017

💔 Test failed - status-appveyor

alexcrichton · Aug 21, 2017

@bors: retry

innosetup download failure #43985

bors · Aug 22, 2017

⌛️ Testing commit d58b40e with merge 942711e...

bors · Aug 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 942711e to master...

nikomatsakis · Aug 22, 2017

OK, so, I think I was a bit hasty in merging this PR. I thought that the only question at hand was whether it was ok for clone to be a lang-item and for us to generate impls of Clone for types where T: Copy. I have no objection to either of those things. In particular, I feel that the existence of said clone impls is already implied by the language, so we're not "growing" the language by having the compiler synthesizing them, just fixing a bug (in discussion on IRC, @eddyb took an opposite approach, claiming that the bug is that types with no clone impl are considered Copy. This is fair, but changing that would be backwards incompatible, since e.g. [u8; 256] would no longer be copy).

However, reading more carefully, I see that this PR generates clone impls for a few other cases beyond that minimal set. To be fair, these were called out in the PR description. I was just hasty.

For example, prior to the PR, [T; 3] is only Clone if T: Copy (i.e., we use a macro to generate impls, and we only generate them if T: Copy, rather than T: Clone). I don't understand that, but it appears to have been a deliberate decision. Perhaps someone on @rust-lang/libs would care to comment.
Also, I do see a distinction between generating a built-in impl for those cases that are Copy (and hence are required to be Clone for the lang to be consistent) and other types (i.e., impls where we must invoke clone() manually).
- So, for example, making [String; 256] be Clone would fall into the latter category (even ignoring the fact that, today, even [String; 2] is not Clone, since String is not Copy).
- I personally am in favor of such a change, but I do think it ought to have gone through a formal consensus process. I don't see the need necessarily for a full RFC, but rfcbot at least.

For those two reasons, I am thinking that perhaps we should back out this change -- or at least disable it temporarily until we reach a consensus. (And perhaps both libs and lang team ought to be involved, given that [String; 2] is not Clone today?)

alexcrichton · Aug 22, 2017

I'm not sure it was necessarily intentional for [String; 2] to not be Clone AFAIK. The only downside I could think of is that it adds a "huge" amount of IR/metadata bloat for something that doesn't actually work most of the time and is used in vanishingly rare situations.

nikomatsakis · Aug 22, 2017

@alexcrichton

I'm not sure it was necessarily intentional for [String; 2] to not be Clone AFAIK. The only downside I could think of is that it adds a "huge" amount of IR/metadata bloat for something that doesn't actually work most of the time and is used in vanishingly rare situations.

Fair. Of course, under the approach implemented in this PR, that is not an issue.

dtolnay · Aug 22, 2017

(String, String) already implemented Clone. I am on board with [String; 2] implementing Clone, and the other changes listed in the PR description.

bluss · Sep 29, 2017

Tracking issue #44496

rust-highfive assigned eddyb Aug 5, 2017

arielb1 added the S-waiting-on-author label Aug 8, 2017

scalexm force-pushed the scalexm:issue-28229 branch 2 times, most recently from 7370f8f to 779d2c5 Aug 8, 2017

scalexm force-pushed the scalexm:issue-28229 branch from 0d108eb to d58b40e Aug 14, 2017

arielb1 referenced this pull request Aug 15, 2017
Merged
Remove the trait selection impl in method::probe #43880

nikomatsakis self-assigned this Aug 21, 2017

bors merged commit d58b40e into rust-lang:master Aug 22, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Aug 22, 2017
Merged
Generator support #43076
7 of 7 tasks complete

bluss added the relnotes label Aug 22, 2017

arielb1 added a commit to arielb1/rust that referenced this pull request Aug 22, 2017

Revert "Auto merge of rust-lang#43690 - scalexm:issue-28229, r=nikoma…
…tsakis" This reverts commit 942711e, reversing changes made to 8df670b.

1e36d0b

arielb1 referenced this pull request Aug 22, 2017
Closed
Revert "Auto merge of #43690 - scalexm:issue-28229, r=nikomatsakis" #44045

killercup referenced this pull request Aug 23, 2017
Closed
September 2017 #37
10 of 10 tasks complete

RalfJung referenced this pull request Aug 23, 2017
Closed
implement built-in Clone #307

This was referenced Aug 28, 2017

Merged

Copy/Clone Closures rust-lang/rfcs#2132

Closed

Make Clone a lang-item and implement it for closures whose members are all Clone rust-lang/rfcs#1369

scalexm referenced this pull request Aug 30, 2017
Merged
RFC - compiler-generated Clone impls for Arrays and Tuples #2133

dimbleby referenced this pull request Oct 13, 2017
Closed
Derive Clone when deriving Copy #934

hcpl added a commit to hcpl/zalgo.rs that referenced this pull request Apr 9, 2018

Remove a `[derive(Clone)]` for Rust 1.20
This should be the relevant issue: rust-lang/rust#43690.

Loading status checks…

ab74dfd

scalexm deleted the scalexm:issue-28229 branch Oct 22, 2018

rust-lang/rust

Join GitHub today

Generate builtin impls for `Clone` #43690

Conversation

scalexm commented Aug 5, 2017

rust-highfive assigned eddyb Aug 5, 2017

This comment has been minimized.

rust-highfive commented Aug 5, 2017

This comment has been minimized.

eddyb commented Aug 5, 2017

This comment has been minimized.

arielb1 commented Aug 6, 2017

arielb1 reviewed Aug 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

eddyb commented Aug 6, 2017

This comment has been minimized.

arielb1 commented Aug 6, 2017 • edited

arielb1 reviewed Aug 7, 2017 View changes

This comment has been minimized.

This comment has been minimized.

arielb1 reviewed Aug 7, 2017 View changes

This comment has been minimized.

arielb1 reviewed Aug 7, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

arielb1 reviewed Aug 7, 2017 View changes

This comment has been minimized.

arielb1 Aug 7, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

arielb1 reviewed Aug 7, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

arielb1 reviewed Aug 7, 2017 View changes

This comment has been minimized.

arielb1 Aug 7, 2017 • edited

This comment has been minimized.

This comment has been minimized.

arielb1 added the S-waiting-on-author label Aug 8, 2017

scalexm force-pushed the scalexm:issue-28229 branch 2 times, most recently from 7370f8f to 779d2c5 Aug 8, 2017

This comment has been minimized.

aturon commented Aug 8, 2017

This comment has been minimized.

scalexm commented Aug 9, 2017

This comment has been minimized.

bors commented Aug 11, 2017

scalexm added some commits Aug 4, 2017

scalexm force-pushed the scalexm:issue-28229 branch from 0d108eb to d58b40e Aug 14, 2017

arielb1 referenced this pull request Aug 15, 2017

bors added a commit that referenced this pull request Aug 15, 2017

nikomatsakis self-assigned this Aug 21, 2017

This comment has been minimized.

nikomatsakis commented Aug 21, 2017

This comment has been minimized.

bors commented Aug 21, 2017

This comment has been minimized.

bors commented Aug 21, 2017

bors added a commit that referenced this pull request Aug 21, 2017

This comment has been minimized.

bors commented Aug 21, 2017

This comment has been minimized.

alexcrichton commented Aug 21, 2017

This comment has been minimized.

bors commented Aug 22, 2017

bors added a commit that referenced this pull request Aug 22, 2017

This comment has been minimized.

bors commented Aug 22, 2017

Hide details View details bors merged commit d58b40e into rust-lang:master Aug 22, 2017 2 checks passed

2 checks passed

bors referenced this pull request Aug 22, 2017

This comment has been minimized.

nikomatsakis commented Aug 22, 2017

This comment has been minimized.

alexcrichton commented Aug 22, 2017

This comment has been minimized.

arielb1 reviewed Aug 6, 2017

View changes

arielb1 commented Aug 6, 2017 •

edited

arielb1 reviewed Aug 7, 2017

View changes

arielb1 reviewed Aug 7, 2017

View changes

arielb1 reviewed Aug 7, 2017

View changes

arielb1 reviewed Aug 7, 2017

View changes

arielb1 Aug 7, 2017 •

edited

arielb1 reviewed Aug 7, 2017

View changes

arielb1 reviewed Aug 7, 2017

View changes

arielb1 Aug 7, 2017 •

edited

scalexm force-pushed the scalexm:issue-28229 branch 2 times, most recently from `7370f8f` to `779d2c5` Aug 8, 2017

scalexm force-pushed the scalexm:issue-28229 branch from `0d108eb` to `d58b40e` Aug 14, 2017

bors merged commit `d58b40e` into rust-lang:master Aug 22, 2017
2 checks passed