Format match exprs even when or-patterns have comments#6893
Conversation
When encountering comments in between elements of an or-pattern in a `match` expression, format the matched value, the arm body and the patterns of all other arms. *Only* skip the pattern with embedded comments.
| () => {} | ||
| () // Comment | ||
| | () => { | ||
| println!("Foo"); |
There was a problem hiding this comment.
This highlights the outstanding problem with this approach: the pattern is kept with bad formatting, where this should instead have been indented to the proper level.
|
Diff-Check failed ❌. We probably have to gate this change on |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
Using rust-lang/rustfmt#6893, reformat the codebase. The result is that matches that *would have* been formatted under normal circumstances get their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime.
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
| /// rust edition 2027 | ||
| Edition2027, |
There was a problem hiding this comment.
Wait, you mentioned in the FIXME below that rustc doesn't have an edition 2027 yet. In that case I'd prefer we don't add that here yet. It's fine that rustfmt has it's own style_edition to gate formatting changes outside of editions that are needed for parsing.
Please revert the Edition::Edition2027 changes from this PR. As long as things are gated on StyleEdition::Edition2027 then we should be good to move forward.
Yeah, anything that could potentially introduce breaking formatting changes needs to be gated. From what I understand that's how the team has pretty much always operated. Things used to be gated on |
|
@rustbot author |
[style] rustfmt `match`es with comments in or-patterns Using rust-lang/rustfmt#6893, I reformatted the whole codebase. The result is that `match`es that *should have* been formatted under normal circumstances but are getting skipped now got their expected format. These match expressions were being entirely skipped because they contain or-patterns with comments in between patterns, causing rustfmt to bail out entirely. The or-patterns with comments themselves remain untouched, but now the match arm bodies and other patterns without comments do get formatted under that PR. Because the fix in rustfmt isn't landed yet, I reworked some of the or-patterns with comments so that formatting doesn't regress. Tried doing this only in larger blocks that are more likely to regress in the meantime. (Introduced and) removed a bunch of stray backticks \` likely left after an editor autoclosed the intended closing \`, resulting in <code>\`name\`\`</code> in comments.
|
Please verify whether the new code is what you requested. @rustbot review |
| #[value = "2027"] | ||
| #[doc_hint = "2027"] | ||
| /// Edition 2027. | ||
| Edition2027, |
There was a problem hiding this comment.
Please revert this Edition2027 change.
| // FIXME: rustc doesn't have an edition 2027 yet. | ||
| Edition::Edition2027 => Self::Edition2024, |
There was a problem hiding this comment.
Please revert this edition 2027 change
| // rustfmt style_edition 2027 | ||
| Edition2027, |
There was a problem hiding this comment.
Currently, the diff check is meant to test against stable formatting changes. We don't need to test Edition2027 because it's unstable. Please revert all Edition2027 changes made to check_diff
| "style-edition", | ||
| "The edition of the Style Guide (unstable).", | ||
| "[2015|2018|2021|2024]", | ||
| "[2015|2018|2021|2024|2027]", |
There was a problem hiding this comment.
Personally, I'd rather we kept these changes focused on match formatting. Please revert this change. Happy to make this change in a different PR.
| Edition::Edition2018 => StyleEdition::Edition2018, | ||
| Edition::Edition2021 => StyleEdition::Edition2021, | ||
| Edition::Edition2024 => StyleEdition::Edition2024, | ||
| Edition::Edition2027 => StyleEdition::Edition2027, |
There was a problem hiding this comment.
Please revert this edition 2027 change
| // Style Edition 2024 | ||
| // Style Edition 2027 |
There was a problem hiding this comment.
Thank you for catching this typo. As mentioned in earlier comments I'd prefer to keep this PR focused on match formatting. Please revert this change.
|
|
||
| fn empty_result(s: &RewriteResult) -> bool { | ||
| !matches!(*s, Ok(ref s) if !s.is_empty()) | ||
| !matches!(*s, Ok(ref s) | Err(RewriteError::InlineComment(ref s)) if !s.is_empty()) |
There was a problem hiding this comment.
Can you help me better understand this change.
| Err(RewriteError::InlineComment(item)) => { | ||
| // Keep the original contents and recover, allowing the subsequent items to be | ||
| // formatted while keeping the original formatting of this item. | ||
| // This is currently only relevant to or-patterns with comments in between patterns. | ||
| // We gate this under future edition 2027 because the amount of code that needs to | ||
| // be reformatted in the wild is quite high. | ||
| item | ||
| } |
There was a problem hiding this comment.
Where is the 2027 gate that you're referring to? Shouldn't this be gated on StyleEdition::Edition2027?
| /// We will keep the original code, report an error, but recover and format code that comes | ||
| /// later properly. | ||
| #[error("Formatting was skipped due to a comment that would have been removed.")] | ||
| InlineComment(String), |
There was a problem hiding this comment.
Is the String meant to represent the entire rewrite or just the comment? Maybe that should be made clearer.
There was a problem hiding this comment.
Please don't modify existing tests. Create a new 2027 test case.
There was a problem hiding this comment.
Please don't modify existing tests. Create a new 2027 test case.
When encountering comments in between elements of an or-pattern in a
matchexpression, format the matched value, the arm body and the patterns of all other arms. Only skip the pattern with embedded comments.Fix #4119, fix #6491, cc #6044, cc #6663.
An alternative approach would be to make pre and post comments part of all patterns, which would allow us to keep them around and format or patterns properly, but wanted to have the minimal possible change that could address a common papercut I've noticed in
rustc.