Make sure we don't lose default struct value when formatting struct #134668
Conversation
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
rustbot
added
the
S-waiting-on-review
src/tools/rustfmt/src/items.rs
Outdated
| if field.default.is_some() { | ||
| return Err(RewriteError::SkipFormatting); | ||
| } |
There was a problem hiding this comment.
RewriteError::SkipFormatting is used to represent #[rustfmt::skip]. I think in this case we could just return Ok(context.snippet(field.span()).to_owned()); (assuming that the span contains the default), or Err(RewriteError::Unknown. At some point I want to add a RewriteError variant for unstable features that don't have defined formatting yet.
compiler-errors
force-pushed
the
default-struct-value-fmt
branch
from
1c691e9 to
3a5362c
Compare
| @@ -1947,6 +1947,10 @@ pub(crate) fn rewrite_struct_field( | |||
| if contains_skip(&field.attrs) { | |||
| return Ok(context.snippet(field.span()).to_owned()); | |||
| } | |||
| // FIXME(default_field_values): Implement formatting. | |||
| if field.default.is_some() { | |||
| return Ok(context.snippet(field.span()).to_owned()); | |||
There was a problem hiding this comment.
I think we need to change this to return Err(RewriteError::Unknown); for now. I don't think the field.span() contains the default values. At least I still saw the error reported in rust-lang/rustfmt#6424 when I built rustfmt from this branch and ran it on
frame_support::construct_runtime!(
pub struct Test {
System: frame_system = 0,
SelfDomainId: pallet_domain_id = 1,
}
);If possible, could you also include the above snippet in the test case.
There was a problem hiding this comment.
You're right, the field span doesn't include the optional const. I just didn't actually re-test my test.
I'm not going to include the snippet because it's identical in behavior to the snippet I included, though. I'll adjust the behavior though, and make sure to re-test it.
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
default-struct-value-fmt
branch
from
3a5362c to
68c46e1
Compare
| @@ -1944,6 +1944,11 @@ pub(crate) fn rewrite_struct_field( | |||
| shape: Shape, | |||
| lhs_max_width: usize, | |||
| ) -> RewriteResult { | |||
| // FIXME(default_field_values): Implement formatting. | |||
There was a problem hiding this comment.
This also should go above the contains_skip since that would mess up:
struct Foo2 {
#[rustfmt::skip]
default_field: Spacing = /* uwu */ 0,
}
which I added a test for.
|
@bors r+ |
|
@ytmimi: 🔑 Insufficient privileges: Not in reviewers |
|
@bors r=ytmimi |
bors
added
S-waiting-on-bors
…ue-fmt, r=ytmimi Make sure we don't lose default struct value when formatting struct The reason why rust-lang/rustfmt#6424 broke when rust-lang#129514 landed is because the parser now *successfully* parses default struct values. That means that where we used to fail in `rewrite_macro_inner`: https://github.com/rust-lang/rust/blob/e108481f74ff123ad98a63bd107a18d13035b275/src/tools/rustfmt/src/macros.rs#L263-L267 ... we now succeed, and we now proceed to format the inner struct as a macro arg. Since formatting was never implemented for default struct values, this means that we’ll always rewrite the struct to exclude the default value. This PR makes it so that we simply don’t rewrite struct fields if they have a default value. r? `@ytmimi`
The reason why rust-lang/rustfmt#6424 broke when #129514 landed is because the parser now successfully parses default struct values. That means that where we used to fail in
rewrite_macro_inner:rust/src/tools/rustfmt/src/macros.rs
Lines 263 to 267 in e108481
... we now succeed, and we now proceed to format the inner struct as a macro arg. Since formatting was never implemented for default struct values, this means that we’ll always rewrite the struct to exclude the default value.
This PR makes it so that we simply don’t rewrite struct fields if they have a default value.
r? @ytmimi