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 May 7, 2024
1 parent 40ec760 commit 2358189
Show file tree
Hide file tree
Showing 10 changed files with 136 additions and 52 deletions.
23 changes: 23 additions & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
35 changes: 25 additions & 10 deletions clippy_lints/src/functions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ 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! {
Expand Down Expand Up @@ -373,19 +374,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 +396,16 @@ 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,
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 {
Expand All @@ -410,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(),
}
}
}
Expand Down Expand Up @@ -461,7 +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);
renamed_function_params::check_impl_item(cx, item, &self.trait_ids);
}

fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
Expand All @@ -471,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);
}
}
}
50 changes: 29 additions & 21 deletions clippy_lints/src/functions/renamed_function_params.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
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 super::RENAMED_FUNCTION_PARAMS;

pub(super) fn check_impl_item(cx: &LateContext<'_>, item: &ImplItem<'_>) {
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 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(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 +33,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 +91,20 @@ 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(of_trait: &TraitRef<'_>, ignored_traits: &DefIdSet) -> bool {
let Some(trait_did) = of_trait.trait_def_id() else {
return false;
};
ignored_traits.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 allow_renamed_params_for,

blacklisted_names: _,
cyclomatic_complexity_threshold: _,
Expand Down Expand Up @@ -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)));
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
# allow-renamed-params-for = []
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
allow-renamed-params-for = [ "..", "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 @@ -10,6 +10,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
allow-one-hash-in-raw-strings
allow-print-in-tests
allow-private-module-inception
allow-renamed-params-for
allow-unwrap-in-tests
allow-useless-vec-in-tests
allowed-dotfiles
Expand Down Expand Up @@ -91,6 +92,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
allow-one-hash-in-raw-strings
allow-print-in-tests
allow-private-module-inception
allow-renamed-params-for
allow-unwrap-in-tests
allow-useless-vec-in-tests
allowed-dotfiles
Expand Down Expand Up @@ -172,6 +174,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni
allow-one-hash-in-raw-strings
allow-print-in-tests
allow-private-module-inception
allow-renamed-params-for
allow-unwrap-in-tests
allow-useless-vec-in-tests
allowed-dotfiles
Expand Down

0 comments on commit 2358189

Please sign in to comment.