From 28d5115067c35a858af5248f0e7b4bc92380798d Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 20 Sep 2023 19:05:37 +0800 Subject: [PATCH 1/3] add new lint that disallow renaming parameters in trait functions --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/functions/mod.rs | 34 ++++++++ .../src/functions/renamed_function_params.rs | 82 +++++++++++++++++++ tests/ui/renamed_function_params.rs | 68 +++++++++++++++ tests/ui/renamed_function_params.stderr | 60 ++++++++++++++ 6 files changed, 246 insertions(+) create mode 100644 clippy_lints/src/functions/renamed_function_params.rs create mode 100644 tests/ui/renamed_function_params.rs create mode 100644 tests/ui/renamed_function_params.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c9ea114081..ce2dda0cc2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5702,6 +5702,7 @@ Released 2018-09-13 [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref [`ref_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_patterns [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro +[`renamed_function_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#renamed_function_params [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once [`repeat_vec_with_capacity`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_vec_with_capacity [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5ff7d8e5134..8da893faa40 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -205,6 +205,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::functions::MUST_USE_CANDIDATE_INFO, crate::functions::MUST_USE_UNIT_INFO, crate::functions::NOT_UNSAFE_PTR_ARG_DEREF_INFO, + crate::functions::RENAMED_FUNCTION_PARAMS_INFO, crate::functions::RESULT_LARGE_ERR_INFO, crate::functions::RESULT_UNIT_ERR_INFO, crate::functions::TOO_MANY_ARGUMENTS_INFO, diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 9cc51fa8cd5..387e0c964cc 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -2,6 +2,7 @@ mod impl_trait_in_params; mod misnamed_getters; mod must_use; mod not_unsafe_ptr_arg_deref; +mod renamed_function_params; mod result; mod too_many_arguments; mod too_many_lines; @@ -359,6 +360,37 @@ declare_clippy_lint! { "`impl Trait` is used in the function's parameters" } +declare_clippy_lint! { + /// ### What it does + /// Lints when the name of function parameters from trait impl is + /// different than its default implementation. + /// + /// ### Why is this bad? + /// Using the default name for parameters of a trait method is often + /// more desirable for consistency's sake. + /// + /// ### Example + /// ```rust + /// impl From for String { + /// fn from(a: A) -> Self { + /// a.0.to_string() + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// impl From for String { + /// fn from(value: A) -> Self { + /// value.0.to_string() + /// } + /// } + /// ``` + #[clippy::version = "1.74.0"] + pub RENAMED_FUNCTION_PARAMS, + restriction, + "renamed function parameters in trait implementation" +} + #[derive(Copy, Clone)] #[allow(clippy::struct_field_names)] pub struct Functions { @@ -395,6 +427,7 @@ impl_lint_pass!(Functions => [ RESULT_LARGE_ERR, MISNAMED_GETTERS, IMPL_TRAIT_IN_PARAMS, + RENAMED_FUNCTION_PARAMS, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -424,6 +457,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions { 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); + renamed_function_params::check_impl_item(cx, item); } fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { diff --git a/clippy_lints/src/functions/renamed_function_params.rs b/clippy_lints/src/functions/renamed_function_params.rs new file mode 100644 index 00000000000..4c0d5aba112 --- /dev/null +++ b/clippy_lints/src/functions/renamed_function_params.rs @@ -0,0 +1,82 @@ +use clippy_utils::diagnostics::span_lint_and_help; +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::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) + { + 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( + 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()) + ); + } + } +} + +struct RenamedParam { + renamed_span: Span, + default_name: Symbol, +} + +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, + }); + } + } + renamed +} + +fn is_ignored_or_empty_symbol(symbol: Symbol) -> bool { + let s = symbol.as_str(); + s.is_empty() || s.starts_with('_') +} + +fn impled_item_def_id(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 + { + impl_.items.iter().find_map(|item| { + if item.id.owner_id == impl_item_id { + item.trait_item_def_id + } else { + None + } + }) + } else { + None + } +} diff --git a/tests/ui/renamed_function_params.rs b/tests/ui/renamed_function_params.rs new file mode 100644 index 00000000000..ccc11968bcc --- /dev/null +++ b/tests/ui/renamed_function_params.rs @@ -0,0 +1,68 @@ +#![warn(clippy::renamed_function_params)] +#![allow(clippy::partialeq_ne_impl)] +#![allow(unused)] + +use std::hash::{Hash, Hasher}; + +struct A; +impl From for String { + fn from(_value: A) -> Self { + String::new() + } +} +impl ToString for A { + fn to_string(&self) -> String { + String::new() + } +} + +struct B(u32); +impl From for String { + fn from(b: B) -> Self { + //~^ ERROR: function parameter name was renamed from its trait default + b.0.to_string() + } +} +impl PartialEq for B { + fn eq(&self, rhs: &Self) -> bool { + //~^ ERROR: function parameter name was renamed from its trait default + self.0 == rhs.0 + } + fn ne(&self, rhs: &Self) -> bool { + //~^ ERROR: function parameter name was renamed from its trait default + self.0 != rhs.0 + } +} + +trait MyTrait { + fn foo(&self, val: u8); + fn bar(a: u8, b: u8); + fn baz(self, _val: u8); +} + +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) {} + 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 + self.0.hash(states); + } + fn hash_slice(date: &[Self], states: &mut H) { + //~^ ERROR: function parameter name was renamed from its trait default + for d in date { + d.hash(states); + } + } +} + +impl B { + fn totally_irrelevant(&self, right: bool) {} + fn some_fn(&self, other: impl MyTrait) {} +} + +fn main() {} diff --git a/tests/ui/renamed_function_params.stderr b/tests/ui/renamed_function_params.stderr new file mode 100644 index 00000000000..e42931a57b6 --- /dev/null +++ b/tests/ui/renamed_function_params.stderr @@ -0,0 +1,60 @@ +error: function parameter name was renamed from its trait default + --> $DIR/renamed_function_params.rs:21:13 + | +LL | fn from(b: B) -> Self { + | ^ + | + = 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 + | +LL | fn eq(&self, rhs: &Self) -> bool { + | ^^^ + | + = help: consider changing the name to: 'other' + +error: function parameter name was renamed from its trait default + --> $DIR/renamed_function_params.rs:31:18 + | +LL | fn ne(&self, rhs: &Self) -> bool { + | ^^^ + | + = help: consider changing the name to: 'other' + +error: function parameter name was renamed from its trait default + --> $DIR/renamed_function_params.rs:44:19 + | +LL | fn foo(&self, i_dont_wanna_use_your_name: u8) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider changing the name to: 'val' + +error: function parameter name was renamed from its trait default + --> $DIR/renamed_function_params.rs:51:31 + | +LL | fn hash(&self, states: &mut H) { + | ^^^^^^ + | + = help: consider changing the name to: 'state' + +error: function parameter name was renamed from its trait default + --> $DIR/renamed_function_params.rs:55: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 + | +LL | fn hash_slice(date: &[Self], states: &mut H) { + | ^^^^^^ + | + = help: consider changing the name to: 'state' + +error: aborting due to 7 previous errors + From 40ec76057295e0a517a888cb4347f86c32f7243a Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 28 Mar 2024 16:29:52 +0800 Subject: [PATCH 2/3] 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 | 106 +++++++++++------- tests/ui/renamed_function_params.rs | 57 ++++++++-- tests/ui/renamed_function_params.stderr | 60 +++++----- 4 files changed, 142 insertions(+), 85 deletions(-) diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 387e0c964cc..9a09dbaed4a 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 4c0d5aba112..cea76996f05 100644 --- a/clippy_lints/src/functions/renamed_function_params.rs +++ b/clippy_lints/src/functions/renamed_function_params.rs @@ -1,73 +1,93 @@ -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 { - 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 +/// 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.parent_hir_node(impl_item_id.into()); + if let Node::Item(item) = trait_node + && let ItemKind::Impl(impl_) = &item.kind { impl_.items.iter().find_map(|item| { if item.id.owner_id == impl_item_id { diff --git a/tests/ui/renamed_function_params.rs b/tests/ui/renamed_function_params.rs index ccc11968bcc..4f06ae706dd 100644 --- a/tests/ui/renamed_function_params.rs +++ b/tests/ui/renamed_function_params.rs @@ -1,5 +1,6 @@ +//@no-rustfix #![warn(clippy::renamed_function_params)] -#![allow(clippy::partialeq_ne_impl)] +#![allow(clippy::partialeq_ne_impl, clippy::to_string_trait_impl)] #![allow(unused)] use std::hash::{Hash, Hasher}; @@ -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 } } @@ -38,22 +39,24 @@ trait MyTrait { fn foo(&self, val: u8); fn bar(a: u8, b: u8); fn baz(self, _val: u8); + fn quz(&self, _: u8); } 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) {} + fn quz(&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 +68,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 e42931a57b6..7193541edb6 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 + --> tests/ui/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 + --> tests/ui/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 + --> tests/ui/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 + --> tests/ui/renamed_function_params.rs:46: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 + --> tests/ui/renamed_function_params.rs:54: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 + --> tests/ui/renamed_function_params.rs:58:30 | LL | fn hash_slice(date: &[Self], states: &mut H) { - | ^^^^ + | ^^^^ ^^^^^^ | - = help: consider changing the name to: 'data' +help: consider using the default names + | +LL | fn hash_slice(data: &[Self], state: &mut H) { + | ~~~~ ~~~~~ -error: function parameter name was renamed from its trait default - --> $DIR/renamed_function_params.rs:55:45 +error: renamed function parameter of trait impl + --> tests/ui/renamed_function_params.rs:79:18 | -LL | fn hash_slice(date: &[Self], states: &mut H) { - | ^^^^^^ - | - = 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 From 46659acdbde160324bae653e704203221df3b980 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Sun, 12 May 2024 22:13:17 +0800 Subject: [PATCH 3/3] add configuration to allow skipping on some certain traits & collect metadata --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 22 ++++++++ clippy_config/src/conf.rs | 23 +++++++++ clippy_lints/src/functions/mod.rs | 35 +++++++++---- .../src/functions/renamed_function_params.rs | 50 +++++++++++-------- clippy_lints/src/lib.rs | 2 + .../default/clippy.toml | 2 + .../extend/clippy.toml | 2 + .../renamed_function_params.default.stderr} | 26 ++++------ .../renamed_function_params.extend.stderr | 34 +++++++++++++ .../renamed_function_params.rs | 11 ++-- .../toml_unknown_key/conf_unknown_key.stderr | 3 ++ 12 files changed, 159 insertions(+), 52 deletions(-) create mode 100644 tests/ui-toml/renamed_function_params/default/clippy.toml create mode 100644 tests/ui-toml/renamed_function_params/extend/clippy.toml rename tests/{ui/renamed_function_params.stderr => ui-toml/renamed_function_params/renamed_function_params.default.stderr} (69%) create mode 100644 tests/ui-toml/renamed_function_params/renamed_function_params.extend.stderr rename tests/{ui => ui-toml/renamed_function_params}/renamed_function_params.rs (85%) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce2dda0cc2a..2abe48afc27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5942,6 +5942,7 @@ Released 2018-09-13 [`allow-one-hash-in-raw-strings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-one-hash-in-raw-strings [`allow-print-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-print-in-tests [`allow-private-module-inception`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-private-module-inception +[`allow-renamed-params-for`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-renamed-params-for [`allow-unwrap-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-unwrap-in-tests [`allow-useless-vec-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-useless-vec-in-tests [`allowed-dotfiles`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allowed-dotfiles diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index f6af9810ca1..bd8c6d14cd5 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -122,6 +122,28 @@ Whether to allow module inception if it's not public. * [`module_inception`](https://rust-lang.github.io/rust-clippy/master/index.html#module_inception) +## `allow-renamed-params-for` +List of trait paths to ignore when checking renamed function parameters. + +#### Example + +```toml +allow-renamed-params-for = [ "std::convert::From" ] +``` + +#### Noteworthy + +- By default, the following traits are ignored: `From`, `TryFrom`, `FromStr` +- `".."` can be used as part of the list to indicate that the configured values should be appended to the +default configuration of Clippy. By default, any configuration will replace the default value. + +**Default Value:** `["core::convert::From", "core::convert::TryFrom", "core::str::FromStr"]` + +--- +**Affected lints:** +* [`renamed_function_params`](https://rust-lang.github.io/rust-clippy/master/index.html#renamed_function_params) + + ## `allow-unwrap-in-tests` Whether `unwrap` should be allowed in test functions or `#[cfg(test)]` diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 5cfcbdb57d7..642abd4f3e7 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -40,6 +40,8 @@ const DEFAULT_DOC_VALID_IDENTS: &[&str] = &[ const DEFAULT_DISALLOWED_NAMES: &[&str] = &["foo", "baz", "quux"]; const DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS: &[&str] = &["i", "j", "x", "y", "z", "w", "n"]; const DEFAULT_ALLOWED_PREFIXES: &[&str] = &["to", "as", "into", "from", "try_into", "try_from"]; +const DEFAULT_ALLOWED_TRAITS_WITH_RENAMED_PARAMS: &[&str] = + &["core::convert::From", "core::convert::TryFrom", "core::str::FromStr"]; /// Conf with parse errors #[derive(Default)] @@ -613,6 +615,23 @@ define_Conf! { /// - Use `".."` as part of the list to indicate that the configured values should be appended to the /// default configuration of Clippy. By default, any configuration will replace the default value (allowed_prefixes: Vec = DEFAULT_ALLOWED_PREFIXES.iter().map(ToString::to_string).collect()), + /// Lint: RENAMED_FUNCTION_PARAMS. + /// + /// List of trait paths to ignore when checking renamed function parameters. + /// + /// #### Example + /// + /// ```toml + /// allow-renamed-params-for = [ "std::convert::From" ] + /// ``` + /// + /// #### Noteworthy + /// + /// - By default, the following traits are ignored: `From`, `TryFrom`, `FromStr` + /// - `".."` can be used as part of the list to indicate that the configured values should be appended to the + /// default configuration of Clippy. By default, any configuration will replace the default value. + (allow_renamed_params_for: Vec = + DEFAULT_ALLOWED_TRAITS_WITH_RENAMED_PARAMS.iter().map(ToString::to_string).collect()), } /// Search for the configuration file. @@ -674,6 +693,10 @@ fn deserialize(file: &SourceFile) -> TryConf { extend_vec_if_indicator_present(&mut conf.conf.doc_valid_idents, DEFAULT_DOC_VALID_IDENTS); extend_vec_if_indicator_present(&mut conf.conf.disallowed_names, DEFAULT_DISALLOWED_NAMES); extend_vec_if_indicator_present(&mut conf.conf.allowed_prefixes, DEFAULT_ALLOWED_PREFIXES); + extend_vec_if_indicator_present( + &mut conf.conf.allow_renamed_params_for, + DEFAULT_ALLOWED_TRAITS_WITH_RENAMED_PARAMS, + ); // TODO: THIS SHOULD BE TESTED, this comment will be gone soon if conf.conf.allowed_idents_below_min_chars.contains("..") { conf.conf diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 9a09dbaed4a..dfcaac9abef 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -7,11 +7,12 @@ mod result; mod too_many_arguments; mod too_many_lines; +use clippy_utils::def_path_def_ids; use rustc_hir as hir; use rustc_hir::intravisit; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; -use rustc_span::def_id::LocalDefId; +use rustc_span::def_id::{DefIdSet, LocalDefId}; use rustc_span::Span; declare_clippy_lint! { @@ -373,9 +374,9 @@ declare_clippy_lint! { /// ```rust /// struct A(u32); /// - /// impl From for String { - /// fn from(a: A) -> Self { - /// a.0.to_string() + /// impl PartialEq for A { + /// fn eq(&self, b: &Self) -> bool { + /// self.0 == b.0 /// } /// } /// ``` @@ -383,9 +384,9 @@ declare_clippy_lint! { /// ```rust /// struct A(u32); /// - /// impl From for String { - /// fn from(value: A) -> Self { - /// value.0.to_string() + /// impl PartialEq for A { + /// fn eq(&self, other: &Self) -> bool { + /// self.0 == other.0 /// } /// } /// ``` @@ -395,13 +396,16 @@ declare_clippy_lint! { "renamed function parameters in trait implementation" } -#[derive(Copy, Clone)] -#[allow(clippy::struct_field_names)] +#[derive(Clone)] pub struct Functions { too_many_arguments_threshold: u64, too_many_lines_threshold: u64, large_error_threshold: u64, avoid_breaking_exported_api: bool, + allow_renamed_params_for: Vec, + /// A set of resolved `def_id` of traits that are configured to allow + /// function params renaming. + trait_ids: DefIdSet, } impl Functions { @@ -410,12 +414,15 @@ impl Functions { too_many_lines_threshold: u64, large_error_threshold: u64, avoid_breaking_exported_api: bool, + allow_renamed_params_for: Vec, ) -> Self { Self { too_many_arguments_threshold, too_many_lines_threshold, large_error_threshold, avoid_breaking_exported_api, + allow_renamed_params_for, + trait_ids: DefIdSet::default(), } } } @@ -461,7 +468,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions { 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); - renamed_function_params::check_impl_item(cx, item); + renamed_function_params::check_impl_item(cx, item, &self.trait_ids); } fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { @@ -471,4 +478,12 @@ impl<'tcx> LateLintPass<'tcx> for Functions { result::check_trait_item(cx, item, self.large_error_threshold); impl_trait_in_params::check_trait_item(cx, item, self.avoid_breaking_exported_api); } + + fn check_crate(&mut self, cx: &LateContext<'tcx>) { + for path in &self.allow_renamed_params_for { + let path_segments: Vec<&str> = path.split("::").collect(); + let ids = def_path_def_ids(cx, &path_segments); + self.trait_ids.extend(ids); + } + } } diff --git a/clippy_lints/src/functions/renamed_function_params.rs b/clippy_lints/src/functions/renamed_function_params.rs index cea76996f05..c7de0385c02 100644 --- a/clippy_lints/src/functions/renamed_function_params.rs +++ b/clippy_lints/src/functions/renamed_function_params.rs @@ -1,18 +1,26 @@ use clippy_utils::diagnostics::span_lint_and_then; use rustc_errors::{Applicability, MultiSpan}; -use rustc_hir::def_id::DefId; +use rustc_hir::def_id::{DefId, DefIdSet}; use rustc_hir::hir_id::OwnerId; -use rustc_hir::{ImplItem, ImplItemKind, ItemKind, Node}; +use rustc_hir::{Impl, ImplItem, ImplItemKind, ImplItemRef, ItemKind, Node, TraitRef}; use rustc_lint::LateContext; 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<'_>) { +pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>, ignored_traits: &DefIdSet) { 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 parent_node = cx.tcx.parent_hir_node(item.hir_id()) + && let Node::Item(parent_item) = parent_node + && let ItemKind::Impl(Impl { + items, + of_trait: Some(trait_ref), + .. + }) = &parent_item.kind + && let Some(did) = trait_item_def_id_of_impl(items, item.owner_id) + && !is_from_ignored_trait(trait_ref, ignored_traits) { 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(); @@ -25,7 +33,7 @@ pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>) { cx, RENAMED_FUNCTION_PARAMS, multi_span, - &format!("renamed function parameter{plural} of trait impl"), + format!("renamed function parameter{plural} of trait impl"), |diag| { diag.multipart_suggestion( format!("consider using the default name{plural}"), @@ -83,20 +91,20 @@ fn is_unused_or_empty_symbol(symbol: Symbol) -> bool { symbol.is_empty() || symbol == kw::Underscore || symbol.as_str().starts_with('_') } -/// 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.parent_hir_node(impl_item_id.into()); - if let Node::Item(item) = trait_node - && let ItemKind::Impl(impl_) = &item.kind - { - impl_.items.iter().find_map(|item| { - if item.id.owner_id == impl_item_id { - item.trait_item_def_id - } else { - None - } - }) - } else { - None - } +/// Get the [`trait_item_def_id`](ImplItemRef::trait_item_def_id) of a relevant impl item. +fn trait_item_def_id_of_impl(items: &[ImplItemRef], target: OwnerId) -> Option { + items.iter().find_map(|item| { + if item.id.owner_id == target { + item.trait_item_def_id + } else { + None + } + }) +} + +fn is_from_ignored_trait(of_trait: &TraitRef<'_>, ignored_traits: &DefIdSet) -> bool { + let Some(trait_did) = of_trait.trait_def_id() else { + return false; + }; + ignored_traits.contains(&trait_did) } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 34dd4785551..c9bd40a711a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -595,6 +595,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { ref allowed_duplicate_crates, allow_comparison_to_zero, ref allowed_prefixes, + ref allow_renamed_params_for, blacklisted_names: _, cyclomatic_complexity_threshold: _, @@ -788,6 +789,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { too_many_lines_threshold, large_error_threshold, avoid_breaking_exported_api, + allow_renamed_params_for.clone(), )) }); store.register_late_pass(move |_| Box::new(doc::Documentation::new(doc_valid_idents, check_private_items))); diff --git a/tests/ui-toml/renamed_function_params/default/clippy.toml b/tests/ui-toml/renamed_function_params/default/clippy.toml new file mode 100644 index 00000000000..5381e70a939 --- /dev/null +++ b/tests/ui-toml/renamed_function_params/default/clippy.toml @@ -0,0 +1,2 @@ +# Ignore `From`, `TryFrom`, `FromStr` by default +# allow-renamed-params-for = [] diff --git a/tests/ui-toml/renamed_function_params/extend/clippy.toml b/tests/ui-toml/renamed_function_params/extend/clippy.toml new file mode 100644 index 00000000000..9b3853e7696 --- /dev/null +++ b/tests/ui-toml/renamed_function_params/extend/clippy.toml @@ -0,0 +1,2 @@ +# Ignore `From`, `TryFrom`, `FromStr` by default +allow-renamed-params-for = [ "..", "std::ops::Add", "renamed_function_params::MyTrait" ] diff --git a/tests/ui/renamed_function_params.stderr b/tests/ui-toml/renamed_function_params/renamed_function_params.default.stderr similarity index 69% rename from tests/ui/renamed_function_params.stderr rename to tests/ui-toml/renamed_function_params/renamed_function_params.default.stderr index 7193541edb6..2d700f60759 100644 --- a/tests/ui/renamed_function_params.stderr +++ b/tests/ui-toml/renamed_function_params/renamed_function_params.default.stderr @@ -1,38 +1,32 @@ error: renamed function parameter of trait impl - --> tests/ui/renamed_function_params.rs:22:13 + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:30:18 | -LL | fn from(b: B) -> Self { - | ^ help: consider using the default name: `value` +LL | fn eq(&self, rhs: &Self) -> bool { + | ^^^ help: consider using the default name: `other` | = note: `-D clippy::renamed-function-params` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::renamed_function_params)]` error: renamed function parameter of trait impl - --> tests/ui/renamed_function_params.rs:28:18 - | -LL | fn eq(&self, rhs: &Self) -> bool { - | ^^^ help: consider using the default name: `other` - -error: renamed function parameter of trait impl - --> tests/ui/renamed_function_params.rs:32:18 + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:34:18 | LL | fn ne(&self, rhs: &Self) -> bool { | ^^^ help: consider using the default name: `other` error: renamed function parameter of trait impl - --> tests/ui/renamed_function_params.rs:46:19 + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:48:19 | -LL | fn foo(&self, i_dont_wanna_use_your_name: u8) {} +LL | fn foo(&self, i_dont_wanna_use_your_name: u8) {} // only lint in `extend` | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the default name: `val` error: renamed function parameter of trait impl - --> tests/ui/renamed_function_params.rs:54:31 + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:55:31 | LL | fn hash(&self, states: &mut H) { | ^^^^^^ help: consider using the default name: `state` error: renamed function parameters of trait impl - --> tests/ui/renamed_function_params.rs:58:30 + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:59:30 | LL | fn hash_slice(date: &[Self], states: &mut H) { | ^^^^ ^^^^^^ @@ -43,10 +37,10 @@ LL | fn hash_slice(data: &[Self], state: &mut H) { | ~~~~ ~~~~~ error: renamed function parameter of trait impl - --> tests/ui/renamed_function_params.rs:79:18 + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:80:18 | LL | fn add(self, b: B) -> C { | ^ help: consider using the default name: `rhs` -error: aborting due to 7 previous errors +error: aborting due to 6 previous errors diff --git a/tests/ui-toml/renamed_function_params/renamed_function_params.extend.stderr b/tests/ui-toml/renamed_function_params/renamed_function_params.extend.stderr new file mode 100644 index 00000000000..e57554fa613 --- /dev/null +++ b/tests/ui-toml/renamed_function_params/renamed_function_params.extend.stderr @@ -0,0 +1,34 @@ +error: renamed function parameter of trait impl + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:30:18 + | +LL | fn eq(&self, rhs: &Self) -> bool { + | ^^^ help: consider using the default name: `other` + | + = note: `-D clippy::renamed-function-params` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::renamed_function_params)]` + +error: renamed function parameter of trait impl + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:34:18 + | +LL | fn ne(&self, rhs: &Self) -> bool { + | ^^^ help: consider using the default name: `other` + +error: renamed function parameter of trait impl + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:55:31 + | +LL | fn hash(&self, states: &mut H) { + | ^^^^^^ help: consider using the default name: `state` + +error: renamed function parameters of trait impl + --> tests/ui-toml/renamed_function_params/renamed_function_params.rs:59:30 + | +LL | fn hash_slice(date: &[Self], states: &mut H) { + | ^^^^ ^^^^^^ + | +help: consider using the default names + | +LL | fn hash_slice(data: &[Self], state: &mut H) { + | ~~~~ ~~~~~ + +error: aborting due to 4 previous errors + diff --git a/tests/ui/renamed_function_params.rs b/tests/ui-toml/renamed_function_params/renamed_function_params.rs similarity index 85% rename from tests/ui/renamed_function_params.rs rename to tests/ui-toml/renamed_function_params/renamed_function_params.rs index 4f06ae706dd..f3eb910abbd 100644 --- a/tests/ui/renamed_function_params.rs +++ b/tests/ui-toml/renamed_function_params/renamed_function_params.rs @@ -1,4 +1,7 @@ //@no-rustfix +//@revisions: default extend +//@[default] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/renamed_function_params/default +//@[extend] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/renamed_function_params/extend #![warn(clippy::renamed_function_params)] #![allow(clippy::partialeq_ne_impl, clippy::to_string_trait_impl)] #![allow(unused)] @@ -18,9 +21,8 @@ impl ToString for A { } struct B(u32); -impl From for String { +impl std::convert::From for String { fn from(b: B) -> Self { - //~^ ERROR: renamed function parameter of trait impl b.0.to_string() } } @@ -43,8 +45,7 @@ trait MyTrait { } impl MyTrait for B { - fn foo(&self, i_dont_wanna_use_your_name: u8) {} - //~^ ERROR: renamed function parameter of trait impl + fn foo(&self, i_dont_wanna_use_your_name: u8) {} // only lint in `extend` fn bar(_a: u8, _: u8) {} fn baz(self, val: u8) {} fn quz(&self, val: u8) {} @@ -77,7 +78,7 @@ enum C { impl std::ops::Add for C { type Output = C; fn add(self, b: B) -> C { - //~^ ERROR: renamed function parameter of trait impl + // only lint in `extend` C::B(b.0) } } diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 722e9b3bc8d..e1ba60911fe 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -10,6 +10,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect allow-one-hash-in-raw-strings allow-print-in-tests allow-private-module-inception + allow-renamed-params-for allow-unwrap-in-tests allow-useless-vec-in-tests allowed-dotfiles @@ -91,6 +92,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect allow-one-hash-in-raw-strings allow-print-in-tests allow-private-module-inception + allow-renamed-params-for allow-unwrap-in-tests allow-useless-vec-in-tests allowed-dotfiles @@ -172,6 +174,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni allow-one-hash-in-raw-strings allow-print-in-tests allow-private-module-inception + allow-renamed-params-for allow-unwrap-in-tests allow-useless-vec-in-tests allowed-dotfiles