Make `inconsistent_struct_constructor` "all fields are shorthand" requirement configurable #13737

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)

y21 · 2024-12-04T04:03:23Z

clippy_lints/src/inconsistent_struct_constructor.rs

+            let sugg = suggestion(cx, fields, &def_order_map);

            if !fulfill_or_allowed(cx, INCONSISTENT_STRUCT_CONSTRUCTOR, Some(ty_hir_id)) {
                span_lint_and_sugg(


Can this use span_lint_and_then and have suggestion() called in the closure? It's a pedantic lint and there's a bit of non trivial work happening in there that could be skipped if the lint isn't enabled

y21 · 2024-12-07T17:49:25Z

clippy_lints/src/inconsistent_struct_constructor.rs

+    fields: &'tcx [hir::ExprField<'tcx>],
+    def_order_map: &FxHashMap<Symbol, usize>,
+) -> String {
+    let ws = fields


For what it's worth, there could also be comments or cfg'd out fields in ws which could be non-obvious from the binding name, but I don't think it's an issue (in fact, the old code removed them entirely so this is an improvement). But I think we should at least have a test for them

struct S { a: i32, b: i32, #[cfg(any())] c: i32, d: i32 } let s = S { d: 0, #[cfg(any())] c: 1, b: 2, a: 3 };

y21 · 2024-12-07T17:54:51Z

clippy_lints/src/inconsistent_struct_constructor.rs

+            let w0_span = field_with_attrs_span(cx.tcx, &w[0]);
+            let w1_span = field_with_attrs_span(cx.tcx, &w[1]);
+            let span = w0_span.with_hi(w1_span.lo()).with_lo(w0_span.hi());
+            snippet_opt(cx, span).unwrap()


We usually use snippet() with some placeholder (" " could work for whitespace), proc macros can give us arbitrarily weird or malformed spans where it could fail that I think it would just be safer to use that here

y21 · 2024-12-07T17:58:47Z

clippy_lints/src/inconsistent_struct_constructor.rs

+        .map(|w| {
+            let w0_span = field_with_attrs_span(cx.tcx, &w[0]);
+            let w1_span = field_with_attrs_span(cx.tcx, &w[1]);
+            let span = w0_span.with_hi(w1_span.lo()).with_lo(w0_span.hi());


small nit: w0_span.between(w1_span)

y21 · 2024-12-07T18:12:24Z

clippy_config/src/conf.rs

+    /// Note that such suggestions are not applied automatically with `--fix`. The following example
+    /// [due to @ronnodas] shows why. Swapping the fields in the constructor produces incompilable code:


I think we should generalize this, or at least also mention that warnings produced by this configuration aren't always as trivial to fix as the default behavior (simply reordering the fields) even if it would compile, as this can change semantics if the initializer expressions have side effects.
Users might need to pull code out of the initializer into variables when the order matters or occasionally allow the lint.

I'm thinking we should probably also have this mentioned in the emitted diagnostic, like "if the field evaluation order doesn't matter, try" instead of simply "try" as the help message when initializers are present to make it clear that the lint does not account for that.

Also, I don't know about the username (the current phrasing "due to person x" makes it sound like the person is being blamed). If you really want to keep the link to the comment, I'd just rephrase this as "The following example from this comment shows how the suggestion can run into borrowcheck errors"

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.

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.

Make "all fields are shorthand" requirement configurable

428844e

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 added 2 commits November 27, 2024 06:26

Handle field attributes in suggestions

0a91eaa

Fix adjacent code

6ddfaba

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 Show resolved Hide resolved

Address review comments

3b2082e

rust-lang#13737 (comment)

y21 reviewed Dec 7, 2024

View reviewed changes

Address all review comments but one

dbff5cd

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `inconsistent_struct_constructor` "all fields are shorthand" requirement configurable #13737

Make `inconsistent_struct_constructor` "all fields are shorthand" requirement configurable #13737

smoelius commented Nov 26, 2024 •

edited

Loading

rustbot commented Nov 26, 2024

samueltardieu Nov 26, 2024

smoelius Nov 27, 2024

smoelius Nov 27, 2024

samueltardieu Nov 27, 2024

smoelius Nov 27, 2024

samueltardieu Nov 27, 2024

y21 Dec 4, 2024

y21 Dec 7, 2024

y21 Dec 7, 2024

y21 Dec 7, 2024

y21 Dec 7, 2024

y21 Dec 7, 2024

smoelius Dec 9, 2024 •

edited

Loading

smoelius commented Dec 9, 2024

		/// Note that such suggestions are not applied automatically with `--fix`. The following example
		/// [due to @ronnodas] shows why. Swapping the fields in the constructor produces incompilable code:

Make inconsistent_struct_constructor "all fields are shorthand" requirement configurable #13737

Are you sure you want to change the base?

Make inconsistent_struct_constructor "all fields are shorthand" requirement configurable #13737

Conversation

smoelius commented Nov 26, 2024 • edited Loading

rustbot commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

smoelius commented Dec 9, 2024

Make `inconsistent_struct_constructor` "all fields are shorthand" requirement configurable #13737

Make `inconsistent_struct_constructor` "all fields are shorthand" requirement configurable #13737

smoelius commented Nov 26, 2024 •

edited

Loading

smoelius Dec 9, 2024 •

edited

Loading