Repository navigation
Conversation
When --minify-syntax is enabled, rewrite if statements whose branches
are expression statements, returns, or throws into the equivalent
shorter form:
if (a) b(); else c(); => a ? b() : c();
if (a) b(); => a && b();
if (!a) b(); => a || b();
if (!a) b; else c; => a ? c : b (branch swap)
if (a) return b; else return c; => return a ? b : c;
if (a) throw b; else throw c; => throw a ? b : c;
if (a) {} else b(); => a || b();
if (a()) {} => a();
Branches containing declarations or jumps (var/let/break/continue/for)
are left as if statements.
react-dom.development.js shrinks by ~1% (318570 -> 315424 bytes) with
no runtime behavior change.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 7:20 AM PT - Jul 20th, 2026
✅ @robobun, your commit 2b90cf2768e7665370b72cac421fdc2862e93cdb passed in 🧪 To try this PR locally: bunx bun-pr 34826That installs a local version of the PR into your bun-34826 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a minifier codegen change that reshapes every if in every --minify-syntax bundle and has already surfaced one downstream-tool interaction (React Compiler's @validateRefAccessDuringRender), so it's worth a human look.
What was reviewed:
- Traced each rewrite arm (empty-yes swap,
!aswap, SExpr/SReturn/SThrow collapse) for semantic equivalence — all check out, including the fall-through wheredatais mutated in place butmangle_ifreturnsNone(e.g.if (!a) {var x} else {var y}→if (a) {var y} else {var x}still pushed as anif). - Confirmed the pass is gated inside the existing
minify_syntaxblock and that{}reachesis_empty_stmtasSEmptyviastmts_to_single_stmt. - Checked function-declaration bodies (
SFunction),break/varbranches, and nestedifall fall through untouched.
Extended reasoning...
Overview
Adds an esbuild-style mangle_if pass to src/js_parser/visit/visit_stmt.rs: when both branches of an if reduce to SExpr/SReturn/SThrow (or one branch is empty), the statement is rewritten to a conditional / logical expression. ~160 net lines in the parser plus three new bundler tests, one source-map snapshot update, and one React Compiler divergence-table entry.
Security risks
None. Pure AST-to-AST rewrite in the minifier; no I/O, no untrusted-length arithmetic, no allocation changes.
Level of scrutiny
Medium-high. The logic is straightforward and closely mirrors esbuild's mangleIf, but it changes the printed shape of every if statement in every minified bundle. The PR itself demonstrates one class of downstream fallout (React Compiler's ref-lazy-init whitelist only recognises the if-statement form), and other tools that pattern-match on output shape could be similarly affected. That's a product/compat call, not a correctness bug, so a maintainer should sign off.
Other factors
- I traced the in-place mutation of
datawhenmangle_ifreturnsNone(the!aswap and the empty-yes →not(test)swap): the resultingifis still semantically equivalent, so pushing*stmtafterward is safe. - Verified via
stmts_to_single_stmtthat an empty{}branch reachesmangle_ifasSEmpty, sois_empty_stmtdoesn't need anSBlockarm. - Test coverage is solid: shape assertions, a runtime side-effect test over both truthy/falsy inputs, a minify-off negative, and explicit
var/breakkeep-as-is cases. The fulltest/bundler/sweep passes per the description.
There was a problem hiding this comment.
No bugs found, but this adds ~160 lines of new minifier AST transforms in the parser hot path with wide-reaching output changes (every minified build), so it's worth a human look.
What was reviewed:
- Traced each
mangle_ifrewrite (empty-branch swap via.not(),!-unwrap for||/?:polarity, mixedreturn;/return x→void 0, unary-but-not-!tests) — all preserve evaluation order and short-circuit semantics. - Confirmed the transform is gated on
minify_syntaxand falls through to the originaliffor non-expression branches (var/break kept as-is per test). - Checked the
bundler_npm.test.tscolumn/filesize snapshot deltas and the new React CompilerMINIFY_SYNTAX_DIVERGENCEentry are consistent with the shorter output.
Extended reasoning...
Overview
Adds an esbuild-style mangle_if pass to src/js_parser/visit/visit_stmt.rs that rewrites if statements into conditional/logical expressions under --minify-syntax. Replaces the previous ~24-line stub (which only dropped fully-empty ifs) with ~160 lines covering: both-branch SExpr/SReturn/SThrow → ternary, single-branch SExpr → &&/||, empty-branch elimination via simplify_unused_expr, and !-test polarity swaps. Three new bundler tests (shape, runtime semantics, off-without-minify), plus snapshot updates in bundler_edgecase.test.ts (VLQ mapping), bundler_npm.test.ts (ReactSSR sourcemap columns + exact filesize), and a new MINIFY_SYNTAX_DIVERGENCE entry in the React Compiler fixture harness.
Security risks
None. Pure AST-to-AST rewrite of already-visited statements; no I/O, no untrusted-length arithmetic, no allocation from user-controlled sizes.
Level of scrutiny
High. The JS parser/minifier is a critical hot path — every bun build --minify output changes shape. The transform must be semantically identical to the input for all inputs (short-circuit, side-effect ordering, return/throw value semantics), and a subtle bug here would ship broken minified bundles. The tests are good (both shape assertions and a runtime semantics test that exercises truthy/falsy inputs), and the ReactSSR test still renders correctly, but the surface area is large enough that a maintainer should confirm the esbuild parity scope and the React Compiler divergence trade-off are acceptable.
Other factors
- The removed code was a limited
can_remove_testcheck; the newis_empty_stmt+simplify_unused_exprpath subsumes it (verified theemptyBoth(a){a()}test covers the side-effecting-test case). - The
.not()helper simplifies double-negation, soif (!a) {} else b()correctly becomesa && b()rather than!!a && b(); theemptyYestest only covers the un-negated direction. - The React Compiler fixture divergence (
@validateRefAccessDuringRenderno longer recognizes the lazy-init pattern afterif→&&rewrite) is a real minify-vs-compiler ordering interaction; it's tracked in the same table as the existing constant-folding divergence, but a maintainer should confirm this is the intended policy. bundler_npm.test.tshasexpectExactFilesizeand hard-coded sourcemap columns — these are brittle by design and the deltas (−1600 bytes, columns shifted left) are consistent with shorter output, but I did not independently reproduce them.
Ported from the test added in #34826, which this PR supersedes. Each rewritten `if` form is checked for shape, then the bundle runs with both inputs and the observed side effects are compared with the original statement. `blocks` covers comma-joined branches, `retMixed` covers `if (a) return; else return x` at the end of a function body, and `emptyBoth` keeps the side effect of the test when both branches are empty.
|
Closing in favor of #41159, which ports the full set of esbuild statement-level passes (including this mangleIf rewrite) on top of current main. This branch conflicts with main and its scope is covered there. The runtime test from this PR is ported to #41159 as |
Ported from the test added in #34826, which this PR supersedes. Each rewritten `if` form is checked for shape, then the bundle runs with both inputs and the observed side effects are compared with the original statement. `blocks` covers comma-joined branches, `retMixed` covers `if (a) return; else return x` at the end of a function body, and `emptyBoth` keeps the side effect of the test when both branches are empty.
1 participant
What does this PR do?
Adds the esbuild-style
mangleIfpass to--minify-syntax. When both branches of anifare expression statements (or bothreturn, or boththrow), the whole statement is rewritten to the shorter expression form.Branches that contain declarations, labels, or jumps (
var/let/break/continue/loop bodies that can't collapse to a single expression) are left asifstatements, and the transform is gated entirely onminify_syntaxso unminified output is unchanged.Because the branch bodies have already been visited by the time
mangle_ifruns, multi-statement blocks likeif (a) { x(); y(); } else { z(); w(); }have already been comma-joined into a singleSExprand also collapse (a ? (x(), y()) : (z(), w())).Size impact
bun build --minifyon react-dom 18.3.1cjs/react-dom.development.js:node --checkaccepts the output; thecompile/ReactSSR+{cjs,esm}+minifytests (which actually render) pass.How did you verify your code works?
minify/IfStatementManglingasserts the output shape for each rewrite and thatvar/breakbranches are kept as-is.minify/IfStatementManglingSemanticsruns every rewritten form with both truthy and falsy inputs and checks the observed side effects match.minify/IfStatementManglingOffWithoutMinifySyntaxchecks nothing changes withoutminifySyntax.edgecase/EmitInvalidSourceMap2mapping snapshot: the__toESMruntime helper containsifstatements that now minify further, shifting the first column in the generated map. The validity assertions in that test still pass; only the exact VLQ string moved.allow-ref-lazy-initialization-with-logicalto the React CompilerMINIFY_SYNTAX_DIVERGENCEtable:@validateRefAccessDuringRenderonly whitelists the lazy-init pattern when it sees anifstatement, so after the rewrite it rejects the fixture. This is the same class of visit-order interaction already tracked there.bun bd test test/bundler/sweep: 3925 pass, 0 fail.Follow-ups I'll send separately (per review feedback):
whiletofor(;;),a ? true : falseto!!a, computed-string property keys, trailingreturn;removal, shorter number printing.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts test/bundler/bundler_npm.test.ts
Fixes #4211