Skip to content

js_parser: simplify the comma operands hoisted out of an unused ternary - #39019

Open
robobun wants to merge 1 commit into
mainfrom
farm/9df1ea41/unused-ternary-comma-test
Open

robobun wants to merge 1 commit into
mainfrom
farm/9df1ea41/unused-ternary-comma-test

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • new Bun.Transpiler({ loader: "js" }).transformSync("(0, b) ? f() : 0;") prints 0, b && f();. Transpiling that output again prints b && f();, so the output of one pass is not a fixed point. Same for (0, b) ? 0 : f(); (0, b || f();), for longer or nested comma chains ((0, 1, b), (0, (1, b))), for declared variables in place of the literal, and for the ternary nested in another unused expression ([... ], !, void, the right side of &&).
  • Cause: the EIf arm of SideEffects::simplify_unused_expr (src/js_parser/scan/scan_side_effects.rs:180-196) rewrites an unused test ? yes : <removed> with Expr::join_with_left_associative_op, which hoists a comma test out of the operator: (a, b) && f() becomes a, (b && f()). The hoisted a is an unused expression from that point on, but nothing simplifies it. The second transpile sees it as the left operand of an unused comma statement (simplify_unused_binary_comma_expr) and drops it.
  • Only the default mode (dead code elimination on, minify syntax off) is affected. With minify: { syntax: true } the visitor already simplifies comma left operands while visiting the comma (src/js_parser/visit/visit_binary.rs:201). That is also why the esbuild code this arm was ported from does not need to do anything here: esbuild only runs this simplification when minifying.
  • Found by the transpiler fixed-point fuzzer in test: add seeded differential fuzz oracles for node:path, Bun.Glob, source maps and the transpiler #39006, which currently keeps comma expressions out of unused expression statements because of this; that exclusion can go once this lands.

Fix

  • The EIf arm joins the test and the surviving branch through a new join_unused_test_with_branch. It walks the comma chain of the test, passes each hoisted operand through simplify_unused_expr (dropping it on None), and joins the last operand, the only one whose value is still used, with the branch via join_with_left_associative_op exactly as before. A test that is not a comma takes the same path as before.
  • This is correct because nothing about evaluation changes: in x1, ..., xn ? yes : <removed> the operands x1..xn-1 are evaluated only for their effects, and keeping exactly the effects of an unused expression is what simplify_unused_expr does (so (u, b) ? f() : 0 with an undeclared u still prints u, b && f(), since reading u can throw). xn feeds the && / || and is left alone.
  • The result is fully simplified in one pass, and for these inputs it is byte for byte what minify: { syntax: true } and esbuild --minify-syntax already print (b && f(), g(), b && f(), b, c && f(), b && c && f(), a && (c && f())).
  • Verified with the new cases in test/bundler/transpiler/transpiler.test.js, which assert the one-pass output and that transpiling that output again leaves it unchanged: 16 of the 17 cases fail on the released build (the 17th is the must-keep u case), all pass with the fix.
  • Random probe (details below): with the fix, 0 non-fixed-point programs and 0 side-effect differences between the source, the once- and the twice-transpiled program over 3 x 3000 generated programs in both modes; the released build has 23-24 non-fixed-point programs per 2000 in the default mode.
  • test/bundler/transpiler/transpiler.test.js, test/bundler/esbuild/dce.test.ts, test/bundler/bundler_minify.test.ts and test/bundler/bundler_edgecase.test.ts pass with the debug build.

Background

  • simplify_unused_expr(expr) takes an expression whose value is not used (an expression statement, the branch of an unused ternary, an item of an unused array literal, ...) and returns an expression that performs the same side effects and nothing else, or None when there are none. It drops literals and reads of declared variables and keeps calls, assignments and reads of undeclared identifiers (those can throw). It runs on every expression statement whenever dead code elimination is enabled, which is the default for Bun.Transpiler, bun build and the runtime, not only when minifying.
  • Expr::join_with_left_associative_op(op, a, b) builds a op b while rotating the tree so that the printer needs no parentheses: a comma on the left is hoisted out ((x, y) op b becomes x, (y op b)) and the same operator on the right is re-associated (a op (b op c) becomes (a op b) op c). The hoisting is what creates the new unused operands this PR simplifies.
  • Fixed point: transpiling the transpiler's own output should print it unchanged. A violation is harmless on its own, but it means a pass left simplification work behind, and the fuzzer in test: add seeded differential fuzz oracles for node:path, Bun.Glob, source maps and the transpiler #39006 asserts the property to catch printer and simplifier disagreements.
Random probe

Programs are one unused expression statement built from commas, ternaries, &&, ||, array literals and ! over literals, declared variables, an undeclared identifier and three logging calls. Each program is transpiled twice; the test checks p1 === p2 and runs the source, p1 and p2, comparing the call logs (or the thrown error class).

system bun 1.4.0, default mode:       programs=2000 notFixedPoint=24 semanticDiffs=0  (x3 seeds: 24, 24, 23)
this branch, default mode:            programs=3000 notFixedPoint=0  semanticDiffs=0  (x3 seeds)
this branch, minify: { syntax: true }: programs=3000 notFixedPoint=0  semanticDiffs=0  (x3 seeds)

The only minify-mode divergence the probe finds is identical before and after this change (!(a, "s") printing as (a, !"s"), a missing string case in Expr::maybe_simplify_not) and is tracked separately.

An unused "(a, b) ? f() : 0" is rewritten to "a, b && f()", which turns
"a" into an unused expression of its own. It was never simplified, so
side-effect-free operands survived one transpile and were removed by the
next one. Simplify the hoisted operands while joining the test with the
remaining branch, so the output is fully simplified in one pass.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c9a2d94a-4a6b-457c-a3e4-ed01ba88bfcd

📥 Commits

Reviewing files that changed from the base of the PR and between 732491c and c015fba.

📒 Files selected for processing (2)
  • src/js_parser/scan/scan_side_effects.rs
  • test/bundler/transpiler/transpiler.test.js

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:55 AM PT - Aug 15th, 2026

@robobun, your commit c015fba is building: #98005

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix pushed, waiting for CI.

Reproduced on the released 1.4.0 build with new Bun.Transpiler({ loader: "js" }): (0, b) ? f() : 0; prints 0, b && f();, and transpiling that again prints b && f();. The 17 shapes added to test/bundler/transpiler/transpiler.test.js (including nested comma chains, operands with side effects, undeclared identifiers and the ternary nested in other unused expressions) assert the one-pass output and that re-transpiling it is unchanged; 16 of them fail without the change in scan_side_effects.rs and all pass with it.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — focused simplifier fix with strong test coverage.

What was reviewed:

  • Traced join_unused_test_with_branch against join_with_left_associative_op, join_with_comma, and to_empty: non-comma tests fall through unchanged; left- and right-nested comma chains preserve evaluation order.
  • Checked that the loop leaves test non-comma so join_with_left_associative_op cannot hoist further unused operands, and that recursion inherits the existing stack-check via simplify_unused_expr.
  • Verified the 17 test cases cover both &&/|| arms, declared vs. undeclared identifiers, nested contexts ([], !, inner ternary, RHS of &&), and the fixed-point property.
Extended reasoning...

Overview

This PR touches one function in src/js_parser/scan/scan_side_effects.rs (the EIf arm of SideEffects::simplify_unused_expr) and adds a 17-case it.each block to test/bundler/transpiler/transpiler.test.js. The two join_with_left_associative_op calls are replaced with a new local helper join_unused_test_with_branch that first walks the comma chain of the ternary's test, passes each hoisted operand through simplify_unused_expr, and only then hands the last (still-used) operand to join_with_left_associative_op. Net Rust delta is ~30 lines including a doc comment.

Security risks

None. This is a pure AST-simplification refinement inside the transpiler's dead-code-elimination pass. No untrusted-input parsing, allocation sizing, FFI, or filesystem/network surface is touched. Expr/ExprData are Copy arena-backed values, so there is no new ownership or lifetime concern; the helper reuses the existing simplify_unused_expr recursion (with its stack check) and join_with_comma (which already handles EMissing).

Level of scrutiny

Medium. The transpiler runs on every JS file, so a semantic mistake here would miscompile user code. I traced the helper against the underlying primitives: for a non-comma test the loop is skipped and behavior is byte-identical to before; for left-associated ((x1, x2), x3) and right-associated (x1, (x2, x3)) chains the loop preserves left-to-right evaluation order and only simplifies operands whose value is discarded — exactly the contract of simplify_unused_expr. After the loop test is guaranteed not to be a comma, so join_with_left_associative_op cannot introduce further unused operands. The undeclared-identifier case ((u, b) ? f() : 0) correctly keeps u because simplify_unused_expr returns Some for unbound identifiers.

Other factors

Test coverage is unusually thorough for a change this size: both surviving-branch polarities, declared vs. undeclared identifiers, longer and nested comma chains, side-effecting operands at each position, and the ternary nested inside [], !, another ternary, and the RHS of && — each asserting exact one-pass output and that a second transpile is a fixed point. The PR description additionally reports a 3×3000-program semantic-diff fuzzer run with zero divergences. The let-chain while let ... && ... syntax is already used elsewhere in src/js_parser/. No prior human review comments to address.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant