From ec3b457b0d3363435a524ed0c0abcbd29f0e331d Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 25 Sep 2023 10:55:05 +0800 Subject: [PATCH] fix doc test failure; apply review suggestions by @Centri3: use multi suggestion; change output message format; add macro expansion check & tests; --- clippy_lints/src/functions/mod.rs | 4 + .../src/functions/renamed_function_params.rs | 104 +++++++++++------- tests/ui/renamed_function_params.rs | 53 +++++++-- tests/ui/renamed_function_params.stderr | 60 +++++----- 4 files changed, 140 insertions(+), 81 deletions(-) diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 387e0c964ccc..9a09dbaed4ad 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -371,6 +371,8 @@ declare_clippy_lint! { /// /// ### Example /// ```rust + /// struct A(u32); + /// /// impl From for String { /// fn from(a: A) -> Self { /// a.0.to_string() @@ -379,6 +381,8 @@ declare_clippy_lint! { /// ``` /// Use instead: /// ```rust + /// struct A(u32); + /// /// impl From for String { /// fn from(value: A) -> Self { /// value.0.to_string() diff --git a/clippy_lints/src/functions/renamed_function_params.rs b/clippy_lints/src/functions/renamed_function_params.rs index 4c0d5aba1122..34ede5f43df8 100644 --- a/clippy_lints/src/functions/renamed_function_params.rs +++ b/clippy_lints/src/functions/renamed_function_params.rs @@ -1,70 +1,94 @@ -use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::diagnostics::span_lint_and_then; +use rustc_errors::{Applicability, MultiSpan}; use rustc_hir::def_id::DefId; use rustc_hir::hir_id::OwnerId; use rustc_hir::{ImplItem, ImplItemKind, ItemKind, Node}; use rustc_lint::LateContext; -use rustc_span::symbol::{Ident, Symbol}; +use rustc_span::symbol::{kw, Ident, Symbol}; use rustc_span::Span; use super::RENAMED_FUNCTION_PARAMS; pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>) { - if let ImplItemKind::Fn(_, body_id) = item.kind && - let Some(did) = impled_item_def_id(cx, item.owner_id) + if !item.span.from_expansion() && + let ImplItemKind::Fn(_, body_id) = item.kind && + let Some(did) = trait_item_def_id_of_impl(cx, item.owner_id) { let mut param_idents_iter = cx.tcx.hir().body_param_names(body_id); let mut default_param_idents_iter = cx.tcx.fn_arg_names(did).iter().copied(); - let renames = renamed_params(&mut default_param_idents_iter, &mut param_idents_iter); - // FIXME: Should we use `MultiSpan` to combine output together? - // But how should we display help message if so. - for rename in renames { - span_lint_and_help( + let renames = RenamedFnArgs::new(&mut default_param_idents_iter, &mut param_idents_iter); + if !renames.0.is_empty() { + let multi_span = renames.multi_span(); + let plural = if renames.0.len() == 1 { + "" + } else { + "s" + }; + span_lint_and_then( cx, RENAMED_FUNCTION_PARAMS, - rename.renamed_span, - "function parameter name was renamed from its trait default", - None, - &format!("consider changing the name to: '{}'", rename.default_name.as_str()) + multi_span, + &format!("renamed function parameter{plural} of trait impl"), + |diag| { + diag.multipart_suggestion( + format!("consider using the default name{plural}"), + renames.0, + Applicability::Unspecified, + ); + } ); } } } -struct RenamedParam { - renamed_span: Span, - default_name: Symbol, -} +struct RenamedFnArgs(Vec<(Span, String)>); -fn renamed_params(default_names: &mut I, current_names: &mut T) -> Vec -where - I: Iterator, - T: Iterator, -{ - let mut renamed = vec![]; - // FIXME: Should we stop if they have different length? - while let (Some(def_name), Some(cur_name)) = (default_names.next(), current_names.next()) { - let current_name = cur_name.name; - let default_name = def_name.name; - if is_ignored_or_empty_symbol(current_name) || is_ignored_or_empty_symbol(default_name) { - continue; - } - if current_name != default_name { - renamed.push(RenamedParam { - renamed_span: cur_name.span, - default_name, - }); +impl RenamedFnArgs { + /// Comparing between an iterator of default names and one with current names, + /// then collect the ones that got renamed. + fn new(default_names: &mut I, current_names: &mut T) -> Self + where + I: Iterator, + T: Iterator, + { + let mut renamed: Vec<(Span, String)> = vec![]; + + debug_assert!(default_names.size_hint() == current_names.size_hint()); + while let (Some(def_name), Some(cur_name)) = (default_names.next(), current_names.next()) { + let current_name = cur_name.name; + let default_name = def_name.name; + if is_unused_or_empty_symbol(current_name) || is_unused_or_empty_symbol(default_name) { + continue; + } + if current_name != default_name { + renamed.push((cur_name.span, default_name.to_string())); + } } + + Self(renamed) + } + + fn multi_span(&self) -> MultiSpan { + self.0 + .iter() + .map(|(span, _)| span) + .copied() + .collect::>() + .into() } - renamed } -fn is_ignored_or_empty_symbol(symbol: Symbol) -> bool { - let s = symbol.as_str(); - s.is_empty() || s.starts_with('_') +fn is_unused_or_empty_symbol(symbol: Symbol) -> bool { + // FIXME: `body_param_names` currently returning empty symbols for `wild` as well, + // so we need to check if the symbol is empty first. + // Therefore the check of whether it's equal to [`kw::Underscore`] has no use for now, + // but it would be nice to keep it here just to be future-proof. + symbol.is_empty() || symbol == kw::Underscore || symbol.as_str().starts_with('_') } -fn impled_item_def_id(cx: &LateContext<'_>, impl_item_id: OwnerId) -> Option { +/// Get the [`trait_item_def_id`](rustc_hir::hir::ImplItemRef::trait_item_def_id) of an impl item. +fn trait_item_def_id_of_impl(cx: &LateContext<'_>, impl_item_id: OwnerId) -> Option { let trait_node = cx.tcx.hir().find_parent(impl_item_id.into())?; if let Node::Item(item) = trait_node && let ItemKind::Impl(impl_) = &item.kind diff --git a/tests/ui/renamed_function_params.rs b/tests/ui/renamed_function_params.rs index ccc11968bcc6..91dec909782f 100644 --- a/tests/ui/renamed_function_params.rs +++ b/tests/ui/renamed_function_params.rs @@ -1,3 +1,4 @@ +//@no-rustfix #![warn(clippy::renamed_function_params)] #![allow(clippy::partialeq_ne_impl)] #![allow(unused)] @@ -19,17 +20,17 @@ impl ToString for A { struct B(u32); impl From for String { fn from(b: B) -> Self { - //~^ ERROR: function parameter name was renamed from its trait default + //~^ ERROR: renamed function parameter of trait impl b.0.to_string() } } impl PartialEq for B { fn eq(&self, rhs: &Self) -> bool { - //~^ ERROR: function parameter name was renamed from its trait default + //~^ ERROR: renamed function parameter of trait impl self.0 == rhs.0 } fn ne(&self, rhs: &Self) -> bool { - //~^ ERROR: function parameter name was renamed from its trait default + //~^ ERROR: renamed function parameter of trait impl self.0 != rhs.0 } } @@ -42,18 +43,18 @@ trait MyTrait { impl MyTrait for B { fn foo(&self, i_dont_wanna_use_your_name: u8) {} - //~^ ERROR: function parameter name was renamed from its trait default - fn bar(_a: u8, _b: u8) {} + //~^ ERROR: renamed function parameter of trait impl + fn bar(_a: u8, _: u8) {} fn baz(self, val: u8) {} } impl Hash for B { fn hash(&self, states: &mut H) { - //~^ ERROR: function parameter name was renamed from its trait default + //~^ ERROR: renamed function parameter of trait impl self.0.hash(states); } fn hash_slice(date: &[Self], states: &mut H) { - //~^ ERROR: function parameter name was renamed from its trait default + //~^ ERROR: renamed function parameters of trait impl for d in date { d.hash(states); } @@ -65,4 +66,42 @@ impl B { fn some_fn(&self, other: impl MyTrait) {} } +#[derive(Copy, Clone)] +enum C { + A, + B(u32), +} + +impl std::ops::Add for C { + type Output = C; + fn add(self, b: B) -> C { + //~^ ERROR: renamed function parameter of trait impl + C::B(b.0) + } +} + +impl From for C { + fn from(_: A) -> C { + C::A + } +} + +trait CustomTraitA { + fn foo(&self, other: u32); +} +trait CustomTraitB { + fn bar(&self, value: u8); +} + +macro_rules! impl_trait { + ($impl_for:ident, $tr:ty, $fn_name:ident, $t:ty) => { + impl $tr for $impl_for { + fn $fn_name(&self, v: $t) {} + } + }; +} + +impl_trait!(C, CustomTraitA, foo, u32); +impl_trait!(C, CustomTraitB, bar, u8); + fn main() {} diff --git a/tests/ui/renamed_function_params.stderr b/tests/ui/renamed_function_params.stderr index e42931a57b6c..33b9aa1b9c15 100644 --- a/tests/ui/renamed_function_params.stderr +++ b/tests/ui/renamed_function_params.stderr @@ -1,60 +1,52 @@ -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:21:13 +error: renamed function parameter of trait impl + --> $DIR/renamed_function_params.rs:22:13 | LL | fn from(b: B) -> Self { - | ^ + | ^ help: consider using the default name: `value` | - = help: consider changing the name to: 'value' = note: `-D clippy::renamed-function-params` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::renamed_function_params)]` -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:27:18 +error: renamed function parameter of trait impl + --> $DIR/renamed_function_params.rs:28:18 | LL | fn eq(&self, rhs: &Self) -> bool { - | ^^^ - | - = help: consider changing the name to: 'other' + | ^^^ help: consider using the default name: `other` -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:31:18 +error: renamed function parameter of trait impl + --> $DIR/renamed_function_params.rs:32:18 | LL | fn ne(&self, rhs: &Self) -> bool { - | ^^^ - | - = help: consider changing the name to: 'other' + | ^^^ help: consider using the default name: `other` -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:44:19 +error: renamed function parameter of trait impl + --> $DIR/renamed_function_params.rs:45:19 | LL | fn foo(&self, i_dont_wanna_use_your_name: u8) {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider changing the name to: 'val' + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the default name: `val` -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:51:31 +error: renamed function parameter of trait impl + --> $DIR/renamed_function_params.rs:52:31 | LL | fn hash(&self, states: &mut H) { - | ^^^^^^ - | - = help: consider changing the name to: 'state' + | ^^^^^^ help: consider using the default name: `state` -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:55:30 +error: renamed function parameters of trait impl + --> $DIR/renamed_function_params.rs:56:30 | LL | fn hash_slice(date: &[Self], states: &mut H) { - | ^^^^ + | ^^^^ ^^^^^^ | - = help: consider changing the name to: 'data' - -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:55:45 +help: consider using the default names | -LL | fn hash_slice(date: &[Self], states: &mut H) { - | ^^^^^^ +LL | fn hash_slice(data: &[Self], state: &mut H) { + | ~~~~ ~~~~~ + +error: renamed function parameter of trait impl + --> $DIR/renamed_function_params.rs:77:18 | - = help: consider changing the name to: 'state' +LL | fn add(self, b: B) -> C { + | ^ help: consider using the default name: `rhs` error: aborting due to 7 previous errors