foundry-rs / foundry-rs/foundry

fix(fmt): forge fmt's CLI path bypasses its own idempotency/re-parse guard

Open
#16,649 0 comments 0 reactions 1 assignee Claimed by @figtracer View on GitHub
Dominant language
Rust
Stars
10.6k
Forks
2.6k
Avg merge
18h 20m
Merged PRs (30d)
510

Description

`forge fmt`'s CLI entry point silently bypasses the formatter's own idempotency/re-parse correctness guard - a mid-line comment-corruption bug (see the linked PR) would have been caught immediately if the guard were wired to the path that actually writes to disk.

## The two paths

`crates/fmt/src/lib.rs` has four public entry points. Three of them (`format_file`, `format_source`, `format`) route through `format_inner`, which formats twice and asserts:

```rust
// Check if the two passes produce the same output (idempotency)
match (first_result.ok_ref(), second_result.ok_ref()) {
(Some(first), Some(second)) if first != second => {
panic!("formatter is not idempotent:\n{}", diff(first, second));
}
_ => {}
}

assert!(
!(first_result.is_ok() && second_result.is_err() && !DEBUG),
"failed to format a second time:\n..."
);
```

The fourth entry point, `format_ast`, has neither check - single pass, no idempotency comparison, no re-parse validation. That is exactly what the CLI calls:

```
crates/forge/src/cmd/fmt.rs:247:
let formatted = forge_fmt::format_ast(gcx, source_unit, source_fmt_config)?;
```

So the safety net covers the library and test APIs and skips the one path that actually writes formatted output back to a user's files.

## Why this matters

The second assertion above is precisely the signature of a real corruption bug: first pass succeeds, second pass fails to re-parse. If `forge fmt` went through the guarded path, that class of bug would surface loudly the first time anyone hit it, instead of silently rewriting their source with no error. Confirmed this isn't a debug-only guard or recent drift: `const DEBUG: bool = false || option_env!("FMT_DEBUG").is_some()`, so `!DEBUG` is true in any normal release build, and the wrapper's logic is materially identical between the current 1.7.1 release and master (an explicit `if { panic! }` vs. an `assert!`, same semantics).

It's also why the crate's own testdata harness (`crates/fmt/tests/formatter.rs`, which asserts idempotency per test case) never caught the concrete for-loop bug: the harness *does* use the guarded path, but only over the fixed testdata corpus, and no corpus file happened to have a comment in a `for` header. Guarded path with the wrong inputs; the actual failing input only reaches the unguarded path.

## Why this is an issue rather than a PR

The obvious fix - repoint the CLI at `format_inner` - is wrong as stated, because the guard's failure mode is `panic!`/`assert!`. Converting silent corruption into a crash-with-backtrace on a user's own file isn't a fix, it's a different bad outcome (and hits the same "This is a bug, report it at github.com/foundry-rs/foundry" banner problem as other panic-on-input bugs). The right shape reuses the check's *logic*, not its panicking delivery: re-parse the CLI's own output and refuse to write, with a clean diagnostic, when it doesn't parse.

That still leaves real product decisions we're not positioned to make unilaterally:

- **Parseability only, or full idempotency?** Idempotency is the stronger guarantee (and the one the existing guard actually asserts), but likelier to trip on inputs that are merely ugly rather than broken.
- **Performance.** Any version of this formats every file twice. On a repo-wide `forge fmt` that's a real, user-visible cost - possibly worth gating behind a flag, or only enabling in `--check` mode.
- **Failure semantics.** Skip the offending file and continue, or fail the whole run? What should the diagnostic say?

## Related

The concrete bug this guard would have caught: #16648 (comment corruption in `for`-loop headers - `forge fmt` rewrites a trailing comment in a way that swallows the loop condition/increment into the comment, and the result can't be re-parsed by a second `forge fmt` pass).

Happy to open a PR once the three questions above have a preferred answer.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.