Auto merge of #6993 - Jarcho:expl_impl_clone, r=llogiq

Improve `expl_impl_clone_on_copy` fixes: #1254 changelog: Check to see if the generic constraints are the same as if using derive for `expl_impl_clone_on_copy`
rust-lang · Mar 28, 2021 · c07103b · c07103b
2 parents 4bcc552 + 879fa5c
commit c07103b
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 49 deletions.
diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs
@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_note, span_lint_and_then};
 use clippy_utils::paths;
-use clippy_utils::ty::is_copy;
+use clippy_utils::ty::{implements_trait, is_copy};
 use clippy_utils::{get_trait_def_id, is_allowed, is_automatically_derived, match_def_path};
 use if_chain::if_chain;
 use rustc_hir::def_id::DefId;
@@ -12,7 +12,7 @@ use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::source_map::Span;
+use rustc_span::{def_id::LOCAL_CRATE, source_map::Span};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for deriving `Hash` but implementing `PartialEq`
@@ -293,48 +293,53 @@ fn check_ord_partial_ord<'tcx>(
 
 /// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint.
 fn check_copy_clone<'tcx>(cx: &LateContext<'tcx>, item: &Item<'_>, trait_ref: &TraitRef<'_>, ty: Ty<'tcx>) {
-    if cx
-        .tcx
-        .lang_items()
-        .clone_trait()
-        .map_or(false, |id| Some(id) == trait_ref.trait_def_id())
-    {
-        if !is_copy(cx, ty) {
+    let clone_id = match cx.tcx.lang_items().clone_trait() {
+        Some(id) if trait_ref.trait_def_id() == Some(id) => id,
+        _ => return,
+    };
+    let copy_id = match cx.tcx.lang_items().copy_trait() {
+        Some(id) => id,
+        None => return,
+    };
+    let (ty_adt, ty_subs) = match *ty.kind() {
+        // Unions can't derive clone.
+        ty::Adt(adt, subs) if !adt.is_union() => (adt, subs),
+        _ => return,
+    };
+    // If the current self type doesn't implement Copy (due to generic constraints), search to see if
+    // there's a Copy impl for any instance of the adt.
+    if !is_copy(cx, ty) {
+        if ty_subs.non_erasable_generics().next().is_some() {
+            let has_copy_impl = cx
+                .tcx
+                .all_local_trait_impls(LOCAL_CRATE)
+                .get(&copy_id)
+                .map_or(false, |impls| {
+                    impls
+                        .iter()
+                        .any(|&id| matches!(cx.tcx.type_of(id).kind(), ty::Adt(adt, _) if ty_adt.did == adt.did))
+                });
+            if !has_copy_impl {
+                return;
+            }
+        } else {
             return;
         }
-
-        match *ty.kind() {
-            ty::Adt(def, _) if def.is_union() => return,
-
-            // Some types are not Clone by default but could be cloned “by hand” if necessary
-            ty::Adt(def, substs) => {
-                for variant in &def.variants {
-                    for field in &variant.fields {
-                        if let ty::FnDef(..) = field.ty(cx.tcx, substs).kind() {
-                            return;
-                        }
-                    }
-                    for subst in substs {
-                        if let ty::subst::GenericArgKind::Type(subst) = subst.unpack() {
-                            if let ty::Param(_) = subst.kind() {
-                                return;
-                            }
-                        }
-                    }
-                }
-            },
-            _ => (),
-        }
-
-        span_lint_and_note(
-            cx,
-            EXPL_IMPL_CLONE_ON_COPY,
-            item.span,
-            "you are implementing `Clone` explicitly on a `Copy` type",
-            Some(item.span),
-            "consider deriving `Clone` or removing `Copy`",
-        );
     }
+    // Derive constrains all generic types to requiring Clone. Check if any type is not constrained for
+    // this impl.
+    if ty_subs.types().any(|ty| !implements_trait(cx, ty, clone_id, &[])) {
+        return;
+    }
+
+    span_lint_and_note(
+        cx,
+        EXPL_IMPL_CLONE_ON_COPY,
+        item.span,
+        "you are implementing `Clone` explicitly on a `Copy` type",
+        Some(item.span),
+        "consider deriving `Clone` or removing `Copy`",
+    );
 }
 
 /// Implementation of the `UNSAFE_DERIVE_DESERIALIZE` lint.

diff --git a/tests/ui/derive.rs b/tests/ui/derive.rs
@@ -35,7 +35,6 @@ impl<'a> Clone for Lt<'a> {
     }
 }
 
-// Ok, `Clone` cannot be derived because of the big array
 #[derive(Copy)]
 struct BigArray {
     a: [u8; 65],
@@ -47,7 +46,6 @@ impl Clone for BigArray {
     }
 }
 
-// Ok, function pointers are not always Clone
 #[derive(Copy)]
 struct FnPtr {
     a: fn() -> !,
@@ -59,7 +57,7 @@ impl Clone for FnPtr {
     }
 }
 
-// Ok, generics
+// Ok, Clone trait impl doesn't have constrained generics.
 #[derive(Copy)]
 struct Generic<T> {
     a: T,
@@ -71,4 +69,21 @@ impl<T> Clone for Generic<T> {
     }
 }
 
+#[derive(Copy)]
+struct Generic2<T>(T);
+impl<T: Clone> Clone for Generic2<T> {
+    fn clone(&self) -> Self {
+        Self(self.0.clone())
+    }
+}
+
+// Ok, Clone trait impl doesn't have constrained generics.
+#[derive(Copy)]
+struct GenericRef<'a, T, U>(T, &'a U);
+impl<T: Clone, U> Clone for GenericRef<'_, T, U> {
+    fn clone(&self) -> Self {
+        Self(self.0.clone(), self.1)
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/derive.stderr b/tests/ui/derive.stderr
@@ -40,7 +40,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:44:1
+  --> $DIR/derive.rs:43:1
    |
 LL | / impl Clone for BigArray {
 LL | |     fn clone(&self) -> Self {
@@ -50,7 +50,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:44:1
+  --> $DIR/derive.rs:43:1
    |
 LL | / impl Clone for BigArray {
 LL | |     fn clone(&self) -> Self {
@@ -60,7 +60,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:56:1
+  --> $DIR/derive.rs:54:1
    |
 LL | / impl Clone for FnPtr {
 LL | |     fn clone(&self) -> Self {
@@ -70,7 +70,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:56:1
+  --> $DIR/derive.rs:54:1
    |
 LL | / impl Clone for FnPtr {
 LL | |     fn clone(&self) -> Self {
@@ -79,5 +79,25 @@ LL | |     }
 LL | | }
    | |_^
 
-error: aborting due to 4 previous errors
+error: you are implementing `Clone` explicitly on a `Copy` type
+  --> $DIR/derive.rs:74:1
+   |
+LL | / impl<T: Clone> Clone for Generic2<T> {
+LL | |     fn clone(&self) -> Self {
+LL | |         Self(self.0.clone())
+LL | |     }
+LL | | }
+   | |_^
+   |
+note: consider deriving `Clone` or removing `Copy`
+  --> $DIR/derive.rs:74:1
+   |
+LL | / impl<T: Clone> Clone for Generic2<T> {
+LL | |     fn clone(&self) -> Self {
+LL | |         Self(self.0.clone())
+LL | |     }
+LL | | }
+   | |_^
+
+error: aborting due to 5 previous errors