fix(fmt): don't corrupt a for-loop's header with a trailing comment on the init clause - #16648
Open
gomesalexandre wants to merge 5 commits into
Open
Conversation
…n the init clause
forge fmt silently corrupted source code when a for-loop's init clause
(e.g. `uint256 i = 0`) had a trailing // comment sharing its source line
with the rest of the header (condition/increment/brace):
for (uint256 i = 0; i < 10; ++i) { // step
x++;
}
became, after forge fmt:
for (uint256 i = 0; // step i < 10; ++i) {
x++;
}
The loop condition and increment were swallowed into the comment, and a
second forge fmt pass failed to re-parse the corrupted output. Since
forge fmt writes formatted output back to disk in place, a repo-wide run
could silently destroy every for loop with a trailing comment on its
header line - recoverable only via git.
Root cause: print_stmt's tail flushes any trailing comment sharing the
statement's last source line via an unbounded scan (print_trailing_comment_no_break(..., None)).
print_for_stmt is the only call site that prints a statement (the init
clause) mid-line rather than as the last thing on its line, so the
unbounded scan swallowed everything printed afterward into the comment.
Fix: added print_stmt_bound, a bounded variant of print_stmt that
suppresses the init clause's own trailing-comment consumption entirely.
The comment is left in the stream and picked up correctly by the
existing print_comments flush that already runs after the header is
fully printed, regardless of where in the header the comment sits.
Every other print_stmt call site is unaffected (print_stmt is now a thin
wrapper passing None, identical to its prior behavior).
Note: the formatter's own double-pass idempotency/re-parse guard
(format_inner) would have caught this immediately, but it isn't wired to
forge fmt's actual write path (format_ast). That's filed separately,
since fixing the wiring involves its own perf/failure-semantics tradeoffs
this PR doesn't make.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gomesalexandre
marked this pull request as ready for review
September 4, 2026 22:35
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
September 4, 2026 22:35
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
figtracer
reviewed
Sep 4, 2026
figtracer
reviewed
Sep 4, 2026
figtracer
self-requested a review
September 4, 2026 22:53
figtracer
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
forge fmtsilently corrupts source code when afor-loop's init clause (e.g.uint256 i = 0) has a trailing//comment sharing its source line with the rest of the header (condition/increment/brace):becomes, after
forge fmt:The loop condition and increment get swallowed into the comment, and a second
forge fmtpass fails to re-parse the corrupted output:Since
forge fmtwrites formatted output back to disk in place, a repo-wide run silently destroys everyforloop with a trailing comment on its header line — recoverable only via git. This is the only finding of a wider OSS bug-hunting pass I've been doing on this repo that destroys user data rather than just printing something wrong or crashing.Root cause
print_stmt's tail (crates/fmt/src/state/sol.rs) flushes any trailing comment sharing the statement's last source line via an unbounded scan:print_trailing_comment_no_break(stmt.span.hi(), None). TheNonemeans "no upper bound — scan forward for the next comment, wherever it is."Every statement-printing call site in the crate prints one statement as the last thing on its own line, where an unbounded scan is harmless (nothing else follows on that line to swallow).
print_for_stmt's call for the init clause (self.print_stmt(init_stmt)) is the exception: the init clause shares its line with the condition, increment, and closing brace that get printed after it returns. Since the whole header sits on one line, the unbounded scan finds the trailing comment and prints it immediately — before the rest of the header exists — so everything printed afterward lands after a//on the same output line.Fix
Added
print_stmt_bound, a bounded variant ofprint_stmt(now a thin wrapper:print_stmt_bound(stmt, None), identical behavior for every other caller). The for-loop init call site now suppresses the init clause's own trailing-comment consumption entirely (rather than bounding it to "the next header clause's position", which an adversarial review round showed still corrupts when the comment sits between clauses — e.g.for (uint i = 0; // c\n i < 10; ++i)— since bounding just prevents the scan window from reaching that far, it doesn't stop the same-line print that follows). Leaving the comment unconsumed at the init step means it stays in the comment stream and gets picked up correctly by the flush that already runs once the entire header has been printed, regardless of which part of the header the comment originally followed.Testing
crates/fmt/testdata/ForStatementComments/(a new corpus, since the existingForStatement/corpus has no comment cases) covering the three original shapes plus three more an adversarial review round surfaced: a comment right after the header with no condition/increment, a missing condition, and a missing increment — all now re-parse cleanly and are single-pass idempotent.crates/fmttest suite (67 tests) passes,cargo clippy -- -D warningsclean,cargo fmt --checkclean.print_for_stmt's init call is the only bare (unbounded)print_stmtcall in the entire crate — every other call site already threads an explicit position bound viaprint_stmt_as_block. This fix doesn't touch or need to touch any of those.Known residual limitation, disclosed rather than hidden: one narrow, unusual shape — a comment immediately after the init clause's own semicolon, forcing the header onto multiple lines (
for (uint256 i = 0; // c\n i < 10; ++i) {}) — converges to a stable, valid, comment-preserving state by the secondforge fmtpass rather than the first (a cosmetic blank-line-placement difference, not corruption; content is preserved and the file always re-parses at every step). This is a symptom of how the formatter's comment-style classification (isolated vs. mixed) is computed from raw source layout rather than being a stable AST property, which is a broader property of the pretty-printer's comment handling, not something specific to this fix's call site. I didn't chase a full single-pass fix for this one sub-shape given its narrowness and the fact that it never risks data loss.One related, pre-existing observation, filed separately rather than folded in here: the formatter has its own double-pass idempotency/re-parse guard (
format_innerincrates/fmt/src/lib.rs) that would have caught the original bug on its very first real-world run — but it isn't wired toforge fmt's actual write path (format_ast, called fromcrates/forge/src/cmd/fmt.rs). Fixing that wiring involves its own perf (formatting every file twice) and failure-semantics (skip vs. fail the whole run) tradeoffs that are a maintainer's call, not something to bundle into this data-corruption fix.receipts
no runtime UI change —
forge fmt's own test suite plus the manual CLI-level round-trip verification described above are the receipts for this PR (a formatter correctness fix, not a runnable app).