Auto merge of #11550 - blyxyas:fix-impl_trait_in_params-for_assocfn, r=dswij

`impl_trait_in_params` now supports impls and traits

Before this PR, the lint `impl_trait_in_params`. This PR gives the lint support for functions in impls and traits. (Also, some pretty heavy refactor)

fixes #11548
changelog:[`impl_trait_in_params`] now supports `impl` blocks and functions in traits

clippy_lints/src/functions/impl_trait_in_params.rs

@@ -1,50 +1,104 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::is_in_test_function;
 
+use rustc_hir as hir;
 use rustc_hir::intravisit::FnKind;
-use rustc_hir::{Body, HirId};
+use rustc_hir::{Body, GenericParam, Generics, HirId, ImplItem, ImplItemKind, TraitItem, TraitItemKind};
 use rustc_lint::LateContext;
-use rustc_span::Span;
+use rustc_span::symbol::Ident;
+use rustc_span::{BytePos, Span};
 
 use super::IMPL_TRAIT_IN_PARAMS;
 
+fn report(
+    cx: &LateContext<'_>,
+    param: &GenericParam<'_>,
+    ident: &Ident,
+    generics: &Generics<'_>,
+    first_param_span: Span,
+) {
+    // No generics with nested generics, and no generics like FnMut(x)
+    span_lint_and_then(
+        cx,
+        IMPL_TRAIT_IN_PARAMS,
+        param.span,
+        "`impl Trait` used as a function parameter",
+        |diag| {
+            if let Some(gen_span) = generics.span_for_param_suggestion() {
+                // If there's already a generic param with the same bound, do not lint **this** suggestion.
+                diag.span_suggestion_with_style(
+                    gen_span,
+                    "add a type parameter",
+                    format!(", {{ /* Generic name */ }}: {}", &param.name.ident().as_str()[5..]),
+                    rustc_errors::Applicability::HasPlaceholders,
+                    rustc_errors::SuggestionStyle::ShowAlways,
+                );
+            } else {
+                diag.span_suggestion_with_style(
+                    Span::new(
+                        first_param_span.lo() - rustc_span::BytePos(1),
+                        ident.span.hi(),
+                        ident.span.ctxt(),
+                        ident.span.parent(),
+                    ),
+                    "add a type parameter",
+                    format!("<{{ /* Generic name */ }}: {}>", &param.name.ident().as_str()[5..]),
+                    rustc_errors::Applicability::HasPlaceholders,
+                    rustc_errors::SuggestionStyle::ShowAlways,
+                );
+            }
+        },
+    );
+}
+
 pub(super) fn check_fn<'tcx>(cx: &LateContext<'_>, kind: &'tcx FnKind<'_>, body: &'tcx Body<'_>, hir_id: HirId) {
-    if cx.tcx.visibility(cx.tcx.hir().body_owner_def_id(body.id())).is_public() && !is_in_test_function(cx.tcx, hir_id)
+    if_chain! {
-    {
+        if let FnKind::ItemFn(ident, generics, _) = kind;
-        if let FnKind::ItemFn(ident, generics, _) = kind {
+        if cx.tcx.visibility(cx.tcx.hir().body_owner_def_id(body.id())).is_public();
+        if !is_in_test_function(cx.tcx, hir_id);
+        then {
             for param in generics.params {
                 if param.is_impl_trait() {
-                    // No generics with nested generics, and no generics like FnMut(x)
+                    report(cx, param, ident, generics, body.params[0].span);
-                    span_lint_and_then(
+                };
-                        cx,
+            }
-                        IMPL_TRAIT_IN_PARAMS,
+        }
-                        param.span,
+    }
-                        "'`impl Trait` used as a function parameter'",
+}
-                        |diag| {
+
-                            if let Some(gen_span) = generics.span_for_param_suggestion() {
+pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
-                                diag.span_suggestion_with_style(
+    if_chain! {
-                                    gen_span,
+        if let ImplItemKind::Fn(_, body_id) = impl_item.kind;
-                                    "add a type parameter",
+        if let hir::Node::Item(item) = cx.tcx.hir().get_parent(impl_item.hir_id());
-                                    format!(", {{ /* Generic name */ }}: {}", &param.name.ident().as_str()[5..]),
+        if let hir::ItemKind::Impl(impl_) = item.kind;
-                                    rustc_errors::Applicability::HasPlaceholders,
+        if let hir::Impl { of_trait, .. } = *impl_;
-                                    rustc_errors::SuggestionStyle::ShowAlways,
+        if of_trait.is_none();
-                                );
+        let body = cx.tcx.hir().body(body_id);
-                            } else {
+        if cx.tcx.visibility(cx.tcx.hir().body_owner_def_id(body.id())).is_public();
-                                diag.span_suggestion_with_style(
+        if !is_in_test_function(cx.tcx, impl_item.hir_id());
-                                    Span::new(
+        then {
-                                        body.params[0].span.lo() - rustc_span::BytePos(1),
+            for param in impl_item.generics.params {
-                                        ident.span.hi(),
+                if param.is_impl_trait() {
-                                        ident.span.ctxt(),
+                    report(cx, param, &impl_item.ident, impl_item.generics, body.params[0].span);
-                                        ident.span.parent(),
+                }
-                                    ),
+            }
-                                    "add a type parameter",
+        }
-                                    format!("<{{ /* Generic name */ }}: {}>", &param.name.ident().as_str()[5..]),
+    }
-                                    rustc_errors::Applicability::HasPlaceholders,
+}
-                                    rustc_errors::SuggestionStyle::ShowAlways,
+
-                                );
+pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>, avoid_breaking_exported_api: bool) {
-                            }
+    if_chain! {
-                        },
+        if !avoid_breaking_exported_api;
-                    );
+        if let TraitItemKind::Fn(_, _) = trait_item.kind;
+        if let hir::Node::Item(item) = cx.tcx.hir().get_parent(trait_item.hir_id());
+        // ^^ (Will always be a trait)
+        if !item.vis_span.is_empty(); // Is public
+        if !is_in_test_function(cx.tcx, trait_item.hir_id());
+        then {
+            for param in trait_item.generics.params {
+                if param.is_impl_trait() {
+                    let sp = trait_item.ident.span.with_hi(trait_item.ident.span.hi() + BytePos(1));
+                    report(cx, param, &trait_item.ident, trait_item.generics, sp.shrink_to_hi());
                 }
             }
         }

clippy_lints/src/functions/mod.rs

@@ -364,14 +364,21 @@ pub struct Functions {
     too_many_arguments_threshold: u64,
     too_many_lines_threshold: u64,
     large_error_threshold: u64,
+    avoid_breaking_exported_api: bool,
 }
 
 impl Functions {
-    pub fn new(too_many_arguments_threshold: u64, too_many_lines_threshold: u64, large_error_threshold: u64) -> Self {
+    pub fn new(
+        too_many_arguments_threshold: u64,
+        too_many_lines_threshold: u64,
+        large_error_threshold: u64,
+        avoid_breaking_exported_api: bool,
+    ) -> Self {
         Self {
             too_many_arguments_threshold,
             too_many_lines_threshold,
             large_error_threshold,
+            avoid_breaking_exported_api,
         }
     }
 }
@@ -415,6 +422,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
         must_use::check_impl_item(cx, item);
         result::check_impl_item(cx, item, self.large_error_threshold);
+        impl_trait_in_params::check_impl_item(cx, item);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
@@ -422,5 +430,6 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
         not_unsafe_ptr_arg_deref::check_trait_item(cx, item);
         must_use::check_trait_item(cx, item);
         result::check_trait_item(cx, item, self.large_error_threshold);
+        impl_trait_in_params::check_trait_item(cx, item, self.avoid_breaking_exported_api);
     }
 }

clippy_lints/src/lib.rs

@@ -762,6 +762,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             too_many_arguments_threshold,
             too_many_lines_threshold,
             large_error_threshold,
+            avoid_breaking_exported_api,
         ))
     });
     let doc_valid_idents = conf.doc_valid_idents.iter().cloned().collect::<FxHashSet<_>>();

tests/ui-toml/impl_trait_in_params/clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = false

tests/ui-toml/impl_trait_in_params/impl_trait_in_params.rs

@@ -0,0 +1,16 @@
+//! As avoid-breaking-exported-api is `false`, nothing here should lint
+#![warn(clippy::impl_trait_in_params)]
+#![no_main]
+//@no-rustfix
+
+pub trait Trait {}
+
+trait Private {
+    fn t(_: impl Trait);
+    fn tt<T: Trait>(_: T);
+}
+
+pub trait Public {
+    fn t(_: impl Trait); //~ ERROR: `impl Trait` used as a function parameter
+    fn tt<T: Trait>(_: T);
+}

tests/ui-toml/impl_trait_in_params/impl_trait_in_params.stderr

@@ -0,0 +1,15 @@
+error: `impl Trait` used as a function parameter
+  --> $DIR/impl_trait_in_params.rs:14:13
+   |
+LL |     fn t(_: impl Trait);
+   |             ^^^^^^^^^^
+   |
+   = note: `-D clippy::impl-trait-in-params` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::impl_trait_in_params)]`
+help: add a type parameter
+   |
+LL |     fn t<{ /* Generic name */ }: Trait>(_: impl Trait);
+   |         +++++++++++++++++++++++++++++++
+
+error: aborting due to previous error
+

tests/ui/impl_trait_in_params.rs

@@ -1,20 +1,47 @@
 #![allow(unused)]
 #![warn(clippy::impl_trait_in_params)]
+
 //@no-rustfix
 pub trait Trait {}
 pub trait AnotherTrait<T> {}
 
 // Should warn
 pub fn a(_: impl Trait) {}
-//~^ ERROR: '`impl Trait` used as a function parameter'
+//~^ ERROR: `impl Trait` used as a function parameter
-//~| NOTE: `-D clippy::impl-trait-in-params` implied by `-D warnings`
 pub fn c<C: Trait>(_: C, _: impl Trait) {}
-//~^ ERROR: '`impl Trait` used as a function parameter'
+//~^ ERROR: `impl Trait` used as a function parameter
-fn d(_: impl AnotherTrait<u32>) {}
 
 // Shouldn't warn
 
 pub fn b<B: Trait>(_: B) {}
 fn e<T: AnotherTrait<u32>>(_: T) {}
+fn d(_: impl AnotherTrait<u32>) {}
+
+//------ IMPLS
+
+pub trait Public {
+    // See test in ui-toml for a case where avoid-breaking-exported-api is set to false
+    fn t(_: impl Trait);
+    fn tt<T: Trait>(_: T) {}
+}
+
+trait Private {
+    // This shouldn't lint
+    fn t(_: impl Trait);
+    fn tt<T: Trait>(_: T) {}
+}
+
+struct S;
+impl S {
+    pub fn h(_: impl Trait) {} //~ ERROR: `impl Trait` used as a function parameter
+    fn i(_: impl Trait) {}
+    pub fn j<J: Trait>(_: J) {}
+    pub fn k<K: AnotherTrait<u32>>(_: K, _: impl AnotherTrait<u32>) {} //~ ERROR: `impl Trait` used as a function parameter
+}
+
+// Trying with traits
+impl Public for S {
+    fn t(_: impl Trait) {}
+}
 
 fn main() {}

tests/ui/impl_trait_in_params.stderr

@@ -1,5 +1,5 @@
-error: '`impl Trait` used as a function parameter'
+error: `impl Trait` used as a function parameter
-  --> $DIR/impl_trait_in_params.rs:8:13
+  --> $DIR/impl_trait_in_params.rs:9:13
    |
 LL | pub fn a(_: impl Trait) {}
    |             ^^^^^^^^^^
@@ -11,7 +11,7 @@ help: add a type parameter
 LL | pub fn a<{ /* Generic name */ }: Trait>(_: impl Trait) {}
    |         +++++++++++++++++++++++++++++++
 
-error: '`impl Trait` used as a function parameter'
+error: `impl Trait` used as a function parameter
   --> $DIR/impl_trait_in_params.rs:11:29
    |
 LL | pub fn c<C: Trait>(_: C, _: impl Trait) {}
@@ -22,5 +22,27 @@ help: add a type parameter
 LL | pub fn c<C: Trait, { /* Generic name */ }: Trait>(_: C, _: impl Trait) {}
    |                  +++++++++++++++++++++++++++++++
 
-error: aborting due to 2 previous errors
+error: `impl Trait` used as a function parameter
+  --> $DIR/impl_trait_in_params.rs:36:17
+   |
+LL |     pub fn h(_: impl Trait) {}
+   |                 ^^^^^^^^^^
+   |
+help: add a type parameter
+   |
+LL |     pub fn h<{ /* Generic name */ }: Trait>(_: impl Trait) {}
+   |             +++++++++++++++++++++++++++++++
+
+error: `impl Trait` used as a function parameter
+  --> $DIR/impl_trait_in_params.rs:39:45
+   |
+LL |     pub fn k<K: AnotherTrait<u32>>(_: K, _: impl AnotherTrait<u32>) {}
+   |                                             ^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: add a type parameter
+   |
+LL |     pub fn k<K: AnotherTrait<u32>, { /* Generic name */ }: AnotherTrait<u32>>(_: K, _: impl AnotherTrait<u32>) {}
+   |                                  +++++++++++++++++++++++++++++++++++++++++++
+
+error: aborting due to 4 previous errors