Skip to content

Commit

Permalink
Fix false positives for extra_unused_type_parameters
Browse files Browse the repository at this point in the history
  • Loading branch information
mkrasnitski committed Feb 11, 2023
1 parent 0f75581 commit 8789b37
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 26 deletions.
49 changes: 35 additions & 14 deletions clippy_lints/src/extra_unused_type_parameters.rs
Expand Up @@ -4,10 +4,12 @@ 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,
BodyId, ExprKind, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind, PredicateOrigin, Ty, TyKind,
WherePredicate,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::hir::nested_filter;
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::{def_id::DefId, Span};

Expand Down Expand Up @@ -55,10 +57,13 @@ struct TypeWalker<'cx, 'tcx> {
/// 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,
/// Whether or not the function has an empty body, in which case any bounded type parameters
/// will not be linted.
fn_body_empty: bool,
}

impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>) -> Self {
fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>, body_id: BodyId) -> Self {
let mut all_params_unused = true;
let ty_params = generics
.params
Expand All @@ -74,12 +79,18 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
}
})
.collect();

let body = cx.tcx.hir().body(body_id).value;
let fn_body_empty =
matches!(&body.kind, ExprKind::Block(block, None) if block.stmts.is_empty() && block.expr.is_none());

Self {
cx,
ty_params,
bounds: FxHashMap::default(),
generics,
all_params_unused,
fn_body_empty,
}
}

Expand All @@ -96,7 +107,7 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
),
};

let source_map = self.cx.tcx.sess.source_map();
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
} else {
Expand Down Expand Up @@ -139,12 +150,17 @@ 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 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);
// Collect spans for any bounds on type parameters. We only keep bounds that appear in
// the list of generics (not in a where-clause).
//
// Also, if the function body is empty, we don't lint the corresponding type parameters
// (See https://github.com/rust-lang/rust-clippy/issues/10319).
if let Some((def_id, _)) = predicate.bounded_ty.peel_refs().as_generic_param() {
if self.fn_body_empty {
self.ty_params.remove(&def_id);
} else if 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 {
Expand All @@ -160,17 +176,22 @@ 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, _) = item.kind {
let mut walker = TypeWalker::new(cx, generics);
if let ItemKind::Fn(_, generics, body_id) = item.kind
&& !in_external_macro(cx.sess(), item.span)
{
let mut walker = TypeWalker::new(cx, generics, body_id);
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);
if let ImplItemKind::Fn(.., body_id) = item.kind
&& trait_ref_of_method(cx, item.owner_id.def_id).is_none()
&& !in_external_macro(cx.sess(), item.span)
{
let mut walker = TypeWalker::new(cx, item.generics, body_id);
walk_impl_item(&mut walker, item);
walker.emit_lint();
}
Expand Down
19 changes: 17 additions & 2 deletions tests/ui/extra_unused_type_parameters.rs
Expand Up @@ -15,15 +15,20 @@ fn used_ret<T: Default>(x: u8) -> T {
T::default()
}

fn unused_bounded<T: Default, U>(x: U) {}
fn unused_bounded<T: Default, U>(x: U) {
unimplemented!();
}

fn unused_where_clause<T, U>(x: U)
where
T: Default,
{
unimplemented!();
}

fn some_unused<A, B, C, D: Iterator<Item = (B, C)>, E>(b: B, c: C) {}
fn some_unused<A, B, C, D: Iterator<Item = (B, C)>, E>(b: B, c: C) {
unimplemented!();
}

fn used_opaque<A>(iter: impl Iterator<Item = A>) -> usize {
iter.count()
Expand Down Expand Up @@ -66,4 +71,14 @@ where
.filter_map(move |(i, a)| if i == index { None } else { Some(a) })
}

mod issue10319 {
fn assert_send<T: Send>() {}

fn assert_send_where<T>()
where
T: Send,
{
}
}

fn main() {}
10 changes: 5 additions & 5 deletions tests/ui/extra_unused_type_parameters.stderr
Expand Up @@ -26,29 +26,29 @@ LL | fn unused_with_lt<'a, T>(x: &'a u8) {}
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) {}
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
--> $DIR/extra_unused_type_parameters.rs:22: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
--> $DIR/extra_unused_type_parameters.rs:29:16
|
LL | fn some_unused<A, B, C, D: Iterator<Item = (B, C)>, E>(b: B, c: C) {}
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
--> $DIR/extra_unused_type_parameters.rs:54:22
|
LL | fn unused_ty_impl<T>(&self) {}
| ^^^
Expand Down
1 change: 0 additions & 1 deletion tests/ui/type_repetition_in_bounds.rs
@@ -1,5 +1,4 @@
#![deny(clippy::type_repetition_in_bounds)]
#![allow(clippy::extra_unused_type_parameters)]

use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign};

Expand Down
8 changes: 4 additions & 4 deletions tests/ui/type_repetition_in_bounds.stderr
@@ -1,5 +1,5 @@
error: this type has already been used as a bound predicate
--> $DIR/type_repetition_in_bounds.rs:9:5
--> $DIR/type_repetition_in_bounds.rs:8:5
|
LL | T: Clone,
| ^^^^^^^^
Expand All @@ -12,23 +12,23 @@ LL | #![deny(clippy::type_repetition_in_bounds)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: this type has already been used as a bound predicate
--> $DIR/type_repetition_in_bounds.rs:26:5
--> $DIR/type_repetition_in_bounds.rs:25:5
|
LL | Self: Copy + Default + Ord,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider combining the bounds: `Self: Clone + Copy + Default + Ord`

error: this type has already been used as a bound predicate
--> $DIR/type_repetition_in_bounds.rs:86:5
--> $DIR/type_repetition_in_bounds.rs:85:5
|
LL | T: Clone,
| ^^^^^^^^
|
= help: consider combining the bounds: `T: ?Sized + Clone`

error: this type has already been used as a bound predicate
--> $DIR/type_repetition_in_bounds.rs:91:5
--> $DIR/type_repetition_in_bounds.rs:90:5
|
LL | T: ?Sized,
| ^^^^^^^^^
Expand Down

0 comments on commit 8789b37

Please sign in to comment.