Skip to content

Commit aa1959b

Browse files
borsbors
committed
Auto merge of #7089 - Jarcho:multiple_impls_generic, r=Jarcho
Don't lint `multiple_inherent_impl` with generic arguments fixes: #5772 changelog: Treat different generic arguments as different types in `multiple_inherent_impl`
2 parents 36be8ba + 760f703 commit aa1959b

File tree

4 files changed

+154
-46
lines changed

4 files changed

+154
-46
lines changed

‎clippy_lints/src/inherent_impl.rs

Lines changed: 93 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
11
//! lint on inherent implementations
22
3-
use clippy_utils::diagnostics::span_lint_and_then;
4-
use clippy_utils::in_macro;
5-
use rustc_hir::def_id::DefIdMap;
6-
use rustc_hir::{def_id, Crate, Impl, Item, ItemKind};
3+
use clippy_utils::diagnostics::span_lint_and_note;
4+
use clippy_utils::{in_macro, is_allowed};
5+
use rustc_data_structures::fx::FxHashMap;
6+
use rustc_hir::{
7+
def_id::{LocalDefId, LOCAL_CRATE},
8+
Crate, Item, ItemKind, Node,
9+
};
710
use rustc_lint::{LateContext, LateLintPass};
8-
use rustc_session::{declare_tool_lint, impl_lint_pass};
11+
use rustc_session::{declare_lint_pass, declare_tool_lint};
912
use rustc_span::Span;
13+
use std::collections::hash_map::Entry;
1014

1115
declare_clippy_lint! {
1216
/// **What it does:** Checks for multiple inherent implementations of a struct
@@ -40,51 +44,96 @@ declare_clippy_lint! {
4044
"Multiple inherent impl that could be grouped"
4145
}
4246

43-
#[allow(clippy::module_name_repetitions)]
44-
#[derive(Default)]
45-
pub struct MultipleInherentImpl {
46-
impls: DefIdMap<Span>,
47-
}
48-
49-
impl_lint_pass!(MultipleInherentImpl => [MULTIPLE_INHERENT_IMPL]);
47+
declare_lint_pass!(MultipleInherentImpl => [MULTIPLE_INHERENT_IMPL]);
5048

5149
impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl {
52-
fn check_item(&mut self, _: &LateContext<'tcx>, item: &'tcx Item<'_>) {
53-
if let ItemKind::Impl(Impl {
54-
ref generics,
55-
of_trait: None,
56-
..
57-
}) = item.kind
50+
fn check_crate_post(&mut self, cx: &LateContext<'tcx>, _: &'tcx Crate<'_>) {
51+
// Map from a type to it's first impl block. Needed to distinguish generic arguments.
52+
// e.g. `Foo<Bar>` and `Foo<Baz>`
53+
let mut type_map = FxHashMap::default();
54+
// List of spans to lint. (lint_span, first_span)
55+
let mut lint_spans = Vec::new();
56+
57+
for (_, impl_ids) in cx
58+
.tcx
59+
.crate_inherent_impls(LOCAL_CRATE)
60+
.inherent_impls
61+
.iter()
62+
.filter(|(id, impls)| {
63+
impls.len() > 1
64+
// Check for `#[allow]` on the type definition
65+
&& !is_allowed(
66+
cx,
67+
MULTIPLE_INHERENT_IMPL,
68+
cx.tcx.hir().local_def_id_to_hir_id(id.expect_local()),
69+
)
70+
})
5871
{
59-
// Remember for each inherent implementation encountered its span and generics
60-
// but filter out implementations that have generic params (type or lifetime)
61-
// or are derived from a macro
62-
if !in_macro(item.span) && generics.params.is_empty() {
63-
self.impls.insert(item.def_id.to_def_id(), item.span);
72+
for impl_id in impl_ids.iter().map(|id| id.expect_local()) {
73+
match type_map.entry(cx.tcx.type_of(impl_id)) {
74+
Entry::Vacant(e) => {
75+
// Store the id for the first impl block of this type. The span is retrieved lazily.
76+
e.insert(IdOrSpan::Id(impl_id));
77+
},
78+
Entry::Occupied(mut e) => {
79+
if let Some(span) = get_impl_span(cx, impl_id) {
80+
let first_span = match *e.get() {
81+
IdOrSpan::Span(s) => s,
82+
IdOrSpan::Id(id) => {
83+
if let Some(s) = get_impl_span(cx, id) {
84+
// Remember the span of the first block.
85+
*e.get_mut() = IdOrSpan::Span(s);
86+
s
87+
} else {
88+
// The first impl block isn't considered by the lint. Replace it with the
89+
// current one.
90+
*e.get_mut() = IdOrSpan::Span(span);
91+
continue;
92+
}
93+
},
94+
};
95+
lint_spans.push((span, first_span));
96+
}
97+
},
98+
}
6499
}
100+
101+
// Switching to the next type definition, no need to keep the current entries around.
102+
type_map.clear();
65103
}
66-
}
67104

68-
fn check_crate_post(&mut self, cx: &LateContext<'tcx>, krate: &'tcx Crate<'_>) {
69-
if !krate.items.is_empty() {
70-
// Retrieve all inherent implementations from the crate, grouped by type
71-
for impls in cx.tcx.crate_inherent_impls(def_id::LOCAL_CRATE).inherent_impls.values() {
72-
// Filter out implementations that have generic params (type or lifetime)
73-
let mut impl_spans = impls.iter().filter_map(|impl_def| self.impls.get(impl_def));
74-
if let Some(initial_span) = impl_spans.next() {
75-
impl_spans.for_each(|additional_span| {
76-
span_lint_and_then(
77-
cx,
78-
MULTIPLE_INHERENT_IMPL,
79-
*additional_span,
80-
"multiple implementations of this structure",
81-
|diag| {
82-
diag.span_note(*initial_span, "first implementation here");
83-
},
84-
)
85-
})
86-
}
87-
}
105+
// `TyCtxt::crate_inherent_impls` doesn't have a defined order. Sort the lint output first.
106+
lint_spans.sort_by_key(|x| x.0.lo());
107+
for (span, first_span) in lint_spans {
108+
span_lint_and_note(
109+
cx,
110+
MULTIPLE_INHERENT_IMPL,
111+
span,
112+
"multiple implementations of this structure",
113+
Some(first_span),
114+
"first implementation here",
115+
);
88116
}
89117
}
90118
}
119+
120+
/// Gets the span for the given impl block unless it's not being considered by the lint.
121+
fn get_impl_span(cx: &LateContext<'_>, id: LocalDefId) -> Option<Span> {
122+
let id = cx.tcx.hir().local_def_id_to_hir_id(id);
123+
if let Node::Item(&Item {
124+
kind: ItemKind::Impl(ref impl_item),
125+
span,
126+
..
127+
}) = cx.tcx.hir().get(id)
128+
{
129+
(!in_macro(span) && impl_item.generics.params.is_empty() && !is_allowed(cx, MULTIPLE_INHERENT_IMPL, id))
130+
.then(|| span)
131+
} else {
132+
None
133+
}
134+
}
135+
136+
enum IdOrSpan {
137+
Id(LocalDefId),
138+
Span(Span),
139+
}

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1161,7 +1161,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
11611161
store.register_early_pass(|| box suspicious_operation_groupings::SuspiciousOperationGroupings);
11621162
store.register_late_pass(|| box suspicious_trait_impl::SuspiciousImpl);
11631163
store.register_late_pass(|| box map_unit_fn::MapUnit);
1164-
store.register_late_pass(|| box inherent_impl::MultipleInherentImpl::default());
1164+
store.register_late_pass(|| box inherent_impl::MultipleInherentImpl);
11651165
store.register_late_pass(|| box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
11661166
store.register_late_pass(|| box unwrap::Unwrap);
11671167
store.register_late_pass(|| box duration_subsec::DurationSubsec);

‎tests/ui/impl.rs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,4 +33,35 @@ impl fmt::Debug for MyStruct {
3333
}
3434
}
3535

36+
// issue #5772
37+
struct WithArgs<T>(T);
38+
impl WithArgs<u32> {
39+
fn f1() {}
40+
}
41+
impl WithArgs<u64> {
42+
fn f2() {}
43+
}
44+
impl WithArgs<u64> {
45+
fn f3() {}
46+
}
47+
48+
// Ok, the struct is allowed to have multiple impls.
49+
#[allow(clippy::multiple_inherent_impl)]
50+
struct Allowed;
51+
impl Allowed {}
52+
impl Allowed {}
53+
impl Allowed {}
54+
55+
struct AllowedImpl;
56+
#[allow(clippy::multiple_inherent_impl)]
57+
impl AllowedImpl {}
58+
// Ok, the first block is skipped by this lint.
59+
impl AllowedImpl {}
60+
61+
struct OneAllowedImpl;
62+
impl OneAllowedImpl {}
63+
#[allow(clippy::multiple_inherent_impl)]
64+
impl OneAllowedImpl {}
65+
impl OneAllowedImpl {} // Lint, only one of the three blocks is allowed.
66+
3667
fn main() {}

‎tests/ui/impl.stderr

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,5 +31,33 @@ LL | | fn first() {}
3131
LL | | }
3232
| |_^
3333

34-
error: aborting due to 2 previous errors
34+
error: multiple implementations of this structure
35+
--> $DIR/impl.rs:44:1
36+
|
37+
LL | / impl WithArgs<u64> {
38+
LL | | fn f3() {}
39+
LL | | }
40+
| |_^
41+
|
42+
note: first implementation here
43+
--> $DIR/impl.rs:41:1
44+
|
45+
LL | / impl WithArgs<u64> {
46+
LL | | fn f2() {}
47+
LL | | }
48+
| |_^
49+
50+
error: multiple implementations of this structure
51+
--> $DIR/impl.rs:65:1
52+
|
53+
LL | impl OneAllowedImpl {} // Lint, only one of the three blocks is allowed.
54+
| ^^^^^^^^^^^^^^^^^^^^^^
55+
|
56+
note: first implementation here
57+
--> $DIR/impl.rs:62:1
58+
|
59+
LL | impl OneAllowedImpl {}
60+
| ^^^^^^^^^^^^^^^^^^^^^^
61+
62+
error: aborting due to 4 previous errors
3563

0 commit comments

Comments
 (0)