add new lint that disallow renaming parameters in trait functions

Author: J-ZhengLi

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

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,

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<A> for String {
+    ///     fn from(a: A) -> Self {
+    ///         a.0.to_string()
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// impl From<A> 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<'_>) {

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<I, T>(default_names: &mut I, current_names: &mut T) -> Vec<RenamedParam>
+where
+    I: Iterator<Item = Ident>,
+    T: Iterator<Item = Ident>,
+{
+    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<DefId> {
+    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
+    }
+}

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<A> 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<B> 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<H: Hasher>(&self, states: &mut H) {
+        //~^ ERROR: function parameter name was renamed from its trait default
+        self.0.hash(states);
+    }
+    fn hash_slice<H: Hasher>(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() {}

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<H: Hasher>(&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<H: Hasher>(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<H: Hasher>(date: &[Self], states: &mut H) {
+   |                                             ^^^^^^
+   |
+   = help: consider changing the name to: 'state'
+
+error: aborting due to 7 previous errors
+