add new lint that disallow renaming parameters in trait functions #11540

J-ZhengLi · 2023-09-20T11:12:10Z

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.

rustbot · 2023-09-20T11:12:15Z

(rustbot has picked a reviewer for you, use r? to override)

xFrednet · 2023-09-20T11:20:05Z

I haven't looked at the implementation, so this might already be implemented: I would recommend that it allows removing leading underscores from the names. Here is an example, of what I mean:

trait Foo {
    fn foo(_param: u32) {}
}

struct FooImpl;

impl Foo for FooImpl {
    //     vvvvv This should be allowed
    fn foo(param: u32) {
        println!("{param}");
    }
}

Potentially, it could also allow adding an underscore, but here I'm uncertain if that's a good idea. Still, this would be the example for that case:

trait Bar {
    fn bar(param: u32) {
        println!("{param}");
    }
}

struct BarImpl;

impl Bar for BarImpl {
    //     vvvvvv Maybe this could be allowed?
    fn bar(_param: u32) {}
}

J-ZhengLi · 2023-09-20T13:45:22Z

I haven't looked at the implementation, so this might already be implemented: I would recommend that it allows removing leading underscores from the names. Here is an example, of what I mean:
trait Foo {
    fn foo(_param: u32) {}
}

struct FooImpl;

impl Foo for FooImpl {
    //     vvvvv This should be allowed
    fn foo(param: u32) {
        println!("{param}");
    }
}
Potentially, it could also allow adding an underscore, but here I'm uncertain if that's a good idea. Still, this would be the example for that case:
trait Bar {
    fn bar(param: u32) {
        println!("{param}");
    }
}

struct BarImpl;

impl Bar for BarImpl {
    //     vvvvvv Maybe this could be allowed?
    fn bar(_param: u32) {}
}

It should already be implemented :D
in fact any name starts with underscore will be skipped, that includes names in trait def and trait impl

Centri3

Just some minor nits.

I still think we should restrict this to some traits (by default) using an attribute like clippy::cant_rename (bikeshedding pending!). We can add this to many libstd traits and crate authors can opt into this if they really want. Consistency isn't as big of a deal when the trait is rarely used already.

clippy_lints/src/functions/renamed_function_params.rs

bors · 2023-10-08T22:19:47Z

☔ The latest upstream changes (presumably #11550) made this pull request unmergeable. Please resolve the merge conflicts.

Jarcho · 2023-10-30T18:33:27Z

Since you've already looked at this. r? @Centri3

J-ZhengLi · 2024-01-18T11:13:37Z

Perhaps this was forgotten 😭 , even I couldn't remember what I did

Plus, seems like she's busy btw? so... umm @rustbot r? @rust-lang/clippy

J-ZhengLi · 2024-03-28T09:10:47Z

some extra info: there are 2644 occurance of this lint after running lintcheck --recursive

Manishearth · 2024-03-29T16:39:33Z

r? clippy

still busy

xFrednet · 2024-03-31T11:50:33Z

Sure, it's on my todo list for the next week :D

xFrednet

Thank you for the implementation, this looks good to me. Time to get the FCP started.

@rustbot label +S-final-comment-period -S-waiting-on-author

FCP: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.20.60renamed_function_params.60/near/431407447

xFrednet · 2024-04-12T09:20:08Z

On zulip it was mentioned that this lint should ideally have a configuration to ignore some lints, like From, TryFrom, and FromStr. Once that is done I believe we can move forward with this. There was also this suggestion:

It looks a bit similar to disallowed_names. Perhaps the name could be disallow_parameter_renames or disallow_renames?

I like the current name, but can understand the reasoning :)

J-ZhengLi · 2024-04-18T09:16:10Z

@xFrednet Finally got some time working on this~ I've added a configuration for it, but again... not confident about that config's name, which is ignored-traits for now, if there's better name plz let me know~

some time I really wish there's an AI that could tell me what should I name my variables/functions/lint/config lol

J-ZhengLi · 2024-04-19T01:07:43Z

clippy_lints/src/functions/renamed_function_params.rs

+
+use super::RENAMED_FUNCTION_PARAMS;
+
+static IGNORED_TRAIT_IDS: OnceLock<DefIdSet> = OnceLock::new();


I'm not sure if I should use OnceLock here, haven't seen them in other lints, maybe there's some kind of concern? idk

I believe some other lints store this in the lint pass struct instead. The IDs can then be collected in fn check_crate(). I would prefer that solution over having a static object. Otherwise, I don't believe there is a reason against OnceLock

Oh I see, I just thought it might be redundant to having a list of trait paths, and a set of their ids stored in the same struct

nvm, it does seems very common

xFrednet

Two NITs that should hopefully be easy to fix and then this should be good to go :D

(Sorry for the delay with the review, my past few weeks have been busy)

xFrednet · 2024-05-04T14:01:06Z

clippy_config/src/conf.rs

+    /// - 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()),


This name is too general, maybe allow_renamed_params_for or renamed_function_params_alllowed_for?

xFrednet · 2024-05-04T14:01:21Z

clippy_config/src/conf.rs

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


xFrednet · 2024-05-04T14:03:59Z

clippy_lints/src/functions/renamed_function_params.rs

+
+use super::RENAMED_FUNCTION_PARAMS;
+
+static IGNORED_TRAIT_IDS: OnceLock<DefIdSet> = OnceLock::new();


I believe some other lints store this in the lint pass struct instead. The IDs can then be collected in fn check_crate(). I would prefer that solution over having a static object. Otherwise, I don't believe there is a reason against OnceLock

@Centri3

apply review suggestions by @Centri3: use multi suggestion; change output message format; add macro expansion check & tests;

J-ZhengLi · 2024-05-07T09:10:27Z

(Sorry for the delay with the review, my past few weeks have been busy)

no worries, me too XD

ping @xFrednet

xFrednet · 2024-05-12T13:53:41Z

In Clippy's realm, a rule takes flight,
No renaming parameters, to keep code tight.
Thanks for the effort, I merge with delight.

=^.^=

bors · 2024-05-12T13:53:43Z

📌 Commit 2358189 has been approved by xFrednet

It is now in the queue for this repository.

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.

bors · 2024-05-12T13:56:10Z

⌛ Testing commit 2358189 with merge d47bc8a...

bors · 2024-05-12T14:02:52Z

💔 Test failed - checks-action_test

J-ZhengLi · 2024-05-12T14:05:39Z

ahh, forgot the cargo collect-metadata again, third times a charm I guess~

xFrednet · 2024-05-12T14:07:45Z

:P

Feel free to r=me after the collection :)

@bors delegate+

bors · 2024-05-12T14:07:48Z

✌️ @J-ZhengLi, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

…metadata

J-ZhengLi · 2024-05-12T14:20:15Z

should be good now

@bors r=@xFrednet

bors · 2024-05-12T14:20:18Z

📌 Commit 46659ac has been approved by xFrednet

It is now in the queue for this repository.

bors · 2024-05-12T14:21:25Z

⌛ Testing commit 46659ac with merge 7cfb9a0...

bors · 2024-05-12T14:29:51Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 7cfb9a0 to master...

rustbot assigned Jarcho Sep 20, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 20, 2023

Centri3 reviewed Sep 20, 2023

View reviewed changes

J-ZhengLi force-pushed the issue11443 branch 3 times, most recently from d1fc8d5 to 1d4f27b Compare September 25, 2023 02:55

J-ZhengLi force-pushed the issue11443 branch from 1d4f27b to 755fd6f Compare October 9, 2023 02:12

rustbot assigned Centri3 and unassigned Jarcho Oct 30, 2023

rustbot assigned Manishearth and unassigned Centri3 Jan 18, 2024

J-ZhengLi force-pushed the issue11443 branch 2 times, most recently from ec3b457 to 5a03c9b Compare March 28, 2024 08:30

rustbot assigned xFrednet and unassigned Manishearth Mar 29, 2024

xFrednet approved these changes Apr 4, 2024

View reviewed changes

rustbot added the S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) label Apr 4, 2024

xFrednet removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 4, 2024

xFrednet added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Apr 12, 2024

J-ZhengLi force-pushed the issue11443 branch from 5a03c9b to 830fd94 Compare April 18, 2024 02:56

J-ZhengLi commented Apr 19, 2024

View reviewed changes

xFrednet reviewed May 4, 2024

View reviewed changes

J-ZhengLi added 2 commits May 7, 2024 10:49

add new lint that disallow renaming parameters in trait functions

28d5115

fix doc test failure;

40ec760

apply review suggestions by @Centri3: use multi suggestion; change output message format; add macro expansion check & tests;

J-ZhengLi force-pushed the issue11443 branch from bdf9ebb to 2358189 Compare May 7, 2024 09:08

xFrednet removed the S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) label May 12, 2024

xFrednet approved these changes May 12, 2024

View reviewed changes

add configuration to allow skipping on some certain traits & collect …

46659ac

…metadata

J-ZhengLi force-pushed the issue11443 branch from 2358189 to 46659ac Compare May 12, 2024 14:13

bors merged commit 7cfb9a0 into rust-lang:master May 12, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new lint that disallow renaming parameters in trait functions #11540

add new lint that disallow renaming parameters in trait functions #11540

J-ZhengLi commented Sep 20, 2023 •

edited

Loading

rustbot commented Sep 20, 2023

xFrednet commented Sep 20, 2023

J-ZhengLi commented Sep 20, 2023

Centri3 left a comment

bors commented Oct 8, 2023

Jarcho commented Oct 30, 2023

J-ZhengLi commented Jan 18, 2024 •

edited

Loading

J-ZhengLi commented Mar 28, 2024

Manishearth commented Mar 29, 2024

xFrednet commented Mar 31, 2024

xFrednet left a comment •

edited

Loading

xFrednet commented Apr 12, 2024

J-ZhengLi commented Apr 18, 2024

J-ZhengLi Apr 19, 2024

xFrednet May 4, 2024

J-ZhengLi May 7, 2024 •

edited

Loading

xFrednet left a comment

xFrednet May 4, 2024

xFrednet May 4, 2024

xFrednet May 4, 2024

J-ZhengLi commented May 7, 2024 •

edited

Loading

xFrednet commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

J-ZhengLi commented May 12, 2024 •

edited

Loading

xFrednet commented May 12, 2024 •

edited

Loading

bors commented May 12, 2024

J-ZhengLi commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024


		use super::RENAMED_FUNCTION_PARAMS;

		static IGNORED_TRAIT_IDS: OnceLock<DefIdSet> = OnceLock::new();

add new lint that disallow renaming parameters in trait functions #11540

add new lint that disallow renaming parameters in trait functions #11540

Conversation

J-ZhengLi commented Sep 20, 2023 • edited Loading

rustbot commented Sep 20, 2023

xFrednet commented Sep 20, 2023

J-ZhengLi commented Sep 20, 2023

Centri3 left a comment

Choose a reason for hiding this comment

bors commented Oct 8, 2023

Jarcho commented Oct 30, 2023

J-ZhengLi commented Jan 18, 2024 • edited Loading

J-ZhengLi commented Mar 28, 2024

Manishearth commented Mar 29, 2024

xFrednet commented Mar 31, 2024

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

xFrednet commented Apr 12, 2024

J-ZhengLi commented Apr 18, 2024

J-ZhengLi Apr 19, 2024

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

J-ZhengLi May 7, 2024 • edited Loading

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

J-ZhengLi commented May 7, 2024 • edited Loading

xFrednet commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

J-ZhengLi commented May 12, 2024 • edited Loading

xFrednet commented May 12, 2024 • edited Loading

bors commented May 12, 2024

J-ZhengLi commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

J-ZhengLi commented Sep 20, 2023 •

edited

Loading

J-ZhengLi commented Jan 18, 2024 •

edited

Loading

xFrednet left a comment •

edited

Loading

J-ZhengLi May 7, 2024 •

edited

Loading

J-ZhengLi commented May 7, 2024 •

edited

Loading

J-ZhengLi commented May 12, 2024 •

edited

Loading

xFrednet commented May 12, 2024 •

edited

Loading