Repository navigation
fix: prevent rustfmt from removing necessary blocks in cfg_select! arms - #7183
Conversation
There was a problem hiding this comment.
Thanks. Unfortunately as we know from
the "how many braces" problem is quite quirky. The heuristics here is not sufficiently precise (i.e. currently we would try to format more than we actually can).
Disclaimer: I reviewed this in a first pass, and then co-reviewed it again with an LLM, which came up with some adversarial cases I didn't initially consider (that caught me off guard as well). I double-checked the cases/reasoning. All communications are hand-written.
Could you elaborate with a concrete example? I'm not sure that I follow. |
Sorry, to clarify, I specifically mean
(I know that this PR is only about not removing when we can't, not adding.) |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Can we add snapshot test cases for
cfg_select! {
_ => { // l1
{ x } // l2
}
}and
cfg_select! {
_ => /* comment */ {{ x }}
}I don't think this is blocking though (snapshot moreso to capture if that gets fixed in a follow-up).
I think we might be able to check the spans between
=> { { inner_stmts } }
arrow_span
outer_lbrace_span
inner_lbrace_span
inner_stmts[0].span
...
inner_rbrace_span
outer_rbrace_span
i.e. if there are comments in any of the following intervals
arrow_span.between(outer_lbrace_span)outer_lbrace_span.between(inner_lbrace_span)inner_rbrace_span.between(outer_rbrace_span)outer_rbrace_spanto end of body (?)
we don't take the "happy" {{}} path. EDIT: maybe only between outer bracee and inner braces?
There was a problem hiding this comment.
Tracked unexpectedly removed comments as follow-up in #7191.
| let trailing_comma = if arm.trailing_comma.is_some() { | ||
| "," | ||
| } else { | ||
| "" | ||
| }; |
There was a problem hiding this comment.
Should/does this need to account for match_block_trailing_comma? Since we add this unconditionally for this "happy" case but conditionally wrt. match_block_trailing_comma for the None branch via rewrite_match_body?
Lines 476 to 480 in 1738c93
There was a problem hiding this comment.
^ Only if you want to address this in this PR. I don't want to block on this, fine as a follow-up as well.
|
(Sorry, I know this is a lot of back-and-forth, didn't notice some things in earlier review passes) |
@jieyouxu I genuinely appreciate it. You're helping me catch a lot of things I missed too. I felt a real sense of urgency to get this fixed after learning that this was impacting miri, and I definitely jumped in without considering a lot of these edge cases. |
|
Yeah no worries, it's very subtle and I would've made exactly the same mistakes... There's just, edge cases for edge cases. |
This comment has been minimized.
This comment has been minimized.
|
Okay, added more edge case tests based on the discussion here and a few other things I wanted to check. |
| fn cfg_select_comments_that_might_get_lost() { | ||
| cfg_select! { | ||
| _ => {{ x }} | ||
| } | ||
| cfg_select! { | ||
| _ => {{ x }} | ||
| } | ||
| cfg_select! { | ||
| _ => {{ x }} | ||
| } | ||
|
|
||
| cfg_select! { |
There was a problem hiding this comment.
Yeah, this is unfortunate but reasonable to address this in a follow-up.
| let trailing_comma = if arm.trailing_comma.is_some() { | ||
| "," | ||
| } else { | ||
| "" | ||
| }; |
There was a problem hiding this comment.
^ Only if you want to address this in this PR. I don't want to block on this, fine as a follow-up as well.
| _ => { | ||
| (/// | ||
| {}) | ||
| } |
There was a problem hiding this comment.
Interesting, we actually add a brace here.
| fn fails() { | ||
| let x = cfg_select! { | ||
| // rustfmt will add braces here | ||
| _ => | ||
| { | ||
| #[cfg(true)] | ||
| 1 | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This one seems quite surprising, but still I'm fine as a follow-up since it only affects unstable language w/ stmt_expr_attributes. The brace adding here is a bit funny, because
#![feature(stmt_expr_attributes)]
fn main() {
let x = cfg_select! {
// rustfmt will add braces here
_ => #[cfg(true)] 1
};
}is invalid syntax, but
#![feature(stmt_expr_attributes)]
fn main() {
let x = cfg_select! {
// rustfmt will add braces here
_ => { #[cfg(true)] 1 }
};
}now compiles 😁 So this is a invalid syntax => compile pass. Program repair gaming.
Considering this is fairly niche it's probably best as a follow-up.
|
@jieyouxu I'll address the wrapping behavior / |
…arms Fixes rustfmt 7045
…nner Also adds some extra test cases to make sure that rustfmt isn't adding / removing braces that would change the code semantics.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Thanks @ytmimi, one last thing (the 2024 edition) in the tests, but otherwise this looks good to me.
You can r=me after adding //@ rustfmt-edition: 2024.
Done! will add this to the queue after CI passes |
btw -- since now we use merge queues for rustfmt repo, you can enable "auto-merge" ("Merge when ready" button in the UI), and it won't be added to the merge before before CI is green 😉 |
rustfmt subtree update 2026-10-22 Subtree update of `rustfmt` to rust-lang/rustfmt@b42f1d8. Created using https://github.com/rust-lang/josh-sync. rustc pull direction: rust-lang/rustfmt#7206 ## Notable PRs - rust-lang/rustfmt#6851 - rust-lang/rustfmt#7048 - rust-lang/rustfmt#7170 - rust-lang/rustfmt#7171 - rust-lang/rustfmt#7174 - rust-lang/rustfmt#7179 - rust-lang/rustfmt#7181 - rust-lang/rustfmt#7183 - rust-lang/rustfmt#7192 --- r? @ytmimi
Fixes #7045