Skip to content

Commit fd3da4d

Browse files
mkrasnitskimkrasnitski
committed
Fix more false positives for extra_unused_type_parameters 
1 parent 8d08917 commit fd3da4d

8 files changed

+92
-55
lines changed

‎clippy_lints/src/extra_unused_type_parameters.rs

Lines changed: 50 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,17 @@ use rustc_data_structures::fx::FxHashMap;
44
use rustc_errors::MultiSpan;
55
use rustc_hir::intravisit::{walk_impl_item, walk_item, walk_param_bound, walk_ty, Visitor};
66
use rustc_hir::{
7-
BodyId, ExprKind, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind, PredicateOrigin, Ty, TyKind,
8-
WherePredicate,
7+
BodyId, ExprKind, GenericBound, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind,
8+
PredicateOrigin, Ty, TyKind, WherePredicate,
99
};
1010
use rustc_lint::{LateContext, LateLintPass, LintContext};
1111
use rustc_middle::hir::nested_filter;
1212
use rustc_middle::lint::in_external_macro;
13-
use rustc_session::{declare_lint_pass, declare_tool_lint};
14-
use rustc_span::{def_id::DefId, Span};
13+
use rustc_session::{declare_tool_lint, impl_lint_pass};
14+
use rustc_span::{
15+
def_id::{DefId, LocalDefId},
16+
Span,
17+
};
1518

1619
declare_clippy_lint! {
1720
/// ### What it does
@@ -38,7 +41,30 @@ declare_clippy_lint! {
3841
complexity,
3942
"unused type parameters in function definitions"
4043
}
41-
declare_lint_pass!(ExtraUnusedTypeParameters => [EXTRA_UNUSED_TYPE_PARAMETERS]);
44+
45+
#[derive(Default)]
46+
pub struct ExtraUnusedTypeParameters {
47+
avoid_breaking_exported_api: bool,
48+
}
49+
50+
impl ExtraUnusedTypeParameters {
51+
pub fn new(avoid_breaking_exported_api: bool) -> Self {
52+
Self {
53+
avoid_breaking_exported_api,
54+
}
55+
}
56+
57+
/// Don't lint external macros or functions with empty bodies. Also, don't lint public items if
58+
/// the `avoid_breaking_exported_api` config option is set.
59+
fn check_false_positive(&self, cx: &LateContext<'_>, span: Span, def_id: LocalDefId, body_id: BodyId) -> bool {
60+
let body = cx.tcx.hir().body(body_id).value;
61+
let fn_empty = matches!(&body.kind, ExprKind::Block(blk, None) if blk.stmts.is_empty() && blk.expr.is_none());
62+
let is_exported = cx.effective_visibilities.is_exported(def_id);
63+
in_external_macro(cx.sess(), span) || (self.avoid_breaking_exported_api && is_exported) || fn_empty
64+
}
65+
}
66+
67+
impl_lint_pass!(ExtraUnusedTypeParameters => [EXTRA_UNUSED_TYPE_PARAMETERS]);
4268

4369
/// A visitor struct that walks a given function and gathers generic type parameters, plus any
4470
/// trait bounds those parameters have.
@@ -56,13 +82,10 @@ struct TypeWalker<'cx, 'tcx> {
5682
/// Otherwise, if any type parameters end up being used, or if any lifetime or const-generic
5783
/// parameters are present, this will be set to `false`.
5884
all_params_unused: bool,
59-
/// Whether or not the function has an empty body, in which case any bounded type parameters
60-
/// will not be linted.
61-
fn_body_empty: bool,
6285
}
6386

6487
impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
65-
fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>, body_id: BodyId) -> Self {
88+
fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>) -> Self {
6689
let mut all_params_unused = true;
6790
let ty_params = generics
6891
.params
@@ -79,17 +102,12 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
79102
})
80103
.collect();
81104

82-
let body = cx.tcx.hir().body(body_id).value;
83-
let fn_body_empty =
84-
matches!(&body.kind, ExprKind::Block(block, None) if block.stmts.is_empty() && block.expr.is_none());
85-
86105
Self {
87106
cx,
88107
ty_params,
89108
bounds: FxHashMap::default(),
90109
generics,
91110
all_params_unused,
92-
fn_body_empty,
93111
}
94112
}
95113

@@ -128,6 +146,12 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
128146
}
129147
}
130148

149+
/// Given a generic bound, if the bound is for a trait that's not a `LangItem`, return the
150+
/// `LocalDefId` for that trait.
151+
fn bound_to_trait_def_id(bound: &GenericBound<'_>) -> Option<LocalDefId> {
152+
bound.trait_ref()?.trait_def_id()?.as_local()
153+
}
154+
131155
impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
132156
type NestedFilter = nested_filter::OnlyBodies;
133157

@@ -151,11 +175,15 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
151175
if let WherePredicate::BoundPredicate(predicate) = predicate {
152176
// Collect spans for any bounds on type parameters. We only keep bounds that appear in
153177
// the list of generics (not in a where-clause).
154-
//
155-
// Also, if the function body is empty, we don't lint the corresponding type parameters
156-
// (See https://github.com/rust-lang/rust-clippy/issues/10319).
157178
if let Some((def_id, _)) = predicate.bounded_ty.peel_refs().as_generic_param() {
158-
if self.fn_body_empty {
179+
// If the bound contains non-public traits, err on the safe side and don't lint the
180+
// corresponding parameter.
181+
if !predicate
182+
.bounds
183+
.iter()
184+
.filter_map(bound_to_trait_def_id)
185+
.all(|def_id| self.cx.effective_visibilities.is_exported(def_id))
186+
{
159187
self.ty_params.remove(&def_id);
160188
} else if let PredicateOrigin::GenericParam = predicate.origin {
161189
self.bounds.insert(def_id, predicate.span);
@@ -176,9 +204,9 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
176204
impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
177205
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
178206
if let ItemKind::Fn(_, generics, body_id) = item.kind
179-
&& !in_external_macro(cx.sess(), item.span)
207+
&& !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id)
180208
{
181-
let mut walker = TypeWalker::new(cx, generics, body_id);
209+
let mut walker = TypeWalker::new(cx, generics);
182210
walk_item(&mut walker, item);
183211
walker.emit_lint();
184212
}
@@ -188,9 +216,9 @@ impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
188216
// Only lint on inherent methods, not trait methods.
189217
if let ImplItemKind::Fn(.., body_id) = item.kind
190218
&& trait_ref_of_method(cx, item.owner_id.def_id).is_none()
191-
&& !in_external_macro(cx.sess(), item.span)
219+
&& !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id)
192220
{
193-
let mut walker = TypeWalker::new(cx, item.generics, body_id);
221+
let mut walker = TypeWalker::new(cx, item.generics);
194222
walk_impl_item(&mut walker, item);
195223
walker.emit_lint();
196224
}

‎clippy_lints/src/lib.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -916,7 +916,11 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
916916
store.register_late_pass(|_| Box::new(permissions_set_readonly_false::PermissionsSetReadonlyFalse));
917917
store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
918918
store.register_late_pass(|_| Box::new(multiple_unsafe_ops_per_block::MultipleUnsafeOpsPerBlock));
919-
store.register_late_pass(|_| Box::new(extra_unused_type_parameters::ExtraUnusedTypeParameters));
919+
store.register_late_pass(move |_| {
920+
Box::new(extra_unused_type_parameters::ExtraUnusedTypeParameters::new(
921+
avoid_breaking_exported_api,
922+
))
923+
});
920924
// add lints here, do not remove this comment, it's used in `new_lint`
921925
}
922926

‎tests/ui/extra_unused_type_parameters.rs

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,17 @@
11
#![allow(unused, clippy::needless_lifetimes)]
22
#![warn(clippy::extra_unused_type_parameters)]
33

4-
fn unused_ty<T>(x: u8) {}
4+
fn unused_ty<T>(x: u8) {
5+
unimplemented!()
6+
}
57

6-
fn unused_multi<T, U>(x: u8) {}
8+
fn unused_multi<T, U>(x: u8) {
9+
unimplemented!()
10+
}
711

8-
fn unused_with_lt<'a, T>(x: &'a u8) {}
12+
fn unused_with_lt<'a, T>(x: &'a u8) {
13+
unimplemented!()
14+
}
915

1016
fn used_ty<T>(x: T, y: u8) {}
1117

@@ -51,7 +57,9 @@ fn used_closure<T: Default + ToString>() -> impl Fn() {
5157
struct S;
5258

5359
impl S {
54-
fn unused_ty_impl<T>(&self) {}
60+
fn unused_ty_impl<T>(&self) {
61+
unimplemented!()
62+
}
5563
}
5664

5765
// Don't lint on trait methods
@@ -71,7 +79,9 @@ where
7179
.filter_map(move |(i, a)| if i == index { None } else { Some(a) })
7280
}
7381

74-
fn unused_opaque<A, B>(dummy: impl Default) {}
82+
fn unused_opaque<A, B>(dummy: impl Default) {
83+
unimplemented!()
84+
}
7585

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

‎tests/ui/extra_unused_type_parameters.stderr

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,64 +1,64 @@
11
error: type parameter goes unused in function definition
22
--> $DIR/extra_unused_type_parameters.rs:4:13
33
|
4-
LL | fn unused_ty<T>(x: u8) {}
4+
LL | fn unused_ty<T>(x: u8) {
55
| ^^^
66
|
77
= help: consider removing the parameter
88
= note: `-D clippy::extra-unused-type-parameters` implied by `-D warnings`
99

1010
error: type parameters go unused in function definition
11-
--> $DIR/extra_unused_type_parameters.rs:6:16
11+
--> $DIR/extra_unused_type_parameters.rs:8:16
1212
|
13-
LL | fn unused_multi<T, U>(x: u8) {}
13+
LL | fn unused_multi<T, U>(x: u8) {
1414
| ^^^^^^
1515
|
1616
= help: consider removing the parameters
1717

1818
error: type parameter goes unused in function definition
19-
--> $DIR/extra_unused_type_parameters.rs:8:23
19+
--> $DIR/extra_unused_type_parameters.rs:12:23
2020
|
21-
LL | fn unused_with_lt<'a, T>(x: &'a u8) {}
21+
LL | fn unused_with_lt<'a, T>(x: &'a u8) {
2222
| ^
2323
|
2424
= help: consider removing the parameter
2525

2626
error: type parameter goes unused in function definition
27-
--> $DIR/extra_unused_type_parameters.rs:18:19
27+
--> $DIR/extra_unused_type_parameters.rs:24:19
2828
|
2929
LL | fn unused_bounded<T: Default, U>(x: U) {
3030
| ^^^^^^^^^^^
3131
|
3232
= help: consider removing the parameter
3333

3434
error: type parameter goes unused in function definition
35-
--> $DIR/extra_unused_type_parameters.rs:22:24
35+
--> $DIR/extra_unused_type_parameters.rs:28:24
3636
|
3737
LL | fn unused_where_clause<T, U>(x: U)
3838
| ^^
3939
|
4040
= help: consider removing the parameter
4141

4242
error: type parameters go unused in function definition
43-
--> $DIR/extra_unused_type_parameters.rs:29:16
43+
--> $DIR/extra_unused_type_parameters.rs:35:16
4444
|
4545
LL | fn some_unused<A, B, C, D: Iterator<Item = (B, C)>, E>(b: B, c: C) {
4646
| ^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^
4747
|
4848
= help: consider removing the parameters
4949

5050
error: type parameter goes unused in function definition
51-
--> $DIR/extra_unused_type_parameters.rs:54:22
51+
--> $DIR/extra_unused_type_parameters.rs:60:22
5252
|
53-
LL | fn unused_ty_impl<T>(&self) {}
53+
LL | fn unused_ty_impl<T>(&self) {
5454
| ^^^
5555
|
5656
= help: consider removing the parameter
5757

5858
error: type parameters go unused in function definition
59-
--> $DIR/extra_unused_type_parameters.rs:74:17
59+
--> $DIR/extra_unused_type_parameters.rs:82:17
6060
|
61-
LL | fn unused_opaque<A, B>(dummy: impl Default) {}
61+
LL | fn unused_opaque<A, B>(dummy: impl Default) {
6262
| ^^^^^^
6363
|
6464
= help: consider removing the parameters

‎tests/ui/new_without_default.rs

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,4 @@
1-
#![allow(
2-
dead_code,
3-
clippy::missing_safety_doc,
4-
clippy::extra_unused_lifetimes,
5-
clippy::extra_unused_type_parameters
6-
)]
1+
#![allow(dead_code, clippy::missing_safety_doc, clippy::extra_unused_lifetimes)]
72
#![warn(clippy::new_without_default)]
83

94
pub struct Foo;

‎tests/ui/new_without_default.stderr

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: you should consider adding a `Default` implementation for `Foo`
2-
--> $DIR/new_without_default.rs:12:5
2+
--> $DIR/new_without_default.rs:7:5
33
|
44
LL | / pub fn new() -> Foo {
55
LL | | Foo
@@ -17,7 +17,7 @@ LL + }
1717
|
1818

1919
error: you should consider adding a `Default` implementation for `Bar`
20-
--> $DIR/new_without_default.rs:20:5
20+
--> $DIR/new_without_default.rs:15:5
2121
|
2222
LL | / pub fn new() -> Self {
2323
LL | | Bar
@@ -34,7 +34,7 @@ LL + }
3434
|
3535

3636
error: you should consider adding a `Default` implementation for `LtKo<'c>`
37-
--> $DIR/new_without_default.rs:84:5
37+
--> $DIR/new_without_default.rs:79:5
3838
|
3939
LL | / pub fn new() -> LtKo<'c> {
4040
LL | | unimplemented!()
@@ -51,7 +51,7 @@ LL + }
5151
|
5252

5353
error: you should consider adding a `Default` implementation for `NewNotEqualToDerive`
54-
--> $DIR/new_without_default.rs:177:5
54+
--> $DIR/new_without_default.rs:172:5
5555
|
5656
LL | / pub fn new() -> Self {
5757
LL | | NewNotEqualToDerive { foo: 1 }
@@ -68,7 +68,7 @@ LL + }
6868
|
6969

7070
error: you should consider adding a `Default` implementation for `FooGenerics<T>`
71-
--> $DIR/new_without_default.rs:185:5
71+
--> $DIR/new_without_default.rs:180:5
7272
|
7373
LL | / pub fn new() -> Self {
7474
LL | | Self(Default::default())
@@ -85,7 +85,7 @@ LL + }
8585
|
8686

8787
error: you should consider adding a `Default` implementation for `BarGenerics<T>`
88-
--> $DIR/new_without_default.rs:192:5
88+
--> $DIR/new_without_default.rs:187:5
8989
|
9090
LL | / pub fn new() -> Self {
9191
LL | | Self(Default::default())
@@ -102,7 +102,7 @@ LL + }
102102
|
103103

104104
error: you should consider adding a `Default` implementation for `Foo<T>`
105-
--> $DIR/new_without_default.rs:203:9
105+
--> $DIR/new_without_default.rs:198:9
106106
|
107107
LL | / pub fn new() -> Self {
108108
LL | | todo!()

‎tests/ui/redundant_field_names.fixed

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// run-rustfix
22

33
#![warn(clippy::redundant_field_names)]
4-
#![allow(clippy::extra_unused_type_parameters, clippy::no_effect, dead_code, unused_variables)]
4+
#![allow(clippy::no_effect, dead_code, unused_variables)]
55

66
#[macro_use]
77
extern crate derive_new;

‎tests/ui/redundant_field_names.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// run-rustfix
22

33
#![warn(clippy::redundant_field_names)]
4-
#![allow(clippy::extra_unused_type_parameters, clippy::no_effect, dead_code, unused_variables)]
4+
#![allow(clippy::no_effect, dead_code, unused_variables)]
55

66
#[macro_use]
77
extern crate derive_new;

0 commit comments

Comments
 (0)