Repository navigation
Conversation
|
Updated 1:49 AM PT - Aug 4th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 29246That installs a local version of the PR into your bun-29246 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDistinguishes parse-time TLA tokens from live top-level await discovered during the visit pass, records live TLA, removes unreachable TLA prior to ESM classification and diagnostics, refines parsing disambiguation for Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/visitExpr.zig`:
- Around line 988-990: When marking a live top-level await inside the visitor
(the branch that sets p.has_live_top_level_await via
p.fn_or_arrow_data_visit.is_outside_fn_or_arrow), also record the location/token
for that live await so downstream CJS diagnostics use the surviving await
location instead of the original parse-time one; update
p.top_level_await_keyword (or the parse-state field that holds the await
span/token) to the current await token/span when you set
p.has_live_top_level_await, ensuring you only overwrite it when the new location
corresponds to a live await discovered during this visit.
In `@test/regression/issue/29243.test.ts`:
- Line 41: Remove the flaky empty-stderr assertions in the test by deleting the
two occurrences of expect(stderr).toBe("") (the assertions at lines that
reference stderr in test/regression/issue/29243.test.ts); keep the existing
exit-code and stdout/content assertions intact (do not change assertions using
exitCode or stdout), and ensure the variable stderr is still collected if used
elsewhere but no longer asserted to be exactly "" to avoid ASAN-induced flakes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dde02832-03b2-4a9d-8a33-8b3e382e96e5
📒 Files selected for processing (5)
src/ast/P.zigsrc/ast/Parser.zigsrc/ast/parsePrefix.zigsrc/ast/visitExpr.zigtest/regression/issue/29243.test.ts
|
Copying my comment in the linked issue in case @robobun hasn't seen it: This bug has nothing to do with |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29243.test.ts (1)
29-29:⚠️ Potential issue | 🟠 MajorRemove empty-
stderrassertions from success-path subprocess tests.Line 29 and Line 70 make these tests flaky on ASAN CI, where benign warnings may appear on
stderreven when behavior is correct.Proposed patch
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); - expect(stderr).toBe(""); expect(stdout).toBe("foo();\n"); expect(exitCode).toBe(0);- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); - expect(stderr).toBe(""); expect(stdout).toContain("var await = 42"); expect(stdout).toContain("globalThis.output = await"); expect(exitCode).toBe(0);Based on learnings: in
test/regression/issue/*.test.ts, avoid asserting spawned Bun subprocessstderris empty because ASAN may emit benign warnings.Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29243.test.ts` at line 29, Remove the brittle empty-stderr assertions in the regression test by deleting the expect(stderr).toBe("") checks (e.g., the assertion at expect(stderr).toBe("") in test/regression/issue/29243.test.ts and the similar one around line 70), leaving the success-path subprocess tests to only assert on stdout/exit behavior; locate the assertions by searching for expect(stderr).toBe("") in the file and remove them so ASAN benign warnings no longer cause flakes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29243.test.ts`:
- Line 29: Remove the brittle empty-stderr assertions in the regression test by
deleting the expect(stderr).toBe("") checks (e.g., the assertion at
expect(stderr).toBe("") in test/regression/issue/29243.test.ts and the similar
one around line 70), leaving the success-path subprocess tests to only assert on
stdout/exit behavior; locate the assertions by searching for
expect(stderr).toBe("") in the file and remove them so ASAN benign warnings no
longer cause flakes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 309a8c18-d823-45aa-964f-06b5d39f07bd
📒 Files selected for processing (1)
test/regression/issue/29243.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/Parser.zig`:
- Around line 649-663: The parser currently clears top_level_await_keyword when
p.has_live_top_level_await is false because s_for_of never sets that flag for
top-level "for await" loops; update the s_for_of visitor (visitStmt.zig) so that
when encountering a for-of statement with data.is_await == true AND the
statement is at module scope (p.current_scope == p.module_scope) you set
p.has_live_top_level_await = true (and ensure p.top_level_await_keyword remains
pointing to the live await range), so the diagnostic in Parser.zig that checks
p.has_live_top_level_await will be preserved for top-level for-await loops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57506c01-8895-4880-b9db-3c8aec88640b
📒 Files selected for processing (4)
src/ast/Parser.zigsrc/ast/parsePrefix.zigsrc/ast/visitExpr.zigtest/regression/issue/29243.test.ts
There was a problem hiding this comment.
This PR is in good shape after extensive review — all identified bugs have been addressed — but the parser changes are complex enough to warrant a human look before merging.
Extended reasoning...
Overview
The PR defers top-level-await rejection in CJS targets from parse time to post-DCE visit time, touching seven parser source files (, , , , , , ) and adding a 452-line regression test.
Security risks
No direct security impact — this is pure parser/bundler logic with no auth, crypto, or user-data exposure.
Level of scrutiny
High. This modifies the JavaScript/TypeScript parser's module-scope disambiguation path, which runs on every CJS build. The change introduces a new scope-stack walk (), a token whitelist (), a new live-TLA tracking flag, and a post-visit error path. The number of edge cases that surfaced during review (TS non-null assertion, for-of/in contextual keywords, / operators, arrow default params, class field initializers, TS namespace scopes, for-await squiggle range) suggests the problem space is larger than it first appears.
Other factors
All reported issues were addressed across multiple fix commits (481e42c → 1cfe041). The final scope-stack approach is more robust than the earlier flag approach. Test coverage is comprehensive. The PR is a net correctness improvement (matching esbuild's behaviour) but given the volume of iterations and the criticality of the code path, a human review is the right call before landing.
|
CI is still failing on The underlying code change — speculatively upgrading an The 18 regression tests cover the path the original issue is on ( |
9647a34 to
78152a0
Compare
f20b3bb to
3bb72fe
Compare
9eaa6db to
85c7417
Compare
|
Rebased onto main. The conflict was non-trivial: main completed the Zig→Rust parser migration and deleted all the Resolution: the fix was already ported to Rust in a prior commit, so I collapsed the branch to a single clean commit containing only the net Rust changes ( All 18 regression tests pass on the debug+ASAN build; original issue #29243 reproduction is fixed. |
|
CI status: the diff's own lanes are green. The parser changes compile and link on every platform (69/73 steps pass, including all build lanes and the The remaining red is unrelated flake on heavy tests, reproduced identically across two consecutive runs on this same commit (builds 65604 and 65695):
Build 65695 has zero ERROR annotations (only the CI already re-ran on its own (65604 → 65695) and hit the same class of failure, so this needs a maintainer to merge rather than another retrigger. |
|
CI status: the diff is green on all lanes that exercise it. Build 65604's failures are unrelated flake:
None touch the parser ( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
`bun build --format=cjs` rejected any top-level `await` at parse time,
even when it sat inside a branch that dead-code elimination would
remove (e.g. behind a `--define`-controlled `if (false) { await
import(...) }`). esbuild parses the `await`, lets DCE drop the
unreachable branch, and only reports the CJS / TLA incompatibility if
a live `await` survives.
Match that behavior:
- `P::is_at_module_scope()` walks the scope stack (stops at the real
module `Entry`, returns false on function/class/TS-namespace
scopes) to decide when an `await` identifier at module scope in a
non-ESM target should be upgraded to an `await` expression.
- `token_starts_await_expr()` gates the upgrade on the following
token: keywords / literals / `{` / `~` / a non-contextual
identifier force the keyword reading, while `of`/`in`/`as`/
`satisfies` and template/operator continuations keep `await` as an
identifier. `!` is left out so TS `await!.foo` non-null assertions
parse.
- `has_live_top_level_await` is set by the `e_await` / `s_for_of`
visitors only in reachable control flow at module scope, and
records the surviving `await`'s range for the diagnostic.
- post-visit in `parse_entry`: in non-ESM targets, clear the pre-DCE
range if nothing survived, or emit "Top-level await is currently
not supported with the \"X\" output format" if it did. Gated on
`!features.top_level_await` so ESM / bake-dev builds are untouched.
- `for await` and the `s_return` ESM-classification check get the
same module-scope / feature gating.
Regression test covers dead/live TLA, `for await`, `await` as an
identifier (incl. tagged template / `as` / `satisfies`), default
params, TS namespaces, and the diagnostic range.
Fixes #29243
…located A switch or with kept wholesale in dead control flow leaked its await-bearing test expression into non-async CJS output. Recurse into their bodies like the other statement kinds; hoisted declarations are already relocated out by the visit pass. Also remove the unreachable in exclusion (reserved word, never tokenizes as an identifier).
50e2dff to
7a28526
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/cli/test/Timings.rs:1-6— This file appears to be an accidental inclusion — it implementsbun test --timings=<path>shard-balancing, which is entirely unrelated to the CJS top-level-await parser fix. It has nomod Timings;declaration anywhere in the tree (so it isn't compiled into any crate), no git history on any branch, no test coverage, and is absent from this PR's own "diff hotspot" and "evidence per changed file" tables. Please drop it from this PR (git rm src/runtime/cli/test/Timings.rs) and land it separately with its ownmodwiring and tests.Extended reasoning...
What the issue is.
src/runtime/cli/test/Timings.rsis a brand-new 242-line file implementingbun test --timings=<path>/--update-timingsshard-balancing (loading per-test-file duration tables, partitioning files across shards by cost, sorting slowest-first, writing timing JSON). None of this has any relation to the PR's stated purpose — deferring the CJS top-level-await diagnostic until after DCE runs (#29243).Evidence that this is an accidental inclusion, verified against the current tree:
- Not compiled.
grep -rn 'mod Timings\|Timings::' src/returns zero matches.src/runtime/cli/test/contains onlyChangedFilesFilter.rs,ParallelRunner.rs,Scanner.rs, andparallel/— there is nomod Timings;declaration wiring this file into any crate. Cargo will not compile it; it is dead source. - No git history.
git log --all -- src/runtime/cli/test/Timings.rsreturns nothing. The file has never existed on any branch in this repository. - The PR's own metadata omits it. The PR description's "diff hotspot" diffstat lists exactly 10 files changed —
Timings.rsis not among them. The robobun "evidence per changed file" table (reads/edits/tests) also lists exactly 10 files and omitsTimings.rs. Yet GitHub's changed-files list shows 11 files. The author-bot's own instrumentation never observed this file being read, edited, or tested during the 14 iterations that produced this PR. - No test coverage.
test/regression/issue/29243.test.tscontains 21 tests, all exercisingbun build --format=cjsawait behavior. Nothing touches--timings.
Step-by-step proof that the file is orphaned dead code. Suppose this PR merges as-is:
Timings.rslands atsrc/runtime/cli/test/Timings.rs.- The Rust module tree for
src/runtime/cli/test/is defined by whatevermoddeclarations exist in its parent — currently onlyChangedFilesFilter,ParallelRunner,Scanner, andparallelare declared (verified: nomod Timingsanywhere insrc/). cargo buildtherefore never seesTimings.rs. It is not type-checked, not compiled, and cannot be called from anywhere.- The types it references (
super::parallel::file_range::FileRange,bun_options_types::context::Shard, etc.) may or may not exist / match — nothing verifies this because nothing compiles it. - The result is 242 lines of untested, uncompiled, unreviewed feature code sitting in the tree with no history explaining where it came from, riding on a parser bugfix PR.
Why this is flagged as blocking. REVIEW.md (the repo's merge-blocking checklist, "everything here has blocked merges") states: "Every line you add must be demonstrably live" — this file has no consumer and is not compiled. "Delete dead code in the same PR that makes it dead" — this file is born dead. "Don't ride file-wide standardization on a focused bugfix" and "New cross-cutting abstractions need maintainer agreement before appearing inside a feature PR" — a 242-line
--timingsshard-balancing subsystem is a new cross-cutting abstraction with zero relation to the CJS TLA fix, and it appears without maintainer discussion, tests, or even a mention in the PR description.Impact and fix. No runtime behavior is affected (the file isn't compiled), but merging it lands untested, unwired, unreviewed code into the tree under the cover of an unrelated bugfix — exactly what REVIEW.md's dead-code and scope rules exist to prevent. Fix:
git rm src/runtime/cli/test/Timings.rsand force-push. If the--timingsfeature is intended, it should land in its own PR with amod Timings;declaration, callers, and test coverage. - Not compiled.
|
The diff is complete and all review threads are resolved. The only failures on the latest CI run (build 88669) are unrelated to this change: one pre-existing worker_threads stress-test SIGABRT that also fails on main (reported separately), and a handful of known-flaky tests that passed on retry (s3, hot reload, bun-install, napi builds on Windows aarch64). All 24 regression tests plus the transpiler, bundler edgecase, minify, and dead-code suites pass locally on this branch. Ready for a maintainer to review and merge. |
…bodies An async or generator function declaration surviving in a dead switch case, with body, or loop body kept the whole statement, leaking its await-bearing test expression into non-async CJS output. Statements reached through the filter's body recursion can drop such declarations: they are block-scoped there, and plain sloppy-mode functions were already relocated. A dead module-level function still hoists and is kept.
maybe_relocate_vars_to_top_level returns ok with no statement when every decl was value-less and absorbed into the hoisted vars; s_for kept the stale SLocal in the init slot, duplicating the declaration and keeping a dead loop (and its await-bearing test) alive in non-ESM output. Assign the relocation result whenever ok, matching s_for_in/s_for_of.
There was a problem hiding this comment.
No new issues found this round; the three earlier leak classes (dead switch/with husks, block-scoped async/generator decls, and for (var x; await …; …) init retention) are all fixed and covered by regression tests. Deferring to a maintainer — this is a substantial parser-semantics change (module-scope await disambiguation, deferred TLA-implies-ESM detection, and DCE-filter behavior changes in should_keep_stmt_in_dead_control_flow / s_for var relocation that affect code beyond the await case) and should get a human look.
What was reviewed:
token_starts_await_exprdisambiguation against identifier continuations (of/as/satisfies/using, tagged templates, TS!) — each has a locking test.is_at_module_scopescope walk vs. TS namespace/declare-enum bodies and function/arrow arg scopes.- The
should_keep_nested_stmt_in_dead_control_flowsplit and thes_forrelocate.okchange for live-code impact — the latter also changes non-deadfor (var x; …)output. has_live_top_level_awaitset sites (E::Await visit,for awaitvisit) vs.is_control_flow_deadand the post-visit clear/error in parse_entry.rs.
Extended reasoning...
Overview
This PR defers the CJS top-level-await rejection from parse time to after the visit pass so DCE can drop unreachable await before the error fires. It touches ten files: the await-prefix parse path (parse_prefix.rs), for await parsing (parse_stmt.rs), a new is_at_module_scope helper and tla_implies_esm gating on is_file_considered_to_have_esm_exports / has_es_module_syntax / implicit strict mode (p.rs), visit-pass liveness tracking (visit_expr.rs, visit_stmt.rs), the post-visit diagnostic (parse_entry.rs), DCE filter changes in should_keep_stmt_in_dead_control_flow including a new nested-body variant and new SSwitch/SWith arms (scan_side_effects.rs), a change to s_for var-init relocation, a new await_range field on S::ForOf (s.rs, react_compiler/codegen.rs), and a 640-line regression suite with 24 concurrent tests.
Security risks
None identified. This is parser/bundler output-shape logic with no auth, network, filesystem-path, or untrusted-size handling. The worst failure mode is emitting syntactically invalid CJS (which Node then rejects at load time) — three such cases were found in earlier review rounds and are now fixed with tests.
Level of scrutiny
High. The parser is production-critical and every JS/TS file flows through the changed pfx_t_identifier path. Several changes have blast radius beyond the await case: (1) the tla_implies_esm gating changes when a file is classified as ESM / when implicit strict mode is applied for non-TLA-capable formats; (2) should_keep_nested_stmt_in_dead_control_flow now drops block-scoped function declarations from dead bodies for all DCE, not just await-bearing ones; (3) the s_for relocation change (if relocate.ok { data.init = relocate.stmt }) alters live-code output — the author confirmed it also removes a redundant var duplication in live for (var y; …) loops. These are reasonable and match esbuild's intent, but a maintainer should confirm the DCE-filter and for-init changes are semantically what they want across the bundler.
Other factors
The PR went through 15 iterations. Earlier automated review rounds found three real correct-rejection → silent-invalid-output regressions (dead switch/with husks kept wholesale; async/generator fn decls in dead bodies keeping their enclosing statement; for (var x; await …; …) init not cleared after full relocation). Each was fixed at the layer that owns the invariant and each has a dedicated regression test. Two acknowledged follow-up limitations remain (parenthesized (await …) at module scope still errors at parse time due to speculative arrow-arg scopes; a same-line @await class decorator edge case) — both are non-regressions vs. main and documented in resolved threads. Test coverage is thorough (24 cases, all draining both pipes and asserting the quiet stream). CI on the latest build passed except for infrastructure flakes (Azure throttling, R2 ServiceUnavailable). All review threads are resolved.
|
Closing this PR because it has been inactive for more than 90 days. |
|
Reopened as #47812 after the stale-bot closure: the bug still reproduces on bun 1.4.3 and the branch rebases cleanly onto current main. Same change; all review threads here were addressed before closure. |
Problem
bun build --format=cjsrejected any top-levelawaitat parse time, even when theawaitsat inside a branch that constant folding / dead code elimination would have removed:esbuild handles this fine: the
if (false) { ... }block is dropped by DCE and the unreachableawaitis never reported. That blocks users who use--defineto gate test-only code paths behind animport("node:fs")that never runs in production.Cause
At module scope, when the target output format didn't support top-level await (
--format=cjs), the parser usedallow_await = .allow_ident. That treatsawaitas a plain identifier and relies on a lexer-levelprev_token_was_await_keywordflag to emit the TLA error the moment a follow-up token can't be interpreted as an identifier continuation (e.g. theimportkeyword). The diagnostic fires at lex/parse time — well before constant folding has a chance to eliminate the dead branch.Fix
Parse
await EXPRas anawaitexpression at module scope even when the output format is CJS, as long as the following token unambiguously starts an expression rather than continuing an identifier (sovar await = 42andconsole.log(await)still work as identifier uses). Track "live" top-level awaits separately in the visit pass (has_live_top_level_await), and only let them imply.esmafter DCE has run. If a live top-level await survives DCE in a CJS target, emit the diagnostic after visit:This matches esbuild's behaviour.
Verification
test/regression/issue/29243.test.tscovers 21 cases. The core four:await→ builds tofoo();, no error (the bug).await→ rejected with the new diagnostic, not the old lex-level one.var await = 42; console.log(await)in a CJS file → still parses as identifier use.awaitinside a non-async function → still reports the existing"await" can only be used inside an "async" functionerror with the "Consider adding the async keyword here" note.Review rounds added coverage for: dead/live
for awaitloops and their diagnostic ranges, tagged templates,await {}, default parameters of non-async functions and arrows, top-levelreturnplus a deadawait, TSawait!/await as T/await satisfies T,await usingdeclarations, TS namespace anddeclare enumbodies, and deadswitch/withstatements. The last case also fixes the dead-control-flow filter (should_keep_stmt_in_dead_control_flow):switch/withhusks were kept wholesale in dead branches, which would have leaked their await-bearing test expressions into non-async CJS output; they now recurse into their bodies like the other statement kinds.The dead-await cases regress on the installed
bunbut pass on this build.Fixes #29243
[review] gate passed · iteration 15 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 0 rejected · iteration 15
evidence per changed file
root cause · written by the author bot
The parser rejected top-level await at parse time, before dead code elimination ran, so an await inside a branch that would later be proven unreachable still produced an error in CJS builds. The fix parses await tolerantly at module scope and defers validation to after the visit pass, recording only awaits encountered in live control flow via a new has_live_top_level_await flag, so unreachable awaits are eliminated along with their enclosing dead branches and an error is reported only when a live top-level await survives in a format that cannot support it. It also corrects var relocation fo…