Auto merge of #10392 - mkrasnitski:false-positives, r=Jarcho

Fix more false positives for `extra_unused_type_parameters`

Builds on #10321. All empty functions are no longer linted, instead of just those that have trait bounds on them. Also, if a trait bound contains a non-public trait (un-exported, but still potentially reachable), then the corresponding type parameter isn't linted.

Finally, added support for the `avoid_breaking_exported_api` config option.

r? `@flip1995`
changelog: none

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<LocalDefId> {
+    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();
         }

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`
 }
 

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<T>(x: u8) {}
+fn unused_ty<T>(x: u8) {
+    unimplemented!()
+}
 
-fn unused_multi<T, U>(x: u8) {}
+fn unused_multi<T, U>(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<T>(x: T, y: u8) {}
 
@@ -51,7 +57,9 @@ fn used_closure<T: Default + ToString>() -> impl Fn() {
 struct S;
 
 impl S {
-    fn unused_ty_impl<T>(&self) {}
+    fn unused_ty_impl<T>(&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<A, B>(dummy: impl Default) {}
+fn unused_opaque<A, B>(dummy: impl Default) {
+    unimplemented!()
+}
+
+mod unexported_trait_bounds {
+    mod private {
+        pub trait Private {}
+    }
+
+    fn priv_trait_bound<T: private::Private>() {
+        unimplemented!();
+    }
+
+    fn unused_with_priv_trait_bound<T: private::Private, U>() {
+        unimplemented!();
+    }
+}
 
 mod issue10319 {
     fn assert_send<T: Send>() {}

tests/ui/extra_unused_type_parameters.stderr

@@ -1,67 +1,75 @@
 error: type parameter goes unused in function definition
   --> $DIR/extra_unused_type_parameters.rs:4:13
    |
-LL | fn unused_ty<T>(x: u8) {}
+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
+  --> $DIR/extra_unused_type_parameters.rs:8:16
    |
-LL | fn unused_multi<T, U>(x: u8) {}
+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
+  --> $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<T: Default, U>(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<T, U>(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<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:54:22
+  --> $DIR/extra_unused_type_parameters.rs:60:22
    |
-LL |     fn unused_ty_impl<T>(&self) {}
+LL |     fn unused_ty_impl<T>(&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<A, B>(dummy: impl Default) {}
+LL | fn unused_opaque<A, B>(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<T: private::Private, U>() {
+   |                                                          ^
+   |
+   = help: consider removing the parameter
+
+error: aborting due to 9 previous errors
 

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;

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<T>`
-  --> $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<T>`
-  --> $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<T>`
-  --> $DIR/new_without_default.rs:203:9
+  --> $DIR/new_without_default.rs:198:9
    |
 LL | /         pub fn new() -> Self {
 LL | |             todo!()

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;