refactor(non_canonical_impls): lint starting from impls (#15749)

Based on
#15520 (comment),
with the following changes/additions:
- No longer use the `Trait` enum for quickly filtering out irrelevant
impls -- instead, check the `trait_of` basically right away:
    1. pre-fetch `DefId`s of the relevant traits into the lint pass
2. reuse `cx.tcx.impl_trait_ref`'s output for the the `DefId` of the
trait being implemented
    3. compare those `DefId`s, which should be very cheap
- Next, check whether `self_ty` implements the other relevant trait.
- Pre-filter impl items in the same (lazy) iterator, but delay the
proc-macro check as much as possible

changelog: none

Not auto-assigning since @blyxyas and/or @Jarcho will be the ones
reviewing it:
r? ghost

Author: Jarcho

clippy_lints/src/lib.rs

@@ -765,7 +765,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(conf)));
     store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit));
     store.register_late_pass(move |_| Box::new(needless_pass_by_ref_mut::NeedlessPassByRefMut::new(conf)));
-    store.register_late_pass(|_| Box::new(non_canonical_impls::NonCanonicalImpls));
+    store.register_late_pass(|tcx| Box::new(non_canonical_impls::NonCanonicalImpls::new(tcx)));
     store.register_late_pass(move |_| Box::new(single_call_fn::SingleCallFn::new(conf)));
     store.register_early_pass(move || Box::new(raw_strings::RawStrings::new(conf)));
     store.register_late_pass(move |_| Box::new(legacy_numeric_constants::LegacyNumericConstants::new(conf)));

clippy_lints/src/non_canonical_impls.rs

@@ -3,10 +3,11 @@ 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 rustc_errors::Applicability;
-use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, LangItem, Node, UnOp};
+use rustc_hir::def_id::DefId;
+use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, LangItem, UnOp};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
-use rustc_middle::ty::{EarlyBinder, TraitRef, TypeckResults};
-use rustc_session::declare_lint_pass;
+use rustc_middle::ty::{EarlyBinder, TyCtxt, TypeckResults};
+use rustc_session::impl_lint_pass;
 use rustc_span::sym;
 use rustc_span::symbol::kw;
 
@@ -107,33 +108,96 @@ declare_clippy_lint! {
     suspicious,
     "non-canonical implementation of `PartialOrd` on an `Ord` type"
 }
-declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]);
+impl_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]);
+
+#[expect(
+    clippy::struct_field_names,
+    reason = "`_trait` suffix is meaningful on its own, \
+              and creating an inner `StoredTraits` struct would just add a level of indirection"
+)]
+pub(crate) struct NonCanonicalImpls {
+    partial_ord_trait: Option<DefId>,
+    ord_trait: Option<DefId>,
+    clone_trait: Option<DefId>,
+    copy_trait: Option<DefId>,
+}
+
+impl NonCanonicalImpls {
+    pub(crate) fn new(tcx: TyCtxt<'_>) -> Self {
+        let lang_items = tcx.lang_items();
+        Self {
+            partial_ord_trait: lang_items.partial_ord_trait(),
+            ord_trait: tcx.get_diagnostic_item(sym::Ord),
+            clone_trait: lang_items.clone_trait(),
+            copy_trait: lang_items.copy_trait(),
+        }
+    }
+}
+
+/// The traits that this lint looks at
+enum Trait {
+    Clone,
+    PartialOrd,
+}
 
 impl LateLintPass<'_> for NonCanonicalImpls {
-    fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) {
-        if let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind
-            && let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id())
-            && let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
-            && let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id)
-            // NOTE: check this early to avoid expensive checks that come after this one
-            && matches!(trait_name, Some(sym::Clone | sym::PartialOrd))
+    fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+        if let ItemKind::Impl(impl_) = item.kind
+            // Both `PartialOrd` and `Clone` have one required method, and `PartialOrd` can have 5 methods in total
+            && (1..=5).contains(&impl_.items.len())
+            && let Some(of_trait) = impl_.of_trait
+            && let Some(trait_did) = of_trait.trait_ref.trait_def_id()
+            // Check this early to hopefully bail out as soon as possible
+            && let trait_ = if Some(trait_did) == self.clone_trait {
+                Trait::Clone
+            } else if Some(trait_did) == self.partial_ord_trait {
+                Trait::PartialOrd
+            } else {
+                return;
+            }
             && !cx.tcx.is_automatically_derived(item.owner_id.to_def_id())
-            && let body = cx.tcx.hir_body(impl_item_id)
-            && let ExprKind::Block(block, ..) = body.value.kind
-            && !block.span.in_external_macro(cx.sess().source_map())
-            && !is_from_proc_macro(cx, impl_item)
         {
-            if trait_name == Some(sym::Clone)
-                && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy)
-                && implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[])
-            {
-                check_clone_on_copy(cx, impl_item, block);
-            } else if trait_name == Some(sym::PartialOrd)
-                && impl_item.ident.name == sym::partial_cmp
-                && let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord)
-                && implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[])
-            {
-                check_partial_ord_on_ord(cx, impl_item, item, &trait_impl, body, block);
+            let mut assoc_fns = impl_
+                .items
+                .iter()
+                .map(|id| cx.tcx.hir_impl_item(*id))
+                .filter_map(|assoc| {
+                    if let ImplItemKind::Fn(_, body_id) = assoc.kind
+                        && let body = cx.tcx.hir_body(body_id)
+                        && let ExprKind::Block(block, ..) = body.value.kind
+                        && !block.span.in_external_macro(cx.sess().source_map())
+                    {
+                        Some((assoc, body, block))
+                    } else {
+                        None
+                    }
+                });
+
+            match trait_ {
+                Trait::Clone => {
+                    if let Some(copy_trait) = self.copy_trait
+                        && let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
+                        && implements_trait(cx, trait_impl.self_ty(), copy_trait, &[])
+                    {
+                        for (assoc, _, block) in assoc_fns {
+                            check_clone_on_copy(cx, assoc, block);
+                        }
+                    }
+                },
+                Trait::PartialOrd => {
+                    if let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
+                        // If `Self` and `Rhs` are not the same type, then a corresponding `Ord` impl is not possible,
+                        // since it doesn't have an `Rhs`
+                        && let [lhs, rhs] = trait_impl.args.as_slice()
+                        && lhs == rhs
+                        && let Some(ord_trait) = self.ord_trait
+                        && implements_trait(cx, trait_impl.self_ty(), ord_trait, &[])
+                        && let Some((assoc, body, block)) =
+                            assoc_fns.find(|(assoc, _, _)| assoc.ident.name == sym::partial_cmp)
+                    {
+                        check_partial_ord_on_ord(cx, assoc, item, body, block);
+                    }
+                },
             }
         }
     }
@@ -151,6 +215,10 @@ fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &B
             return;
         }
 
+        if is_from_proc_macro(cx, impl_item) {
+            return;
+        }
+
         span_lint_and_sugg(
             cx,
             NON_CANONICAL_CLONE_IMPL,
@@ -162,7 +230,7 @@ fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &B
         );
     }
 
-    if impl_item.ident.name == sym::clone_from {
+    if impl_item.ident.name == sym::clone_from && !is_from_proc_macro(cx, impl_item) {
         span_lint_and_sugg(
             cx,
             NON_CANONICAL_CLONE_IMPL,
@@ -179,7 +247,6 @@ fn check_partial_ord_on_ord<'tcx>(
     cx: &LateContext<'tcx>,
     impl_item: &ImplItem<'_>,
     item: &Item<'_>,
-    trait_impl: &TraitRef<'_>,
     body: &Body<'_>,
     block: &Block<'tcx>,
 ) {
@@ -204,12 +271,7 @@ fn check_partial_ord_on_ord<'tcx>(
         && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
     {
         return;
-    }
-    // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
-    // suggestion tons more complex.
-    else if let [lhs, rhs, ..] = trait_impl.args.as_slice()
-        && lhs != rhs
-    {
+    } else if is_from_proc_macro(cx, impl_item) {
         return;
     }
 

tests/ui/non_canonical_clone_impl.fixed

@@ -4,7 +4,7 @@
 #![no_main]
 
 extern crate proc_macros;
-use proc_macros::with_span;
+use proc_macros::inline_macros;
 
 // lint
 
@@ -100,18 +100,45 @@ impl<A: Copy> Clone for Uwu<A> {
 
 impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}
 
-// should skip proc macros, see https://github.com/rust-lang/rust-clippy/issues/12788
-#[derive(proc_macro_derive::NonCanonicalClone)]
-pub struct G;
+#[inline_macros]
+mod issue12788 {
+    use proc_macros::{external, with_span};
 
-with_span!(
-    span
+    // lint -- not an external macro
+    inline!(
+        #[derive(Copy)]
+        pub struct A;
 
-    #[derive(Copy)]
-    struct H;
-    impl Clone for H {
-        fn clone(&self) -> Self {
-            todo!()
+        impl Clone for A {
+            fn clone(&self) -> Self { *self }
         }
-    }
-);
+    );
+
+    // do not lint -- should skip external macros
+    external!(
+        #[derive(Copy)]
+        pub struct B;
+
+        impl Clone for B {
+            fn clone(&self) -> Self {
+                todo!()
+            }
+        }
+    );
+
+    // do not lint -- should skip proc macros
+    #[derive(proc_macro_derive::NonCanonicalClone)]
+    pub struct C;
+
+    with_span!(
+        span
+
+        #[derive(Copy)]
+        struct D;
+        impl Clone for D {
+            fn clone(&self) -> Self {
+                todo!()
+            }
+        }
+    );
+}

tests/ui/non_canonical_clone_impl.rs

@@ -4,7 +4,7 @@
 #![no_main]
 
 extern crate proc_macros;
-use proc_macros::with_span;
+use proc_macros::inline_macros;
 
 // lint
 
@@ -114,18 +114,48 @@ impl<A: Copy> Clone for Uwu<A> {
 
 impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}
 
-// should skip proc macros, see https://github.com/rust-lang/rust-clippy/issues/12788
-#[derive(proc_macro_derive::NonCanonicalClone)]
-pub struct G;
+#[inline_macros]
+mod issue12788 {
+    use proc_macros::{external, with_span};
 
-with_span!(
-    span
+    // lint -- not an external macro
+    inline!(
+        #[derive(Copy)]
+        pub struct A;
 
-    #[derive(Copy)]
-    struct H;
-    impl Clone for H {
-        fn clone(&self) -> Self {
-            todo!()
+        impl Clone for A {
+            fn clone(&self) -> Self {
+                //~^ non_canonical_clone_impl
+                todo!()
+            }
         }
-    }
-);
+    );
+
+    // do not lint -- should skip external macros
+    external!(
+        #[derive(Copy)]
+        pub struct B;
+
+        impl Clone for B {
+            fn clone(&self) -> Self {
+                todo!()
+            }
+        }
+    );
+
+    // do not lint -- should skip proc macros
+    #[derive(proc_macro_derive::NonCanonicalClone)]
+    pub struct C;
+
+    with_span!(
+        span
+
+        #[derive(Copy)]
+        struct D;
+        impl Clone for D {
+            fn clone(&self) -> Self {
+                todo!()
+            }
+        }
+    );
+}

tests/ui/non_canonical_clone_impl.stderr

@@ -41,5 +41,17 @@ LL | |         *self = source.clone();
 LL | |     }
    | |_____^ help: remove it
 
-error: aborting due to 4 previous errors
+error: non-canonical implementation of `clone` on a `Copy` type
+  --> tests/ui/non_canonical_clone_impl.rs:127:37
+   |
+LL |               fn clone(&self) -> Self {
+   |  _____________________________________^
+LL | |
+LL | |                 todo!()
+LL | |             }
+   | |_____________^ help: change this to: `{ *self }`
+   |
+   = note: this error originates in the macro `__inline_mac_mod_issue12788` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 5 previous errors