Repository navigation
Struct fields: remove unnecessary parens - #7161
matthewhughes934 wants to merge 2 commits into
Conversation
rustbot
added
the
S-waiting-on-author
| .rewrite_result(context, Shape::legacy(budget, shape.indent + 1)) | ||
| .map(|ty_str| format!("({})", ty_str)); | ||
| } | ||
| let (ty, _, _, _, _) = unwrap_parens( |
There was a problem hiding this comment.
Discuss: should this have a style edition gate? I feel like it is a bug fix (inconsistency in expression vs. type formatting), but it also could change existing code.
There was a problem hiding this comment.
@matthewhughes934 for now let's add the style edition gate
There was a problem hiding this comment.
@matthewhughes934 for now let's add the style edition gate
👍 36d00f6
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Like is already done for expressions, e.g.
let _ = ((var)) -> let _ = (var)
Do this by extending the function used for expressions. This is part of
my working looking in to issue rust-lang#6642 (but doesn't
directly address it)
5549214 to
deb911f
Compare
| // rustfmt-style_edition: 2027 | ||
|
|
||
| struct F { | ||
| f: ((u32)), |
There was a problem hiding this comment.
Are parens removed if there are comments?
There was a problem hiding this comment.
Should probably also add a test case with a lot more parens. Something like f: (((((u32))))), to make sure it's idempotent and doesn't just remove one set of parens every time you run rustfmt.
There was a problem hiding this comment.
Are parens removed if there are comments?
only parens without any leading/trailing comments, e.g.
struct S {
f: (/* comment */((u8))),
}Would be re-written to:
struct S {
f: ((u8)),
}Notably:
-
The comment is still lost (since this change doesn't resolve the original issue)
-
This causes a non-idempotent format, since
struct S { f: ((u8)), }
is then formatted to
struct S { f: (u8), }
So I think this shouldn't be a standalone change, and I'll need to just combine it with my change to not remove those comments (I created this PR to try and split that work into a few smaller changes)
There was a problem hiding this comment.
Also, if this doesn't already it should probably also be configured using remove_nested_parens. We'll want to add a test case to show that parens aren't removed when remove_nested_parens=false. It's true by default so it's not necessary in this case, but might be good to be explicit about it.
I think we should also add a style_edition=2024 test case to show that parens aren't removed even with remove_nested_parens=true to highlight the issue we're solving.
ytmimi
added
S-waiting-on-author
changing the order, I think #7168 needs to go first |
This branch has not been deployed
3 participants
Like is already done for expressions, e.g.
Do this by extending the function used for expressions. This is part of
my working looking in to issue #6642 (but doesn't
directly address it)