refactor(non_canonical_impls): lint starting from impls

Author: ada4a

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, 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,36 +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)
-                // 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
-                && 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, 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);
+                    }
+                },
             }
         }
     }
@@ -154,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,
@@ -165,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,
@@ -206,6 +271,8 @@ fn check_partial_ord_on_ord<'tcx>(
         && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
     {
         return;
+    } else if is_from_proc_macro(cx, impl_item) {
+        return;
     }
 
     span_lint_and_then(