Auto merge of #10536 - mkrasnitski:suggestions, r=flip1995

Add suggestions to `extra_unused_type_parameters`

Change the `extra_unused_type_parameters` lint to provide machine applicable suggestions rather than just help messages. Exception to this are cases when any unused type parameters appear bounded in where clauses - for now I've deemed these cases unfixable and separated them out. Future work might be able to provide suggestions in these cases.

Also, added a test case for the `avoid_breaking_exported_api` config option.

r? `@flip1995`

changelog: [`extra_unused_type_parameters`]: Now provides fixable suggestions.

Author: bors

clippy_lints/src/extra_unused_type_parameters.rs

@@ -1,10 +1,10 @@
-use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then};
 use clippy_utils::trait_ref_of_method;
-use rustc_data_structures::fx::FxHashMap;
-use rustc_errors::MultiSpan;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_impl_item, walk_item, walk_param_bound, walk_ty, Visitor};
 use rustc_hir::{
-    BodyId, ExprKind, GenericBound, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind,
+    BodyId, ExprKind, GenericBound, GenericParam, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind,
     PredicateOrigin, Ty, TyKind, WherePredicate,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -53,13 +53,19 @@ impl ExtraUnusedTypeParameters {
         }
     }
 
-    /// Don't lint external macros or functions with empty bodies. Also, don't lint public items if
-    /// the `avoid_breaking_exported_api` config option is set.
-    fn check_false_positive(&self, cx: &LateContext<'_>, span: Span, def_id: LocalDefId, body_id: BodyId) -> bool {
+    /// Don't lint external macros or functions with empty bodies. Also, don't lint exported items
+    /// if the `avoid_breaking_exported_api` config option is set.
+    fn is_empty_exported_or_macro(
+        &self,
+        cx: &LateContext<'_>,
+        span: Span,
+        def_id: LocalDefId,
+        body_id: BodyId,
+    ) -> bool {
         let body = cx.tcx.hir().body(body_id).value;
         let fn_empty = matches!(&body.kind, ExprKind::Block(blk, None) if blk.stmts.is_empty() && blk.expr.is_none());
         let is_exported = cx.effective_visibilities.is_exported(def_id);
-        in_external_macro(cx.sess(), span) || (self.avoid_breaking_exported_api && is_exported) || fn_empty
+        in_external_macro(cx.sess(), span) || fn_empty || (is_exported && self.avoid_breaking_exported_api)
     }
 }
 
@@ -69,85 +75,129 @@ impl_lint_pass!(ExtraUnusedTypeParameters => [EXTRA_UNUSED_TYPE_PARAMETERS]);
 /// trait bounds those parameters have.
 struct TypeWalker<'cx, 'tcx> {
     cx: &'cx LateContext<'tcx>,
-    /// Collection of all the function's type parameters.
+    /// Collection of the function's type parameters. Once the function has been walked, this will
+    /// contain only unused type parameters.
     ty_params: FxHashMap<DefId, Span>,
-    /// Collection of any (inline) trait bounds corresponding to each type parameter.
-    bounds: FxHashMap<DefId, Span>,
+    /// Collection of any inline trait bounds corresponding to each type parameter.
+    inline_bounds: FxHashMap<DefId, Span>,
+    /// Collection of any type parameters with trait bounds that appear in a where clause.
+    where_bounds: FxHashSet<DefId>,
     /// The entire `Generics` object of the function, useful for querying purposes.
     generics: &'tcx Generics<'tcx>,
-    /// The value of this will remain `true` if *every* parameter:
-    ///   1. Is a type parameter, and
-    ///   2. Goes unused in the function.
-    /// Otherwise, if any type parameters end up being used, or if any lifetime or const-generic
-    /// parameters are present, this will be set to `false`.
-    all_params_unused: bool,
 }
 
 impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
     fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>) -> Self {
-        let mut all_params_unused = true;
         let ty_params = generics
             .params
             .iter()
-            .filter_map(|param| {
-                if let GenericParamKind::Type { synthetic, .. } = param.kind {
-                    (!synthetic).then_some((param.def_id.into(), param.span))
-                } else {
-                    if !param.is_elided_lifetime() {
-                        all_params_unused = false;
-                    }
-                    None
-                }
+            .filter_map(|param| match param.kind {
+                GenericParamKind::Type { synthetic, .. } if !synthetic => Some((param.def_id.into(), param.span)),
+                _ => None,
             })
             .collect();
 
         Self {
             cx,
             ty_params,
-            bounds: FxHashMap::default(),
+            inline_bounds: FxHashMap::default(),
+            where_bounds: FxHashSet::default(),
             generics,
-            all_params_unused,
         }
     }
 
-    fn mark_param_used(&mut self, def_id: DefId) {
-        if self.ty_params.remove(&def_id).is_some() {
-            self.all_params_unused = false;
-        }
+    fn get_bound_span(&self, param: &'tcx GenericParam<'tcx>) -> Span {
+        self.inline_bounds
+            .get(&param.def_id.to_def_id())
+            .map_or(param.span, |bound_span| param.span.with_hi(bound_span.hi()))
+    }
+
+    fn emit_help(&self, spans: Vec<Span>, msg: &str, help: &'static str) {
+        span_lint_and_help(self.cx, EXTRA_UNUSED_TYPE_PARAMETERS, spans, msg, None, help);
+    }
+
+    fn emit_sugg(&self, spans: Vec<Span>, msg: &str, help: &'static str) {
+        let suggestions: Vec<(Span, String)> = spans.iter().copied().zip(std::iter::repeat(String::new())).collect();
+        span_lint_and_then(self.cx, EXTRA_UNUSED_TYPE_PARAMETERS, spans, msg, |diag| {
+            diag.multipart_suggestion(help, suggestions, Applicability::MachineApplicable);
+        });
     }
 
     fn emit_lint(&self) {
-        let (msg, help) = match self.ty_params.len() {
+        let explicit_params = self
+            .generics
+            .params
+            .iter()
+            .filter(|param| !param.is_elided_lifetime() && !param.is_impl_trait())
+            .collect::<Vec<_>>();
+
+        let extra_params = explicit_params
+            .iter()
+            .enumerate()
+            .filter(|(_, param)| self.ty_params.contains_key(&param.def_id.to_def_id()))
+            .collect::<Vec<_>>();
+
+        let (msg, help) = match extra_params.len() {
             0 => return,
             1 => (
-                "type parameter goes unused in function definition",
+                format!(
+                    "type parameter `{}` goes unused in function definition",
+                    extra_params[0].1.name.ident()
+                ),
                 "consider removing the parameter",
             ),
             _ => (
-                "type parameters go unused in function definition",
+                format!(
+                    "type parameters go unused in function definition: {}",
+                    extra_params
+                        .iter()
+                        .map(|(_, param)| param.name.ident().to_string())
+                        .collect::<Vec<_>>()
+                        .join(", ")
+                ),
                 "consider removing the parameters",
             ),
         };
 
-        let source_map = self.cx.sess().source_map();
-        let span = if self.all_params_unused {
-            self.generics.span.into() // Remove the entire list of generics
+        // If any parameters are bounded in where clauses, don't try to form a suggestion.
+        // Otherwise, the leftover where bound would produce code that wouldn't compile.
+        if extra_params
+            .iter()
+            .any(|(_, param)| self.where_bounds.contains(&param.def_id.to_def_id()))
+        {
+            let spans = extra_params
+                .iter()
+                .map(|(_, param)| self.get_bound_span(param))
+                .collect::<Vec<_>>();
+            self.emit_help(spans, &msg, help);
         } else {
-            MultiSpan::from_spans(
-                self.ty_params
+            let spans = if explicit_params.len() == extra_params.len() {
+                vec![self.generics.span] // Remove the entire list of generics
+            } else {
+                let mut end: Option<LocalDefId> = None;
+                extra_params
                     .iter()
-                    .map(|(def_id, &span)| {
-                        // Extend the span past any trait bounds, and include the comma at the end.
-                        let span_to_extend = self.bounds.get(def_id).copied().map_or(span, Span::shrink_to_hi);
-                        let comma_range = source_map.span_extend_to_next_char(span_to_extend, '>', false);
-                        let comma_span = source_map.span_through_char(comma_range, ',');
-                        span.with_hi(comma_span.hi())
+                    .rev()
+                    .map(|(idx, param)| {
+                        if let Some(next) = explicit_params.get(idx + 1) && end != Some(next.def_id) {
+                        // Extend the current span forward, up until the next param in the list.
+                        param.span.until(next.span)
+                    } else {
+                        // Extend the current span back to include the comma following the previous
+                        // param. If the span of the next param in the list has already been
+                        // extended, we continue the chain. This is why we're iterating in reverse.
+                        end = Some(param.def_id);
+
+                        // idx will never be 0, else we'd be removing the entire list of generics
+                        let prev = explicit_params[idx - 1];
+                        let prev_span = self.get_bound_span(prev);
+                        self.get_bound_span(param).with_lo(prev_span.hi())
+                    }
                     })
-                    .collect(),
-            )
+                    .collect()
+            };
+            self.emit_sugg(spans, &msg, help);
         };
-
-        span_lint_and_help(self.cx, EXTRA_UNUSED_TYPE_PARAMETERS, span, msg, None, help);
     }
 }
 
@@ -162,7 +212,7 @@ 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.mark_param_used(def_id);
+            self.ty_params.remove(&def_id);
         } 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
@@ -176,9 +226,18 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
 
     fn visit_where_predicate(&mut self, predicate: &'tcx WherePredicate<'tcx>) {
         if let WherePredicate::BoundPredicate(predicate) = predicate {
-            // Collect spans for any bounds on type parameters. We only keep bounds that appear in
-            // the list of generics (not in a where-clause).
+            // Collect spans for any bounds on type parameters.
             if let Some((def_id, _)) = predicate.bounded_ty.peel_refs().as_generic_param() {
+                match predicate.origin {
+                    PredicateOrigin::GenericParam => {
+                        self.inline_bounds.insert(def_id, predicate.span);
+                    },
+                    PredicateOrigin::WhereClause => {
+                        self.where_bounds.insert(def_id);
+                    },
+                    PredicateOrigin::ImplTrait => (),
+                }
+
                 // If the bound contains non-public traits, err on the safe side and don't lint the
                 // corresponding parameter.
                 if !predicate
@@ -187,12 +246,10 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
                     .filter_map(bound_to_trait_def_id)
                     .all(|id| self.cx.effective_visibilities.is_exported(id))
                 {
-                    self.mark_param_used(def_id);
-                } else if let PredicateOrigin::GenericParam = predicate.origin {
-                    self.bounds.insert(def_id, predicate.span);
+                    self.ty_params.remove(&def_id);
                 }
             }
-            // Only walk the right-hand side of where-bounds
+            // Only walk the right-hand side of where bounds
             for bound in predicate.bounds {
                 walk_param_bound(self, bound);
             }
@@ -207,7 +264,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
 impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
         if let ItemKind::Fn(_, generics, body_id) = item.kind
-            && !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id)
+            && !self.is_empty_exported_or_macro(cx, item.span, item.owner_id.def_id, body_id)
         {
             let mut walker = TypeWalker::new(cx, generics);
             walk_item(&mut walker, item);
@@ -219,7 +276,7 @@ impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
         // Only lint on inherent methods, not trait methods.
         if let ImplItemKind::Fn(.., body_id) = item.kind
             && trait_ref_of_method(cx, item.owner_id.def_id).is_none()
-            && !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id)
+            && !self.is_empty_exported_or_macro(cx, item.span, item.owner_id.def_id, body_id)
         {
             let mut walker = TypeWalker::new(cx, item.generics);
             walk_impl_item(&mut walker, item);

tests/ui-toml/extra_unused_type_parameters/clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = true

tests/ui-toml/extra_unused_type_parameters/extra_unused_type_parameters.rs

@@ -0,0 +1,9 @@
+pub struct S;
+
+impl S {
+    pub fn exported_fn<T>() {
+        unimplemented!();
+    }
+}
+
+fn main() {}

tests/ui/extra_unused_type_parameters.fixed

@@ -0,0 +1,105 @@
+// run-rustfix
+
+#![allow(unused, clippy::needless_lifetimes)]
+#![warn(clippy::extra_unused_type_parameters)]
+
+fn unused_ty(x: u8) {
+    unimplemented!()
+}
+
+fn unused_multi(x: u8) {
+    unimplemented!()
+}
+
+fn unused_with_lt<'a>(x: &'a u8) {
+    unimplemented!()
+}
+
+fn used_ty<T>(x: T, y: u8) {}
+
+fn used_ref<'a, T>(x: &'a T) {}
+
+fn used_ret<T: Default>(x: u8) -> T {
+    T::default()
+}
+
+fn unused_bounded<U>(x: U) {
+    unimplemented!();
+}
+
+fn some_unused<B, C>(b: B, c: C) {
+    unimplemented!();
+}
+
+fn used_opaque<A>(iter: impl Iterator<Item = A>) -> usize {
+    iter.count()
+}
+
+fn used_ret_opaque<A>() -> impl Iterator<Item = A> {
+    std::iter::empty()
+}
+
+fn used_vec_box<T>(x: Vec<Box<T>>) {}
+
+fn used_body<T: Default + ToString>() -> String {
+    T::default().to_string()
+}
+
+fn used_closure<T: Default + ToString>() -> impl Fn() {
+    || println!("{}", T::default().to_string())
+}
+
+struct S;
+
+impl S {
+    fn unused_ty_impl(&self) {
+        unimplemented!()
+    }
+}
+
+// Don't lint on trait methods
+trait Foo {
+    fn bar<T>(&self);
+}
+
+impl Foo for S {
+    fn bar<T>(&self) {}
+}
+
+fn skip_index<A, Iter>(iter: Iter, index: usize) -> impl Iterator<Item = A>
+where
+    Iter: Iterator<Item = A>,
+{
+    iter.enumerate()
+        .filter_map(move |(i, a)| if i == index { None } else { Some(a) })
+}
+
+fn unused_opaque(dummy: impl Default) {
+    unimplemented!()
+}
+
+mod unexported_trait_bounds {
+    mod private {
+        pub trait Private {}
+    }
+
+    fn priv_trait_bound<T: private::Private>() {
+        unimplemented!();
+    }
+
+    fn unused_with_priv_trait_bound<T: private::Private>() {
+        unimplemented!();
+    }
+}
+
+mod issue10319 {
+    fn assert_send<T: Send>() {}
+
+    fn assert_send_where<T>()
+    where
+        T: Send,
+    {
+    }
+}
+
+fn main() {}