Skip to content

Commit

Permalink
add configuration to allow skipping on some certain traits
Browse files Browse the repository at this point in the history
  • Loading branch information
J-ZhengLi committed Apr 18, 2024
1 parent 830fd94 commit bdf9ebb
Show file tree
Hide file tree
Showing 10 changed files with 130 additions and 51 deletions.
18 changes: 18 additions & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ 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_IGNORED_TRAITS: &[&str] = &["core::convert::From", "core::convert::TryFrom", "core::str::FromStr"];

/// Conf with parse errors
#[derive(Default)]
Expand Down Expand Up @@ -610,6 +611,22 @@ 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
/// ignored-traits = [ "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.
(ignored_traits: Vec<String> = DEFAULT_IGNORED_TRAITS.iter().map(ToString::to_string).collect()),
}

/// Search for the configuration file.
Expand Down Expand Up @@ -671,6 +688,7 @@ 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.ignored_traits, DEFAULT_IGNORED_TRAITS);
// TODO: THIS SHOULD BE TESTED, this comment will be gone soon
if conf.conf.allowed_idents_below_min_chars.contains("..") {
conf.conf
Expand Down
20 changes: 11 additions & 9 deletions clippy_lints/src/functions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,19 +373,19 @@ declare_clippy_lint! {
/// ```rust
/// struct A(u32);
///
/// impl From<A> for String {
/// fn from(a: A) -> Self {
/// a.0.to_string()
/// impl PartialEq for A {
/// fn eq(&self, b: &Self) -> bool {
/// self.0 == b.0
/// }
/// }
/// ```
/// Use instead:
/// ```rust
/// struct A(u32);
///
/// impl From<A> for String {
/// fn from(value: A) -> Self {
/// value.0.to_string()
/// impl PartialEq for A {
/// fn eq(&self, other: &Self) -> bool {
/// self.0 == other.0
/// }
/// }
/// ```
Expand All @@ -395,13 +395,13 @@ declare_clippy_lint! {
"renamed function parameters in trait implementation"
}

#[derive(Copy, Clone)]
#[allow(clippy::struct_field_names)]
#[derive(Clone)]
pub struct Functions {
too_many_arguments_threshold: u64,
too_many_lines_threshold: u64,
large_error_threshold: u64,
avoid_breaking_exported_api: bool,
ignored_traits: Vec<String>,
}

impl Functions {
Expand All @@ -410,12 +410,14 @@ impl Functions {
too_many_lines_threshold: u64,
large_error_threshold: u64,
avoid_breaking_exported_api: bool,
ignored_traits: Vec<String>,
) -> Self {
Self {
too_many_arguments_threshold,
too_many_lines_threshold,
large_error_threshold,
avoid_breaking_exported_api,
ignored_traits,
}
}
}
Expand Down Expand Up @@ -461,7 +463,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);
renamed_function_params::check_impl_item(cx, item, &self.ignored_traits);
}

fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
Expand Down
63 changes: 42 additions & 21 deletions clippy_lints/src/functions/renamed_function_params.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,30 @@
use clippy_utils::def_path_def_ids;
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_errors::{Applicability, MultiSpan};
use rustc_hir::def_id::DefId;
use rustc_hir::def_id::{DefId, DefIdSet};
use rustc_hir::hir_id::OwnerId;
use rustc_hir::{ImplItem, ImplItemKind, ItemKind, Node};
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 std::sync::OnceLock;

use super::RENAMED_FUNCTION_PARAMS;

pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>) {
static IGNORED_TRAIT_IDS: OnceLock<DefIdSet> = OnceLock::new();

pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>, ignored_traits: &[String]) {
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 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(cx, 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();
Expand All @@ -25,7 +37,7 @@ pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>) {
cx,
RENAMED_FUNCTION_PARAMS,
multi_span,
&format!("renamed function parameter{plural} of trait impl"),
format!("renamed function parameter{plural} of trait impl"),
|diag| {
diag.multipart_suggestion(
format!("consider using the default name{plural}"),
Expand Down Expand Up @@ -83,20 +95,29 @@ fn is_unused_or_empty_symbol(symbol: Symbol) -> bool {
symbol.is_empty() || symbol == kw::Underscore || symbol.as_str().starts_with('_')
}

/// 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 {
item.trait_item_def_id
} else {
None
}
})
} else {
None
}
/// 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(cx: &LateContext<'_>, of_trait: &TraitRef<'_>, ignored_traits: &[String]) -> bool {
let Some(trait_did) = of_trait.trait_def_id() else {
return false;
};
let ignored_trait_ids = IGNORED_TRAIT_IDS.get_or_init(|| {
let mut id_set = DefIdSet::new();
for path in ignored_traits {
let path_segments: Vec<&str> = path.split("::").collect();
let ids = def_path_def_ids(cx, &path_segments);
id_set.extend(ids);
}
id_set
});
ignored_trait_ids.contains(&trait_did)
}
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 ignored_traits,

blacklisted_names: _,
cyclomatic_complexity_threshold: _,
Expand Down Expand Up @@ -777,6 +778,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
too_many_lines_threshold,
large_error_threshold,
avoid_breaking_exported_api,
ignored_traits.clone(),
))
});
store.register_late_pass(move |_| Box::new(doc::Documentation::new(doc_valid_idents, check_private_items)));
Expand Down
2 changes: 2 additions & 0 deletions tests/ui-toml/renamed_function_params/default/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Ignore `From`, `TryFrom`, `FromStr` by default
# ignored-traits = []
2 changes: 2 additions & 0 deletions tests/ui-toml/renamed_function_params/extend/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Ignore `From`, `TryFrom`, `FromStr` by default
ignored-traits = [ "..", "std::ops::Add", "renamed_function_params::MyTrait" ]
Original file line number Diff line number Diff line change
@@ -1,38 +1,32 @@
error: renamed function parameter of trait impl
--> tests/ui/renamed_function_params.rs:22:13
--> tests/ui-toml/renamed_function_params/renamed_function_params.rs:30:18
|
LL | fn from(b: B) -> Self {
| ^ help: consider using the default name: `value`
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/renamed_function_params.rs:28:18
|
LL | fn eq(&self, rhs: &Self) -> bool {
| ^^^ help: consider using the default name: `other`

error: renamed function parameter of trait impl
--> tests/ui/renamed_function_params.rs:32:18
--> 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/renamed_function_params.rs:46:19
--> tests/ui-toml/renamed_function_params/renamed_function_params.rs:48:19
|
LL | fn foo(&self, i_dont_wanna_use_your_name: u8) {}
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/renamed_function_params.rs:54:31
--> 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/renamed_function_params.rs:58:30
--> tests/ui-toml/renamed_function_params/renamed_function_params.rs:59:30
|
LL | fn hash_slice<H: Hasher>(date: &[Self], states: &mut H) {
| ^^^^ ^^^^^^
Expand All @@ -43,10 +37,10 @@ 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
--> 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 7 previous errors
error: aborting due to 6 previous errors

Original file line number Diff line number Diff line change
@@ -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

Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@no-rustfix
//@revisions: default extend
//@[default] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/renamed_function_params/default
//@[extend] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/renamed_function_params/extend
#![warn(clippy::renamed_function_params)]
#![allow(clippy::partialeq_ne_impl, clippy::to_string_trait_impl)]
#![allow(unused)]
Expand All @@ -18,9 +21,8 @@ impl ToString for A {
}

struct B(u32);
impl From<B> for String {
impl std::convert::From<B> for String {
fn from(b: B) -> Self {
//~^ ERROR: renamed function parameter of trait impl
b.0.to_string()
}
}
Expand All @@ -43,8 +45,7 @@ trait MyTrait {
}

impl MyTrait for B {
fn foo(&self, i_dont_wanna_use_your_name: u8) {}
//~^ ERROR: renamed function parameter of trait impl
fn foo(&self, i_dont_wanna_use_your_name: u8) {} // only lint in `extend`
fn bar(_a: u8, _: u8) {}
fn baz(self, val: u8) {}
fn quz(&self, val: u8) {}
Expand Down Expand Up @@ -77,7 +78,7 @@ enum C {
impl std::ops::Add<B> for C {
type Output = C;
fn add(self, b: B) -> C {
//~^ ERROR: renamed function parameter of trait impl
// only lint in `extend`
C::B(b.0)
}
}
Expand Down
3 changes: 3 additions & 0 deletions tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
excessive-nesting-threshold
future-size-threshold
ignore-interior-mutability
ignored-traits
large-error-threshold
literal-representation-threshold
matches-for-let-else
Expand Down Expand Up @@ -121,6 +122,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
excessive-nesting-threshold
future-size-threshold
ignore-interior-mutability
ignored-traits
large-error-threshold
literal-representation-threshold
matches-for-let-else
Expand Down Expand Up @@ -201,6 +203,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni
excessive-nesting-threshold
future-size-threshold
ignore-interior-mutability
ignored-traits
large-error-threshold
literal-representation-threshold
matches-for-let-else
Expand Down

0 comments on commit bdf9ebb

Please sign in to comment.