mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
bors 2024-05-12 14:21:22 +00:00
commit 7cfb9a0d6f
13 changed files with 413 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
CHANGELOG.md
View File

@ -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
@ -5942,6 +5943,7 @@ Released 2018-09-13
[`allow-one-hash-in-raw-strings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-one-hash-in-raw-strings
[`allow-print-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-print-in-tests
[`allow-private-module-inception`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-private-module-inception
[`allow-renamed-params-for`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-renamed-params-for
[`allow-unwrap-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-unwrap-in-tests
[`allow-useless-vec-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-useless-vec-in-tests
[`allowed-dotfiles`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allowed-dotfiles

22
book/src/lint_configuration.md
View File

@ -122,6 +122,28 @@ Whether to allow module inception if it's not public.
* [`module_inception`](https://rust-lang.github.io/rust-clippy/master/index.html#module_inception)
## `allow-renamed-params-for`
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.
**Default Value:** `["core::convert::From", "core::convert::TryFrom", "core::str::FromStr"]`
---
**Affected lints:**
* [`renamed_function_params`](https://rust-lang.github.io/rust-clippy/master/index.html#renamed_function_params)
## `allow-unwrap-in-tests`
Whether `unwrap` should be allowed in test functions or `#[cfg(test)]`

23
clippy_config/src/conf.rs
View File

@ -40,6 +40,8 @@
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 @@ pub fn get_configuration_metadata() -> Vec<ClippyConfiguration> {
/// - 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

1
clippy_lints/src/declared_lints.rs
View File

@ -206,6 +206,7 @@
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,

59
clippy_lints/src/functions/mod.rs
View File

@ -2,15 +2,17 @@
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 @@
"`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 @@ pub fn new(
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 @@ pub fn new(
RESULT_LARGE_ERR,
MISNAMED_GETTERS,
IMPL_TRAIT_IN_PARAMS,
RENAMED_FUNCTION_PARAMS,
]);
impl<'tcx> LateLintPass<'tcx> for Functions {
@ -424,6 +468,7 @@ fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<
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 @@ fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitIte
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);
}
}
}

110
clippy_lints/src/functions/renamed_function_params.rs Normal file
View File

@ -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)
}

2
clippy_lints/src/lib.rs
View File

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

2
tests/ui-toml/renamed_function_params/default/clippy.toml Normal file
View File

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

2
tests/ui-toml/renamed_function_params/extend/clippy.toml Normal file
View File

@ -0,0 +1,2 @@
# Ignore `From`, `TryFrom`, `FromStr` by default
allow-renamed-params-for = [ "..", "std::ops::Add", "renamed_function_params::MyTrait" ]

46
tests/ui-toml/renamed_function_params/renamed_function_params.default.stderr Normal file
View File

@ -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

34
tests/ui-toml/renamed_function_params/renamed_function_params.extend.stderr Normal file
View File

@ -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

110
tests/ui-toml/renamed_function_params/renamed_function_params.rs Normal file
View File

@ -0,0 +1,110 @@
//@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)]
use std::hash::{Hash, Hasher};
struct A;
impl From<A> for String {
fn from(_value: A) -> Self {
String::new()
}
}
impl ToString for A {
fn to_string(&self) -> String {
String::new()
}
}
struct B(u32);
impl std::convert::From<B> for String {
fn from(b: B) -> Self {
b.0.to_string()
}
}
impl PartialEq for B {
fn eq(&self, rhs: &Self) -> bool {
//~^ ERROR: renamed function parameter of trait impl
self.0 == rhs.0
}
fn ne(&self, rhs: &Self) -> bool {
//~^ ERROR: renamed function parameter of trait impl
self.0 != rhs.0
}
}
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) {} // only lint in `extend`
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: renamed function parameter of trait impl
self.0.hash(states);
}
fn hash_slice<H: Hasher>(date: &[Self], states: &mut H) {
//~^ ERROR: renamed function parameters of trait impl
for d in date {
d.hash(states);
}
}
}
impl B {
fn totally_irrelevant(&self, right: bool) {}
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 {
// only lint in `extend`
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() {}

3
tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
View File

@ -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
@ -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
@ -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