Auto merge of #11540 - J-ZhengLi:issue11443, r=xFrednet

add new lint that disallow renaming parameters in trait functions

fixes: #11443
fixes: #486

changelog: add new lint [`renamed_function_params`]

Note that the lint name is not final, because I have a bad reputation in naming things, and I don't trust myself.

CHANGELOG.md

@@ -5703,6 +5703,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_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<String> = 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<String> =
+        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

clippy_lints/src/declared_lints.rs

@@ -206,6 +206,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,15 +2,17 @@ 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;
 
+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! {
@@ -359,13 +361,51 @@ declare_clippy_lint! {
     "`impl Trait` is used in the function's parameters"
 }
 
-#[derive(Copy, Clone)]
-#[allow(clippy::struct_field_names)]
+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
+    /// struct A(u32);
+    ///
+    /// impl PartialEq for A {
+    ///     fn eq(&self, b: &Self) -> bool {
+    ///         self.0 == b.0
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// struct A(u32);
+    ///
+    /// impl PartialEq for A {
+    ///     fn eq(&self, other: &Self) -> bool {
+    ///         self.0 == other.0
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.74.0"]
+    pub RENAMED_FUNCTION_PARAMS,
+    restriction,
+    "renamed function parameters in trait implementation"
+}
+
+#[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<String>,
+    /// A set of resolved `def_id` of traits that are configured to allow
+    /// function params renaming.
+    trait_ids: DefIdSet,
 }
 
 impl Functions {
@@ -374,12 +414,15 @@ impl Functions {
         too_many_lines_threshold: u64,
         large_error_threshold: u64,
         avoid_breaking_exported_api: bool,
+        allow_renamed_params_for: Vec<String>,
     ) -> 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(),
         }
     }
 }
@@ -395,6 +438,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 +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, &self.trait_ids);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
@@ -433,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);
+        }
+    }
 }

clippy_lints/src/functions/renamed_function_params.rs

@@ -0,0 +1,110 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use rustc_errors::{Applicability, MultiSpan};
+use rustc_hir::def_id::{DefId, DefIdSet};
+use rustc_hir::hir_id::OwnerId;
+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<'_>, ignored_traits: &DefIdSet) {
+    if !item.span.from_expansion()
+        && let ImplItemKind::Fn(_, body_id) = item.kind
+        && 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();
+
+        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,
+                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 RenamedFnArgs(Vec<(Span, String)>);
+
+impl RenamedFnArgs {
+    /// Comparing between an iterator of default names and one with current names,
+    /// then collect the ones that got renamed.
+    fn new<I, T>(default_names: &mut I, current_names: &mut T) -> Self
+    where
+        I: Iterator<Item = Ident>,
+        T: Iterator<Item = Ident>,
+    {
+        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::<Vec<Span>>()
+            .into()
+    }
+}
+
+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('_')
+}
+
+/// 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<DefId> {
+    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)
+}

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)));

tests/ui-toml/renamed_function_params/default/clippy.toml

@@ -0,0 +1,2 @@
+# Ignore `From`, `TryFrom`, `FromStr` by default
+# allow-renamed-params-for = []

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" ]

tests/ui-toml/renamed_function_params/renamed_function_params.default.stderr

@@ -0,0 +1,46 @@
+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:48:19
+   |
+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-toml/renamed_function_params/renamed_function_params.rs:55:31
+   |
+LL |     fn hash<H: Hasher>(&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<H: Hasher>(date: &[Self], states: &mut H) {
+   |                              ^^^^           ^^^^^^
+   |
+help: consider using the default names
+   |
+LL |     fn hash_slice<H: Hasher>(data: &[Self], state: &mut H) {
+   |                              ~~~~           ~~~~~
+
+error: renamed function parameter of trait impl
+  --> 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 6 previous errors
+

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<H: Hasher>(&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<H: Hasher>(date: &[Self], states: &mut H) {
+   |                              ^^^^           ^^^^^^
+   |
+help: consider using the default names
+   |
+LL |     fn hash_slice<H: Hasher>(data: &[Self], state: &mut H) {
+   |                              ~~~~           ~~~~~
+
+error: aborting due to 4 previous errors
+