Fix non-idempotent block doc comment closer rewrite - #7017
Conversation
Diff-Checkhttps://github.com/rust-lang/rustfmt/actions/runs/31167102874/job/92830360025 Two failures: I feel like the new formatting is actually more correct, at least that's how the Java block doc comments I've seen are formatted. According to the comment, I think the new formatting is actually the intended formatting? AFAICT the style guide doesn't specify this. |
| /**Second comment. | ||
| foo | ||
| */ |
There was a problem hiding this comment.
hmm things didn't get aligned in this case. Was that expected?
There was a problem hiding this comment.
Ah yeah, let me double check
There was a problem hiding this comment.
I'm not 100% if this is the intended alignment per se, but at least the stable formatting is also not aligned (versus when there's a continuing *)
rustfmt 1.9.0-stable (88d9e12ae1 2026-08-18):
$ rustfmt +stable foo.rs --check --config-path=/dev/null
Diff in /home/joe/repos/rustfmt/foo.rs:3:
*/
pub mod outer {
/**Second comment.
-foo
-*/
+ foo
+ */
pub struct Inner {
/**Third comment.
-*/
+ */
pub octets: Vec<u8>,
}
}The second comment case is idempotent for stable/nightly.
|
Two failures:
Overall I think this new alignment makes sense. Should we turn these failures into formatting test cases? |
See <rust-lang#6639>. Compared to the reported MCVE, the test case added: - Adds another layer of outermost block doc comment for the outer module. - Also exercises inner block doc comments (the reported example involves only outer block doc comments).
# The symptom
Consider the example
```rs
pub mod outer {
/**First comment.
*/
pub struct Inner {
pub octets: Vec<u8>,
}
}
```
Previously, this was *non-idempotent* and required **2** passes to
converge to a final formatting.
## Format 1
Format pass 1 tries to realign the block doc comment closer with the
starter `/**`. Notice that the closer `*/` aligns with `/**` without a
leading whitespace in the closer.
```diff
pub mod outer {
/**First comment.
-*/
+ */
pub struct Inner {
pub octets: Vec<u8>,
}
```
After this pass, if you run `rustfmt --check` on this, rustfmt will
report formatting difference:
```text
Diff in /Users/joe.xu/Documents/repos/rustfmt/foo.rs:1:
pub mod outer {
/**First comment.
- */
+ */
pub struct Inner {
pub octets: Vec<u8>,
}
```
## Format 2
Run `rustfmt` again, then formatting converges to
```rs
pub mod outer {
/**First comment.
*/
pub struct Inner {
pub octets: Vec<u8>,
}
}
```
Notice that the closer `*/`'s `*` now has a leading whitespace, aligning
it with the first `*` in the starter `/**`.
# Analysis
What went wrong? The formatting pathways hit here are fairly localized,
and mostly concern the pathway `identify_comment ->
light_rewrite_comment`.
The `light_rewrite_comment` helper has a bug where the implementation
doesn't agree with the indent expressed by the comment:
```text
// This is basically just l.trim(), but in the case that a line starts
// with `*` we want to leave one space before it, so it aligns with the
// `*` in `/*`.
```
Take the same `/**First comment.\n*/` example, the execution trace
broadly looks like:
```
rustfmt_nightly::comment::identify_comment{}
TRACE rustfmt_nightly::comment style=DoubleBullet
TRACE rustfmt_nightly::comment block comment
TRACE rustfmt_nightly::comment has_bare_lines=false, first_group_ending=20
TRACE rustfmt_nightly::comment first_group="/**First comment.\n*/", rest=""
TRACE rustfmt_nightly::comment !normalize_comments && !wrap_comments && !(is_doc_comment && format_code_in_doc_comments)
rustfmt_nightly::comment::light_rewrite_comment{orig="/**First comment.\n*/", offset=Indent { block_indent: 4, alignment: 0 }, is_doc_comment=true}
TRACE rustfmt_nightly::comment first_non_whitespace=0
TRACE rustfmt_nightly::comment left_trimmed="/**First comment."
TRACE rustfmt_nightly::comment first_non_whitespace=0
TRACE rustfmt_nightly::comment left_trimmed="*/"
```
The previous implementation was
```rs
let first_non_whitespace = l.find(|c| !char::is_whitespace(c));
let left_trimmed = if let Some(fnw) = first_non_whitespace {
if l.as_bytes()[fnw] == b'*' && fnw > 0 {
&l[fnw - 1..]
} else {
&l[fnw..]
}
} else {
""
};
```
The problem was that, when the closer line has no leading whitespace,
i.e. `l = "*/"`, `first_non_whitespace` would trigger on `*`, and so
`fnw = 0`. This would hit the else branch `&l[fnw..]`, which eventually
produces an closer alignment without a leading whitespace:
```rs
/**
*/ <- missing leading whitespace
```
# Fix
The key logical fix here is to drop the `fnw > 0` condition, and in the
`*`-leading branch, pad a whitespace and directly use the
leading-whitespace trimmed portion `&l[fnw..]`.
I used a `Cow` here because the leading-whitespace padding would need a
`format!` allocation, whereas the other branch need only a string slice
and no additional allocation. *Maybe* it's not worth the extra
complexity and we should just use `String`, but yeah.
For better or worse, for outer block doc comments, this patch does
unfortunately change the comment formatting for cases like
```
$ cat foo.rs
/**
*/
mod foo {}
```
Stable rustfmt considers this already well-formatted.
```bash
$ rustfmt +stable --version
rustfmt 1.9.0-stable (8bab26f4f6 2026-07-14)
```
```bash
$ rustfmt +stable foo.rs --check --config-path=/dev/null
```
With this patch, we only consider the closer well-formatted if its
asterisk `*` is aligned with the first `*` in the opener.
```
$ rustfmt-dev foo.rs --check --config-path=/dev/null
Diff in /Users/joe.xu/Documents/repos/rustfmt/foo.rs:1:
/**
-*/
+ */
mod foo {}
```
I believe this is acceptable, since:
1. Comments are explicitly carved out from stable formatting stability
guarantees, and
2. This impacts block doc comments, which IME is extremely rarely used.
The difference manifests in: (1) The closer's asterisk `*` now alignments with starter asterisk. Previously there was a `rustc_mir_transform` block doc comment that had a misaligned closer: ```rs /** * Integrator. * * Integrates blocks from the callee function into the calling function. * Updates block indices, references to locals and other control flow * stuff. */ <-- ``` This is now properly aligned with the starter. (2) Continuing asterisks now align with starter asterisk. Previously there was a `rustc_codegen_llvm` block doc comment ```rs /* * A note on nomenclature of linking: "extern", "foreign", and "upcall". * [...] * */ ``` where continuing asterisks were flush with line beginnings and not aligned with starter asterisk. Continuing asteriks now align with the starter asterisk.
e95bcc4 to
892ea2c
Compare
|
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. |
|
Changes since last review:
@rustbot review |
There was a problem hiding this comment.
This solves a real idempotence issue so I think we can move forward as is. We'll address the alignment without an asterisk * in a follow up PR. Since this impacts stable default formatting we should emphasize this change when updating the CHANGELOG.
Summary
Fixes #6639, where the formatting for block doc comments (both inner and outer styles) was non-idempotent on the closer, and takes 2 format passes to converge.
The symptom
Consider the example
Previously, this was non-idempotent and required 2 passes to converge to a final formatting.
Format pass 1
Format pass 1 tries to realign the block doc comment closer with the starter
/**. Notice that the closer*/aligns with/**without a leading whitespace in the closer.pub mod outer { /**First comment. -*/ + */ pub struct Inner { pub octets: Vec<u8>, }After this pass, if you run
rustfmt --checkon this, rustfmt will report formatting difference:Format pass 2
Run
rustfmtagain, then formatting converges toNotice that the closer
*/'s*now has a leading whitespace, aligning it with the first*in the starter/**.Analysis
What went wrong? The formatting pathways hit here are fairly localized, and mostly concern the pathway
identify_comment -> light_rewrite_comment.The
light_rewrite_commenthelper has a bug where the implementation doesn't agree with the intent expressed by the comment:Take the same
/**First comment.\n*/example, the execution trace broadly looks like:The previous implementation was
The problem was that, when the closer line has no leading whitespace, i.e.
l = "*/",first_non_whitespacewould trigger on*, and sofnw = 0. This would hit the else branch&l[fnw..], which eventually produces an closer alignment without a leading whitespace:Fix
The key logical fix here is to drop the
fnw > 0condition, and in the*-leading branch, pad a whitespace and directly use the leading-whitespace trimmed portion&l[fnw..].I used a
Cowhere because the leading-whitespace padding would need aformat!allocation, whereas the other branch need only a string slice and no additional allocation. Maybe it's not worth the extra complexity and we should just useString, but yeah.Test coverage
*-leading line between starter/closer lines, and a non-*-line between starter/closer lines.rustctree.Formatting stability
This PR technically changes stable default formatting. However:
Diff-Check {Edition 2024, Style Edition 2024}: https://github.com/rust-lang/rustfmt/actions/runs/31167102874/job/92830360025
AI usage disclaimer
I did not use an LLM to create changes or comments in this PR. I used an LLM to review the changes in this PR.