parser: preserve TDZ for const declared in switch case#30936
Conversation
|
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:
WalkthroughThis PR fixes Bun's parser to prevent const/let declarations in one switch case from being inlined into sibling cases, which violated JavaScript's TDZ (Temporal Dead Zone) semantics. The fix disables the single-use inlining and const-local-prefix optimizations within switch blocks, adds regression tests covering TDZ and shadowing behavior, and invalidates the runtime transpiler cache. ChangesSwitch-case const scoping and TDZ fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/visit/visit_stmt.rs`:
- Around line 2174-2175: The call p.visit_stmts(&mut _stmts,
StmtsKind::SwitchStmt) currently disables LowerUsingDeclarationsContext because
visit_stmts treats StmtsKind::SwitchStmt as a blanket "disable using lowering"
signal; change this so the TDZ/single-use-substitution behavior and the
using/await-using lowering behavior are independent: either add a new StmtsKind
variant (e.g., SwitchForTDZ) or add a boolean flag (e.g., allow_lower_using:
bool) to visit_stmts and its callers, update the guard in visit/mod.rs to check
the specific TDZ-only signal (or the new flag) instead of StmtsKind::SwitchStmt,
and update the single caller (the p.visit_stmts(&mut _stmts,
StmtsKind::SwitchStmt) site) to pass the new variant/flag so switch-case TDZ
handling remains but using-lowering still runs inside case bodies.
In `@test/bundler/transpiler/runtime-transpiler.test.ts`:
- Line 225: The test contains intentional const declarations inside sibling
switch cases (e.g. the declaration "const CONSTANT = 2;") which trigger
lint/correctness/noSwitchDeclarations; add a targeted suppression comment "//
biome-ignore lint/correctness/noSwitchDeclarations" immediately above each such
intentional declaration (including the other similar declarations in the same
test file) so the TDZ behavior tests stay intact and the linter is satisfied.
🪄 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: 615ec262-a300-433d-839f-fd44e2f6180a
📒 Files selected for processing (3)
src/js_parser/visit/mod.rssrc/js_parser/visit/visit_stmt.rstest/bundler/transpiler/runtime-transpiler.test.ts
|
Updated 5:20 PM PT - May 17th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 30936That installs a local version of the PR into your bun-30936 --bun |
4dbea61 to
36f2461
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/30932.test.ts`:
- Around line 3-18: The header comment block in
test/regression/issue/30932.test.ts contains an extended multi-line rationale;
trim it to the standard two-line regression header: first line the issue URL
(https://github.com/oven-sh/bun/issues/30932) and second line a single concise
bug-description, and move the rest of the long explanatory prose out of the file
(e.g., into the PR description or a separate markdown note). Locate the top
block comment that begins with the issue URL and replace the multi-paragraph
explanation with one short summary line so the file follows the established
two-line pattern.
🪄 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: 59efcea3-84d4-4faf-bf3f-10ef37f12253
📒 Files selected for processing (4)
src/js_parser/visit/mod.rssrc/js_parser/visit/visit_stmt.rssrc/jsc/RuntimeTranspilerCache.rstest/regression/issue/30932.test.ts
Every case in a switch statement shares one lexical scope but each case is entered conditionally, so a `const` declared in one case does not dominate references to that name in sibling cases. The visit pass was performing two inlining passes that both ignored this and silently bypassed the TDZ `ReferenceError` the spec requires. 1. const-local-prefix inlining (`const_values`) — a leading run of `const X = literal` in a scope is recorded and every reference to `X` later in the scope is replaced with the literal. Because all cases share one block scope, `const CONSTANT = 2` in `case '*'` was treated as part of the prefix for the whole switch body and `CONSTANT` in `case 'a'` was rewritten to `2`. 2. single-use substitution — `visit_stmts` walks a block's output and inlines `const X = expr` into its lone use, deleting the decl. It runs once per case body, but `use_count_estimate` is global: while visiting `case '*'`, the counter has only seen `case '*'`s references, so a single-use pattern reads as 1 and the decl is deleted out from under a sibling case's reference. Fix: mark the switch body scope `is_after_const_local_prefix = true` so no `const` inside any case is added to `const_values`, and gate the single-use substitution on `fn_or_arrow_data_visit.is_inside_switch` so case-body visits skip it. `is_inside_switch` resets at function boundaries, so nested functions inside a case body are unaffected. Bumps the runtime transpiler cache version so stale pre-fix entries are invalidated instead of still producing the buggy output. Closes #30932 Closes #18477
36f2461 to
f6d558b
Compare
There was a problem hiding this comment.
All my earlier concerns are resolved (using-lowering decoupled via is_inside_switch, both compiled cache-version constants now at 21). Deferring to a human for final sign-off since this touches the parser's visit pass and trades a minor minify-time pessimization for correctness.
Extended reasoning...
Overview
This PR fixes #30932/#18477 by disabling two inlining optimizations inside switch case bodies so that const declarations in one case are not inlined into sibling cases (which silently bypassed the spec-required TDZ ReferenceError). The change touches:
src/js_parser/visit/visit_stmt.rs— setsis_after_const_local_prefix = trueon the switch body scope and keepsStmtsKind::Nonefor case bodies (so using-lowering is unaffected).src/js_parser/visit/mod.rs— adds!p.fn_or_arrow_data_visit.is_inside_switchto the single-use-substitution guard.src/jsc/RuntimeTranspilerCache.rsandsrc/bundler/cache.rs— bumpEXPECTED_VERSION/RUNTIME_TRANSPILER_CACHE_VERSIONfrom 20 → 21.test/regression/issue/30932.test.ts— five regression tests covering literal-inlining, single-use-substitution, outer-shadowing, same-case use, and fall-through.
Security risks
None. This is a transpiler-output correctness fix; no auth, crypto, network, or user-input handling is involved.
Level of scrutiny
High. The JS parser's visit pass runs on every transpiled file in both the runtime and bundler, so any change here has very broad blast radius. That said, the change itself is conservative — it only disables optimizations in a narrow context, which is always correctness-safe — and is well-covered by the new tests.
Other factors
This PR went through four rounds of review feedback from me, all of which the author addressed: (1) the original StmtsKind::SwitchStmt approach inadvertently disabled using-lowering for case bodies — fixed by switching to is_inside_switch; (2) the cache version bump was initially missing, then incomplete — now both compiled constants agree at 21; (3) a minor perf pessimization for nested {} blocks inside case bodies under minify_syntax was acknowledged and accepted as a deliberate tradeoff (the in-code comment matches actual behavior). The reference-only .zig mirror was intentionally left unsynced per the port convention. All review threads are resolved and the bug-hunting pass found nothing new. Given the parser is production-critical and the implementation approach changed mid-review, a human should give the final sign-off rather than a bot auto-approval.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Wouldn't this be a bug at any scope and not just switch case statements?
|
Switch is the only JS construct where multiple conditionally-entered branches share a single lexical scope — every other construct ( function test(cond) {
if (cond) {
const X = 42;
return X;
}
return X; // ReferenceError: X is not defined (unbound, not TDZ)
}Both passes in this PR (the const-local-prefix inliner and the single-use substituter) only touch declarations and references within one scope, so the branch-entry condition is what matters. Two independent checks confirm switch is the only path where one scope spans multiple Happy to rename the guard ( |
Repro (from #30932)
CONSTANT=2ReferenceError: Cannot access 'CONSTANT' before initializationReferenceError: Cannot access 'CONSTANT' before initializationRoot cause
Every case in a switch shares one lexical scope but each case is entered conditionally, so a
constdeclared in one case does not dominate references to the same name in sibling cases. Two inlining passes in the visit pass were ignoring this and firing across case boundaries:const_values) — a leading run ofconst X = literalin a scope gets recorded and every later reference toXin the scope is replaced with the literal. Because all cases share one block scope,const CONSTANT = 2incase '*'landed in the prefix for the whole switch body andCONSTANTincase 'a'was rewritten to2.visit_stmtswalks a block's output and inlines the initializer of aconst X = exprinto its lone use, deleting the decl. It runs once per case body, butuse_count_estimateis global: while visitingcase '*', the counter has only seencase '*''s references, soconst X = Math.random()followed byreturn Xreads as single-use and the decl disappears — leavingcase 'a''s reference dangling (throwsX is not definedinstead of a TDZ error).Fix
s_switch, setis_after_const_local_prefix = trueon the switch body scope so noconstinside any case is added toconst_values.visit_stmtson!p.fn_or_arrow_data_visit.is_inside_switch.is_inside_switchis already set bys_switcharound each case-body visit and resets across function boundaries (it lives onFnOrArrowDataVisit), so nested functions inside a case body still get the optimization. Using this flag rather thanStmtsKind::SwitchStmtkeeps the using-lowering path atmod.rs:1488unaffected.RuntimeTranspilerCache::EXPECTED_VERSIONandRUNTIME_TRANSPILER_CACHE_VERSIONfrom 20 → 21 so pre-fix cached entries (which contain the incorrectly-inlined output) are invalidated on upgrade. Both compiled mirrors are bumped; the reference-only.zigmirror is left alone per the port convention.Declare + use within the same case is unaffected. Nested block scopes inside a case (
case 'a': { const X = 1; ... }) get their own scope and still get const-local-prefix inlining; single-use substitution stays disabled one block deep (correctness-safe pessimization — the optimization only fires underminify_syntax+ DCE).Tests
test/regression/issue/30932.test.tsadds five cases:Three of the five fail without the fix; all five pass with it.
Closes #30932
Closes #18477
Supersedes #27189 (same fix, stale branch targeting a renamed file).
CI status: the diff is green. Build 55571 had two "Expired" (wallclock-timeout) lanes on
debian-13-x64-asan-test-bunanddarwin-14-aarch64-test-bun— neither lane actually ran my file to failure. Build 55589 (empty re-roll) had oneubuntu-25-dot-04-aarch64-test-bunflake atexit status -1(job killed externally) while two concurrent runs of the same lane on the same sha passed. No failures I can pin on this diff; needs a maintainer merge when capacity frees up.