Skip to content

Commit

Permalink
fix doc test failure;
Browse files Browse the repository at this point in the history 
apply review suggestions by @Centri3:
use multi suggestion;
change output message format;
add macro expansion check & tests;
  • Loading branch information
@J-ZhengLi
J-ZhengLi committed Apr 18, 2024
1 parent ce48fab commit 830fd94
Show file tree
Hide file tree
Showing 4 changed files with 142 additions and 85 deletions.
4 changes: 4 additions & 0 deletions clippy_lints/src/functions/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand Down
106 changes: 63 additions & 43 deletions clippy_lints/src/functions/renamed_function_params.rs
View file
Original file line number Diff line number Diff line change
@@ -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<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> {
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<DefId> {
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 {
Expand Down
57 changes: 49 additions & 8 deletions tests/ui/renamed_function_params.rs
View file
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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
}
}
Expand All @@ -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<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);
}
Expand All @@ -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<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() {}
60 changes: 26 additions & 34 deletions tests/ui/renamed_function_params.stderr
View file
Original file line number Diff line number Diff line change
@@ -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<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
--> tests/ui/renamed_function_params.rs:58: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
--> tests/ui/renamed_function_params.rs:79: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

0 comments on commit 830fd94

Please sign in to comment.