Auto merge of #10925 - Centri3:needless_clone_impl2, r=xFrednet

add lint [`incorrect_clone_impl_on_copy_type`]

Split off from #10788.

Closes #10700

----

changelog: new lint [`incorrect_clone_impl_on_copy_type`]
[#10925](#10925)

Author: bors

CHANGELOG.md

@@ -4834,6 +4834,7 @@ Released 2018-09-13
 [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops
 [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping
 [`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor
+[`incorrect_clone_impl_on_copy_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#incorrect_clone_impl_on_copy_type
 [`index_refutable_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice
 [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
 [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask

clippy_lints/src/declared_lints.rs

@@ -206,6 +206,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO,
     crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO,
     crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_INFO,
+    crate::incorrect_impls::INCORRECT_CLONE_IMPL_ON_COPY_TYPE_INFO,
     crate::index_refutable_slice::INDEX_REFUTABLE_SLICE_INFO,
     crate::indexing_slicing::INDEXING_SLICING_INFO,
     crate::indexing_slicing::OUT_OF_BOUNDS_INDEXING_INFO,

clippy_lints/src/incorrect_impls.rs

@@ -0,0 +1,124 @@
+use clippy_utils::{diagnostics::span_lint_and_sugg, get_parent_node, last_path_segment, ty::implements_trait};
+use rustc_errors::Applicability;
+use rustc_hir::{ExprKind, ImplItem, ImplItemKind, ItemKind, Node, UnOp};
+use rustc_hir_analysis::hir_ty_to_ty;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::EarlyBinder;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::{sym, symbol};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for manual implementations of `Clone` when `Copy` is already implemented.
+    ///
+    /// ### Why is this bad?
+    /// If both `Clone` and `Copy` are implemented, they must agree. This is done by dereferencing
+    /// `self` in `Clone`'s implementation. Anything else is incorrect.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// #[derive(Eq, PartialEq)]
+    /// struct A(u32);
+    ///
+    /// impl Clone for A {
+    ///     fn clone(&self) -> Self {
+    ///         Self(self.0)
+    ///     }
+    /// }
+    ///
+    /// impl Copy for A {}
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// #[derive(Eq, PartialEq)]
+    /// struct A(u32);
+    ///
+    /// impl Clone for A {
+    ///     fn clone(&self) -> Self {
+    ///         *self
+    ///     }
+    /// }
+    ///
+    /// impl Copy for A {}
+    /// ```
+    #[clippy::version = "1.72.0"]
+    pub INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
+    correctness,
+    "manual implementation of `Clone` on a `Copy` type"
+}
+declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE]);
+
+impl LateLintPass<'_> for IncorrectImpls {
+    #[expect(clippy::needless_return)]
+    fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
+        let node = get_parent_node(cx.tcx, impl_item.hir_id());
+        let Some(Node::Item(item)) = node else {
+            return;
+        };
+        let ItemKind::Impl(imp) = item.kind else {
+            return;
+        };
+        let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else {
+            return;
+        };
+        let trait_impl_def_id = trait_impl.def_id;
+        if cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) {
+            return;
+        }
+        let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir().impl_item(impl_item.impl_item_id()).kind else {
+            return;
+        };
+        let body = cx.tcx.hir().body(impl_item_id);
+        let ExprKind::Block(block, ..) = body.value.kind else {
+            return;
+        };
+        // Above is duplicated from the `duplicate_manual_partial_ord_impl` branch.
+        // Remove it while solving conflicts once that PR is merged.
+
+        // Actual implementation; remove this comment once aforementioned PR is merged
+        if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl_def_id)
+            && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy)
+            && implements_trait(
+                    cx,
+                    hir_ty_to_ty(cx.tcx, imp.self_ty),
+                    copy_def_id,
+                    trait_impl.substs,
+                )
+        {
+            if impl_item.ident.name == sym::clone {
+                if block.stmts.is_empty()
+                    && let Some(expr) = block.expr
+                    && let ExprKind::Unary(UnOp::Deref, inner) = expr.kind
+                    && let ExprKind::Path(qpath) = inner.kind
+                    && last_path_segment(&qpath).ident.name == symbol::kw::SelfLower
+                {} else {
+                    span_lint_and_sugg(
+                        cx,
+                        INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
+                        block.span,
+                        "incorrect implementation of `clone` on a `Copy` type",
+                        "change this to",
+                        "{ *self }".to_owned(),
+                        Applicability::MaybeIncorrect,
+                    );
+
+                    return;
+                }
+            }
+
+            if impl_item.ident.name == sym::clone_from {
+                span_lint_and_sugg(
+                    cx,
+                    INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
+                    impl_item.span,
+                    "incorrect implementation of `clone_from` on a `Copy` type",
+                    "remove this",
+                    String::new(),
+                    Applicability::MaybeIncorrect,
+                );
+
+                return;
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -150,6 +150,7 @@ mod implicit_return;
 mod implicit_saturating_add;
 mod implicit_saturating_sub;
 mod inconsistent_struct_constructor;
+mod incorrect_impls;
 mod index_refutable_slice;
 mod indexing_slicing;
 mod infinite_iter;
@@ -1047,6 +1048,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let stack_size_threshold = conf.stack_size_threshold;
     store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(stack_size_threshold)));
     store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit));
+    store.register_late_pass(|_| Box::new(incorrect_impls::IncorrectImpls));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

tests/ui/clone_on_copy_impl.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::incorrect_clone_impl_on_copy_type)]
+
 use std::fmt;
 use std::marker::PhantomData;
 

tests/ui/derive.rs

@@ -1,4 +1,4 @@
-#![allow(dead_code)]
+#![allow(clippy::incorrect_clone_impl_on_copy_type, dead_code)]
 #![warn(clippy::expl_impl_clone_on_copy)]
 
 #[derive(Copy)]

tests/ui/incorrect_clone_impl_on_copy_type.fixed

@@ -0,0 +1,97 @@
+//@run-rustfix
+#![allow(clippy::clone_on_copy, unused)]
+#![no_main]
+
+// lint
+
+struct A(u32);
+
+impl Clone for A {
+    fn clone(&self) -> Self { *self }
+
+    
+}
+
+impl Copy for A {}
+
+// do not lint
+
+struct B(u32);
+
+impl Clone for B {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl Copy for B {}
+
+// do not lint derived (clone's implementation is `*self` here anyway)
+
+#[derive(Clone, Copy)]
+struct C(u32);
+
+// do not lint derived (fr this time)
+
+struct D(u32);
+
+#[automatically_derived]
+impl Clone for D {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl Copy for D {}
+
+// do not lint if clone is not manually implemented
+
+struct E(u32);
+
+#[automatically_derived]
+impl Clone for E {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl Copy for E {}
+
+// lint since clone is not derived
+
+#[derive(Copy)]
+struct F(u32);
+
+impl Clone for F {
+    fn clone(&self) -> Self { *self }
+
+    
+}
+
+// do not lint since copy has more restrictive bounds
+
+#[derive(Eq, PartialEq)]
+struct Uwu<A: Copy>(A);
+
+impl<A: Copy> Clone for Uwu<A> {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}

tests/ui/incorrect_clone_impl_on_copy_type.rs

@@ -0,0 +1,107 @@
+//@run-rustfix
+#![allow(clippy::clone_on_copy, unused)]
+#![no_main]
+
+// lint
+
+struct A(u32);
+
+impl Clone for A {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl Copy for A {}
+
+// do not lint
+
+struct B(u32);
+
+impl Clone for B {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl Copy for B {}
+
+// do not lint derived (clone's implementation is `*self` here anyway)
+
+#[derive(Clone, Copy)]
+struct C(u32);
+
+// do not lint derived (fr this time)
+
+struct D(u32);
+
+#[automatically_derived]
+impl Clone for D {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl Copy for D {}
+
+// do not lint if clone is not manually implemented
+
+struct E(u32);
+
+#[automatically_derived]
+impl Clone for E {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl Copy for E {}
+
+// lint since clone is not derived
+
+#[derive(Copy)]
+struct F(u32);
+
+impl Clone for F {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+// do not lint since copy has more restrictive bounds
+
+#[derive(Eq, PartialEq)]
+struct Uwu<A: Copy>(A);
+
+impl<A: Copy> Clone for Uwu<A> {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+
+    fn clone_from(&mut self, source: &Self) {
+        source.clone();
+        *self = source.clone();
+    }
+}
+
+impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}