From 5ed338dff9ab543460e83c9fbe462373c0b99a82 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Thu, 21 Sep 2023 22:37:32 +0200 Subject: [PATCH 1/2] `impl_trait_in_params` now supports impls and traits --- .../src/functions/impl_trait_in_params.rs | 120 +++++++++++++----- clippy_lints/src/functions/mod.rs | 11 +- clippy_lints/src/lib.rs | 1 + .../impl_trait_in_params/true/clippy.toml | 1 + .../true/impl_trait_in_params.rs | 10 ++ tests/ui/impl_trait_in_params.rs | 29 ++++- tests/ui/impl_trait_in_params.stderr | 30 ++++- 7 files changed, 158 insertions(+), 44 deletions(-) create mode 100644 tests/ui-toml/impl_trait_in_params/true/clippy.toml create mode 100644 tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs diff --git a/clippy_lints/src/functions/impl_trait_in_params.rs b/clippy_lints/src/functions/impl_trait_in_params.rs index 597fca8888516..4a776dd26133e 100644 --- a/clippy_lints/src/functions/impl_trait_in_params.rs +++ b/clippy_lints/src/functions/impl_trait_in_params.rs @@ -1,50 +1,100 @@ 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::symbol::Ident; use rustc_span::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 */ }}: {}", ¶m.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 */ }}: {}>", ¶m.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 let FnKind::ItemFn(ident, generics, _) = kind { + if_chain! { + 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) - 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() { - diag.span_suggestion_with_style( - gen_span, - "add a type parameter", - format!(", {{ /* Generic name */ }}: {}", ¶m.name.ident().as_str()[5..]), - rustc_errors::Applicability::HasPlaceholders, - rustc_errors::SuggestionStyle::ShowAlways, - ); - } else { - diag.span_suggestion_with_style( - Span::new( - body.params[0].span.lo() - rustc_span::BytePos(1), - ident.span.hi(), - ident.span.ctxt(), - ident.span.parent(), - ), - "add a type parameter", - format!("<{{ /* Generic name */ }}: {}>", ¶m.name.ident().as_str()[5..]), - rustc_errors::Applicability::HasPlaceholders, - rustc_errors::SuggestionStyle::ShowAlways, - ); - } - }, - ); + report(cx, param, ident, generics, body.params[0].span); + }; + } + } + } +} + +pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { + if_chain! { + if let ImplItemKind::Fn(_, body_id) = impl_item.kind; + if let hir::Node::Item(item) = cx.tcx.hir().get_parent(impl_item.hir_id()); + if let hir::ItemKind::Impl(impl_) = item.kind; + if let hir::Impl { of_trait, .. } = *impl_; + if of_trait.is_none(); + let body = cx.tcx.hir().body(body_id); + if cx.tcx.visibility(cx.tcx.hir().body_owner_def_id(body.id())).is_public(); + if !is_in_test_function(cx.tcx, impl_item.hir_id()); + then { + for param in impl_item.generics.params { + if param.is_impl_trait() { + report(cx, param, &impl_item.ident, impl_item.generics, body.params[0].span); + } + } + } + } +} + +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(sig, _) = trait_item.kind; + if !is_in_test_function(cx.tcx, trait_item.hir_id()); + then { + for param in trait_item.generics.params { + if param.is_impl_trait() { + report(cx, param, &trait_item.ident, trait_item.generics, sig.decl.inputs[0].span); } } } diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index ee10334c67f13..55ebd44a60c20 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/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); } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fa009c8b2d6c8..8a4c2fc2c43dd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -758,6 +758,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::>(); diff --git a/tests/ui-toml/impl_trait_in_params/true/clippy.toml b/tests/ui-toml/impl_trait_in_params/true/clippy.toml new file mode 100644 index 0000000000000..0e7ffd070a352 --- /dev/null +++ b/tests/ui-toml/impl_trait_in_params/true/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = true \ No newline at end of file diff --git a/tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs b/tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs new file mode 100644 index 0000000000000..d1176ab896355 --- /dev/null +++ b/tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs @@ -0,0 +1,10 @@ +//! As avoid-breaking-exported-api is `true`, nothing here should lint +#![warn(clippy::impl_trait_in_params)] +#![no_main] + +trait Trait {} + +trait T { + fn t(_: impl Trait); + fn tt(_: T); +} diff --git a/tests/ui/impl_trait_in_params.rs b/tests/ui/impl_trait_in_params.rs index b652e4a4abe2e..825bb76e3f781 100644 --- a/tests/ui/impl_trait_in_params.rs +++ b/tests/ui/impl_trait_in_params.rs @@ -1,20 +1,41 @@ #![allow(unused)] #![warn(clippy::impl_trait_in_params)] + //@no-rustfix pub trait Trait {} pub trait AnotherTrait {} // Should warn pub fn a(_: impl Trait) {} -//~^ ERROR: '`impl Trait` used as a function parameter' -//~| NOTE: `-D clippy::impl-trait-in-params` implied by `-D warnings` +//~^ ERROR: `impl Trait` used as a function parameter pub fn c(_: C, _: impl Trait) {} -//~^ ERROR: '`impl Trait` used as a function parameter' -fn d(_: impl AnotherTrait) {} +//~^ ERROR: `impl Trait` used as a function parameter // Shouldn't warn pub fn b(_: B) {} fn e>(_: T) {} +fn d(_: impl AnotherTrait) {} + +//------ IMPLS + +trait T { + // See test in ui-toml for a case where avoid-breaking-exported-api is set to true + fn t(_: impl Trait); + fn tt(_: 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) {} + pub fn k>(_: K, _: impl AnotherTrait) {} //~ ERROR: `impl Trait` used as a function parameter +} + +// Trying with traits +impl T for S { + fn t(_: impl Trait) {} +} fn main() {} diff --git a/tests/ui/impl_trait_in_params.stderr b/tests/ui/impl_trait_in_params.stderr index 36b4f27e9a456..9f6e545c3f28a 100644 --- a/tests/ui/impl_trait_in_params.stderr +++ b/tests/ui/impl_trait_in_params.stderr @@ -1,5 +1,5 @@ -error: '`impl Trait` used as a function parameter' - --> $DIR/impl_trait_in_params.rs:8:13 +error: `impl Trait` used as a function parameter + --> $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, _: impl Trait) {} @@ -22,5 +22,27 @@ help: add a type parameter LL | pub fn c(_: C, _: impl Trait) {} | +++++++++++++++++++++++++++++++ -error: aborting due to 2 previous errors +error: `impl Trait` used as a function parameter + --> $DIR/impl_trait_in_params.rs:30: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:33:45 + | +LL | pub fn k>(_: K, _: impl AnotherTrait) {} + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: add a type parameter + | +LL | pub fn k, { /* Generic name */ }: AnotherTrait>(_: K, _: impl AnotherTrait) {} + | +++++++++++++++++++++++++++++++++++++++++++ + +error: aborting due to 4 previous errors From 775573768ed4bef5e0ebbfe7aa782a615eff2c51 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Sat, 23 Sep 2023 20:36:06 +0200 Subject: [PATCH 2/2] Fix tests, only lint for public tests --- .../src/functions/impl_trait_in_params.rs | 10 +++++++--- tests/ui-toml/impl_trait_in_params/clippy.toml | 1 + .../impl_trait_in_params/impl_trait_in_params.rs | 16 ++++++++++++++++ .../impl_trait_in_params.stderr | 15 +++++++++++++++ .../impl_trait_in_params/true/clippy.toml | 1 - .../true/impl_trait_in_params.rs | 10 ---------- tests/ui/impl_trait_in_params.rs | 12 +++++++++--- tests/ui/impl_trait_in_params.stderr | 4 ++-- 8 files changed, 50 insertions(+), 19 deletions(-) create mode 100644 tests/ui-toml/impl_trait_in_params/clippy.toml create mode 100644 tests/ui-toml/impl_trait_in_params/impl_trait_in_params.rs create mode 100644 tests/ui-toml/impl_trait_in_params/impl_trait_in_params.stderr delete mode 100644 tests/ui-toml/impl_trait_in_params/true/clippy.toml delete mode 100644 tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs diff --git a/clippy_lints/src/functions/impl_trait_in_params.rs b/clippy_lints/src/functions/impl_trait_in_params.rs index 4a776dd26133e..ee66c841ed25c 100644 --- a/clippy_lints/src/functions/impl_trait_in_params.rs +++ b/clippy_lints/src/functions/impl_trait_in_params.rs @@ -6,7 +6,7 @@ use rustc_hir::intravisit::FnKind; use rustc_hir::{Body, GenericParam, Generics, HirId, ImplItem, ImplItemKind, TraitItem, TraitItemKind}; use rustc_lint::LateContext; use rustc_span::symbol::Ident; -use rustc_span::Span; +use rustc_span::{BytePos, Span}; use super::IMPL_TRAIT_IN_PARAMS; @@ -89,12 +89,16 @@ pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { 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(sig, _) = trait_item.kind; + 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() { - report(cx, param, &trait_item.ident, trait_item.generics, sig.decl.inputs[0].span); + 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()); } } } diff --git a/tests/ui-toml/impl_trait_in_params/clippy.toml b/tests/ui-toml/impl_trait_in_params/clippy.toml new file mode 100644 index 0000000000000..87e1f235741dd --- /dev/null +++ b/tests/ui-toml/impl_trait_in_params/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = false \ No newline at end of file diff --git a/tests/ui-toml/impl_trait_in_params/impl_trait_in_params.rs b/tests/ui-toml/impl_trait_in_params/impl_trait_in_params.rs new file mode 100644 index 0000000000000..08fc7edf1c861 --- /dev/null +++ b/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); +} + +pub trait Public { + fn t(_: impl Trait); //~ ERROR: `impl Trait` used as a function parameter + fn tt(_: T); +} diff --git a/tests/ui-toml/impl_trait_in_params/impl_trait_in_params.stderr b/tests/ui-toml/impl_trait_in_params/impl_trait_in_params.stderr new file mode 100644 index 0000000000000..80c4f5ed4b0c0 --- /dev/null +++ b/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 + diff --git a/tests/ui-toml/impl_trait_in_params/true/clippy.toml b/tests/ui-toml/impl_trait_in_params/true/clippy.toml deleted file mode 100644 index 0e7ffd070a352..0000000000000 --- a/tests/ui-toml/impl_trait_in_params/true/clippy.toml +++ /dev/null @@ -1 +0,0 @@ -avoid-breaking-exported-api = true \ No newline at end of file diff --git a/tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs b/tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs deleted file mode 100644 index d1176ab896355..0000000000000 --- a/tests/ui-toml/impl_trait_in_params/true/impl_trait_in_params.rs +++ /dev/null @@ -1,10 +0,0 @@ -//! As avoid-breaking-exported-api is `true`, nothing here should lint -#![warn(clippy::impl_trait_in_params)] -#![no_main] - -trait Trait {} - -trait T { - fn t(_: impl Trait); - fn tt(_: T); -} diff --git a/tests/ui/impl_trait_in_params.rs b/tests/ui/impl_trait_in_params.rs index 825bb76e3f781..a6251a370d1d0 100644 --- a/tests/ui/impl_trait_in_params.rs +++ b/tests/ui/impl_trait_in_params.rs @@ -19,8 +19,14 @@ fn d(_: impl AnotherTrait) {} //------ IMPLS -trait T { - // See test in ui-toml for a case where avoid-breaking-exported-api is set to true +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 Private { + // This shouldn't lint fn t(_: impl Trait); fn tt(_: T) {} } @@ -34,7 +40,7 @@ impl S { } // Trying with traits -impl T for S { +impl Public for S { fn t(_: impl Trait) {} } diff --git a/tests/ui/impl_trait_in_params.stderr b/tests/ui/impl_trait_in_params.stderr index 9f6e545c3f28a..0ae7a3672d190 100644 --- a/tests/ui/impl_trait_in_params.stderr +++ b/tests/ui/impl_trait_in_params.stderr @@ -23,7 +23,7 @@ LL | pub fn c(_: C, _: impl Trait) {} | +++++++++++++++++++++++++++++++ error: `impl Trait` used as a function parameter - --> $DIR/impl_trait_in_params.rs:30:17 + --> $DIR/impl_trait_in_params.rs:36:17 | LL | pub fn h(_: impl Trait) {} | ^^^^^^^^^^ @@ -34,7 +34,7 @@ LL | pub fn h<{ /* Generic name */ }: Trait>(_: impl Trait) {} | +++++++++++++++++++++++++++++++ error: `impl Trait` used as a function parameter - --> $DIR/impl_trait_in_params.rs:33:45 + --> $DIR/impl_trait_in_params.rs:39:45 | LL | pub fn k>(_: K, _: impl AnotherTrait) {} | ^^^^^^^^^^^^^^^^^^^^^^