add lint for struct field names #11496

jonboh · 2023-09-13T18:45:37Z

changelog: [struct_field_names]: lint structs with the same pre/postfix in all fields or with fields that are pre/postfixed with the name of the struct.

fixes #2555

I've followed general structure and naming from the code in enum_variants lint, which implements the same logic for enum variants.

rustbot · 2023-09-13T18:45:41Z

r? @dswij

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

bors · 2023-09-17T14:40:33Z

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

Alexendoo · 2023-09-21T17:52:21Z

r? @y21

rustbot · 2023-09-21T17:52:23Z

Failed to set assignee to y21: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

y21

One question: would it make sense to allow is_* fields? I imagine this would be a common case where people would want to allow the lint

clippy_lints/src/struct_fields.rs

tests/ui/struct_fields.rs

clippy_lints/src/struct_fields.rs

clippy_utils/src/str_utils.rs

clippy_lints/src/struct_fields.rs

jonboh · 2023-09-22T15:50:31Z

One question: would it make sense to allow is_* fields? I imagine this would be a common case where people would want to allow the lint

Yes, I agree it would make sense to silence the lint in this case. I could add that prefix as an exception so that it does not trigger the lint, but I wasn't sure if adding particular cases was appropiate.

I think even in the case in which is_* makes sense it is possible to factor out that meaning into the struct name.

struct Data {
    is_correct: bool
    is_duplicaded: bool
    is_encrypted: bool
}

struct DataChecks {
    correct: bool
    duplicated: bool
    encrypted: bool
}

I don't have a strong preference for the second case.
Do you think it is worthwhile adding the specific case for is_*?

y21 · 2023-09-22T18:57:21Z

The idea of allowing is_* came from this #[allow] here, I was of the opinion that changing the is_local field to local would make it less clear. Thinking about it more though and looking at your examples, this might just be fine and we could also keep it like that

y21

We should also have a test for the configuation option (in tests/ui-toml) to make sure it works and continues to work to avoid issues such as #11481

clippy_lints/src/struct_fields.rs

y21 · 2023-09-23T14:27:03Z

Also I wonder if we can merge this file with enum_variants since they do seem very related and share a bit of logic. There's also other lints as part of the same lint pass such as module_name_repetitions, for catching a shared {pre,post}fix between modules and structs or other items

jonboh · 2023-09-23T14:35:24Z

Yeah, they could be in the same file, although it might be good to change the name enum_variants at that point. For me enum_variants its already kind of a misnomer when the file contains a bunch more things than checking the enum variants.
Initially I thought it might be better to split the enum_variants file so that the logic related to module inception would be in a different file.

The module inception part already checks the struct names as well, so we could merge everything together with a more general name for the file (instead of enum_variants).

y21 · 2023-09-23T15:08:22Z

I agree that the lint pass name enum_variants is not great for what it is now. All lints in it are checking for common prefixes and suffixes in items (not necessarily enums). Renaming it to something more general and putting struct_field_names in it sounds good. I'm not sure what a good name for it could be. Maybe item_name_repetitions? Would include both modules, struct (fields) and enum (variants)

prevent ice when threshold is 0 and enum has no variants changelog: [`lint_name`]: prevent ice when threshold is 0 and enum has no variants r? `@y21` Fixes the same ice issue raised during review of #11496

prevent ice when threshold is 0 and enum has no variants changelog: [`enum_variant_names`]: prevent ice when threshold is 0 and enum has no variants r? `@y21` Fixes the same ice issue raised during review of #11496

bors · 2023-09-24T21:36:22Z

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

y21

The change from enum_variants to item_name_repetitions is great, I like that. It's probably worth including another changelog entry in the description that says that the enum_variant_name_threshold config option was renamed to item_name_threshold.

It also looks like the error in CI happens because it's testing that each directory in tests/ui-toml has an associated test, but tests/ui-toml/item_name_repetitions doesn't have any direct test files, only directories. Might have to flatten this to item_name_repetitions_threshold_0 and item_name_repetitions_threshold_5 🤔

clippy_lints/src/utils/conf.rs

clippy_lints/src/item_name_repetitions.rs

y21 · 2023-09-26T20:27:29Z

clippy_lints/src/item_name_repetitions.rs

+    };
+    let mut post = pre.clone();
+    post.reverse();
+    for field in fields.iter().skip(1) {


The rest of this function from here on should be basically nearly equivalent to what's in check_variants once the other PR gets merged, in terms of what it does to the two Vec<&str>s, right? Just emits a different lint in the end.

Depending on if that PR gets merged soon we could perhaps move some of this to its own function so that the other lint gets to reuse the logic and we wouldn't need to duplicate stuff here

yeah I agree, I can wait for the bugfix to be merged and refactor this part.

tests/ui/redundant_pub_crate.rs

jonboh · 2023-09-27T16:51:51Z

The change from enum_variants to item_name_repetitions is great, I like that. It's probably worth including another changelog entry in the description that says that the enum_variant_name_threshold config option was renamed to item_name_threshold.

It also looks like the error in CI happens because it's testing that each directory in tests/ui-toml has an associated test, but tests/ui-toml/item_name_repetitions doesn't have any direct test files, only directories. Might have to flatten this to item_name_repetitions_threshold_0 and item_name_repetitions_threshold_5 🤔

I think it was failing because I had one stderr file with the old name that did not have any associated test file. I think I've fixed it now. I'm going to check that the tests are actually running in the CI (running cargo test both of them run)

y21

Some last comments from my side, mostly just nits and a question :)

There's still some duplicate looking code in check_{variants,fields}, but it's subtly different in places that I'm not sure it's easily refactorable into its own function, so I'm also not sure it's worth blocking the PR over it (but if you have ideas for addressing this, feel free to do that too). There's the other PR which makes parts of check_variant match the behavior from check_fields more closely, so it might be easier to refactor it with those changes.

I also did a lintcheck run and there didn't appear to be any FPs (actually there weren't any matches at all, which is not necessarily a bad thing), though only on the 26 default crates

@Alexendoo do you have some other things to comment on that I might have missed?

clippy_lints/src/item_name_repetitions.rs

jonboh · 2023-10-17T16:37:45Z

I think it makes sense to move the struct_field_names lint to pedantic. I've left enum_variant_names in style.

y21

Before we can merge this, one last comment about the empty enum test. After this I think we're good. 😄
(Also, the changelog in the description still says that a config option was renamed. Not true anymore)

tests/ui-toml/item_name_repetitions/threshold0/clippy.toml

y21

Alright, this looks ready to be merged 🎉. Thank you for implementing this lint and for your patience!

@bors r+

y21 · 2023-10-17T20:34:45Z

🤔 @bors ping

bors · 2023-10-17T20:34:47Z

😪 I'm awake I'm awake

y21 · 2023-10-17T20:35:51Z

Let's try again? @bors r+

bors · 2023-10-17T20:35:53Z

📌 Commit 80025e0 has been approved by y21

It is now in the queue for this repository.

bors · 2023-10-17T20:37:00Z

⌛ Testing commit 80025e0 with merge 01c0a20...

add lint for struct field names changelog: [`struct_field_names`]: lint structs with the same pre/postfix in all fields or with fields that are pre/postfixed with the name of the struct. fixes #2555 I've followed general structure and naming from the code in [enum_variants](https://github.com/rust-lang/rust-clippy/blob/b788addfcc955368b9771b77d312c248fab60253/clippy_lints/src/enum_variants.rs) lint, which implements the same logic for enum variants.

bors · 2023-10-17T20:37:28Z

💔 Test failed - checks-action_test

y21 · 2023-10-17T21:03:21Z

ERROR: PR body must contain 'changelog: ...'

I guess it doesn't like that linebreak after changelog? @jonboh can you try removing that?

jonboh · 2023-10-18T06:44:21Z

linebreak removed.

Thank you for taking the time to review the PR :D

y21 · 2023-10-18T10:00:48Z

@bors retry

bors · 2023-10-18T10:01:54Z

⌛ Testing commit 80025e0 with merge ffdc3ce...

add lint for struct field names changelog: [`struct_field_names`]: lint structs with the same pre/postfix in all fields or with fields that are pre/postfixed with the name of the struct. fixes #2555 I've followed general structure and naming from the code in [enum_variants](https://github.com/rust-lang/rust-clippy/blob/b788addfcc955368b9771b77d312c248fab60253/clippy_lints/src/enum_variants.rs) lint, which implements the same logic for enum variants.

bors · 2023-10-18T10:11:52Z

💔 Test failed - checks-action_test

jonboh · 2023-10-18T10:30:15Z

It seems I need to update the book with the new struct fields config parameter. I'll try to submit the change in a few hours

y21 · 2023-10-18T15:07:32Z

Yep, just running cargo collect-metadata should do it all for you

side effect for `enum_variants`: use .first() instead of .get(0) in enum_variants lint move to_camel_case to str_util module move module, enum and struct name repetitions check to a single file `item_name_repetitions` rename enum_variants threshold config option

y21 · 2023-10-18T18:38:01Z

Third time's the charm. @bors retry

y21 · 2023-10-18T18:46:21Z

@bors r+

bors · 2023-10-18T18:46:23Z

📌 Commit 8b02dac has been approved by y21

It is now in the queue for this repository.

bors · 2023-10-18T18:47:29Z

⌛ Testing commit 8b02dac with merge fe21991...

bors · 2023-10-18T19:01:20Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing fe21991 to master...

y21 · 2023-10-18T19:02:11Z

🎉

That took a few tries.
Thanks again for your work here @jonboh !

rustbot assigned dswij Sep 13, 2023

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

Alexendoo assigned Alexendoo and unassigned dswij Sep 21, 2023

y21 reviewed Sep 21, 2023

View reviewed changes

jonboh mentioned this pull request Sep 22, 2023

prevent ice when threshold is 0 and enum has no variants #11552

Merged

jonboh force-pushed the prefix_postfix_struct branch from 654cea6 to 931cac8 Compare September 22, 2023 19:12

y21 reviewed Sep 23, 2023

View reviewed changes

clippy_lints/src/struct_fields.rs Outdated Show resolved Hide resolved

clippy_lints/src/struct_fields.rs Outdated Show resolved Hide resolved

clippy_lints/src/struct_fields.rs Outdated Show resolved Hide resolved

Alexendoo assigned y21 Sep 23, 2023

jonboh force-pushed the prefix_postfix_struct branch 2 times, most recently from 0824c5d to 04728ad Compare September 24, 2023 19:52

y21 reviewed Sep 26, 2023

View reviewed changes

jonboh force-pushed the prefix_postfix_struct branch 3 times, most recently from 32caf58 to 4bd5a78 Compare September 27, 2023 17:35

y21 reviewed Sep 28, 2023

View reviewed changes

jonboh force-pushed the prefix_postfix_struct branch from 07c662e to a2577a4 Compare September 28, 2023 20:38

jonboh force-pushed the prefix_postfix_struct branch from c0d8961 to e59d512 Compare October 17, 2023 16:35

y21 reviewed Oct 17, 2023

View reviewed changes

tests/ui-toml/item_name_repetitions/threshold0/clippy.toml Show resolved Hide resolved

jonboh force-pushed the prefix_postfix_struct branch from e59d512 to 80025e0 Compare October 17, 2023 19:57

y21 approved these changes Oct 17, 2023

View reviewed changes

jonboh force-pushed the prefix_postfix_struct branch from 80025e0 to 8b02dac Compare October 18, 2023 17:20

bors merged commit fe21991 into rust-lang:master Oct 18, 2023
7 checks passed

bors mentioned this pull request Oct 18, 2023

fix enum_variant_names depending lint depending on order #11498

Merged

add lint for struct field names #11496

add lint for struct field names #11496

Conversation

jonboh commented Sep 13, 2023 • edited Loading

rustbot commented Sep 13, 2023

bors commented Sep 17, 2023

Alexendoo commented Sep 21, 2023

rustbot commented Sep 21, 2023

y21 left a comment

Choose a reason for hiding this comment

jonboh commented Sep 22, 2023

y21 commented Sep 22, 2023

y21 left a comment

Choose a reason for hiding this comment

y21 commented Sep 23, 2023

jonboh commented Sep 23, 2023

y21 commented Sep 23, 2023

bors commented Sep 24, 2023

y21 left a comment

Choose a reason for hiding this comment

y21 Sep 26, 2023

Choose a reason for hiding this comment

jonboh Sep 27, 2023

Choose a reason for hiding this comment

jonboh commented Sep 27, 2023

y21 left a comment

Choose a reason for hiding this comment

jonboh commented Oct 17, 2023

y21 left a comment

Choose a reason for hiding this comment

y21 left a comment

Choose a reason for hiding this comment

y21 commented Oct 17, 2023

bors commented Oct 17, 2023

y21 commented Oct 17, 2023

bors commented Oct 17, 2023

bors commented Oct 17, 2023

bors commented Oct 17, 2023

y21 commented Oct 17, 2023

jonboh commented Oct 18, 2023

y21 commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

jonboh commented Oct 18, 2023

y21 commented Oct 18, 2023

y21 commented Oct 18, 2023

y21 commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

y21 commented Oct 18, 2023

jonboh commented Sep 13, 2023 •

edited

Loading