fix: incompatibility of non_canonical_clone_impl and implicit_return

clippy_lints/src/non_canonical_impls.rs

@@ -1,10 +1,11 @@
+use super::implicit_return::IMPLICIT_RETURN;
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::res::{MaybeDef, MaybeQPath};
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{is_from_proc_macro, last_path_segment, std_or_core};
+use clippy_utils::{is_from_proc_macro, is_lint_allowed, last_path_segment, std_or_core};
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, LangItem, UnOp};
+use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Stmt, StmtKind, UnOp};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::ty::{TyCtxt, TypeckResults};
 use rustc_session::impl_lint_pass;
@@ -185,8 +186,8 @@ impl LateLintPass<'_> for NonCanonicalImpls {
                     if let Some(copy_trait) = self.copy_trait
                         && implements_trait(cx, trait_impl.self_ty(), copy_trait, &[])
                     {
-                        for (assoc, _, block) in assoc_fns {
-                            check_clone_on_copy(cx, assoc, block);
+                        for (assoc, body, _) in assoc_fns {
+                            check_clone_on_copy(cx, assoc, body.value);
                         }
                     }
                 },
@@ -208,29 +209,34 @@ impl LateLintPass<'_> for NonCanonicalImpls {
     }
 }
 
-fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &Block<'_>) {
-    if impl_item.ident.name == sym::clone {
-        if block.stmts.is_empty()
-            && let Some(expr) = block.expr
-            && let ExprKind::Unary(UnOp::Deref, deref) = expr.kind
-            && let ExprKind::Path(qpath) = deref.kind
-            && last_path_segment(&qpath).ident.name == kw::SelfLower
-        {
-            // this is the canonical implementation, `fn clone(&self) -> Self { *self }`
-            return;
-        }
+fn is_deref_self(expr: &Expr<'_>) -> bool {
+    if let ExprKind::Unary(UnOp::Deref, deref) = expr.kind
+        && let ExprKind::Path(qpath) = deref.kind
+        && last_path_segment(&qpath).ident.name == kw::SelfLower
+    {
+        return true;
+    }
+    false
+}
 
+fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, body_expr: &Expr<'_>) {
+    if impl_item.ident.name == sym::clone {
         if is_from_proc_macro(cx, impl_item) {
             return;
         }
 
+        let add_return = match is_canonical_clone_body(body_expr) {
+            IsCanonical::WithReturn if is_lint_allowed(cx, IMPLICIT_RETURN, body_expr.hir_id) => false,
+            IsCanonical::WithReturn | IsCanonical::WithoutReturn => return,
+            IsCanonical::No => !is_lint_allowed(cx, IMPLICIT_RETURN, body_expr.hir_id),
+        };
         span_lint_and_sugg(
             cx,
             NON_CANONICAL_CLONE_IMPL,
-            block.span,
+            body_expr.span,
             "non-canonical implementation of `clone` on a `Copy` type",
             "change this to",
-            "{ *self }".to_owned(),
+            if add_return { "{ return *self; }" } else { "{ *self }" }.to_owned(),
             Applicability::MaybeIncorrect,
         );
     }
@@ -248,6 +254,40 @@ fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &B
     }
 }
 
+enum IsCanonical {
+    WithoutReturn,
+    WithReturn,
+    No,
+}
+
+fn is_canonical_clone_body(body_expr: &Expr<'_>) -> IsCanonical {
+    let ExprKind::Block(block, ..) = body_expr.kind else {
+        return IsCanonical::No;
+    };
+    let single_expr = match (block.stmts, block.expr) {
+        ([], Some(expr)) => Some(expr),
+        (
+            [
+                Stmt {
+                    kind: StmtKind::Expr(expr) | StmtKind::Semi(expr),
+                    ..
+                },
+            ],
+            None,
+        ) => Some(*expr),
+        _ => None,
+    };
+    let Some(expr) = single_expr else {
+        return IsCanonical::No;
+    };
+
+    match expr.kind {
+        ExprKind::Ret(Some(ret)) if is_deref_self(ret) => IsCanonical::WithReturn,
+        _ if is_deref_self(expr) => IsCanonical::WithoutReturn,
+        _ => IsCanonical::No,
+    }
+}
+
 fn check_partial_ord_on_ord<'tcx>(
     cx: &LateContext<'tcx>,
     impl_item: &ImplItem<'_>,
@@ -269,7 +309,7 @@ fn check_partial_ord_on_ord<'tcx>(
     // Fix #12683, allow [`needless_return`] here
     else if block.expr.is_none()
         && let Some(stmt) = block.stmts.first()
-        && let rustc_hir::StmtKind::Semi(Expr {
+        && let StmtKind::Semi(Expr {
             kind: ExprKind::Ret(Some(ret)),
             ..
         }) = stmt.kind

tests/ui/non_canonical_clone_impl.fixed

@@ -6,8 +6,6 @@
 extern crate proc_macros;
 use proc_macros::inline_macros;
 
-// lint
-
 struct A(u32);
 
 impl Clone for A {
@@ -31,12 +29,10 @@ impl Clone for B {
 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]
@@ -54,7 +50,6 @@ impl Clone for D {
 impl Copy for D {}
 
 // do not lint if clone is not manually implemented
-
 struct E(u32);
 
 #[automatically_derived]
@@ -83,7 +78,6 @@ impl Clone for F {
 }
 
 // do not lint since copy has more restrictive bounds
-
 #[derive(Eq, PartialEq)]
 struct Uwu<A: Copy>(A);
 
@@ -104,7 +98,7 @@ impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}
 mod issue12788 {
     use proc_macros::{external, with_span};
 
-    // lint -- not an external macro
+    // lint non-external macro
     inline!(
         #[derive(Copy)]
         pub struct A;
@@ -114,7 +108,7 @@ mod issue12788 {
         }
     );
 
-    // do not lint -- should skip external macros
+    // do not lint external macros
     external!(
         #[derive(Copy)]
         pub struct B;
@@ -126,7 +120,7 @@ mod issue12788 {
         }
     );
 
-    // do not lint -- should skip proc macros
+    // do not lint proc macros
     #[derive(proc_macro_derive::NonCanonicalClone)]
     pub struct C;
 
@@ -142,3 +136,36 @@ mod issue12788 {
         }
     );
 }
+
+struct N(u32);
+
+impl Clone for N {
+    fn clone(&self) -> Self { *self }
+}
+
+impl Copy for N {}
+
+/// Test for corner cases with `implicit_return` enabled
+mod with_implicit_return {
+    #![warn(clippy::implicit_return)]
+    #![allow(clippy::needless_return)]
+
+    // Don't lint `return *self` under `implicit_return`
+    struct G(u32);
+
+    impl Clone for G {
+        fn clone(&self) -> Self {
+            return *self;
+        }
+    }
+
+    impl Copy for G {}
+
+    struct H(u32);
+
+    impl Clone for H {
+        fn clone(&self) -> Self { return *self; }
+    }
+
+    impl Copy for H {}
+}

tests/ui/non_canonical_clone_impl.rs

@@ -6,8 +6,6 @@
 extern crate proc_macros;
 use proc_macros::inline_macros;
 
-// lint
-
 struct A(u32);
 
 impl Clone for A {
@@ -38,12 +36,10 @@ impl Clone for B {
 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]
@@ -61,7 +57,6 @@ impl Clone for D {
 impl Copy for D {}
 
 // do not lint if clone is not manually implemented
-
 struct E(u32);
 
 #[automatically_derived]
@@ -97,7 +92,6 @@ impl Clone for F {
 }
 
 // do not lint since copy has more restrictive bounds
-
 #[derive(Eq, PartialEq)]
 struct Uwu<A: Copy>(A);
 
@@ -118,7 +112,7 @@ impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}
 mod issue12788 {
     use proc_macros::{external, with_span};
 
-    // lint -- not an external macro
+    // lint non-external macro
     inline!(
         #[derive(Copy)]
         pub struct A;
@@ -131,7 +125,7 @@ mod issue12788 {
         }
     );
 
-    // do not lint -- should skip external macros
+    // do not lint external macros
     external!(
         #[derive(Copy)]
         pub struct B;
@@ -143,7 +137,7 @@ mod issue12788 {
         }
     );
 
-    // do not lint -- should skip proc macros
+    // do not lint proc macros
     #[derive(proc_macro_derive::NonCanonicalClone)]
     pub struct C;
 
@@ -159,3 +153,44 @@ mod issue12788 {
         }
     );
 }
+
+struct N(u32);
+
+impl Clone for N {
+    fn clone(&self) -> Self {
+        //~^ non_canonical_clone_impl
+        { *self }
+    }
+}
+
+impl Copy for N {}
+
+/// Test for corner cases with `implicit_return` enabled
+mod with_implicit_return {
+    #![warn(clippy::implicit_return)]
+    #![allow(clippy::needless_return)]
+
+    // Don't lint `return *self` under `implicit_return`
+    struct G(u32);
+
+    impl Clone for G {
+        fn clone(&self) -> Self {
+            return *self;
+        }
+    }
+
+    impl Copy for G {}
+
+    struct H(u32);
+
+    impl Clone for H {
+        fn clone(&self) -> Self {
+            //~^ non_canonical_clone_impl
+            {
+                return *self;
+            }
+        }
+    }
+
+    impl Copy for H {}
+}