From 528bb639d4a72310b625b947874aad27b5ee5088 Mon Sep 17 00:00:00 2001 From: Michael Krasnitski Date: Wed, 22 Feb 2023 10:36:39 -0500 Subject: [PATCH] Fix more false positives for `extra_unused_type_parameters` --- .../src/extra_unused_type_parameters.rs | 83 +++++++++++++------ clippy_lints/src/lib.rs | 6 +- tests/ui/extra_unused_type_parameters.rs | 34 ++++++-- tests/ui/extra_unused_type_parameters.stderr | 34 +++++--- tests/ui/new_without_default.rs | 7 +- tests/ui/new_without_default.stderr | 14 ++-- tests/ui/redundant_field_names.fixed | 2 +- tests/ui/redundant_field_names.rs | 2 +- 8 files changed, 122 insertions(+), 60 deletions(-) diff --git a/clippy_lints/src/extra_unused_type_parameters.rs b/clippy_lints/src/extra_unused_type_parameters.rs index 040473c9ffc6..20565e1d232e 100644 --- a/clippy_lints/src/extra_unused_type_parameters.rs +++ b/clippy_lints/src/extra_unused_type_parameters.rs @@ -4,14 +4,17 @@ 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::{ - BodyId, ExprKind, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind, PredicateOrigin, Ty, TyKind, - WherePredicate, + BodyId, ExprKind, GenericBound, GenericParamKind, Generics, ImplItem, ImplItemKind, Item, ItemKind, + PredicateOrigin, Ty, TyKind, WherePredicate, }; 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}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{ + def_id::{DefId, LocalDefId}, + Span, +}; declare_clippy_lint! { /// ### What it does @@ -38,7 +41,29 @@ declare_clippy_lint! { complexity, "unused type parameters in function definitions" } -declare_lint_pass!(ExtraUnusedTypeParameters => [EXTRA_UNUSED_TYPE_PARAMETERS]); + +pub struct ExtraUnusedTypeParameters { + avoid_breaking_exported_api: bool, +} + +impl ExtraUnusedTypeParameters { + pub fn new(avoid_breaking_exported_api: bool) -> Self { + Self { + avoid_breaking_exported_api, + } + } + + /// Don't lint external macros or functions with empty bodies. Also, don't lint public items if + /// the `avoid_breaking_exported_api` config option is set. + fn check_false_positive(&self, cx: &LateContext<'_>, span: Span, def_id: LocalDefId, body_id: BodyId) -> bool { + let body = cx.tcx.hir().body(body_id).value; + let fn_empty = matches!(&body.kind, ExprKind::Block(blk, None) if blk.stmts.is_empty() && blk.expr.is_none()); + let is_exported = cx.effective_visibilities.is_exported(def_id); + in_external_macro(cx.sess(), span) || (self.avoid_breaking_exported_api && is_exported) || fn_empty + } +} + +impl_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. @@ -56,13 +81,10 @@ 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>, body_id: BodyId) -> Self { + fn new(cx: &'cx LateContext<'tcx>, generics: &'tcx Generics<'tcx>) -> Self { let mut all_params_unused = true; let ty_params = generics .params @@ -79,17 +101,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, + } + } + + fn mark_param_used(&mut self, def_id: DefId) { + if self.ty_params.remove(&def_id).is_some() { + self.all_params_unused = false; } } @@ -128,14 +151,18 @@ impl<'cx, 'tcx> TypeWalker<'cx, 'tcx> { } } +/// Given a generic bound, if the bound is for a trait that's not a `LangItem`, return the +/// `LocalDefId` for that trait. +fn bound_to_trait_def_id(bound: &GenericBound<'_>) -> Option { + bound.trait_ref()?.trait_def_id()?.as_local() +} + 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; - } + self.mark_param_used(def_id); } 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 @@ -151,12 +178,16 @@ impl<'cx, 'tcx> Visitor<'tcx> for TypeWalker<'cx, 'tcx> { if let WherePredicate::BoundPredicate(predicate) = predicate { // 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); + // If the bound contains non-public traits, err on the safe side and don't lint the + // corresponding parameter. + if !predicate + .bounds + .iter() + .filter_map(bound_to_trait_def_id) + .all(|id| self.cx.effective_visibilities.is_exported(id)) + { + self.mark_param_used(def_id); } else if let PredicateOrigin::GenericParam = predicate.origin { self.bounds.insert(def_id, predicate.span); } @@ -176,9 +207,9 @@ 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, body_id) = item.kind - && !in_external_macro(cx.sess(), item.span) + && !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id) { - let mut walker = TypeWalker::new(cx, generics, body_id); + let mut walker = TypeWalker::new(cx, generics); walk_item(&mut walker, item); walker.emit_lint(); } @@ -188,9 +219,9 @@ impl<'tcx> LateLintPass<'tcx> for ExtraUnusedTypeParameters { // Only lint on inherent methods, not trait methods. 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) + && !self.check_false_positive(cx, item.span, item.owner_id.def_id, body_id) { - let mut walker = TypeWalker::new(cx, item.generics, body_id); + let mut walker = TypeWalker::new(cx, item.generics); walk_impl_item(&mut walker, item); walker.emit_lint(); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7336fa19b126..b63cb4df48fe 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -916,7 +916,11 @@ 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)); + store.register_late_pass(move |_| { + Box::new(extra_unused_type_parameters::ExtraUnusedTypeParameters::new( + avoid_breaking_exported_api, + )) + }); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/extra_unused_type_parameters.rs b/tests/ui/extra_unused_type_parameters.rs index 2894fda2f471..480174342765 100644 --- a/tests/ui/extra_unused_type_parameters.rs +++ b/tests/ui/extra_unused_type_parameters.rs @@ -1,11 +1,17 @@ #![allow(unused, clippy::needless_lifetimes)] #![warn(clippy::extra_unused_type_parameters)] -fn unused_ty(x: u8) {} +fn unused_ty(x: u8) { + unimplemented!() +} -fn unused_multi(x: u8) {} +fn unused_multi(x: u8) { + unimplemented!() +} -fn unused_with_lt<'a, T>(x: &'a u8) {} +fn unused_with_lt<'a, T>(x: &'a u8) { + unimplemented!() +} fn used_ty(x: T, y: u8) {} @@ -51,7 +57,9 @@ fn used_closure() -> impl Fn() { struct S; impl S { - fn unused_ty_impl(&self) {} + fn unused_ty_impl(&self) { + unimplemented!() + } } // Don't lint on trait methods @@ -71,7 +79,23 @@ where .filter_map(move |(i, a)| if i == index { None } else { Some(a) }) } -fn unused_opaque(dummy: impl Default) {} +fn unused_opaque(dummy: impl Default) { + unimplemented!() +} + +mod unexported_trait_bounds { + mod private { + pub trait Private {} + } + + fn priv_trait_bound() { + unimplemented!(); + } + + fn unused_with_priv_trait_bound() { + unimplemented!(); + } +} mod issue10319 { fn assert_send() {} diff --git a/tests/ui/extra_unused_type_parameters.stderr b/tests/ui/extra_unused_type_parameters.stderr index aea3ee310f7d..86c88fc9bf00 100644 --- a/tests/ui/extra_unused_type_parameters.stderr +++ b/tests/ui/extra_unused_type_parameters.stderr @@ -1,30 +1,30 @@ error: type parameter goes unused in function definition --> $DIR/extra_unused_type_parameters.rs:4:13 | -LL | fn unused_ty(x: u8) {} +LL | fn unused_ty(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 + --> $DIR/extra_unused_type_parameters.rs:8:16 | -LL | fn unused_multi(x: u8) {} +LL | fn unused_multi(x: u8) { | ^^^^^^ | = help: consider removing the parameters error: type parameter goes unused in function definition - --> $DIR/extra_unused_type_parameters.rs:8:23 + --> $DIR/extra_unused_type_parameters.rs:12:23 | -LL | fn unused_with_lt<'a, T>(x: &'a u8) {} +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 + --> $DIR/extra_unused_type_parameters.rs:24:19 | LL | fn unused_bounded(x: U) { | ^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | fn unused_bounded(x: U) { = help: consider removing the parameter error: type parameter goes unused in function definition - --> $DIR/extra_unused_type_parameters.rs:22:24 + --> $DIR/extra_unused_type_parameters.rs:28:24 | LL | fn unused_where_clause(x: U) | ^^ @@ -40,7 +40,7 @@ LL | fn unused_where_clause(x: U) = help: consider removing the parameter error: type parameters go unused in function definition - --> $DIR/extra_unused_type_parameters.rs:29:16 + --> $DIR/extra_unused_type_parameters.rs:35:16 | LL | fn some_unused, E>(b: B, c: C) { | ^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^ @@ -48,20 +48,28 @@ LL | fn some_unused, E>(b: B, c: C) { = help: consider removing the parameters error: type parameter goes unused in function definition - --> $DIR/extra_unused_type_parameters.rs:54:22 + --> $DIR/extra_unused_type_parameters.rs:60:22 | -LL | fn unused_ty_impl(&self) {} +LL | fn unused_ty_impl(&self) { | ^^^ | = help: consider removing the parameter error: type parameters go unused in function definition - --> $DIR/extra_unused_type_parameters.rs:74:17 + --> $DIR/extra_unused_type_parameters.rs:82:17 | -LL | fn unused_opaque(dummy: impl Default) {} +LL | fn unused_opaque(dummy: impl Default) { | ^^^^^^ | = help: consider removing the parameters -error: aborting due to 8 previous errors +error: type parameter goes unused in function definition + --> $DIR/extra_unused_type_parameters.rs:95:58 + | +LL | fn unused_with_priv_trait_bound() { + | ^ + | + = help: consider removing the parameter + +error: aborting due to 9 previous errors diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs index 7803418cb047..65809023f8df 100644 --- a/tests/ui/new_without_default.rs +++ b/tests/ui/new_without_default.rs @@ -1,9 +1,4 @@ -#![allow( - dead_code, - clippy::missing_safety_doc, - clippy::extra_unused_lifetimes, - clippy::extra_unused_type_parameters -)] +#![allow(dead_code, clippy::missing_safety_doc, clippy::extra_unused_lifetimes)] #![warn(clippy::new_without_default)] pub struct Foo; diff --git a/tests/ui/new_without_default.stderr b/tests/ui/new_without_default.stderr index 583dd327d6a5..212a69ab94e6 100644 --- a/tests/ui/new_without_default.stderr +++ b/tests/ui/new_without_default.stderr @@ -1,5 +1,5 @@ error: you should consider adding a `Default` implementation for `Foo` - --> $DIR/new_without_default.rs:12:5 + --> $DIR/new_without_default.rs:7:5 | LL | / pub fn new() -> Foo { LL | | Foo @@ -17,7 +17,7 @@ LL + } | error: you should consider adding a `Default` implementation for `Bar` - --> $DIR/new_without_default.rs:20:5 + --> $DIR/new_without_default.rs:15:5 | LL | / pub fn new() -> Self { LL | | Bar @@ -34,7 +34,7 @@ LL + } | error: you should consider adding a `Default` implementation for `LtKo<'c>` - --> $DIR/new_without_default.rs:84:5 + --> $DIR/new_without_default.rs:79:5 | LL | / pub fn new() -> LtKo<'c> { LL | | unimplemented!() @@ -51,7 +51,7 @@ LL + } | error: you should consider adding a `Default` implementation for `NewNotEqualToDerive` - --> $DIR/new_without_default.rs:177:5 + --> $DIR/new_without_default.rs:172:5 | LL | / pub fn new() -> Self { LL | | NewNotEqualToDerive { foo: 1 } @@ -68,7 +68,7 @@ LL + } | error: you should consider adding a `Default` implementation for `FooGenerics` - --> $DIR/new_without_default.rs:185:5 + --> $DIR/new_without_default.rs:180:5 | LL | / pub fn new() -> Self { LL | | Self(Default::default()) @@ -85,7 +85,7 @@ LL + } | error: you should consider adding a `Default` implementation for `BarGenerics` - --> $DIR/new_without_default.rs:192:5 + --> $DIR/new_without_default.rs:187:5 | LL | / pub fn new() -> Self { LL | | Self(Default::default()) @@ -102,7 +102,7 @@ LL + } | error: you should consider adding a `Default` implementation for `Foo` - --> $DIR/new_without_default.rs:203:9 + --> $DIR/new_without_default.rs:198:9 | LL | / pub fn new() -> Self { LL | | todo!() diff --git a/tests/ui/redundant_field_names.fixed b/tests/ui/redundant_field_names.fixed index 276266a2dd80..ec7f8ae923a7 100644 --- a/tests/ui/redundant_field_names.fixed +++ b/tests/ui/redundant_field_names.fixed @@ -1,7 +1,7 @@ // run-rustfix #![warn(clippy::redundant_field_names)] -#![allow(clippy::extra_unused_type_parameters, clippy::no_effect, dead_code, unused_variables)] +#![allow(clippy::no_effect, dead_code, unused_variables)] #[macro_use] extern crate derive_new; diff --git a/tests/ui/redundant_field_names.rs b/tests/ui/redundant_field_names.rs index f674141c138e..73122016cf69 100644 --- a/tests/ui/redundant_field_names.rs +++ b/tests/ui/redundant_field_names.rs @@ -1,7 +1,7 @@ // run-rustfix #![warn(clippy::redundant_field_names)] -#![allow(clippy::extra_unused_type_parameters, clippy::no_effect, dead_code, unused_variables)] +#![allow(clippy::no_effect, dead_code, unused_variables)] #[macro_use] extern crate derive_new;