Parenthesize async when it starts a for-of loop initializer#31326
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 26 minutes and 5 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:02 AM PT - May 24th, 2026
❌ @robobun, your commit b0a9218 has some failures in 🧪 To try this PR locally: bunx bun-pr 31326That installs a local version of the PR into your bun-31326 --bun |
There was a problem hiding this comment.
LGTM — small, well-tested printer correctness fix mirroring the existing let handling; the inline nit about EIndex is cosmetic (output is still valid JS) and not blocking.
Extended reasoning...
Overview
This PR fixes a fuzzer-found printer bug in src/js_printer/lib.rs: when the for-of init is the identifier async (e.g. parsed from the escaped spelling \\u0061sync), the printer emitted the bare token sequence for (async of …), which violates the spec's [lookahead ∉ { let, async of }] restriction and fails to reparse in both Bun and JSC. The fix adds an IsFollowedByOf expr flag that SForOf threads through print_for_loop_init, and the EIdentifier arm now wraps async in parens when it sits at for_of_init_start with that flag set — exactly mirroring the existing let wrap on the same line. SForIn and the plain SFor pass ExprFlag::none(), so they're unaffected. A new test block in transpiler.test.js covers the positive case, the for-await case, and the negative cases (async.x, x[async], for-in, and the existing (let) behavior).
Security risks
None. This is purely a JS-printer output-shaping change — no I/O, auth, parsing of untrusted config, or memory-unsafety surface. The only behavioral effect is adding a pair of parentheses in a narrow grammar corner.
Level of scrutiny
Low-to-moderate. The diff is ~30 lines, additive, and slots into a well-established pattern (the let-at-for-of-init-start wrap that was already on the same line). The flag plumbing through print_for_loop_init is mechanical and only the SExpr branch consumes it. The PR description notes this matches esbuild's handling, and the full transpiler test suite passes.
Other factors
The bug-hunting system flagged one nit: EIndex forwards flags unfiltered to its target (line 3826), so for (\\u0061sync[0] of …) would print as for ((async)[0] of …) — two unnecessary bytes, since the spec restriction is on the two-token sequence async of, not async [. I verified this against the source: EDot masks the flag at line 3771 but EIndex does not. However, the emitted output is still valid, reparseable JS with identical semantics, so this is a minor minification non-optimality in an extremely obscure input, not a correctness regression. It does not block the actual fix, which turns invalid output into valid output. No prior human reviews or outstanding comments on the PR.
The for-of grammar forbids the initializer from starting with the token sequence `async of`, so an identifier spelled `\u0061sync` (or an already-parenthesized `(async)`) must not be printed as a bare `async` there. Wrap it in parentheses, mirroring the existing `let` handling.
d9795f8 to
40e47d6
Compare
There was a problem hiding this comment.
LGTM — the inline nit about EIndex over-parenthesizing async[0] is cosmetic and non-blocking.
Extended reasoning...
Overview
This PR fixes a fuzzer-found printer correctness bug: the identifier \u0061sync used as a for-of loop variable was being printed as the bare token sequence async of, which is forbidden by the for-of grammar's lookahead restriction and rejected by both Bun's parser and JSC. The fix adds an IsFollowedByOf expr flag, threads it through print_for_loop_init, and parenthesizes async in EIdentifier when it appears at for_of_init_start with that flag — exactly mirroring the existing let handling one line above. Two files touched: ~20 lines in src/js_printer/lib.rs and a new test block in test/bundler/transpiler/transpiler.test.js.
Security risks
None. This is pure output-formatting logic in the JS printer with no auth, crypto, filesystem, or network surface. The worst possible failure mode is emitting extra parentheses or (pre-fix) unparseable output.
Level of scrutiny
The printer is on the hot path for all transpiled code, but this change is surgical: a new enum variant, a one-line extension to an existing wrap condition, and an extra parameter plumbed through three call sites (SForIn/SForOf/SFor) with the obvious values. The logic is guarded by both for_of_init_start position equality and the new flag, so it cannot fire outside for-of initializers. The test coverage is good: it asserts the wrap happens for \u0061sync and (async) and for await, asserts no wrap for async.x, x[async], and for-in, asserts (let) still works, and asserts the unparenthesized form is a parse error.
Other factors
I previously left an inline nit that EIndex forwards the new flag to its target unfiltered, so for (\u0061sync[0] of x) picks up unnecessary parens around async. That output is still valid, reparseable JS with identical semantics — it's a 2-byte minification pessimization for an input that essentially never occurs in real code. It does not regress correctness and does not block this fix, which is a strict improvement over the current behavior of emitting invalid JS.
`for (\u0061sync[0] of x)` doesn't need parentheses around `async` because the grammar restriction only applies to the exact token sequence `async of`, matching the existing dot-expression behavior.
There was a problem hiding this comment.
LGTM — the EIndex follow-up in b0a9218 addresses my earlier nit, and the added async[0] test covers it.
Extended reasoning...
Overview
Adds an IsFollowedByOf expr flag so the JS printer parenthesizes the identifier async when it is the entire for-of initializer (e.g. for (async of x) → for ((async) of x)), fixing fuzzer-found output that neither Bun nor JSC could reparse. Touches src/js_printer/lib.rs (flag enum, EIdentifier/EIndex arms, SFor* call sites, print_for_loop_init signature) and adds a test block in test/bundler/transpiler/transpiler.test.js.
Security risks
None. Pure output-formatting change in the printer; no parsing, I/O, auth, or untrusted-input handling is affected beyond producing syntactically valid JS where it previously produced invalid JS.
Level of scrutiny
Low–medium. The change is narrow and strictly additive: the new wrap condition only fires when the symbol name is literally async, the flag is set (only by SForOf), and the writer is at for_of_init_start — exactly mirroring the existing let handling on the same line. The EIndex flag-removal added in the follow-up commit only touches the newly introduced flag, so it cannot regress pre-existing behavior. Worst-case failure mode is extra parentheses (still valid JS), not broken output.
Other factors
- My earlier inline nit (EIndex forwarding
IsFollowedByOfto its target) was addressed in b0a9218 with the suggestedflags.remove(...)and the suggestedasync[0]test; the thread is resolved. - Tests cover the positive case,
for await, the no-paren cases (async.x,async[0],x[async],for-in), the existing(let)behavior, and that the bareasync ofspelling is still a parse error. - No CODEOWNERS entry for
src/js_printer/or this test file. - The single CI failure (
v8-heap-snapshot.test.tsSIGTRAP on macOS x64) is unrelated to the printer and predates the follow-up commit.
|
CI status — all lanes green. Every individual check on b0a9218 now passes, including the macOS lanes after their agent retries:
The only remaining red mark is the aggregate So: nothing left to fix on the branch; the per-lane checks are the accurate signal. A Buildkite rebuild would only serve to turn the aggregate status green. |
* oven/main (20 new commits): webcore: free Blob's owned content type on drop (oven-sh#31358) Support cross-compiling macOS binaries from Linux (oven-sh#31303) test: forward keep-alive requests in proxy.test.ts's mock proxy (oven-sh#31352) Port Bun.stringWidth to C++ with explicit SIMD (oven-sh#31351) Fix quadratic hang reporting duplicate-binding parse errors in the transpiler (oven-sh#31341) shell: don't abort when a glob's directory prefix doesn't exist (oven-sh#31367) Error instead of crashing on deeply nested statements in the transpiler (oven-sh#31333) Fix JSX transform panic when a bare `key` prop precedes `key` with a value (oven-sh#31350) Cap ANSI markdown indentation so deeply nested lists render in linear time (oven-sh#31366) css: bound selector-list expansion when compiling nesting for older targets (oven-sh#31277) node:http2: reassemble HEADERS+CONTINUATION before HPACK decoding (oven-sh#31323) Fix `await using` expression printing `using` as `await` (oven-sh#31324) Parenthesize `async` when it starts a for-of loop initializer (oven-sh#31326) Print Infinity and negative numeric property keys as computed properties (oven-sh#31328) css: keep required grouping parens in @container conditions when minifying (oven-sh#31330) Fix panic on anonymous export default class with an auto-accessor field (oven-sh#31331) node:http2: send GOAWAY frames on stream 0 (oven-sh#31353) parser: fix Scope mismatch while visiting panic from decorators on dropped class members (oven-sh#31340) webcrypto: reject oversized BufferSource inputs instead of aborting (oven-sh#31356) Error instead of crashing on deeply nested TypeScript types in the transpiler (oven-sh#31361) Resolved conflicts: - scripts/build.ts: kept both OHOS and macOS-cross argv entries - scripts/build/config.ts: kept both OHOS and macOS-cross config fields - scripts/build/deps/webkit.ts: kept OHOS fno-pic exclusion, adopted upstream -flto=thin
What does this PR do?
Fixes a printer bug found by fuzzing (invariant: printed output must reparse):
\u0061syncis a legal identifier spelling ofasyncand is valid as a for-of loop variable, but the for-of grammar has the restriction[lookahead ∉ { let, async of }], so the token sequenceasync ofmay not start the initializer. The printer emitted the identifier as a bareasync, producing output that neither Bun's parser nor JSC accepts.Cause
src/js_printer/lib.rsonly parenthesizedletat the start of a for-of initializer (for ((let) of x)), notasync.Fix
Add an
IsFollowedByOfexpr flag thatSForOfpasses intoprint_for_loop_init. When an identifier namedasyncis printed at the very start of a for-of initializer and is directly followed byof, it is wrapped in parentheses — mirroring the existinglethandling and esbuild's behavior.for (async.x of y),for (async[0] of y),for (x[async] of y),for (async in x)are unchanged (no parens needed).for await (\u0061sync of x)is also wrapped: Bun's parser (and JSC) reject the bare spelling there as well.Verification
test/bundler/transpiler/transpiler.test.js(for-of loop variable named async): fails on bun without this change, passes with it.test/bundler/transpiler/transpiler.test.jspasses with the debug build (147 pass / 0 fail).let \u0061sync; for (\u0061sync of [7, 8]) log.push(async * 2)executes correctly with the debug build.