From ec9174248dbcd6bbce4512b6455c99aa714e3c1c Mon Sep 17 00:00:00 2001 From: Samuel Onoja Date: Mon, 22 Dec 2025 03:20:08 +0100 Subject: [PATCH] Fix `multiple_inherent_impl` false negatives for generic impl blocks --- clippy_lints/src/inherent_impl.rs | 23 +++++--- tests/ui/impl.rs | 42 +++++++++++++-- tests/ui/impl.stderr | 90 ++++++++++++++++++++++++++++--- 3 files changed, 138 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/inherent_impl.rs b/clippy_lints/src/inherent_impl.rs index f59c7615d745..14928a1be13b 100644 --- a/clippy_lints/src/inherent_impl.rs +++ b/clippy_lints/src/inherent_impl.rs @@ -101,7 +101,21 @@ impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl { InherentImplLintScope::Crate => Criterion::Crate, }; let is_test = is_cfg_test(cx.tcx, hir_id) || is_in_cfg_test(cx.tcx, hir_id); - match type_map.entry((impl_ty, criterion, is_test)) { + let predicates = { + // Gets the predicates (bounds) for the given impl block, + // sorted for consistent comparison to allow distinguishing between impl blocks + // with different generic bounds. + let mut predicates = cx + .tcx + .predicates_of(impl_id) + .predicates + .iter() + .map(|(clause, _)| *clause) + .collect::>(); + predicates.sort_by_key(|c| format!("{c:?}")); + predicates + }; + match type_map.entry((impl_ty, predicates, criterion, is_test)) { Entry::Vacant(e) => { // Store the id for the first impl block of this type. The span is retrieved lazily. e.insert(IdOrSpan::Id(impl_id)); @@ -152,15 +166,12 @@ impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl { fn get_impl_span(cx: &LateContext<'_>, id: LocalDefId) -> Option { let id = cx.tcx.local_def_id_to_hir_id(id); if let Node::Item(&Item { - kind: ItemKind::Impl(impl_item), + kind: ItemKind::Impl(_), span, .. }) = cx.tcx.hir_node(id) { - (!span.from_expansion() - && impl_item.generics.params.is_empty() - && !fulfill_or_allowed(cx, MULTIPLE_INHERENT_IMPL, [id])) - .then_some(span) + (!span.from_expansion() && !fulfill_or_allowed(cx, MULTIPLE_INHERENT_IMPL, [id])).then_some(span) } else { None } diff --git a/tests/ui/impl.rs b/tests/ui/impl.rs index e6044cc50781..75761a34c86e 100644 --- a/tests/ui/impl.rs +++ b/tests/ui/impl.rs @@ -14,6 +14,7 @@ impl MyStruct { } impl<'a> MyStruct { + //~^ multiple_inherent_impl fn lifetimed() {} } @@ -90,10 +91,12 @@ struct Lifetime<'s> { } impl Lifetime<'_> {} -impl Lifetime<'_> {} // false negative +impl Lifetime<'_> {} +//~^ multiple_inherent_impl impl<'a> Lifetime<'a> {} -impl<'a> Lifetime<'a> {} // false negative +impl<'a> Lifetime<'a> {} +//~^ multiple_inherent_impl impl<'b> Lifetime<'b> {} // false negative? @@ -104,6 +107,39 @@ struct Generic { } impl Generic {} -impl Generic {} // false negative +impl Generic {} +//~^ multiple_inherent_impl + +use std::fmt::Debug; + +#[derive(Debug)] +struct GenericWithBounds(T); + +impl GenericWithBounds { + fn make_one(_one: T) -> Self { + todo!() + } +} + +impl GenericWithBounds { + //~^ multiple_inherent_impl + fn make_two(_two: T) -> Self { + todo!() + } +} + +struct MultipleTraitBounds(T); + +impl MultipleTraitBounds { + fn debug_fn() {} +} + +impl MultipleTraitBounds { + fn clone_fn() {} +} + +impl MultipleTraitBounds { + fn debug_clone_fn() {} +} fn main() {} diff --git a/tests/ui/impl.stderr b/tests/ui/impl.stderr index 93d4b3998f90..9c4aaf183d70 100644 --- a/tests/ui/impl.stderr +++ b/tests/ui/impl.stderr @@ -19,7 +19,24 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::multiple_inherent_impl)]` error: multiple implementations of this structure - --> tests/ui/impl.rs:26:5 + --> tests/ui/impl.rs:16:1 + | +LL | / impl<'a> MyStruct { +LL | | +LL | | fn lifetimed() {} +LL | | } + | |_^ + | +note: first implementation here + --> tests/ui/impl.rs:6:1 + | +LL | / impl MyStruct { +LL | | fn first() {} +LL | | } + | |_^ + +error: multiple implementations of this structure + --> tests/ui/impl.rs:27:5 | LL | / impl super::MyStruct { LL | | @@ -37,7 +54,7 @@ LL | | } | |_^ error: multiple implementations of this structure - --> tests/ui/impl.rs:48:1 + --> tests/ui/impl.rs:49:1 | LL | / impl WithArgs { LL | | @@ -47,7 +64,7 @@ LL | | } | |_^ | note: first implementation here - --> tests/ui/impl.rs:45:1 + --> tests/ui/impl.rs:46:1 | LL | / impl WithArgs { LL | | fn f2() {} @@ -55,28 +72,85 @@ LL | | } | |_^ error: multiple implementations of this structure - --> tests/ui/impl.rs:71:1 + --> tests/ui/impl.rs:72:1 | LL | impl OneAllowedImpl {} | ^^^^^^^^^^^^^^^^^^^^^^ | note: first implementation here - --> tests/ui/impl.rs:68:1 + --> tests/ui/impl.rs:69:1 | LL | impl OneAllowedImpl {} | ^^^^^^^^^^^^^^^^^^^^^^ error: multiple implementations of this structure - --> tests/ui/impl.rs:84:1 + --> tests/ui/impl.rs:85:1 | LL | impl OneExpected {} | ^^^^^^^^^^^^^^^^^^^ | note: first implementation here - --> tests/ui/impl.rs:81:1 + --> tests/ui/impl.rs:82:1 | LL | impl OneExpected {} | ^^^^^^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: multiple implementations of this structure + --> tests/ui/impl.rs:94:1 + | +LL | impl Lifetime<'_> {} + | ^^^^^^^^^^^^^^^^^^^^ + | +note: first implementation here + --> tests/ui/impl.rs:93:1 + | +LL | impl Lifetime<'_> {} + | ^^^^^^^^^^^^^^^^^^^^ + +error: multiple implementations of this structure + --> tests/ui/impl.rs:98:1 + | +LL | impl<'a> Lifetime<'a> {} + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: first implementation here + --> tests/ui/impl.rs:97:1 + | +LL | impl<'a> Lifetime<'a> {} + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: multiple implementations of this structure + --> tests/ui/impl.rs:110:1 + | +LL | impl Generic {} + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: first implementation here + --> tests/ui/impl.rs:109:1 + | +LL | impl Generic {} + | ^^^^^^^^^^^^^^^^^^^^^ + +error: multiple implementations of this structure + --> tests/ui/impl.rs:124:1 + | +LL | / impl GenericWithBounds { +LL | | +LL | | fn make_two(_two: T) -> Self { +LL | | todo!() +LL | | } +LL | | } + | |_^ + | +note: first implementation here + --> tests/ui/impl.rs:118:1 + | +LL | / impl GenericWithBounds { +LL | | fn make_one(_one: T) -> Self { +LL | | todo!() +LL | | } +LL | | } + | |_^ + +error: aborting due to 10 previous errors