Skip to content

Commit 528bb63

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

File tree

8 files changed

+122
-60
lines changed

8 files changed

+122
-60
lines changed

‎clippy_lints/src/extra_unused_type_parameters.rs‎

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

4368
/// A visitor struct that walks a given function and gathers generic type parameters, plus any
4469
/// trait bounds those parameters have.
@@ -56,13 +81,10 @@ struct TypeWalker<'cx, 'tcx> {
5681
/// Otherwise, if any type parameters end up being used, or if any lifetime or const-generic
5782
/// parameters are present, this will be set to `false`.
5883
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,
6284
}
6385

6486
impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
65-
fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>, body_id: BodyId) -> Self {
87+
fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>) -> Self {
6688
let mut all_params_unused = true;
6789
let ty_params = generics
6890
.params
@@ -79,17 +101,18 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
79101
})
80102
.collect();
81103

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-
86104
Self {
87105
cx,
88106
ty_params,
89107
bounds: FxHashMap::default(),
90108
generics,
91109
all_params_unused,
92-
fn_body_empty,
110+
}
111+
}
112+
113+
fn mark_param_used(&mut self, def_id: DefId) {
114+
if self.ty_params.remove(&def_id).is_some() {
115+
self.all_params_unused = false;
93116
}
94117
}
95118

@@ -128,14 +151,18 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> {
128151
}
129152
}
130153

154+
/// Given a generic bound, if the bound is for a trait that's not a `LangItem`, return the
155+
/// `LocalDefId` for that trait.
156+
fn bound_to_trait_def_id(bound: &GenericBound<'_>) -> Option<LocalDefId> {
157+
bound.trait_ref()?.trait_def_id()?.as_local()
158+
}
159+
131160
impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
132161
type NestedFilter = nested_filter::OnlyBodies;
133162

134163
fn visit_ty(&mut self, t: &'tcx Ty<'tcx>) {
135164
if let Some((def_id, _)) = t.peel_refs().as_generic_param() {
136-
if self.ty_params.remove(&def_id).is_some() {
137-
self.all_params_unused = false;
138-
}
165+
self.mark_param_used(def_id);
139166
} else if let TyKind::OpaqueDef(id, _, _) = t.kind {
140167
// Explicitly walk OpaqueDef. Normally `walk_ty` would do the job, but it calls
141168
// `visit_nested_item`, which checks that `Self::NestedFilter::INTER` is set. We're
@@ -151,12 +178,16 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
151178
if let WherePredicate::BoundPredicate(predicate) = predicate {
152179
// Collect spans for any bounds on type parameters. We only keep bounds that appear in
153180
// 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).
157181
if let Some((def_id, _)) = predicate.bounded_ty.peel_refs().as_generic_param() {
158-
if self.fn_body_empty {
159-
self.ty_params.remove(&def_id);
182+
// If the bound contains non-public traits, err on the safe side and don't lint the
183+
// corresponding parameter.
184+
if !predicate
185+
.bounds
186+
.iter()
187+
.filter_map(bound_to_trait_def_id)
188+
.all(|id| self.cx.effective_visibilities.is_exported(id))
189+
{
190+
self.mark_param_used(def_id);
160191
} else if let PredicateOrigin::GenericParam = predicate.origin {
161192
self.bounds.insert(def_id, predicate.span);
162193
}
@@ -176,9 +207,9 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> {
176207
impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
177208
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
178209
if let ItemKind::Fn(_, generics, body_id) = item.kind
179-
&& !in_external_macro(cx.sess(), item.span)
210+
&& !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id)
180211
{
181-
let mut walker = TypeWalker::new(cx, generics, body_id);
212+
let mut walker = TypeWalker::new(cx, generics);
182213
walk_item(&mut walker, item);
183214
walker.emit_lint();
184215
}
@@ -188,9 +219,9 @@ impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters {
188219
// Only lint on inherent methods, not trait methods.
189220
if let ImplItemKind::Fn(.., body_id) = item.kind
190221
&& trait_ref_of_method(cx, item.owner_id.def_id).is_none()
191-
&& !in_external_macro(cx.sess(), item.span)
222+
&& !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id)
192223
{
193-
let mut walker = TypeWalker::new(cx, item.generics, body_id);
224+
let mut walker = TypeWalker::new(cx, item.generics);
194225
walk_impl_item(&mut walker, item);
195226
walker.emit_lint();
196227
}

‎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: 29 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,23 @@ 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+
}
85+
86+
mod unexported_trait_bounds {
87+
mod private {
88+
pub trait Private {}
89+
}
90+
91+
fn priv_trait_bound<T: private::Private>() {
92+
unimplemented!();
93+
}
94+
95+
fn unused_with_priv_trait_bound<T: private::Private, U>() {
96+
unimplemented!();
97+
}
98+
}
7599

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

‎tests/ui/extra_unused_type_parameters.stderr‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,75 @@
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
6565

66-
error: aborting due to 8 previous errors
66+
error: type parameter goes unused in function definition
67+
--> $DIR/extra_unused_type_parameters.rs:95:58
68+
|
69+
LL | fn unused_with_priv_trait_bound<T: private::Private, U>() {
70+
| ^
71+
|
72+
= help: consider removing the parameter
73+
74+
error: aborting due to 9 previous errors
6775

‎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;

0 commit comments

Comments
 (0)