multiple_inherent_impl code cleanup

Author: Jarcho

clippy_lints/src/inherent_impl.rs

@@ -48,7 +48,10 @@ declare_lint_pass!(MultipleInherentImpl => [MULTIPLE_INHERENT_IMPL]);
 
 impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl {
     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
@@ -58,6 +61,7 @@ impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl {
             .iter()
             .filter(|(id, impls)| {
                 impls.len() > 1
+                    // Check for `#[allow]` on the type definition
                     && !is_allowed(
                         cx,
                         MULTIPLE_INHERENT_IMPL,
@@ -68,23 +72,38 @@ impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl {
             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) {
-                            if let Some(first_span) = e.get_mut().as_span(cx) {
-                                lint_spans.push((span, first_span));
-                            } else {
-                                *e.get_mut() = IdOrSpan::Span(span);
-                            }
+                            let first_span = match *e.get() {
+                                IdOrSpan::Span(s) => s,
+                                IdOrSpan::Id(id) => match get_impl_span(cx, id) {
+                                    Some(s) => {
+                                        // Remember the span of the first block.
+                                        *e.get_mut() = IdOrSpan::Span(s);
+                                        s
+                                    },
+                                    None => {
+                                        // 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(
@@ -99,6 +118,7 @@ impl<'tcx> LateLintPass<'tcx> for MultipleInherentImpl {
     }
 }
 
+/// 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 {
@@ -118,14 +138,3 @@ enum IdOrSpan {
     Id(LocalDefId),
     Span(Span),
 }
-impl IdOrSpan {
-    fn as_span(&mut self, cx: &LateContext<'_>) -> Option<Span> {
-        match *self {
-            Self::Id(id) => get_impl_span(cx, id).map(|span| {
-                *self = Self::Span(span);
-                span
-            }),
-            Self::Span(span) => Some(span),
-        }
-    }
-}