Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra_unused_type_parameters lint #10028

Merged
merged 1 commit into from
Feb 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion book/src/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
178 changes: 178 additions & 0 deletions clippy_lints/src/extra_unused_type_parameters.rs
Original file line number Diff line number Diff line change
@@ -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> {
mkrasnitski marked this conversation as resolved.
Show resolved Hide resolved
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())
mkrasnitski marked this conversation as resolved.
Show resolved Hide resolved
})
.collect(),
)
};

span_lint_and_help(self.cx, EXTRA_UNUSED_TYPE_PARAMETERS, span, msg, None, help);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've put so much effort in crafting this awesome spans and message, I think this should be span_lint_and_sugg. The non-multispan cases can then be tested with // run-rustfix in the test file. You'll have to split up the test files for that though: fixable and unfixable.

}
}

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();
}
}
}
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}

Expand Down
69 changes: 69 additions & 0 deletions tests/ui/extra_unused_type_parameters.rs
Original file line number Diff line number Diff line change
@@ -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() {}
59 changes: 59 additions & 0 deletions tests/ui/extra_unused_type_parameters.stderr
Original file line number Diff line number Diff line change
@@ -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

1 change: 1 addition & 0 deletions tests/ui/needless_lifetimes.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(
unused,
clippy::boxed_local,
clippy::extra_unused_type_parameters,
clippy::needless_pass_by_value,
clippy::unnecessary_wraps,
dyn_drop,
Expand Down
1 change: 1 addition & 0 deletions tests/ui/needless_lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(
unused,
clippy::boxed_local,
clippy::extra_unused_type_parameters,
clippy::needless_pass_by_value,
clippy::unnecessary_wraps,
dyn_drop,
Expand Down
Loading