Auto merge of rust-lang#10028 - mkrasnitski:extra_unused_type_paramet…

…ers, r=flip1995

Add `extra_unused_type_parameters` lint

Closes rust-lang#9240. ~~Keeping this as draft for now, because of a bug that I don't know how to fix.~~ It seems that opaque return types are not walked properly, for some unknown reason. As in, the following:

```rust
fn used_ret_opaque<A>() -> impl Iterator<Item = A> {
    std::iter::empty()
}
```
This triggers the lint even though it shouldn't. Discussion on Zulip didn't illuminate any possible reasons why, so PR-ing this now to increase visibility.

---

changelog: new lint: [`extra_unused_type_parameters`]
[rust-lang#10028](rust-lang#10028)
<!-- changelog_checked -->

CHANGELOG.md

@@ -4383,6 +4383,7 @@ Released 2018-09-13
 [`extend_from_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_from_slice
 [`extend_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_with_drain
 [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes
+[`extra_unused_type_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_type_parameters
 [`fallible_impl_from`]: https://rust-lang.github.io/rust-clippy/master/index.html#fallible_impl_from
 [`field_reassign_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#field_reassign_with_default
 [`filetype_is_file`]: https://rust-lang.github.io/rust-clippy/master/index.html#filetype_is_file

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 550 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 600 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
 You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.

book/src/README.md

@@ -6,7 +6,7 @@
 A collection of lints to catch common mistakes and improve your
 [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 550 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 600 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint
 level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how

clippy_lints/src/declared_lints.rs

@@ -156,6 +156,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::exhaustive_items::EXHAUSTIVE_STRUCTS_INFO,
     crate::exit::EXIT_INFO,
     crate::explicit_write::EXPLICIT_WRITE_INFO,
+    crate::extra_unused_type_parameters::EXTRA_UNUSED_TYPE_PARAMETERS_INFO,
     crate::fallible_impl_from::FALLIBLE_IMPL_FROM_INFO,
     crate::float_literal::EXCESSIVE_PRECISION_INFO,
     crate::float_literal::LOSSY_FLOAT_LITERAL_INFO,

clippy_lints/src/extra_unused_type_parameters.rs

@@ -0,0 +1,178 @@
+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, PredicateOrigin, 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};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for type parameters in generics that are never used anywhere else.
+    ///
+    /// ### Why is this bad?
+    /// Functions cannot infer the value of unused type parameters; therefore, calling them
+    /// requires using a turbofish, which serves no purpose but to satisfy the compiler.
+    ///
+    /// ### Example
+    /// ```rust
+    /// // unused type parameters
+    /// fn unused_ty<T>(x: u8) {
+    ///     // ..
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn no_unused_ty(x: u8) {
+    ///     // ..
+    /// }
+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub EXTRA_UNUSED_TYPE_PARAMETERS,
+    complexity,
+    "unused type parameters in function definitions"
+}
+declare_lint_pass!(ExtraUnusedTypeParameters => [EXTRA_UNUSED_TYPE_PARAMETERS]);
+
+/// A visitor struct that walks a given function and gathers generic type parameters, plus any
+/// trait bounds those parameters have.
+struct TypeWalker<'cx, 'tcx> {
+    cx: &'cx LateContext<'tcx>,
+    /// Collection of all the type parameters and their spans.
+    ty_params: FxHashMap<DefId, Span>,
+    /// Collection of any (inline) trait bounds corresponding to each type parameter.
+    bounds: FxHashMap<DefId, Span>,
+    /// 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 { .. } = param.kind {
+                    Some((param.def_id.into(), param.span))
+                } else {
+                    if !param.is_elided_lifetime() {
+                        all_params_unused = false;
+                    }
+                    None
+                }
+            })
+            .collect();
+        Self {
+            cx,
+            ty_params,
+            bounds: FxHashMap::default(),
+            generics,
+            all_params_unused,
+        }
+    }
+
+    fn emit_lint(&self) {
+        let (msg, help) = match self.ty_params.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.all_params_unused {
+            self.generics.span.into() // Remove the entire list of generics
+        } else {
+            MultiSpan::from_spans(
+                self.ty_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())
+                    })
+                    .collect(),
+            )
+        };
+
+        span_lint_and_help(self.cx, EXTRA_UNUSED_TYPE_PARAMETERS, span, msg, None, help);
+    }
+}
+
+impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
+    type NestedFilter = nested_filter::OnlyBodies;
+
+    fn visit_ty(&mut self, t: &'tcx Ty<'tcx>) {
+        if let Some((def_id, _)) = t.peel_refs().as_generic_param() {
+            if self.ty_params.remove(&def_id).is_some() {
+                self.all_params_unused = false;
+            }
+        } 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
+            // using `OnlyBodies`, so the check ends up failing and the type isn't fully walked.
+            let item = self.nested_visit_map().item(id);
+            walk_item(self, item);
+        } else {
+            walk_ty(self, t);
+        }
+    }
+
+    fn visit_where_predicate(&mut self, predicate: &'tcx WherePredicate<'tcx>) {
+        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()
+                && let PredicateOrigin::GenericParam = predicate.origin
+            {
+                self.bounds.insert(def_id, predicate.span);
+            }
+            // Only walk the right-hand side of where-bounds
+            for bound in predicate.bounds {
+                walk_param_bound(self, bound);
+            }
+        }
+    }
+
+    fn nested_visit_map(&mut self) -> Self::Map {
+        self.cx.tcx.hir()
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        if let ItemKind::Fn(_, generics, _) = item.kind {
+            let mut walker = TypeWalker::new(cx, generics);
+            walk_item(&mut walker, item);
+            walker.emit_lint();
+        }
+    }
+
+    fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'tcx>) {
+        // Only lint on inherent methods, not trait methods.
+        if let ImplItemKind::Fn(..) = item.kind && trait_ref_of_method(cx, item.owner_id.def_id).is_none() {
+            let mut walker = TypeWalker::new(cx, item.generics);
+            walk_impl_item(&mut walker, item);
+            walker.emit_lint();
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -122,6 +122,7 @@ mod excessive_bools;
 mod exhaustive_items;
 mod exit;
 mod explicit_write;
+mod extra_unused_type_parameters;
 mod fallible_impl_from;
 mod float_literal;
 mod floating_point_arithmetic;
@@ -910,6 +911,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(permissions_set_readonly_false::PermissionsSetReadonlyFalse));
     store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
     store.register_late_pass(|_| Box::new(multiple_unsafe_ops_per_block::MultipleUnsafeOpsPerBlock));
+    store.register_late_pass(|_| Box::new(extra_unused_type_parameters::ExtraUnusedTypeParameters));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

tests/ui/extra_unused_type_parameters.rs

@@ -0,0 +1,69 @@
+#![allow(unused, clippy::needless_lifetimes)]
+#![warn(clippy::extra_unused_type_parameters)]
+
+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) {}
+
+fn used_ret<T: Default>(x: u8) -> T {
+    T::default()
+}
+
+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()
+}
+
+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<T>(&self) {}
+}
+
+// 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 main() {}

tests/ui/extra_unused_type_parameters.stderr

@@ -0,0 +1,59 @@
+error: type parameter goes unused in function definition
+  --> $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 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:8:23
+   |
+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:18:19
+   |
+LL | fn unused_bounded<T: Default, U>(x: U) {}
+   |                   ^^^^^^^^^^^
+   |
+   = help: consider removing the parameter
+
+error: type parameter goes unused in function definition
+  --> $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 7 previous errors
+

tests/ui/needless_lifetimes.fixed

@@ -5,6 +5,7 @@
 #![allow(
     unused,
     clippy::boxed_local,
+    clippy::extra_unused_type_parameters,
     clippy::needless_pass_by_value,
     clippy::unnecessary_wraps,
     dyn_drop,

tests/ui/needless_lifetimes.rs

@@ -5,6 +5,7 @@
 #![allow(
     unused,
     clippy::boxed_local,
+    clippy::extra_unused_type_parameters,
     clippy::needless_pass_by_value,
     clippy::unnecessary_wraps,
     dyn_drop,