Repository navigation
Don't preserve first line of multi-line block comments independently of the rest of the comment - #7217
Don't preserve first line of multi-line block comments independently of the rest of the comment#7217randomPoison wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
The 2024 style edition is already stable. I think there's technically a carve out in the stability guarantee for comments, but I'm not 100% sure we should apply that here to change the current behavior.
@jieyouxu what do you think? I think we might need to gate this on 2027
There was a problem hiding this comment.
@randomPoison the issue also mentions wrap_comments, but the tests don't include that at all.
I'm also not sure the behavior is correct. It feels like multi-line comments should stay indented.
|
Reminder, once the PR becomes ready for a review, use |
55ed167 to
c76d391
Compare
|
Oh, sorry about the missing wrt indentation, I agree that the subsequent lines of the block comment should be indented to align with the first line, but that seemed like a breaking change to me. The existing logic gives subsequent lines block indentation, rather than aligning them with the first line. I also looked at correcting the indentation and it's pretty trivial to do so, but that seems like it would need an edition gate. I've added a second commit that fixes indentation with a 2027 edition gate, let me know how that looks. Without the indentation fix, does this need an edition gate? The existing behavior produced invalid code, so I would think that nobody could be relying on the stability of that behavior. |
|
@rustbot ready |
|
I was under the impression that we already aligned the comment so when I saw that it wasn't being aligned in the test that confused me. Assuming this isn't changing any behavior then I think we're good, but I'll take another look to confirm |
|
I also want to take a look at this more closely (specifically the impact on stable default format). Can we split this PR into 2, i.e.
I don't want to mix changes affecting only the unstable option together with changes that affects stable default format. |
88231f9 to
a9746aa
Compare
|
Sure, splitting the indentation change into its own PR makes sense, I've removed that commit from this PR and opened #7218. This PR now only addresses the |
That change should be landed independently of this branch, but I need the fix here in order to test how --file-lines handles multi-line block comments.
|
(Hold on I need to double-check, I think this is still affecting stable default format 🤔) |
|
Ugh yeah you're right, there are some differences in how block comments are laid out as a result of us now calling Edition-gating the new behavior would be easy, but that would leave the broken behavior in older editions. It would be nice to fix the bug in all editions while preserving the old layout behavior for the places where rustfmt was producing valid code. |
|
I've updated This feels a bit hacky to me since we're duplicating a little bit of logic from the other branch and from within |
There was a problem hiding this comment.
That this needs to affect stable default format still is surprising to me.
Edition-gating the new behavior would be easy, but that would leave the broken behavior in older editions. It would be nice to fix the bug in all editions while preserving the old layout behavior for the places where rustfmt was producing valid code.
I don't find the comment formatting changes to pull the weight for breaking SE 2024, so that would have to be style edition gated. Fixing the unstable options formatting doesn't need to be style edition gated.
| //@ rustfmt-style_edition: 2024 | ||
| //@ rustfmt-wrap_comments: false | ||
|
|
||
| fn main() {} /* first | ||
| second | ||
| third */ |
There was a problem hiding this comment.
Not for this PR: All 3 seems a bit surprising to me. I feel like we should not be modifying this comment when wrap_comments: false regardless of edition 🤔 This is regular block comment not block doc comment 🤔
| fn main() {} /* first | ||
| second | ||
| third */ |
There was a problem hiding this comment.
Not for this PR: This wrap comment behavior seems weird to me too, why should second lose all prefix new space, but not the final line with */?
There was a problem hiding this comment.
I'm finding it a bit difficult to understand what actually changed for stable default formatting. Could you help to make review easier by massaging this PR into only 2 commits:
- All the tests you have here, pre functional change.
- The actual functional change, with updated
tests/target/snapshots?
I'm trying to understand the diff but it's tricky.
There was a problem hiding this comment.
... Oh. Is it because this PR still has changes from #7218?
There was a problem hiding this comment.
Okay I've reworked the changes into 2 commits, the first one with all of the new tests showing the old behavior (including the places where rustfmt was producing invalid code), and the second one with the code changes and updated tests that reflect the new behavior.
Also as far as I can tell this PR doesn't have any of the changes from #7218, I already removed them.
| } else if self.config.style_edition() < StyleEdition::Edition2027 | ||
| && on_same_line | ||
| && !self.config.wrap_comments() | ||
| && !self.config.normalize_comments() | ||
| { | ||
| // Trailing block comments historically had each continuation line trimmed | ||
| // independently, with the first line preserved, which resulted in broken block | ||
| // comments when the first line was rewritten into a line comment independently | ||
| // of the continuation lines. This preserves the original behavior for older | ||
| // editions in the cases where rustfmt was producing valid code. | ||
| if let Some((first_line, other_lines)) = subslice.split_once('\n') { | ||
| self.push_str(first_line); | ||
| self.push_str(&comment_indent.to_string_with_newline(self.config)); | ||
| self.push_str(&light_rewrite_comment( | ||
| other_lines, | ||
| comment_indent, | ||
| self.config, | ||
| false, | ||
| )); | ||
| } else { | ||
| self.push_str(subslice); | ||
| } |
There was a problem hiding this comment.
[REWRITE 1/2]: I'm a bit confused. Can you say more on why this needs to impact stable default formatting? In particular, why are we using light_rewrite_comment rather than rewrite_comment?
There was a problem hiding this comment.
The impact to stable formatting comes from the fact that we're now feeding the entire block comment into rewrite_comment. Previously the first line was preserved verbatim, and only the continuation lines were given to rewrite_comment. This caused identify_comment (called by rewrite_comment) to incorrectly treat the continuation lines as // comments, which then resulted in the lines being fed through light_rewrite_comments, which is what gives us the behavior of trimming the leading whitespace on those continuation lines. With normalize_comments enabled, this also caused only the continuation lines to be rewritten with //.
With the new logic, the whole comment goes through rewrite_comment, which allows identify_comment to correctly identify it as a /* block comment, which then results in the comment going through trim_left_preserve_layout, giving a slightly different behavior of trimming the leading whitespace on the continuation lines. This is where the impact to stable formatting comes from, since trim_left_preserve_layout rewrites the continuation lines slightly differently than light_rewrite_comments did.
With normalize_comments and wrap_comments disabled, the behavior given by light_rewrite_comments for the continuation lines is the stable behavior. So the compatibility fix I added preserves the behavior of passing only the continuation lines through light_rewrite_comments when normalize_comments and wrap_comments are disabled, while allowing the full comment to go through rewrite_comment on the newer edition or in the case where rustfmt was previously producing invalid code.
If we don't care about fixing the bug in the 2024 edition, we can remove the compatibility branch entirely, and gate the bug fix on the 2027 edition. This would allow us to keep light_rewrite_comment private, and would be a smaller change, at the cost of leaving the bug in the 2024 edition.
There was a problem hiding this comment.
Oh, also, to be clear, the changes in this PR as it exists right now should NOT affect stable formatting. This extra branch I've added where we directly call light_rewrite_comment is specifically preserving the stable behavior. When I say above that it affects stable formatting, I'm referring to what would happen without this extra preservation branch.
There was a problem hiding this comment.
I found a better way to preserve the existing stable behavior, so I've removed this compatibility branch entirely. The new logic also has a comment that tries to contextualize the expected behavior, let me know if it makes sense to you.
Capture the pre-fix output for wrapping, normalization, and style editions, including the malformed comments produced by the existing formatter.
4ab562a to
66b68ac
Compare
|
@rustbot ready General question about procedure: Should I |
66b68ac to
aa8e830
Compare
Yeah, this helps a lot (at least for me), since I use the labels to filter which PRs I should revisit in my review queue (I have too many github notifications and they're way too easy to lose). I'll take another look soon. ... Which is a good point since I yoinked the labels from rust-lang/rust but didn't backlink contrib docs from e.g. https://rustc-dev-guide.rust-lang.org/pr-lifecycle.html#waiting-for-reviews |
Fixes #6339
In style edition 2024, if there are multiple lines of line comments and the first line trails some code, we preserve the first line independently of the subsequent lines which get rewritten separately. This logic was also being applied to block comments, causing the first line of the comment to be preserved while subsequent lines are rewritten into line comments, causing the block comment to be unclosed. This PR updates the logic to only apply this first line preservation to line comments.
AI Usage Disclosure
An LLM was used to:
/* ... */comment into/* ... //, with no terminating*/#6339 and turn it into a test fixture.