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 @@ 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 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 @@ "`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 @@ pub fn new( RESULT_LARGE_ERR, MISNAMED_GETTERS, IMPL_TRAIT_IN_PARAMS, + RENAMED_FUNCTION_PARAMS, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -424,6 +457,7 @@ 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); + 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 +