js_parser: collapse ParseStatementOptions scope bools and SideEffects::Result into enums

[robobun]

Two dependent-bool pairs in the parser whose cross product included states that are never constructed and would be meaningless if they were.

ParseStatementOptions scope

is_module_scope and is_namespace_scope are mutually exclusive: is_module_scope is only set at the two parse entry points (parse_entry.rs), is_namespace_scope only inside parse_type_script_namespace_stmt. Every consumer treats them as a three-way switch.

Replaced with enum StatementScope { Nested, Module, Namespace }. The seven copies of

if !opts.is_module_scope
    && (!opts.is_namespace_scope || !opts.is_typescript_declare)

fold into if !opts.allows_esm_import_export(), and the if/else if at the top of t_export becomes an exhaustive match.

scan_side_effects::Result

Result { ok, value, side_effects } only carries a meaningful value / side_effects when ok == true; every caller guards on .ok before reading either field. { ok: false, value: true } was never constructed.

Replaced with Option<Known { value, side_effects }> so to_boolean and to_null_or_undefined return None for the unknown case and callers use if let Some(..) / is_some_and(..) instead of a separate .ok read.

Why

Making both states unrepresentable removes the possibility of a future change constructing { is_module_scope: true, is_namespace_scope: true } or reading .value without checking .ok, both of which would silently miscompile. No behavior change.

Verification

This is a type-level refactor with no semantic change, so there is no test that fails before and passes after. Existing coverage exercises every touched path and passes unchanged:

• test/bundler/esbuild/dce.test.ts (78 pass)
• test/bundler/esbuild/ts.test.ts (57 pass)
• test/bundler/esbuild/default.test.ts (151 pass)
• test/bundler/esbuild/importstar_ts.test.ts (23 pass)
• test/bundler/bundler_minify.test.ts (42 pass)
• test/bundler/bundler_edgecase.test.ts (117 pass)
• test/bundler/bundler_regressions.test.ts (9 pass, incl. NamespaceTracking#12337)
• test/bundler/transpiler/transpiler.test.js (183 pass, 12 snapshots)

Net: 13 files changed, +284 / -328.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 7 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: 2b232542-55b8-4b32-94e9-97a31dedf848

📥 Commits

Reviewing files that changed from the base of the PR and between f0043ff and 733e16b.

📒 Files selected for processing (13)

• src/js_parser/p.rs
• src/js_parser/parse/mod.rs
• src/js_parser/parse/parse_entry.rs
• src/js_parser/parse/parse_fn.rs
• src/js_parser/parse/parse_skip_typescript.rs
• src/js_parser/parse/parse_stmt.rs
• src/js_parser/parse/parse_suffix.rs
• src/js_parser/parse/parse_typescript.rs
• src/js_parser/parser.rs
• src/js_parser/scan/scan_side_effects.rs
• src/js_parser/visit/visit_binary.rs
• src/js_parser/visit/visit_expr.rs
• src/js_parser/visit/visit_stmt.rs

---

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

[claude]

I didn't find any bugs — every rewrite site checks out as semantically equivalent — but this touches core parser control flow across 13 files, so it's worth a human pass.

What was reviewed:

• Verified allows_esm_import_export() is truth-table-equivalent to the old !is_module_scope && (!is_namespace_scope || !is_typescript_declare) guard at all seven sites, and that scope != Nested matches is_module_scope || is_namespace_scope.
• Checked scope: opts.scope propagation into skip_type_script_{type,interface}_stmt — those only read .is_module(), so passing Namespace where the old code passed default-false bools is a no-op.
• Traced every to_boolean/to_null_or_undefined caller: .ok && cond → is_some_and/if let Some mappings preserve the else-if fallthrough in BinLogicalOr, and the EBigInt check_equality mapping (Some(false)→value=true, None→None) matches the old ok=equal.is_some(), value=equal==Some(false).
• Confirmed the let-else return in the E::If visitor is equivalent — nothing followed the old if !ok {} fallthrough.

Extended reasoning...

Overview

Two independent type-level refactors in src/js_parser/:

1. ParseStatementOptions.{is_module_scope, is_namespace_scope}: bool → scope: StatementScope { Nested, Module, Namespace } (9 files).
2. SideEffects::Result { ok, value, side_effects } → Option<Known { value, side_effects }> returned from to_boolean/to_null_or_undefined (5 files).

Net −44 lines. No new tests (pure refactor); 660+ existing bundler/transpiler tests reported passing.

Security risks

None. Parser-internal state representation only; no user-facing surface, no I/O, no unsafe blocks touched.

Level of scrutiny

Medium-high. This is core JS/TS parser statement dispatch and DCE constant-folding. Every JS file Bun processes runs through these paths, so a subtle polarity flip would silently miscompile. I mechanically verified each of the ~30 rewrite sites against the old boolean expressions and found them all equivalent — the two non-obvious ones (the scope: opts.scope propagation into type/interface skip helpers, and the added return in the ternary visitor's unknown branch) are safe because the downstream code only reads .is_module() and there is no code after the old fallthrough respectively.

Other factors

• The Rust compiler enforces exhaustiveness, so any missed caller of the old Result type or old bool fields would fail to build; grep confirms no residual references.
• The comment-cop bot flags on the new doc comments are false positives — those are ordinary type documentation, not workaround justifications.
• Deferring rather than approving because 13 files of parser hot-path is above my threshold for auto-approval, not because I found anything wrong.

[robobun]

Self-review: 29 adversarial probes (behavior parity at every rewritten guard, scope-propagation blast radius into the type/interface skippers, Option<Known> niche sizing, call-site safety, etc.) all came back inapplicable or refuted. git diff -w confirms the visit_expr.rs / visit_binary.rs churn is de-indentation from if let Some(..) replacing if res.ok { .. }.

Pure type-level refactor, no semantic change. There is no test that can fail before and pass after; the existing bundler DCE/minify/TS/transpiler suites (660+ tests) cover every touched path and pass unchanged. Ready for a maintainer pass.

[claude]

I didn't find any bugs — every rewritten guard checks out as semantically equivalent — but this is a 13-file refactor across the parser, DCE, and visit passes, so it's worth a maintainer look.

What was reviewed:

• allows_esm_import_export() is exactly is_module_scope || (is_namespace_scope && is_typescript_declare) — matches all seven old open-coded guards via De Morgan.
• The three scope: opts.scope propagations (previously only is_module_scope was forwarded) reach skip_type_script_{type,interface}_stmt, which only read .is_module() — the Namespace-vs-Nested widening is unobservable.
• visit_expr.rs ternary: the new let-else { …; return } + tail if/else has the same control flow as the old if !ok { … } else { …; return } — nothing followed the old block.
• Option<Known> callers: each .ok && … → is_some_and/if let Some pair traced; the EBigInt and BinLogical{Or,And} recursive arms preserve the same None fall-throughs.

Extended reasoning...

Overview

Two independent type-level refactors in src/js_parser/:

1. ParseStatementOptions: the mutually-exclusive is_module_scope/is_namespace_scope bools are collapsed into enum StatementScope { Nested, Module, Namespace }, with a helper allows_esm_import_export() replacing seven copies of the same compound guard. Touches parser.rs, p.rs, parse_entry.rs, parse_stmt.rs, parse_fn.rs, parse_typescript.rs, parse_skip_typescript.rs, parse/mod.rs.
2. SideEffects::Result: { ok, value, side_effects } becomes Option<Known { value, side_effects }>, so to_boolean/to_null_or_undefined return None for unknown and callers use if let Some/is_some_and. Touches scan_side_effects.rs, visit_binary.rs, visit_expr.rs, visit_stmt.rs, parse_suffix.rs.

Net +284/−328. The removed struct fields make completeness compiler-enforced — any missed read would fail to build.

Security risks

None. This is internal parser state representation; no user-facing surface, no I/O, no untrusted-input parsing changes.

Level of scrutiny

High — js_parser is on the hot path for every file Bun transpiles or bundles, and to_boolean/to_null_or_undefined drive DCE and minification decisions. A subtle guard inversion here would silently miscompile user code. I traced each of the ~30 rewritten conditionals against its old form:

• t_export if/else if → exhaustive match on StatementScope: verified all three arms.
• (is_module_scope || is_namespace_scope) → scope != Nested: verified.
• !is_module_scope && !is_namespace_scope → scope == Nested: verified.
• The one non-mechanical bit — scope: opts.scope replacing is_module_scope: opts.is_module_scope at three propagation sites — widens what's forwarded, but the receivers (skip_type_script_{type,interface}_stmt) only ever check .is_module(), so the widening is inert.
• The visit_expr.rs ternary block was restructured from if !ok { … } else { … return } to let Some(..) else { …; return } + tail if/else; I confirmed nothing followed the old block, so the added early return and removed trailing returns are equivalent.

Other factors

The PR author ran 8 bundler test files (660+ tests) covering DCE, minify, TS namespaces, and the transpiler; all pass unchanged. The comment-cop bot's doc-comment complaints were addressed in 733e16b and all threads are resolved. The author's own self-review closes with "Ready for a maintainer pass" — I agree that a human should sign off on a parser refactor of this footprint even though I found nothing wrong.