Rollup merge of #148807 - nnethercote:doc-Copy-Clone-problem, r=saethlin

Document (and test) a problem with `Clone`/`Copy` deriving.

I think this is useful information. I have worked on `derive` impls quite a bit and didn't know about this issue until today.

r? `@saethlin`

Author: Zalathar

compiler/rustc_resolve/src/macros.rs

@@ -384,6 +384,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
         //   is applied, so they have to be produced by the container's expansion rather
         //   than by individual derives.
         // - Derives in the container need to know whether one of them is a built-in `Copy`.
+        //   (But see the comment mentioning #124794 below.)
         // Temporarily take the data to avoid borrow checker conflicts.
         let mut derive_data = mem::take(&mut self.derive_data);
         let entry = derive_data.entry(expn_id).or_insert_with(|| DeriveData {
@@ -440,7 +441,13 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
             .collect();
         self.helper_attrs.insert(expn_id, helper_attrs);
         // Mark this derive as having `Copy` either if it has `Copy` itself or if its parent derive
-        // has `Copy`, to support cases like `#[derive(Clone, Copy)] #[derive(Debug)]`.
+        // has `Copy`, to support `#[derive(Copy, Clone)]`, `#[derive(Clone, Copy)]`, or
+        // `#[derive(Copy)] #[derive(Clone)]`. We do this because the code generated for
+        // `derive(Clone)` changes if `derive(Copy)` is also present.
+        //
+        // FIXME(#124794): unfortunately this doesn't work with `#[derive(Clone)] #[derive(Copy)]`.
+        // When the `Clone` impl is generated the `#[derive(Copy)]` hasn't been processed and
+        // `has_derive_copy` hasn't been set yet.
         if entry.has_derive_copy || self.has_derive_copy(parent_scope.expansion) {
             self.containers_deriving_copy.insert(expn_id);
         }

tests/ui/deriving/deriving-all-codegen.rs

@@ -24,15 +24,15 @@ use std::from::From;
 #[derive(Clone, Copy, Debug, Default, Hash, PartialEq, Eq, PartialOrd, Ord)]
 struct Empty;
 
-// A basic struct. Note: because this derives `Copy`, it gets the simple
+// A basic struct. Note: because this derives `Copy`, it gets the trivial
 // `clone` implemention that just does `*self`.
 #[derive(Clone, Copy, Debug, Default, Hash, PartialEq, Eq, PartialOrd, Ord)]
 struct Point {
     x: u32,
     y: u32,
 }
 
-// A basic packed struct. Note: because this derives `Copy`, it gets the simple
+// A basic packed struct. Note: because this derives `Copy`, it gets the trivial
 // `clone` implemention that just does `*self`.
 #[derive(Clone, Copy, Debug, Default, Hash, PartialEq, Eq, PartialOrd, Ord)]
 #[repr(packed)]
@@ -49,7 +49,7 @@ struct SingleField {
     foo: bool,
 }
 
-// A large struct. Note: because this derives `Copy`, it gets the simple
+// A large struct. Note: because this derives `Copy`, it gets the trivial
 // `clone` implemention that just does `*self`.
 #[derive(Clone, Copy, Debug, Default, Hash, PartialEq, Eq, PartialOrd, Ord)]
 struct Big {
@@ -79,25 +79,25 @@ struct Reorder {
     b10: &'static *const bool,
 }
 
-// A struct that doesn't impl `Copy`, which means it gets the non-simple
+// A struct that doesn't impl `Copy`, which means it gets the non-trivial
 // `clone` implemention that clones the fields individually.
 #[derive(Clone)]
 struct NonCopy(u32);
 
-// A packed struct that doesn't impl `Copy`, which means it gets the non-simple
+// A packed struct that doesn't impl `Copy`, which means it gets the non-trivial
 // `clone` implemention that clones the fields individually.
 #[derive(Clone)]
 #[repr(packed)]
 struct PackedNonCopy(u32);
 
-// A struct that impls `Copy` manually, which means it gets the non-simple
+// A struct that impls `Copy` manually, which means it gets the non-trivial
 // `clone` implemention that clones the fields individually.
 #[derive(Clone)]
 struct ManualCopy(u32);
 impl Copy for ManualCopy {}
 
 // A packed struct that impls `Copy` manually, which means it gets the
-// non-simple `clone` implemention that clones the fields individually.
+// non-trivial `clone` implemention that clones the fields individually.
 #[derive(Clone)]
 #[repr(packed)]
 struct PackedManualCopy(u32);
@@ -218,3 +218,20 @@ pub union Union {
     pub u: u32,
     pub i: i32,
 }
+
+#[derive(Copy, Clone)]
+struct FooCopyClone(i32);
+
+#[derive(Clone, Copy)]
+struct FooCloneCopy(i32);
+
+#[derive(Copy)]
+#[derive(Clone)]
+struct FooCopyAndClone(i32);
+
+// FIXME(#124794): the previous three structs all have a trivial `Copy`-aware
+// `clone`. But this one doesn't because when the `clone` is generated the
+// `derive(Copy)` hasn't yet been seen.
+#[derive(Clone)]
+#[derive(Copy)]
+struct FooCloneAndCopy(i32);

tests/ui/deriving/deriving-all-codegen.stdout

@@ -84,7 +84,7 @@ impl ::core::cmp::Ord for Empty {
     }
 }
 
-// A basic struct. Note: because this derives `Copy`, it gets the simple
+// A basic struct. Note: because this derives `Copy`, it gets the trivial
 // `clone` implemention that just does `*self`.
 struct Point {
     x: u32,
@@ -171,7 +171,7 @@ impl ::core::cmp::Ord for Point {
     }
 }
 
-// A basic packed struct. Note: because this derives `Copy`, it gets the simple
+// A basic packed struct. Note: because this derives `Copy`, it gets the trivial
 // `clone` implemention that just does `*self`.
 #[repr(packed)]
 struct PackedPoint {
@@ -409,7 +409,7 @@ impl ::core::cmp::Ord for SingleField {
     }
 }
 
-// A large struct. Note: because this derives `Copy`, it gets the simple
+// A large struct. Note: because this derives `Copy`, it gets the trivial
 // `clone` implemention that just does `*self`.
 struct Big {
     b1: u32,
@@ -676,7 +676,7 @@ impl ::core::cmp::PartialOrd for Reorder {
     }
 }
 
-// A struct that doesn't impl `Copy`, which means it gets the non-simple
+// A struct that doesn't impl `Copy`, which means it gets the non-trivial
 // `clone` implemention that clones the fields individually.
 struct NonCopy(u32);
 #[automatically_derived]
@@ -687,7 +687,7 @@ impl ::core::clone::Clone for NonCopy {
     }
 }
 
-// A packed struct that doesn't impl `Copy`, which means it gets the non-simple
+// A packed struct that doesn't impl `Copy`, which means it gets the non-trivial
 // `clone` implemention that clones the fields individually.
 #[repr(packed)]
 struct PackedNonCopy(u32);
@@ -699,7 +699,7 @@ impl ::core::clone::Clone for PackedNonCopy {
     }
 }
 
-// A struct that impls `Copy` manually, which means it gets the non-simple
+// A struct that impls `Copy` manually, which means it gets the non-trivial
 // `clone` implemention that clones the fields individually.
 struct ManualCopy(u32);
 #[automatically_derived]
@@ -712,7 +712,7 @@ impl ::core::clone::Clone for ManualCopy {
 impl Copy for ManualCopy {}
 
 // A packed struct that impls `Copy` manually, which means it gets the
-// non-simple `clone` implemention that clones the fields individually.
+// non-trivial `clone` implemention that clones the fields individually.
 #[repr(packed)]
 struct PackedManualCopy(u32);
 #[automatically_derived]
@@ -1791,3 +1791,62 @@ impl ::core::clone::Clone for Union {
 }
 #[automatically_derived]
 impl ::core::marker::Copy for Union { }
+
+struct FooCopyClone(i32);
+#[automatically_derived]
+impl ::core::marker::Copy for FooCopyClone { }
+#[automatically_derived]
+#[doc(hidden)]
+unsafe impl ::core::clone::TrivialClone for FooCopyClone { }
+#[automatically_derived]
+impl ::core::clone::Clone for FooCopyClone {
+    #[inline]
+    fn clone(&self) -> FooCopyClone {
+        let _: ::core::clone::AssertParamIsClone<i32>;
+        *self
+    }
+}
+
+struct FooCloneCopy(i32);
+#[automatically_derived]
+#[doc(hidden)]
+unsafe impl ::core::clone::TrivialClone for FooCloneCopy { }
+#[automatically_derived]
+impl ::core::clone::Clone for FooCloneCopy {
+    #[inline]
+    fn clone(&self) -> FooCloneCopy {
+        let _: ::core::clone::AssertParamIsClone<i32>;
+        *self
+    }
+}
+#[automatically_derived]
+impl ::core::marker::Copy for FooCloneCopy { }
+
+struct FooCopyAndClone(i32);
+#[automatically_derived]
+#[doc(hidden)]
+unsafe impl ::core::clone::TrivialClone for FooCopyAndClone { }
+#[automatically_derived]
+impl ::core::clone::Clone for FooCopyAndClone {
+    #[inline]
+    fn clone(&self) -> FooCopyAndClone {
+        let _: ::core::clone::AssertParamIsClone<i32>;
+        *self
+    }
+}
+#[automatically_derived]
+impl ::core::marker::Copy for FooCopyAndClone { }
+
+// FIXME(#124794): the previous three structs all have a trivial `Copy`-aware
+// `clone`. But this one doesn't because when the `clone` is generated the
+// `derive(Copy)` hasn't yet been seen.
+struct FooCloneAndCopy(i32);
+#[automatically_derived]
+impl ::core::marker::Copy for FooCloneAndCopy { }
+#[automatically_derived]
+impl ::core::clone::Clone for FooCloneAndCopy {
+    #[inline]
+    fn clone(&self) -> FooCloneAndCopy {
+        FooCloneAndCopy(::core::clone::Clone::clone(&self.0))
+    }
+}