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

use_field_init_shorthand option breaks numbered struct field initialization #5488

Closed
foxcob opened this issue Aug 7, 2022 · 6 comments · Fixed by #5520
Closed

use_field_init_shorthand option breaks numbered struct field initialization #5488

foxcob opened this issue Aug 7, 2022 · 6 comments · Fixed by #5520
Assignees
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted only-with-option requires a non-default option value to reproduce p-low

Comments

@foxcob
Copy link

foxcob commented Aug 7, 2022

If I have a struct type with an implicit field such as

struct MyStruct(u64);

And I attempt to initialize it to 0

let instance = MyStruct { 0: 0 };

With the option "use_field_init_shorthand = true", rustfmt will incorrectly shorten the initialization to:

let instance = MyStruct { 0 };

This code will no longer compile after running rustfmt.

@calebcartwright
Copy link
Member

calebcartwright commented Aug 7, 2022

Is there any reason in particular you're trying to initialize it that way instead of the more canonical:

let instance = MyStruct(0);

The other way is valid syntax of course and we should probably adjust the rule to handle this, but with a tuple struct you'd typically see the aforementioned syntax instead.

@calebcartwright calebcartwright added bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce p-low good first issue Issues up for grabs, also good candidates for new rustfmt contributors labels Aug 7, 2022
@calebcartwright
Copy link
Member

For anyone interested in working on this, think you'll probably want to start around the below. Likely just need to enhance the check to not fall into the shorthand mode if the field name is itself an invalid ident (e.g. 0)

rustfmt/src/expr.rs

Lines 1721 to 1723 in e041c20

Some(ref e) if e.as_str() == name && context.config.use_field_init_shorthand() => {
Some(attrs_str + name)
}

@foxcob
Copy link
Author

foxcob commented Aug 7, 2022

Is there any reason in particular you're trying to initialize it that way instead of the more canonical:

let instance = MyStruct(0);

The other way is valid syntax of course and we should probably adjust the rule to handle this, but with a tuple struct you'd typically see the aforementioned syntax instead.

Ignorance! The syntax I was using worked even though it felt clumsy. Of course there’s a better way and now I know. Glad I could identify the issue though.

@calebcartwright
Copy link
Member

No worries, and thanks for bringing it to our attention! It's something we should handle anyway, so better we know than not know and fortunately there's a readily available (and more idiomatic) option for folks in the interim

@RajivTS
Copy link
Contributor

RajivTS commented Aug 20, 2022

@rustbot claim

@RajivTS
Copy link
Contributor

RajivTS commented Aug 27, 2022

Hi @calebcartwright, just wanted to validate the expectation here before implementing the fix and was hoping you could help. AFAIU if the name of the field is any literal (i.e. numeric values, quoted strings, etc.), we want to avoid falling into the short-hand mode?

If the above is true, I am thinking of validating the ExprKind of P<Expr> contained within ExprField by checking if it is a literal (e.g. 0 in the above example). If it returns true, we do not fall in the short-hand mode, otherwise we follow the current code flow.

Please let me know if I am overcomplicating or have missed something obvious.

RajivTS added a commit to RajivTS/rustfmt that referenced this issue Aug 27, 2022
Closes rust-lang#5488

Fix for rust-lang#5488. Before applying shorthand initialization for structs,
check if identifier is a literal (e.g. tuple struct). If yes, then do
not apply short hand initialization.

Added test case to validate the changes for the fix.
RajivTS added a commit to RajivTS/rustfmt that referenced this issue Aug 28, 2022
Closes rust-lang#5488

Fix for rust-lang#5488. Before applying shorthand initialization for structs,
check if identifier is a literal (e.g. tuple struct). If yes, then do
not apply short hand initialization.

Added test case to validate the changes for the fix.
RajivTS added a commit to RajivTS/rustfmt that referenced this issue Aug 28, 2022
Closes rust-lang#5488

Fix for rust-lang#5488. Before applying shorthand initialization for structs,
check if identifier is a literal (e.g. tuple struct). If yes, then do
not apply short hand initialization.

Added test case to validate the changes for the fix.
calebcartwright pushed a commit that referenced this issue Feb 2, 2023
Closes #5488

Fix for #5488. Before applying shorthand initialization for structs,
check if identifier is a literal (e.g. tuple struct). If yes, then do
not apply short hand initialization.

Added test case to validate the changes for the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted only-with-option requires a non-default option value to reproduce p-low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@calebcartwright @foxcob @RajivTS and others