Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Nov 26, 2024

Fixes #11846.

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
Copy link
Collaborator

rustbot commented Nov 26, 2024

r? @y21

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 26, 2024
Comment on lines 223 to 225
#[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 },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @samueltardieu.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem should be resolved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@smoelius smoelius marked this pull request as draft November 27, 2024 01:19
@smoelius smoelius force-pushed the all-fields-are-shorthand branch from 1218259 to 6ddfaba Compare November 27, 2024 11:46
@smoelius smoelius marked this pull request as ready for review November 27, 2024 12:01
smoelius added a commit to smoelius/rust-clippy that referenced this pull request Nov 28, 2024
Comment on lines 112 to 115
let sugg = suggestion(cx, fields, &def_order_map);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
};

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: w0_span.between(w1_span)

Comment on lines 531 to 532
/// 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@smoelius smoelius Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

smoelius added a commit to smoelius/rust-clippy that referenced this pull request Dec 9, 2024
@smoelius
Copy link
Contributor Author

smoelius commented Dec 9, 2024

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

Nits re my changes are welcome.

@bors
Copy link
Contributor

bors commented Dec 15, 2024

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

@smoelius smoelius force-pushed the all-fields-are-shorthand branch from dbff5cd to 29c1dbd Compare December 15, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question re inconsistent_struct_constructor requirement
5 participants