Conversation
The single-use let/const inliner at the tail of visit_stmts trusts use_count_estimate. A switch case body is one slice of the switch block scope, so a declaration in one case can still be used by a later case that has not been visited yet. The inliner saw a count of 1 and deleted the declaration, which left the later use dangling. Move the minify-syntax tail of visit_stmts into mangle_stmts. visit_stmts skips it for StmtsKind::SwitchStmt and s_switch runs it on each case body once all cases are visited. Case bodies keep the same inlining and statement merging as any other block. Bump the runtime transpiler cache version so cached output from the old inliner is not reused.
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 34 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
CI on 06d24c5: 180 of 181 jobs pass. The one red lane is darwin x64, where |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix that defers the mangle pass until use counts are final, rather than disabling it.
What was reviewed:
mangle_stmtsis a pure extract of the oldvisit_stmtstail; body is unchanged, and it runs in the same switch block scope (beforepop_scope), socurrent_scope().parent/contains_direct_evalsee the same state — actually strictly better, since adirect evalin a later case now correctly suppresses removal in earlier cases too.is_inside_switchis restored before the new mangle loop, but that flag is only read durings_breakvisiting, not bymangle_stmts, so no behavior change.- Test
capturelist checks out (the leading"v"is thefunction capture(v)definition matchingcaptureFile's indexOf scan);onlystill inlines tov + 1, cross-case decls stay. Cache version bump is required and present.
Extended reasoning...
Overview
This PR fixes a minify-syntax correctness bug where let/const declarations in one switch case clause were being inlined and deleted before later clauses (which share the same block scope) had their uses counted. The fix extracts the mangle tail of visit_stmts into a new mangle_stmts() method, has visit_stmts skip mangling for StmtsKind::SwitchStmt, and has s_switch invoke mangle_stmts on each case body only after all cases have been visited. Files touched: src/js_parser/visit/mod.rs (extract method, ~20 lines net), src/js_parser/visit/visit_stmt.rs (10-line loop after the visit loop), src/jsc/RuntimeTranspilerCache.rs (version bump 27→28), and test/bundler/bundler_minify.test.ts (two new tests).
Security risks
None. This is a minifier/transpiler correctness fix with no auth, crypto, network, or filesystem surface. The only user-facing effect is that previously-broken switch fall-through code now runs correctly instead of throwing ReferenceError.
Level of scrutiny
Moderate — the JS parser's visit/mangle pass runs on every file Bun transpiles or bundles, so a regression here would be wide-blast. However, the change is deliberately conservative: mangle_stmts is a verbatim extract of the existing tail (the let p = self; alias keeps the body byte-identical), and it is called from s_switch while still inside the pushed switch block scope, so current_scope().parent.is_some() and contains_direct_eval observe the same values they did before. Deferring the pass until all cases are visited only gives the inliner more complete use_count_estimate data, never less — it cannot cause a new incorrect inlining, only prevent premature ones. I checked that is_inside_switch (restored before the new loop) is not read by anything mangle_stmts reaches; its only reader is the break-outside-loop diagnostic during visiting.
Other factors
Tests are well-designed per REVIEW.md: the itBundled case asserts both the negative contract (tag, s, n survive as identifiers via capture) and the positive contract (only still inlines to v + 1 in the same clause), plus a run.stdout check for end-to-end semantics; the second test exercises the runtime transpiler via bun -e with bunExe()/bunEnv, drains pipes concurrently, and asserts stdout/stderr before exit code. The transpiler cache version bump ensures stale broken output is invalidated. No CODEOWNERS cover these paths. The PR description notes an alternative open PR (#30936) that guards on is_inside_switch instead — this PR's defer approach is strictly better since it preserves same-clause inlining, and that claim is covered by the sameClause/v + 1 capture assertion.
|
The review above asks for no changes. One detail it points out is worth stating plainly: |
`RuntimeFeatures.minify_syntax_statements` replaces the `minify_syntax && (bundle || transform_only)` predicate. The bundler's parse task derives it from `minify_syntax`, and `bun build --no-bundle` sets it through the transpiler options. `Bun.Transpiler`, `bun pm diff` and the runtime transpiler leave it off, so their output is unchanged from main. The `===` to `==`, `a === null || a === void 0`, logical chain and `simplify_boolean` rewrites move behind the same flag. The switch case bodies are mangled from `s_switch` once every case is visited, so the single-use inliner sees the uses in later cases (same fix as #40791). The runtime transpiler cache version goes to 29 for it. A tagged template whose tag was a conditional keeps `this` unbound: `(a ? o.m : o.m)\`x\`` prints as `(0, o.m)\`x\``.
|
#41159 moves the same |
This branch has not been deployed
2 participants
Problem
bun file.js,bun build --minify-syntax), alet/constdeclared in onecaseclause and used in a later clause is inlined into its first use and the declaration is deleted.case 1: const tag = {}; use(tag); case 2: return tag;becomescase 1: use({}); case 2: return tag;.visit_stmts(src/js_parser/visit/mod.rs). It trustsuse_count_estimate, buts_switch(src/js_parser/visit/visit_stmt.rs) callsvisit_stmtsonce per case body, so uses in later cases are not counted yet. esbuild has the same bug.Fix
visit_stmtsintomangle_stmts.visit_stmtsskips it forStmtsKind::SwitchStmt.s_switchruns it on each case body after every case is visited, when every use in the switch scope is counted.RuntimeTranspilerCache::EXPECTED_VERSIONgoes to 28 so cached output from the old inliner is not reused.test/bundler/bundler_minify.test.ts(two new tests, both fail on 1.4.1). The other suites run are listed in the notes.Background
visit_stmtsmangles it: the inliner replaces the single use of alet/constwith its initializer and drops the declaration.switchpushes one block scope for all of its cases, but each case body is a separate statement list, so this one scope is visited in severalvisit_stmtscalls. Every other construct visits a scope in one call.StmtsKind::SwitchStmtalready defersusinglowering to the switch level for the same reason.is_inside_switch, which turns it off at any depth inside a switch, and also disables const folding in switch bodies to keep TDZ errors. This PR only fixes the dangling use. Const folding across cases (Bun inlines constants across switch cases, preventing an error that would happen in node #18477) is unchanged.Notes
Repro from the report, run as
bun sw.mjs(node prints["object","s1!"]):1.4.1 prints
["undefined","ReferenceError"]. With this change it prints["object","s1!"].Other shapes checked against node with the fixed build (all match): a Duff-style decoder that pushes onto an array declared in the first case (1.4.1 throws
acc is not defined), a declaration read only by a closure in a later case, aletreassigned in thedefaultclause, a nested switch sharing a declaration with the outer case, and a declaration used as a latercasevalue. Case values are visited in the same loop as the bodies, so a declaration used in a latercase t:expression had the same bug.Why defer instead of guarding: a guard on
kind == SwitchStmt(or onis_inside_switch, as in #30936) would stop inliningcase 1: const x = foo(); return x.y;shapes, which are common in unbraced case clauses. Deferring the whole mangle pass keeps the output identical to a braced block.bun build --minify-syntaxoutput for a file with mixed same-case and cross-case declarations is byte-identical between 1.4.1 and this build except for the declarations that must now stay.esbuild 0.21.5 and 0.25.1 produce the same wrong output for the repro. The current esbuild
mangleStmtsstill runs per case body.Suites run with the debug build, all green:
test/bundler/bundler_minify.test.ts,bundler_edgecase,bundler_regressions,bundler_cjs2esm,bundler_minify_symbol_for,transpiler_constant_fold_eqeq,esbuild/{dce,default,ts,lower,extra},transpiler/transpiler.test.js,transpiler/runtime-transpiler,cli/run/transpiler-cache.cargo clippy -p bun_js_parseris clean.cargo fmt --checkand prettier pass.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file