refactor(non_canonical_impls): lint starting from `impl`s (#15749) Based on https://github.com/rust-lang/rust-clippy/pull/15520#discussion_r2372599748, 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
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 688b133..41702ae 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs
@@ -765,7 +765,7 @@ 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)));
diff --git a/clippy_lints/src/non_canonical_impls.rs b/clippy_lints/src/non_canonical_impls.rs index 5c1406d..3285023 100644 --- a/clippy_lints/src/non_canonical_impls.rs +++ b/clippy_lints/src/non_canonical_impls.rs
@@ -3,10 +3,11 @@ 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 @@ 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 @@ return; } + if is_from_proc_macro(cx, impl_item) { + return; + } + span_lint_and_sugg( cx, NON_CANONICAL_CLONE_IMPL, @@ -162,7 +230,7 @@ ); } - 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 @@ cx: &LateContext<'tcx>, impl_item: &ImplItem<'_>, item: &Item<'_>, - trait_impl: &TraitRef<'_>, body: &Body<'_>, block: &Block<'tcx>, ) { @@ -204,12 +271,7 @@ && 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; }
diff --git a/tests/ui/non_canonical_clone_impl.fixed b/tests/ui/non_canonical_clone_impl.fixed index 11616f2..d9be98d 100644 --- a/tests/ui/non_canonical_clone_impl.fixed +++ b/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: 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!() + } + } + ); +}
diff --git a/tests/ui/non_canonical_clone_impl.rs b/tests/ui/non_canonical_clone_impl.rs index 7d10191..3db22bd 100644 --- a/tests/ui/non_canonical_clone_impl.rs +++ b/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: 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!() + } + } + ); +}
diff --git a/tests/ui/non_canonical_clone_impl.stderr b/tests/ui/non_canonical_clone_impl.stderr index 5500984..cf36a8f 100644 --- a/tests/ui/non_canonical_clone_impl.stderr +++ b/tests/ui/non_canonical_clone_impl.stderr
@@ -41,5 +41,17 @@ 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
diff --git a/tests/ui/non_canonical_partial_ord_impl.fixed b/tests/ui/non_canonical_partial_ord_impl.fixed index 6915e19..aa23fd9 100644 --- a/tests/ui/non_canonical_partial_ord_impl.fixed +++ b/tests/ui/non_canonical_partial_ord_impl.fixed
@@ -1,5 +1,9 @@ +//@aux-build:proc_macro_derive.rs #![no_main] +extern crate proc_macros; +use proc_macros::inline_macros; + use std::cmp::Ordering; // lint @@ -163,6 +167,73 @@ } } +#[inline_macros] +mod issue12788 { + use std::cmp::Ordering; + + use proc_macros::{external, with_span}; + + // lint -- not an external macro + inline!( + #[derive(PartialEq, Eq)] + pub struct A; + + impl Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } + } + + impl PartialOrd for A { + //~^ non_canonical_partial_ord_impl + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) } + } + ); + + // do not lint -- should skip external macros + external!( + #[derive(PartialEq, Eq)] + pub struct B; + + impl Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } + } + + impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + todo!(); + } + } + + ); + + // do not lint -- should skip proc macros + #[derive(proc_macro_derive::NonCanonicalClone)] + pub struct C; + + with_span!( + span + + #[derive(PartialEq, Eq)] + pub struct D; + + impl Ord for D { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } + } + + impl PartialOrd for D { + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + todo!(); + } + } + + ); +} + // #13640, do not lint #[derive(Eq, PartialEq)]
diff --git a/tests/ui/non_canonical_partial_ord_impl.rs b/tests/ui/non_canonical_partial_ord_impl.rs index 7ce4cdc..da7f73f 100644 --- a/tests/ui/non_canonical_partial_ord_impl.rs +++ b/tests/ui/non_canonical_partial_ord_impl.rs
@@ -1,5 +1,9 @@ +//@aux-build:proc_macro_derive.rs #![no_main] +extern crate proc_macros; +use proc_macros::inline_macros; + use std::cmp::Ordering; // lint @@ -167,6 +171,75 @@ } } +#[inline_macros] +mod issue12788 { + use std::cmp::Ordering; + + use proc_macros::{external, with_span}; + + // lint -- not an external macro + inline!( + #[derive(PartialEq, Eq)] + pub struct A; + + impl Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } + } + + impl PartialOrd for A { + //~^ non_canonical_partial_ord_impl + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + todo!(); + } + } + ); + + // do not lint -- should skip external macros + external!( + #[derive(PartialEq, Eq)] + pub struct B; + + impl Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } + } + + impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + todo!(); + } + } + + ); + + // do not lint -- should skip proc macros + #[derive(proc_macro_derive::NonCanonicalClone)] + pub struct C; + + with_span!( + span + + #[derive(PartialEq, Eq)] + pub struct D; + + impl Ord for D { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } + } + + impl PartialOrd for D { + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + todo!(); + } + } + + ); +} + // #13640, do not lint #[derive(Eq, PartialEq)]
diff --git a/tests/ui/non_canonical_partial_ord_impl.stderr b/tests/ui/non_canonical_partial_ord_impl.stderr index 9bd6b1f..8e55603 100644 --- a/tests/ui/non_canonical_partial_ord_impl.stderr +++ b/tests/ui/non_canonical_partial_ord_impl.stderr
@@ -1,5 +1,5 @@ error: non-canonical implementation of `partial_cmp` on an `Ord` type - --> tests/ui/non_canonical_partial_ord_impl.rs:16:1 + --> tests/ui/non_canonical_partial_ord_impl.rs:20:1 | LL | / impl PartialOrd for A { LL | | @@ -15,7 +15,7 @@ = help: to override `-D warnings` add `#[allow(clippy::non_canonical_partial_ord_impl)]` error: non-canonical implementation of `partial_cmp` on an `Ord` type - --> tests/ui/non_canonical_partial_ord_impl.rs:51:1 + --> tests/ui/non_canonical_partial_ord_impl.rs:55:1 | LL | / impl PartialOrd for C { LL | | @@ -32,7 +32,22 @@ | error: non-canonical implementation of `partial_cmp` on an `Ord` type - --> tests/ui/non_canonical_partial_ord_impl.rs:198:1 + --> tests/ui/non_canonical_partial_ord_impl.rs:191:9 + | +LL | / impl PartialOrd for A { +LL | | +LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + | | _____________________________________________________________________- +LL | || todo!(); +LL | || } + | ||_____________- help: change this to: `{ Some(self.cmp(other)) }` +LL | | } + | |__________^ + | + = note: this error originates in the macro `__inline_mac_mod_issue12788` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: non-canonical implementation of `partial_cmp` on an `Ord` type + --> tests/ui/non_canonical_partial_ord_impl.rs:271:1 | LL | / impl PartialOrd for K { LL | | @@ -45,7 +60,7 @@ | |__^ error: non-canonical implementation of `partial_cmp` on an `Ord` type - --> tests/ui/non_canonical_partial_ord_impl.rs:216:1 + --> tests/ui/non_canonical_partial_ord_impl.rs:289:1 | LL | / impl PartialOrd for L { LL | | @@ -57,5 +72,5 @@ LL | | } | |__^ -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors