Improve multiple_inherent_impl lint

Treat different generic arguments as different types.
Allow the lint to be ignored on the type definition, or any impl blocks.
This commit is contained in:
Jason Newcomb 2021-04-15 12:20:43 -04:00
parent b1c675f3fc
commit 760f70312e
No known key found for this signature in database
GPG Key ID: DA59E8643A37ED06
4 changed files with 155 additions and 47 deletions

View File

@ -1,12 +1,16 @@
//! lint on inherent implementations
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::in_macro;
use rustc_hir::def_id::DefIdMap;
use rustc_hir::{def_id, Crate, Impl, Item, ItemKind};
use clippy_utils::diagnostics::span_lint_and_note;
use clippy_utils::{in_macro, is_allowed};
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::{
def_id::{LocalDefId, LOCAL_CRATE},
Crate, Item, ItemKind, Node,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;
use std::collections::hash_map::Entry;
declare_clippy_lint! {
/// **What it does:** Checks for multiple inherent implementations of a struct
@ -40,51 +44,96 @@
"Multiple inherent impl that could be grouped"
}
#[allow(clippy::module_name_repetitions)]
#[derive(Default)]
pub struct MultipleInherentImpl {
impls: DefIdMap<Span>,
}
impl_lint_pass!(MultipleInherentImpl => [MULTIPLE_INHERENT_IMPL]);
declare_lint_pass!(MultipleInherentImpl => [MULTIPLE_INHERENT_IMPL]);
impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl {
fn check_item(&mut self, _: &LateContext<'tcx>, item: &'tcx Item<'_>) {
if let ItemKind::Impl(Impl {
ref generics,
of_trait: None,
..
}) = item.kind
{
// Remember for each inherent implementation encountered its span and generics
// but filter out implementations that have generic params (type or lifetime)
// or are derived from a macro
if !in_macro(item.span) && generics.params.is_empty() {
self.impls.insert(item.def_id.to_def_id(), item.span);
}
}
}
fn check_crate_post(&mut self, cx: &LateContext<'tcx>, _: &'tcx Crate<'_>) {
// Map from a type to it's first impl block. Needed to distinguish generic arguments.
// e.g. `Foo<Bar>` and `Foo<Baz>`
let mut type_map = FxHashMap::default();
// List of spans to lint. (lint_span, first_span)
let mut lint_spans = Vec::new();
fn check_crate_post(&mut self, cx: &LateContext<'tcx>, krate: &'tcx Crate<'_>) {
if !krate.items.is_empty() {
// Retrieve all inherent implementations from the crate, grouped by type
for impls in cx.tcx.crate_inherent_impls(def_id::LOCAL_CRATE).inherent_impls.values() {
// Filter out implementations that have generic params (type or lifetime)
let mut impl_spans = impls.iter().filter_map(|impl_def| self.impls.get(impl_def));
if let Some(initial_span) = impl_spans.next() {
impl_spans.for_each(|additional_span| {
span_lint_and_then(
cx,
MULTIPLE_INHERENT_IMPL,
*additional_span,
"multiple implementations of this structure",
|diag| {
diag.span_note(*initial_span, "first implementation here");
},
)
})
for (_, impl_ids) in cx
.tcx
.crate_inherent_impls(LOCAL_CRATE)
.inherent_impls
.iter()
.filter(|(id, impls)| {
impls.len() > 1
// Check for `#[allow]` on the type definition
&& !is_allowed(
cx,
MULTIPLE_INHERENT_IMPL,
cx.tcx.hir().local_def_id_to_hir_id(id.expect_local()),
)
})
{
for impl_id in impl_ids.iter().map(|id| id.expect_local()) {
match type_map.entry(cx.tcx.type_of(impl_id)) {
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));
},
Entry::Occupied(mut e) => {
if let Some(span) = get_impl_span(cx, impl_id) {
let first_span = match *e.get() {
IdOrSpan::Span(s) => s,
IdOrSpan::Id(id) => {
if let Some(s) = get_impl_span(cx, id) {
// Remember the span of the first block.
*e.get_mut() = IdOrSpan::Span(s);
s
} else {
// The first impl block isn't considered by the lint. Replace it with the
// current one.
*e.get_mut() = IdOrSpan::Span(span);
continue;
}
},
};
lint_spans.push((span, first_span));
}
},
}
}
// Switching to the next type definition, no need to keep the current entries around.
type_map.clear();
}
// `TyCtxt::crate_inherent_impls` doesn't have a defined order. Sort the lint output first.
lint_spans.sort_by_key(|x| x.0.lo());
for (span, first_span) in lint_spans {
span_lint_and_note(
cx,
MULTIPLE_INHERENT_IMPL,
span,
"multiple implementations of this structure",
Some(first_span),
"first implementation here",
);
}
}
}
/// Gets the span for the given impl block unless it's not being considered by the lint.
fn get_impl_span(cx: &LateContext<'_>, id: LocalDefId) -> Option<Span> {
let id = cx.tcx.hir().local_def_id_to_hir_id(id);
if let Node::Item(&Item {
kind: ItemKind::Impl(ref impl_item),
span,
..
}) = cx.tcx.hir().get(id)
{
(!in_macro(span) && impl_item.generics.params.is_empty() && !is_allowed(cx, MULTIPLE_INHERENT_IMPL, id))
.then(|| span)
} else {
None
}
}
enum IdOrSpan {
Id(LocalDefId),
Span(Span),
}

View File

@ -1154,7 +1154,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_early_pass(|| box suspicious_operation_groupings::SuspiciousOperationGroupings);
store.register_late_pass(|| box suspicious_trait_impl::SuspiciousImpl);
store.register_late_pass(|| box map_unit_fn::MapUnit);
store.register_late_pass(|| box inherent_impl::MultipleInherentImpl::default());
store.register_late_pass(|| box inherent_impl::MultipleInherentImpl);
store.register_late_pass(|| box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
store.register_late_pass(|| box unwrap::Unwrap);
store.register_late_pass(|| box duration_subsec::DurationSubsec);

View File

@ -33,4 +33,35 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
}
}
// issue #5772
struct WithArgs<T>(T);
impl WithArgs<u32> {
fn f1() {}
}
impl WithArgs<u64> {
fn f2() {}
}
impl WithArgs<u64> {
fn f3() {}
}
// Ok, the struct is allowed to have multiple impls.
#[allow(clippy::multiple_inherent_impl)]
struct Allowed;
impl Allowed {}
impl Allowed {}
impl Allowed {}
struct AllowedImpl;
#[allow(clippy::multiple_inherent_impl)]
impl AllowedImpl {}
// Ok, the first block is skipped by this lint.
impl AllowedImpl {}
struct OneAllowedImpl;
impl OneAllowedImpl {}
#[allow(clippy::multiple_inherent_impl)]
impl OneAllowedImpl {}
impl OneAllowedImpl {} // Lint, only one of the three blocks is allowed.
fn main() {}

View File

@ -31,5 +31,33 @@ LL | | fn first() {}
LL | | }
| |_^
error: aborting due to 2 previous errors
error: multiple implementations of this structure
--> $DIR/impl.rs:44:1
|
LL | / impl WithArgs<u64> {
LL | | fn f3() {}
LL | | }
| |_^
|
note: first implementation here
--> $DIR/impl.rs:41:1
|
LL | / impl WithArgs<u64> {
LL | | fn f2() {}
LL | | }
| |_^
error: multiple implementations of this structure
--> $DIR/impl.rs:65:1
|
LL | impl OneAllowedImpl {} // Lint, only one of the three blocks is allowed.
| ^^^^^^^^^^^^^^^^^^^^^^
|
note: first implementation here
--> $DIR/impl.rs:62:1
|
LL | impl OneAllowedImpl {}
| ^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 4 previous errors