Repository navigation
js_parser: stop dropping side effects when folding !/typeof on array/object/class literals - #34254
Conversation
The unary simplifier folded `![array]`, `!{obj}`, `!void expr` and
`typeof [array]`/`typeof {obj}`/`typeof class{}` to a constant while
discarding the side effects inside the operand. esbuild only performs
these folds when the operand is known to have no side effects.
* UnNot: require SideEffects::NoSideEffects from to_boolean() before
replacing with an EBoolean, matching esbuild's UnOpNot handling.
* Tag::typeof_: drop EArray/EObject/EClass so the helper honours its
"no side effects" contract (array elements, object properties and
class static initializers can run arbitrary code). EArrayJSON,
EObjectJSON and ERegExp remain because they are pure literals.
Also guard e_index's maybe_rewrite_property_access with
optional_chain.is_none(), mirroring e_dot. Without this, enum constant
inlining turned `Foo?.["A"]()` into `0()`, losing the optional-chain
guard that the dotted form `Foo?.A` already preserved.
Updated the existing transpiler constant-folding assertions for
`typeof []`/`typeof {}` to reflect that these are no longer folded,
matching esbuild.
|
Reproduced with Pushback review addressed in 0dc5386: restored Status: diff is green, ready for maintainer review. All transpiler and bundler tests pass locally and on CI. Remaining red lanes across builds #73311, #73346, #73390 are unrelated flaky tests (next-pages dev server, spawn, worker-message-port, socket.io, fetch backpressure/TLS, net memleak, install registry, napi, valkey, webview, es-module-lexer timeout); none touch the parser. |
|
Warning Review limit reached
Next review available in: 6 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 (4)
Comment |
|
Updated 4:10 PM PT - Jul 15th, 2026
❌ @robobun, your commit 66d74a4 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34254That installs a local version of the PR into your bun-34254 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the three fixes each mirror an existing guard in the same file and only make folding more conservative — but this touches js_parser and drops a previously-asserted optimization (typeof [] → "object"), so it's worth a maintainer glance.
Checked: UnNot now requires NoSideEffects (same pattern as visit_binary.rs:395); e_index's new optional_chain.is_none() guard matches e_dot at visit_expr.rs:1441.
Checked: Tag::typeof_ callers all flow through to_typeof() whose doc contract already required no side effects — dropping EArray/EObject/EClass aligns impl with contract.
Checked: EFunction/EArrow/ERegExp/E*JSON remaining in typeof_ are genuinely pure at evaluation.
Extended reasoning...
Overview
Three targeted correctness fixes in the expression simplifier, all in the direction of less aggressive constant folding:
src/ast/expr.rs:Tag::typeof_dropsEArray/EObject/EClasssotypeof [f()]is no longer folded to"object"while discardingf().src/js_parser/visit/visit_expr.rsUnNot: addsside_effects == NoSideEffectsbefore replacing!exprwith a boolean literal.src/js_parser/visit/visit_expr.rse_index: gatesmaybe_rewrite_property_accessonoptional_chain.is_none(), soEnum?.["A"]is no longer inlined to0.
Plus a new test file and updates to transpiler.test.js reflecting that typeof [] / typeof {} are no longer folded.
Security risks
None. This is transpiler constant-folding logic; no auth, crypto, I/O, or untrusted-input parsing surface changed. The changes only cause the transpiler to preserve more of the input.
Level of scrutiny
Medium-high. js_parser runs on every file Bun processes, so regressions have wide blast radius. That said, each change here strictly narrows an optimization (adds a guard), so the failure mode is "missed optimization" rather than "wrong output". Each guard is copied from an adjacent, already-shipping site: the NoSideEffects conjunction matches visit_binary.rs:395, and the optional_chain.is_none() guard matches the e_dot path at visit_expr.rs:1441. The typeof_ change realigns the implementation with its own doc comment at expr.rs:3142.
Other factors
- The one non-mechanical decision is dropping
EArray/EObject/EClassfromtypeof_wholesale rather than inspecting their contents for purity. That's what esbuild'sTypeofWithoutSideEffectsdoes andtypeof_only sees the tag, so it's the natural fix — but it does meantypeof []on a genuinely-pure empty literal is no longer folded, and existing test assertions were updated to reflect that. A maintainer should confirm they're fine with that trade-off. - I confirmed the remaining
typeof_arms (EFunction,EArrow,ERegExp,EArrayJSON,EObjectJSON, primitives) are pure at evaluation time, so no analogous bug remains there. - Test coverage is thorough: both minify modes, all three literal kinds,
!/!!/typeof/typeof ===, a runtime spawn test asserting side effects actually execute, and negative tests that pure primitives still fold. PR reports the wider bundler/esbuild suites pass. - No CODEOWNERS on these paths; no prior reviewer comments to address.
There was a problem hiding this comment.
I didn't find any bugs. The three changes are each strictly more conservative (less folding / less rewriting), so they can't introduce new miscompilations — but this is the core transpiler simplifier that runs on every file, and it changes existing typeof [] / typeof {} folding behaviour, so a human should sign off.
What was reviewed:
Tag::typeof_— only caller is theUnTypeoffold atvisit_expr.rs:1223;EFunction/EArrow/ERegExp/E*JSONremain and are genuinely side-effect-free.UnNot— the newNoSideEffectsguard matches the pattern already used atscan_side_effects.rs:84/95.e_indexoptional-chain guard — mirrors the existinge_dotguard atvisit_expr.rs:1441.- Updated
transpiler.test.jsassertions match the new (esbuild-aligned) output; no other callers ofto_typeof/typeof_exist.
Extended reasoning...
Overview
Three narrow correctness fixes in the expression simplifier, all in src/js_parser/visit/visit_expr.rs and src/ast/expr.rs:
UnNotfolding now requiresside_effects == NoSideEffectsin addition to.okbefore replacing!exprwith a boolean literal.Tag::typeof_dropsEArray/EObject/EClasssotypeof [f()]etc. are no longer folded to a string constant.EArrayJSON/EObjectJSON/ERegExp/EFunction/EArrowremain (all side-effect-free at evaluation time).e_indexnow guardsmaybe_rewrite_property_accesswithoptional_chain.is_none(), matching the siblinge_dotpath at line 1441 and esbuild's behaviour, soEnum?.["A"]is no longer inlined to a bare constant.
Plus a new 24-case test file and updates to 11 existing assertions in transpiler.test.js.
Security risks
None. Pure AST-level constant-folding logic; no I/O, auth, crypto, or untrusted-input parsing changes.
Level of scrutiny
High — this is the transpiler's expression simplifier, which runs on every JS/TS file Bun processes (runtime, bundler, bun build --no-bundle). A regression here miscompiles user code silently. That said, every change is in the conservative direction: each one adds a bailout condition to an existing optimization. The worst plausible failure mode is losing a valid optimization (e.g. typeof [] no longer folds to "object"), not introducing a new wrong transformation.
Other factors
- I confirmed
Tag::typeof_has exactly one effective call site (visit_expr.rs:1223viaSideEffects::typeof_→to_typeof), so narrowing it can't break other consumers.SideEffects::to_type_ofexists but has no callers. - The
NoSideEffectscheck pattern is already used identically elsewhere inscan_side_effects.rs(lines 82-99), so this is following an established idiom. - The
e_indexguard is a copy of the existinge_dotguard — the asymmetry was clearly an oversight. - Test coverage is thorough: both minify on/off, all three literal kinds, a runtime execution assertion, and negative tests confirming pure primitives still fold. The PR ran the wider bundler/esbuild test suites clean.
- The one behaviour change a human should explicitly acknowledge:
typeof []andtypeof {}(with pure/empty contents) no longer fold. This matches esbuild and is reflected in the updatedtranspiler.test.jsassertions, but it is a visible output change to previously-asserted behaviour.
Deferring because core-transpiler changes that alter existing test expectations warrant a human sign-off, even when the diff itself looks correct.
* UnNot: also fold when expr_can_be_removed_if_unused() is true, so pure
![]/!{}/![1,2,3] keep folding to false. This mirrors esbuild's
SimplifyBooleanExpr default arm and avoids regressing the minify output
for side-effect-free literals.
* Cover every CouldHaveSideEffects branch of to_boolean under UnNot:
!(f(),true), !(f()||1), !(f()&&0), !typeof f().
* Add enum optional-chain tests to the existing property-access-inlining
describe, including a non-identifier key under minify (the path that
reaches the new e_index guard).
* Trim the new test file to the runtime spawn assertion; the expectPrinted
assertions now live next to the existing constant-folding coverage in
transpiler.test.js.
There was a problem hiding this comment.
Beyond the comment-length nit: I checked the other callers of Tag::typeof_/to_typeof — only the UnTypeof visitor and SideEffects wrappers reach it, so dropping EArray/EObject/EClass does not silently regress a separate comparison-folding path. The new optional_chain.is_none() guard in e_index mirrors the existing one for e_dot (visit_expr.rs:1442). EFunction/EArrow remain in typeof_; function/arrow expressions are pure at evaluation time so keeping them is correct.
Extended reasoning...
This is a semantic change to the JS simplifier that affects every transpiled file, so I am not approving. The logic itself looks right and matches the documented contract on to_typeof ("has no side effects (i.e. can be removed without consequence)"), and the UnNot gate now short-circuits on NoSideEffects before falling back to expr_can_be_removed_if_unused, which is why pure []/!{} still fold. I traced the callers of Tag::typeof_ to confirm the removal is scoped to the one visitor path, and confirmed the e_index optional-chain guard is the same shape as the pre-existing e_dot guard. Noting these here so a human reviewer does not need to re-derive them.
There was a problem hiding this comment.
I didn't find any correctness bugs, but this touches the parser's constant-folding hot path and changes existing optimization behavior, so it's worth a human look.
What was reviewed:
Tag::typeof_now honors its no-side-effects contract; checked callers (to_typeof/to_type_of→UnTypeofvisitor) — all fall through safely onNone.UnNotguard:expr_can_be_removed_if_unusedfallback keeps[]→!1for pure operands, so no regression there.e_indexoptional-chain guard mirrors the existinge_dotguard at visit_expr.rs:1442 exactly.- Noted:
typeof []/typeof {}on pure literals are no longer folded (unlike[]) — the asymmetry is becausetypeof_only sees the tag; a maintainer may want to weigh keeping that fold vs. the simpler fix.
Extended reasoning...
Overview
Three targeted fixes in the JS parser's expression simplifier, all correctness-over-optimization:
src/ast/expr.rs—Tag::typeof_()dropsEArray/EObject/EClass(whose contents can run arbitrary code), keeping only guaranteed-pure tags. This makes the function honor the contract stated into_typeof()'s doc comment.src/js_parser/visit/visit_expr.rs—UnNotnow requiresNoSideEffectsorexpr_can_be_removed_if_unusedbefore folding to a boolean;e_indexgains the sameoptional_chain.is_none()guard thate_dotalready has (line 1442) so enum inlining doesn't strip?.semantics.- Tests: a new runtime spawn test plus ~30 transpiler-output assertions in
transpiler.test.js, including updates to 10 pre-existing assertions that now expect unfolded output.
Security risks
None. This is pure AST-rewriting logic operating on already-parsed data; no I/O, no auth, no untrusted length arithmetic. The change is strictly more conservative (folds less), so it can't introduce new incorrect code emission — only reduce optimization.
Level of scrutiny
High. js_parser is production-critical: every line of JS/TS Bun compiles flows through this visitor, and the removed typeof []/typeof {} folds change output for existing user code. The fix itself is small and well-argued against esbuild's reference (TypeofWithoutSideEffects, UnOpNot), but two things merit a maintainer's judgment:
- Asymmetry:
UnNotretains the pure-literal fold viaexpr_can_be_removed_if_unused, butUnTypeofdoes not (becauseTag::typeof_only sees the tag, not the expr). So[]still folds to!1buttypeof []no longer folds to"object". This matches esbuild, but a maintainer might prefer adding the sameexpr_can_be_removed_if_unusedcheck at theUnTypeofcall site instead. - Changed existing assertions: 10 assertions in
transpiler.test.jswere rewritten from folded to unfolded. That's the correct move given the fix, but per CLAUDE.md's "never silently weaken an existing test" guidance, a human should confirm the optimization loss is acceptable.
Other factors
- The
e_indexguard is a mechanical mirror of thee_dotguard two hundred lines below — verified byte-for-byte pattern match. - Test coverage is thorough: every
CouldHaveSideEffectsbranch ofto_boolean, both minify modes for the enum optional-chain case, and a runtime subprocess proof. - My prior nit (4-line header comment) was addressed in 66d74a4.
- The bundled
e_indexoptional-chain fix is a separate bug in the same visitor; it's small and correct but does widen the PR's scope slightly.
### What `#34249` changed TypeScript enum lowering so that only **module-scope** enums emit `var`; an enum in a function, method, or block body now emits `let`. `#34250` merged four minutes earlier and added `it("rejects yield/await/this/super in enum initializers")`, whose expectations still assert `var x` for **block-scoped** enums. Each PR was green against a `main` that lacked the other's change, so the collision only appeared once both had landed — and since `main` pushes run no test shards, nothing caught it. The result is that `main` asserts output its own parser no longer produces. `test/bundler/transpiler/transpiler.test.js` currently fails on every PR that merges `main`. ### The fix Updates the five stale expectations to `let`. All five are enums nested in a function or method body: | Line | Case | |---|---| | 771 | `function *f() { enum x { y = (function*() { yield 1 })() } }` | | 775 | `async function f() { enum x { y = (async () => await 1)() } }` | | 785 | `function *f() { enum x { y = 1 } yield 1; }` | | 789 | `async function f() { enum x { y = 1 } await 1; }` | | 793 | `class C extends B { m() { enum x { y = 1 } super.foo(); } }` | Deliberately unchanged: - **Line 779** — `enum x { y = (function() { return this })() }` is top-level, so `var` is still correct. - **Namespace expectations** — namespaces only appear at module scope or nested in another namespace, where both the old and new predicates agree. ### Note for reviewers Only the line-771 failure is visible in CI: `expectPrinted_` throws at the first mismatch, masking the other four. A fix touching only the reported line would go red again on the next one, so all five are updated together. ### Verification Ran `test/bundler/transpiler/transpiler.test.js` against a build containing `#34249`: - pristine `main`: 176 pass / 4 fail - with this change: 177 pass / 3 fail The change flips exactly the one enum test and touches nothing else. The 3 remaining failures are unrelated to this diff — they are skew between that build and two commits that landed after it (`#34254`, `#34258`), and build from source in CI. Also swept the repo for any other assertion of enum-lowering text (the closure IIFE shape `(x ||= {})` / `(x = x || {})`). Only two files assert it: this one, and `test/js/node/module/require-extensions.test.ts:129`, whose fixture declares a top-level enum and is correctly `var`.
2 participants
Problem
The expression simplifier folds
!exprandtypeof exprto a constant without checking whether the operand has side effects, so calls inside array/object/class literals are silently deleted:esbuild keeps both unchanged (
const r = ![sideEffect()];/const r = typeof [sideEffect()];).A related issue in the same visitor: enum constant inlining runs on
Foo?.["A"](computed optional-chain access), turningFoo?.["A"]()into0(). The dotted formFoo?.Awas already guarded.Cause
e_unary/UnNotcalledSideEffects::to_boolean()but only checked.ok, ignoring theCouldHaveSideEffectsflag thatEArray/EObject/EClasscorrectly set.Tag::typeof_returnedSome("object")/Some("function")forEArray/EObject/EClass, but its contract (and doc comment) is that the operand has no side effects. Array elements, object properties, and class static initializers can run arbitrary code.e_indexcalledmaybe_rewrite_property_accesswithout theoptional_chain.is_none()guard thate_dotalready has (and that esbuild applies to both).Fix
UnNot: requireside_effects == NoSideEffectsbefore replacing with anEBoolean, matching esbuild'sUnOpNotpath.Tag::typeof_: dropEArray/EObject/EClass.EArrayJSON/EObjectJSON/ERegExpremain since they are pure literals. This matches esbuild'sTypeofWithoutSideEffects.e_index: guardmaybe_rewrite_property_accesswithoptional_chain.is_none().The existing
constant foldingassertions fortypeof []/typeof {}are updated to reflect that these are no longer folded, matching esbuild's behaviour.Verification
Also ran
transpiler.test.js,bundler_minify.test.ts,esbuild/dce.test.ts,esbuild/default.test.ts,bundler_edgecase.test.ts,bundler_string.test.ts: all pass.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file