Improve help for extra_unused_type_parameters

mkrasnitski · mkrasnitski · commit d5785f055faa · 2022-12-12T17:55:01.000-05:00
diff --git a/clippy_lints/src/extra_unused_type_parameters.rs b/clippy_lints/src/extra_unused_type_parameters.rs
@@ -1,12 +1,15 @@
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::span_lint_and_help;
 use clippy_utils::trait_ref_of_method;
 use rustc_data_structures::fx::FxHashMap;
+use rustc_errors::MultiSpan;
 use rustc_hir::intravisit::{walk_impl_item, walk_item, walk_param_bound, walk_ty, Visitor};
-use rustc_hir::{GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind, Ty, TyKind, WherePredicate};
+use rustc_hir::{
+    GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind, LifetimeParamKind, Ty, TyKind, WherePredicate,
+};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::nested_filter;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{def_id::DefId, Span};
+use rustc_span::{def_id::DefId, BytePos, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -39,32 +42,77 @@ declare_lint_pass!(ExtraUnusedTypeParameters => [EXTRA_UNUSED_TYPE_PARAMETERS]);
 struct TypeWalker<'cx, 'tcx> {
     cx: &'cx LateContext<'tcx>,
     map: FxHashMap<DefId, Span>,
+    bound_map: FxHashMap<DefId, Span>,
+    generics: &'tcx Generics<'tcx>,
+    some_params_used: bool,
+    has_non_ty_params: bool,
 }
 
 impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
-    fn new(cx: &'cx LateContext<'tcx>, generics: &Generics<'tcx>) -> Self {
+    fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>) -> Self {
+        let mut has_non_ty_params = false;
+        let map = generics
+            .params
+            .iter()
+            .filter_map(|param| match &param.kind {
+                GenericParamKind::Type { .. } => Some((param.def_id.into(), param.span)),
+                GenericParamKind::Lifetime {
+                    kind: LifetimeParamKind::Elided,
+                } => None,
+                _ => {
+                    has_non_ty_params = true;
+                    None
+                },
+            })
+            .collect();
         Self {
             cx,
-            map: generics
-                .params
-                .iter()
-                .filter_map(|param| match param.kind {
-                    GenericParamKind::Type { .. } => Some((param.def_id.into(), param.span)),
-                    _ => None,
-                })
-                .collect(),
+            map,
+            generics,
+            has_non_ty_params,
+            bound_map: FxHashMap::default(),
+            some_params_used: false,
         }
     }
 
     fn emit_lint(&self) {
-        for span in self.map.values() {
-            span_lint(
-                self.cx,
-                EXTRA_UNUSED_TYPE_PARAMETERS,
-                *span,
+        let (msg, help) = match self.map.len() {
+            0 => return,
+            1 => (
                 "type parameter goes unused in function definition",
-            );
-        }
+                "consider removing the parameter",
+            ),
+            _ => (
+                "type parameters go unused in function definition",
+                "consider removing the parameters",
+            ),
+        };
+
+        let source_map = self.cx.tcx.sess.source_map();
+        let span = if self.some_params_used || self.has_non_ty_params {
+            MultiSpan::from_spans(
+                self.map
+                    .iter()
+                    .map(|(def_id, &(mut span))| {
+                        if let Some(bound_span) = self.bound_map.get(def_id) {
+                            span = span.with_hi(bound_span.hi());
+                        }
+                        span = source_map.span_extend_to_next_char(span, ',', false);
+                        span = span.with_hi(BytePos(span.hi().0 + 1));
+
+                        let max_hi = self.generics.span.hi();
+                        if span.hi() >= max_hi {
+                            span = span.with_hi(BytePos(max_hi.0 - 1));
+                        }
+                        span
+                    })
+                    .collect(),
+            )
+        } else {
+            self.generics.span.into()
+        };
+
+        span_lint_and_help(self.cx, EXTRA_UNUSED_TYPE_PARAMETERS, span, msg, None, help);
     }
 }
 
@@ -73,7 +121,9 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
 
     fn visit_ty(&mut self, t: &'tcx Ty<'tcx>) {
         if let Some((def_id, _)) = t.peel_refs().as_generic_param() {
-            self.map.remove(&def_id);
+            if self.map.remove(&def_id).is_some() {
+                self.some_params_used = true;
+            }
         } else if let TyKind::OpaqueDef(id, _, _) = t.kind {
             // Explicitly walk OpaqueDef. Normally `walk_ty` would do the job, but it calls
             // `visit_nested_item`, which checks that `Self::NestedFilter::INTER` is set. We're
@@ -86,9 +136,16 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
     }
 
     fn visit_where_predicate(&mut self, predicate: &'tcx WherePredicate<'tcx>) {
-        if let WherePredicate::BoundPredicate(where_bound_predicate) = predicate {
-            // Only check the right-hand side of where-bounds
-            for bound in where_bound_predicate.bounds {
+        if let WherePredicate::BoundPredicate(predicate) = predicate {
+            // Collect spans for bounds that appear in the list of generics (not in a where-clause)
+            // for use in forming the help message
+            if let Some((def_id, _)) = predicate.bounded_ty.peel_refs().as_generic_param()
+                && predicate.span < self.generics.where_clause_span
+            {
+                self.bound_map.insert(def_id, predicate.span);
+            }
+            // Only walk the right-hand side of where-bounds
+            for bound in predicate.bounds {
                 walk_param_bound(self, bound);
             }
         }
diff --git a/tests/ui/extra_unused_type_parameters.rs b/tests/ui/extra_unused_type_parameters.rs
@@ -5,6 +5,8 @@ fn unused_ty<T>(x: u8) {}
 
 fn unused_multi<T, U>(x: u8) {}
 
+fn unused_with_lt<'a, T>(x: &'a u8) {}
+
 fn used_ty<T>(x: T, y: u8) {}
 
 fn used_ref<'a, T>(x: &'a T) {}
@@ -13,7 +15,15 @@ fn used_ret<T: Default>(x: u8) -> T {
     T::default()
 }
 
-fn unused_with_bound<T: Default>(x: u8) {}
+fn unused_bounded<T: Default, U>(x: U) {}
+
+fn unused_where_clause<T, U>(x: U)
+where
+    T: Default,
+{
+}
+
+fn some_unused<A, B, C, D: Iterator<Item = (B, C)>, E>(b: B, c: C) {}
 
 fn used_opaque<A>(iter: impl Iterator<Item = A>) -> usize {
     iter.count()
diff --git a/tests/ui/extra_unused_type_parameters.stderr b/tests/ui/extra_unused_type_parameters.stderr
@@ -1,34 +1,59 @@
 error: type parameter goes unused in function definition
-  --> $DIR/extra_unused_type_parameters.rs:4:14
+  --> $DIR/extra_unused_type_parameters.rs:4:13
    |
 LL | fn unused_ty<T>(x: u8) {}
-   |              ^
+   |             ^^^
    |
+   = help: consider removing the parameter
    = note: `-D clippy::extra-unused-type-parameters` implied by `-D warnings`
 
-error: type parameter goes unused in function definition
-  --> $DIR/extra_unused_type_parameters.rs:6:17
+error: type parameters go unused in function definition
+  --> $DIR/extra_unused_type_parameters.rs:6:16
    |
 LL | fn unused_multi<T, U>(x: u8) {}
-   |                 ^
+   |                ^^^^^^
+   |
+   = help: consider removing the parameters
 
 error: type parameter goes unused in function definition
-  --> $DIR/extra_unused_type_parameters.rs:6:20
+  --> $DIR/extra_unused_type_parameters.rs:8:23
    |
-LL | fn unused_multi<T, U>(x: u8) {}
-   |                    ^
+LL | fn unused_with_lt<'a, T>(x: &'a u8) {}
+   |                       ^
+   |
+   = help: consider removing the parameter
 
 error: type parameter goes unused in function definition
-  --> $DIR/extra_unused_type_parameters.rs:16:22
+  --> $DIR/extra_unused_type_parameters.rs:18:19
+   |
+LL | fn unused_bounded<T: Default, U>(x: U) {}
+   |                   ^^^^^^^^^^^
    |
-LL | fn unused_with_bound<T: Default>(x: u8) {}
-   |                      ^
+   = help: consider removing the parameter
 
 error: type parameter goes unused in function definition
-  --> $DIR/extra_unused_type_parameters.rs:39:23
+  --> $DIR/extra_unused_type_parameters.rs:20:24
+   |
+LL | fn unused_where_clause<T, U>(x: U)
+   |                        ^^
+   |
+   = help: consider removing the parameter
+
+error: type parameters go unused in function definition
+  --> $DIR/extra_unused_type_parameters.rs:26:16
+   |
+LL | fn some_unused<A, B, C, D: Iterator<Item = (B, C)>, E>(b: B, c: C) {}
+   |                ^^       ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^
+   |
+   = help: consider removing the parameters
+
+error: type parameter goes unused in function definition
+  --> $DIR/extra_unused_type_parameters.rs:49:22
    |
 LL |     fn unused_ty_impl<T>(&self) {}
-   |                       ^
+   |                      ^^^
+   |
+   = help: consider removing the parameter
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors
 
