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`

4 files changed, 154 insertions, 46 deletions

diff --git a/clippy_lints/src/inherent_impl.rs b/clippy_lints/src/inherent_impl.rs
index c31013e49be..980bcc78f5d 100644
--- a/clippy_lints/src/inherent_impl.rs
+++ b/clippy_lints/src/inherent_impl.rs
@@ -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 @@ declare_clippy_lint! {
     "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
+    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();
+
+        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()),
+                    )
+            })
         {
-            // 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);
+            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();
         }
-    }
 
-    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");
-                            },
-                        )
-                    })
-                }
-            }
+        // `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),
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 18882755aeb..2c83409b402 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -1161,7 +1161,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);
diff --git a/tests/ui/impl.rs b/tests/ui/impl.rs
index 1c46e3a5337..39443775015 100644
--- a/tests/ui/impl.rs
+++ b/tests/ui/impl.rs
@@ -33,4 +33,35 @@ impl fmt::Debug for MyStruct {
     }
 }
 
+// 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() {}
diff --git a/tests/ui/impl.stderr b/tests/ui/impl.stderr
index aab688cc2d8..8703ecac93e 100644
--- a/tests/ui/impl.stderr
+++ b/tests/ui/impl.stderr
@@ -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