Make `inconsistent_struct_constructor` "all fields are shorthand" requirement configurable by smoelius · Pull Request #13737 · rust-lang/rust-clippy

smoelius · 2024-11-26T19:56:02Z

This PR has three commits:

The first commit adds an initializer-suggestions configuration to control suggestion applicability when initializers are present. The following are the options:
- "none": do not suggest
- "maybe-incorrect": suggest, but do not apply suggestions with --fix
- "machine-applicable": suggest and apply suggestions with --fix
The second commit fixes suggestions to handle field attributes (problem noticed by @samueltardieu).
The third commit adds initializer-suggestions = "machine-applicable" to Clippy's clippy.toml and applies the suggestions. (Nothing seems to break.)

changelog: make inconsistent_struct_constructor "all fields are shorthand" requirement configurable

rustbot · 2024-11-26T19:56:08Z

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

samueltardieu · 2024-11-26T21:40:49Z

clippy_lints/src/macro_metavars_in_unsafe.rs

-            #[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning
-            expn_depth: if body.value.span.from_expansion() { 1 } else { 0 },
            macro_unsafe_blocks: Vec::new(),
+            expn_depth: if body.value.span.from_expansion() { 1 } else { 0 },


The #[expect] should have moved along with the field. That explains why you had to add your third commit, as you lost the attribute.

You should at the minimum re-add the attribute in the second commit and drop the third commit. But the attribute situation should be examined more closely.

Thanks, @samueltardieu.

This problem should be resolved.

Do you mean manually, or automatically? Will attributes now move with the fields, or is this a bug that needs fixing? (I don't seem to find tests with this case that we now know is problematic)

Do you mean manually, or automatically? Will attributes now move with the fields...

Yes, that is what I meant. (Thanks very much for noticing this, BTW.)

Here is the test I added: 0a91eaa#diff-377444135d6554122eb9b6c08b368b9d297fe68a76dfbcd4d9754ae5f85e1588R27-R41

Now, whether a similar problem exists for other lints, I can't say. I find it a little unfortunate that a field expression's span doesn't include its attributes. But there are no doubt arguments for doing it this way.

Indeed, the new test is a great addition. Thanks!

book/src/lint_configuration.md

rust-lang#13737 (comment)

clippy_lints/src/inconsistent_struct_constructor.rs

clippy_config/src/conf.rs

y21 · 2024-12-07T18:29:56Z

clippy_config/src/conf.rs

+    ///
+    /// [due to @ronnodas]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
+    #[lints(inconsistent_struct_constructor)]
+    initializer_suggestions: bool = false,


What do you think about naming this warn_inconsistent_struct_field_initializers? IMO "suggestions" isn't so accurate anymore after the last change since whether this is enabled or not has no effect on the suggestion now

What do you think about naming this warn_inconsistent_struct_field_initializers? IMO "suggestions" isn't so accurate anymore after the last change since whether this is enabled or not has no effect on the suggestion now

Could I suggest lint_inconsistent_struct_field_initializers, or some other verb besides warn?

camsteffen once pointed out to me that whether a lint actually warns is configurable.

Yeah that works too. warn seemed consistent with other similar named configs (there are two that start with warn_*), but I agree with you that it's a bit of a misnomer when someone denies the lint

This comment is not yet addressed: rust-lang#13737 (comment)

smoelius · 2024-12-09T11:28:44Z

I think I have addressed all of the comments but this one: #13737 (comment)

Nits re my changes are welcome.

bors · 2024-12-15T16:09:52Z

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

rust-lang#13737 (comment)

This comment is not yet addressed: rust-lang#13737 (comment)

y21

LGTM with the configuration fixed, sorry it took a bit longer for me to get to this PR again. Can you also squash the commits?

y21 · 2024-12-26T22:33:15Z

clippy_config/src/conf.rs

+    ///
+    /// [due to @ronnodas]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
+    #[lints(inconsistent_struct_constructor)]
+    initializer_suggestions: bool = false,


Yeah that works too. warn seemed consistent with other similar named configs (there are two that start with warn_*), but I agree with you that it's a bit of a misnomer when someone denies the lint

Handle field attributes in suggestions Fix adjacent code Address review comments rust-lang#13737 (comment) Address all review comments but one This comment is not yet addressed: rust-lang#13737 (comment) `initializer_suggestions` -> `lint_inconsistent_struct_field_initializers`

smoelius · 2024-12-27T00:43:57Z

Oh, good. As of right now, I can access eb83c07.

Please let me know if there is anything else you would like changed, @y21. And thank you very much for the review. 🙏

y21

Looks good to me, thanks!

@y21

… deprecated configurations (#14280) This PR does two things: - It renames `inconsistent_struct_constructor`'s configuration from `lint-inconsistent-struct-field-initializers` to `check-inconsistent-struct-field-initializers`. (I should have suggested `check-...` in [#13737](#13737 (comment)).) - It causes Clippy to no longer suggest deprecated configurations. (Previously, Clippy would suggest `cyclomatic-complexity-threshold`, for example.) r? @y21 changelog: Rename `lint-inconsistent-struct-field-initializers` to `check-inconsistent-struct-field-initializers` changelog: No longer suggest deprecated configurations

rustbot assigned y21 Nov 26, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 26, 2024

samueltardieu reviewed Nov 26, 2024

View reviewed changes

smoelius marked this pull request as draft November 27, 2024 01:19

smoelius force-pushed the all-fields-are-shorthand branch from 1218259 to 6ddfaba Compare November 27, 2024 11:46

smoelius marked this pull request as ready for review November 27, 2024 12:01

samueltardieu approved these changes Nov 27, 2024

View reviewed changes

y21 reviewed Nov 28, 2024

View reviewed changes

book/src/lint_configuration.md Outdated Show resolved Hide resolved

smoelius added a commit to smoelius/rust-clippy that referenced this pull request Nov 28, 2024

Address review comments

3b2082e

rust-lang#13737 (comment)

y21 reviewed Dec 7, 2024

View reviewed changes

smoelius added a commit to smoelius/rust-clippy that referenced this pull request Dec 9, 2024

Address all review comments but one

dbff5cd

This comment is not yet addressed: rust-lang#13737 (comment)

smoelius force-pushed the all-fields-are-shorthand branch from dbff5cd to 29c1dbd Compare December 15, 2024 18:19

smoelius added a commit to smoelius/rust-clippy that referenced this pull request Dec 15, 2024

Address review comments

57fb2f1

rust-lang#13737 (comment)

smoelius added a commit to smoelius/rust-clippy that referenced this pull request Dec 15, 2024

Address all review comments but one

29c1dbd

This comment is not yet addressed: rust-lang#13737 (comment)

y21 approved these changes Dec 26, 2024

View reviewed changes

smoelius force-pushed the all-fields-are-shorthand branch from eb83c07 to 8a38bcc Compare December 27, 2024 00:37

y21 approved these changes Dec 27, 2024

View reviewed changes

y21 added this pull request to the merge queue Dec 27, 2024

Merged via the queue into rust-lang:master with commit a8968e5 Dec 27, 2024
9 checks passed

smoelius deleted the all-fields-are-shorthand branch December 27, 2024 22:16

smoelius mentioned this pull request Feb 23, 2025

Rename inconsistent_struct_constructor configuration; don't suggest deprecated configurations #14280

Merged

Comments

Conversation

smoelius commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoelius Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoelius commented Dec 9, 2024

Uh oh!

bors commented Dec 15, 2024

Uh oh!

y21 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoelius commented Dec 27, 2024

Uh oh!

y21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smoelius commented Nov 26, 2024 •

edited

Loading

smoelius Dec 9, 2024 •

edited

Loading

y21 left a comment •

edited

Loading