fix doc test failure;

apply review suggestions by @Centri3: use multi suggestion; change output message format; add macro expansion check & tests;
rust-lang · Sep 25, 2023 · 1d4f27b · 1d4f27b
1 parent 959eb3c
commit 1d4f27b
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 81 deletions.
diff --git 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<A> 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<A> 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
@@ -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<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,
-            });
+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()
     }
-    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<DefId> {
+/// 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<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

diff --git 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<B> 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<H: Hasher>(&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<H: Hasher>(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<B> for C {
+    type Output = C;
+    fn add(self, b: B) -> C {
+        //~^ ERROR: renamed function parameter of trait impl
+        C::B(b.0)
+    }
+}
+
+impl From<A> 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
@@ -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<H: Hasher>(&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<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
+help: consider using the default names
    |
-LL |     fn hash_slice<H: Hasher>(date: &[Self], states: &mut H) {
-   |                                             ^^^^^^
+LL |     fn hash_slice<H: Hasher>(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