bundler: trim empty else {} left behind by DCE#30275
Conversation
|
Updated 6:54 PM PT - May 5th, 2026
❌ @robobun, your commit ef598cb has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30275That installs a local version of the PR into your bun-30275 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughIf/else pruning now removes empty ChangesDead Code Elimination & Ambiguous-Else Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — both prior concerns (.s_label in wrapToAvoidAmbiguousElse, and the dead_code_elimination or minify_syntax gate) are addressed, with a regression test for the dangling-else case.
Extended reasoning...
Overview
This PR fixes #30271 by trimming empty else {} remnants left behind after DCE prunes a dead branch body. It touches:
src/js_parser/ast/visitStmt.zig: widens the else-trim gate fromminify_syntaxtodead_code_elimination or minify_syntaxand matches both.s_emptyand zero-stmt.s_block.src/js_printer/js_printer.zig: adds.s_labeltraversal towrapToAvoidAmbiguousElseso that trimming an inner labeledif's emptyelsecannot cause a dangling-else miscompilation.test/bundler/bundler_edgecase.test.ts: two new tests — one for the basic empty-else trim, one for the labeled dangling-else guard.docs/guides/runtime/define-constant.mdx: aligns the documented before/after output with what the bundler actually emits and points to--minify-syntaxfor full scaffolding collapse.
Follow-up on prior review
My previous review flagged two issues; commit dae5236 addresses both:
- 🔴 dangling-else via labeled
if: fixed by adding.s_label => |current| { s = ¤t.stmt.data; }towrapToAvoidAmbiguousElse(verified at js_printer.zig:5145-5147), matching esbuild. The newDCEEmptyElseTrimmedLabeledDanglingElse#30271test exercises exactly the failing input and assertsouter-elseis printed. - 🟡
deadCodeElimination: false, minify: { syntax: true }regression: fixed by gating on the uniondead_code_elimination or minify_syntax, with the rationale captured in the code comment.
Security risks
None. This is a transpiler/bundler output-shaping change with no auth, crypto, network, or filesystem implications.
Level of scrutiny
Moderate — bundler correctness changes can silently miscompile user code, which is why the dangling-else interaction mattered. With the printer fix and runtime-checked regression test in place, the only remaining behavior change is dropping a semantically empty else clause, which is safe given the printer now correctly guards the ambiguous-else case (including through labels). The minify_syntax path was already exercising this trim before; this PR just enables it for the default DCE config too.
Other factors
The change is small (~20 LOC of logic), well-explained, follows the existing esbuild-derived pattern, and has targeted tests covering both the happy path and the edge case I raised. The docs update is a straightforward accuracy fix.
There was a problem hiding this comment.
LGTM — both issues from the prior review (dangling-else via .s_label and the minify_syntax-only gate regression) are fixed in dae5236 with regression tests; the Windows HTTP test timeout in CI is unrelated.
Extended reasoning...
Overview
Four files touched: src/js_parser/ast/visitStmt.zig widens the empty-else trim to fire under dead_code_elimination or minify_syntax and to recognize zero-length .s_block as empty; src/js_printer/js_printer.zig adds the missing .s_label arm to wrapToAvoidAmbiguousElse so trimming an inner else can't produce a dangling-else miscompilation; test/bundler/bundler_edgecase.test.ts adds two regression tests (one asserting no else in output, one asserting runtime stdout outer-else for the labeled dangling-else case); docs/guides/runtime/define-constant.mdx is updated to match actual default-config output.
Security risks
None. Pure code-generation/printing change in the bundler with no auth, crypto, FS, or network surface.
Level of scrutiny
Bundler output correctness is production-critical, so I gave this a careful pass on the first revision and flagged the .s_label dangling-else miscompilation and the minify_syntax-gate regression. Both were fixed exactly as suggested in dae5236 and both now have dedicated tests — the dangling-else one runs the bundle and asserts stdout, which is the strongest kind of guard. The remaining logic change is ~15 lines, mirrors esbuild's SLabel handling, and the bug-hunting pass on the latest revision found nothing.
Other factors
No CODEOWNERS cover these paths. All prior inline comments are resolved. The single CI failure (test-http-should-emit-close-when-connection-is-aborted.ts timeout on Windows) is unrelated to bundler/DCE changes.
`bun build --define Bun.env.NODE_ENV="'production'" test.ts` on:
```ts
if ("foo" === "foo") {
console.log("success!");
} else {
console.log("fail!");
}
```
produced:
```js
if (true) {
console.log("success!");
} else {}
```
The constant-folded test expression and the pruned dead branch are correct,
but the empty `else {}` remnant was left behind. The [docs guide for
`--define`](https://bun.com/docs/guides/runtime/define-constant) also
promised full collapse to just `console.log("success!");` — which only
ever happened with `--minify-syntax`.
`visitStmt`'s `s_if` handler had an 'else trim' gated on `minify_syntax`
that also only matched `.s_empty`. But `visitSingleStmtBlock` only
converts an empty block to `.s_empty` when `minify_syntax` is on —
otherwise the pruned body stays as an `.s_block` with `stmts.len == 0`.
So under default config the trim never fired.
Gate the else-trim on `(dead_code_elimination or minify_syntax)` and
treat an `.s_block` with zero stmts as equivalent to `.s_empty`. The
scaffolding collapse (`if (true) { A }` -> `A`) intentionally stays
behind `--minify-syntax` since tests in `transpiler.test.js`'s DCE block
lock that contract in, matching esbuild. This change is strictly about
cleaning up the empty-else remnant.
The printer's `wrapToAvoidAmbiguousElse` didn't traverse `.s_label`, so
trimming the inner `else {}` from `if (a) L: if (b) c(); else {} else d();`
would produce `if (a) L: if (b) c(); else d();` — the `else` re-binds to
the inner `if` and `d()` no longer runs when `a` is falsy. Before the
trim change, the inner `else {}` survived under default config and
shielded the outer `else`. Now that the trim runs under DCE (default), a
labeled inner `if` with an empty/DCE'd `else` would silently miscompile.
Add the `.s_label` arm to match esbuild.
Also updates the define-constant docs page so the promised before/after
output matches what the bundler actually emits, and points users to
`--minify-syntax` / `--minify` for the full scaffolding collapse shown as
the final step.
`test/bundler/bundler_edgecase.test.ts`'s `DCEEmptyElseTrimmed#30271`
asserts the output of the repro contains neither `fail` nor `else`.
`DCEEmptyElseTrimmedLabeledDanglingElse#30271` builds the
labeled-inner-if case and asserts runtime stdout is `outer-else`. Both
fail on main and pass on this branch.
Fixes #30271
robobun
force-pushed
the
farm/d4fed240/dce-empty-else-trim
branch
from
447e846 to
ef598cb
Compare
## Problem
`bun build --define Bun.env.NODE_ENV="'production'" test.ts` on:
```ts
if ("foo" === "foo") {
console.log("success!");
} else {
console.log("fail!");
}
```
produced:
```js
if (true) {
console.log("success!");
} else {}
```
The constant-folded test expression and the pruned dead branch are
correct, but the empty `else {}` remnant is left behind. The [`--define`
docs guide](https://bun.com/docs/guides/runtime/define-constant) also
promised full collapse to just `console.log("success!");` — which only
ever happened with `--minify-syntax`.
## Cause
`visitStmt`'s `s_if` handler had an "else trim" gated on
`minify_syntax`:
```zig
// Trim unnecessary "else" clauses
if (p.options.features.minify_syntax) {
if (data.no != null and @as(Stmt.Tag, data.no.?.data) == .s_empty) {
data.no = null;
}
}
```
Two issues:
1. Gated on `minify_syntax`, even though the thing it's cleaning up (an
`else` body emptied by DCE statement-pruning) is produced by DCE itself.
2. Checked for `.s_empty`, but `visitSingleStmtBlock` only converts an
empty block to `.s_empty` when `minify_syntax` is on — otherwise the
pruned body stays as an `.s_block` with `stmts.len == 0`.
The scaffolding collapse (`if (true) { A }` → `A`) intentionally stays
behind `--minify-syntax` since tests (e.g. `transpiler.test.js`'s DCE
block, `"if (undefined) { let y = Math.random(); }" → "if (undefined)
{}"`) lock that contract in, matching esbuild. This change is strictly
about cleaning up the ugly empty-else remnant.
## Fix
Gate the else-trim on `dead_code_elimination` and treat an `.s_block`
with zero stmts as equivalent to `.s_empty`:
```zig
if (p.options.features.dead_code_elimination) {
if (data.no) |no_stmt| {
const no_is_empty = switch (no_stmt.data) {
.s_empty => true,
.s_block => |block| block.stmts.len == 0,
else => false,
};
if (no_is_empty) {
data.no = null;
}
}
}
```
Also updates the define-constant docs page so the promised before/after
output matches what the bundler actually emits, and points users to
`--minify-syntax` / `--minify` for the full scaffolding collapse shown
as the final step.
## Verification
`test/bundler/bundler_edgecase.test.ts`'s `DCEEmptyElseTrimmed#30271`
asserts the output of the repro contains neither `fail` nor `else`. It
fails on `main` (output is `if (true) { console.log("success"); } else
{}`) and passes on this branch (`if (true) { console.log("success");
}`).
Existing DCE tests (`bundler_edgecase.test.ts`'s `DCE*`,
`transpiler.test.js`'s DCE block, `esbuild/dce.test.ts`,
`esbuild/default.test.ts`, `esbuild/ts.test.ts`,
`bundler_minify.test.ts`) all pass — no test depended on an `else {}`
remnant surviving.
Fixes oven-sh#30271
2 participants
Problem
bun build --define Bun.env.NODE_ENV="'production'" test.tson:produced:
The constant-folded test expression and the pruned dead branch are correct, but the empty
else {}remnant is left behind. The--definedocs guide also promised full collapse to justconsole.log("success!");— which only ever happened with--minify-syntax.Cause
visitStmt'ss_ifhandler had an "else trim" gated onminify_syntax:Two issues:
minify_syntax, even though the thing it's cleaning up (anelsebody emptied by DCE statement-pruning) is produced by DCE itself..s_empty, butvisitSingleStmtBlockonly converts an empty block to.s_emptywhenminify_syntaxis on — otherwise the pruned body stays as an.s_blockwithstmts.len == 0.The scaffolding collapse (
if (true) { A }→A) intentionally stays behind--minify-syntaxsince tests (e.g.transpiler.test.js's DCE block,"if (undefined) { let y = Math.random(); }" → "if (undefined) {}") lock that contract in, matching esbuild. This change is strictly about cleaning up the ugly empty-else remnant.Fix
Gate the else-trim on
dead_code_eliminationand treat an.s_blockwith zero stmts as equivalent to.s_empty:Also updates the define-constant docs page so the promised before/after output matches what the bundler actually emits, and points users to
--minify-syntax/--minifyfor the full scaffolding collapse shown as the final step.Verification
test/bundler/bundler_edgecase.test.ts'sDCEEmptyElseTrimmed#30271asserts the output of the repro contains neitherfailnorelse. It fails onmain(output isif (true) { console.log("success"); } else {}) and passes on this branch (if (true) { console.log("success"); }).Existing DCE tests (
bundler_edgecase.test.ts'sDCE*,transpiler.test.js's DCE block,esbuild/dce.test.ts,esbuild/default.test.ts,esbuild/ts.test.ts,bundler_minify.test.ts) all pass — no test depended on anelse {}remnant surviving.Fixes #30271