Repository navigation
Conversation
|
Updated 1:57 AM PT - Jul 11th, 2026
❌ @robobun, your commit 897b7f6 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33378That installs a local version of the PR into your bun-33378 --bun |
WalkthroughThis PR changes ChangesExport Replacement Independent of Dead Code Elimination
Related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/transpiler/transpiler.test.js`:
- Around line 1663-1679: The namespace regression test is only covering the
default deadCodeElimination behavior, so it misses the DCE-off path this change
is meant to protect. Move the existing “does not touch a namespace member that
happens to share the name” assertions into the same deadCodeElimination loop
used by the surrounding Bun.Transpiler tests, or duplicate them for both true
and false so the namespace function/class cases are exercised under both
settings.
- Around line 1626-1659: The default-export cases in the transpiler tests only
verify transform output, but they should also assert the scanned exports list
like the nearby forms-based tests. Update the three defaultValues cases in
transpiler.test.js to call transpiler.scan(code).exports in addition to
transformSync, using the same Transpiler setup in each test. This is especially
important for the eliminate/replace/inject default-export paths so regressions
in how default and __N_SSG are reported through scanning are caught
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69944db9-6915-4410-9666-7dfd2b315b83
📒 Files selected for processing (4)
src/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/visit/visit_stmt.rstest/bundler/transpiler/transpiler.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for extending the namespace guard to s_local and adding the const case to the test — that closes out my earlier note. I don't see any remaining issues, but the control-flow restructuring in s_export_default/s_function/s_class (splitting mark_as_dead from should_replace_export, the early return that skips class lowering, and the new inject_default_replacement! on dead paths) is subtle enough that a human should sign off.
Extended reasoning...
Overview
This PR fixes Bun.Transpiler's exports.eliminate/exports.replace so they apply to export function and export class declarations regardless of the deadCodeElimination setting, and stops them from incorrectly matching TypeScript namespace members. It touches four files: src/js_parser/p.rs (append_part now returns whether it appended), src/js_parser/parse/parse_entry.rs (callers gate the hoisting parts.pop() on that return), src/js_parser/visit/visit_stmt.rs (splits mark_as_dead into should_replace_export + DCE across s_function/s_class/s_local, adds a macro to inject default replacements on dead paths, and makes the export default class replacement branch push and return early to avoid lowering overwriting the replacement), and test/bundler/transpiler/transpiler.test.js (a comprehensive matrix over DCE on/off × 4 declaration forms × eliminate/replace/inject, plus default-export forms, ordering, namespace non-interference, and a spawned subprocess for the previously-aborting case).
Since my last review, commit 9dd04bc addressed my inline note by adding enclosing_namespace_arg_ref.is_none() to the s_local gate and a const assertion to the namespace test, and also folded in both CodeRabbit nits (default-export .scan().exports assertions and moving the namespace test inside the DCE loop).
Security risks
None identified. This is transpiler output-shaping logic; no auth, crypto, filesystem, or network surface. The closest security-adjacent angle is that exports.eliminate is used by frameworks to strip server-only code from client bundles, and this PR makes that stripping more reliable (previously function/class exports could leak through with DCE off).
Level of scrutiny
Medium-high. The parser visitor is a critical, high-traffic code path where control-flow ordering matters (statement hoisting, is_control_flow_dead restore, lowering vs. replacement). The refactor is well-reasoned and the removed if mark_as_dead inside else if !mark_as_dead was provably dead, and || was_export_inside_namespace is now subsumed by !should_replace_export since the two flags are mutually exclusive — but verifying those equivalences requires tracing several interacting booleans across ~200 lines. That's beyond what I'd auto-approve.
Other factors
Test coverage is excellent: the new describe block asserts exact output for the full DCE × form × operation matrix, includes .scan().exports assertions, pins statement ordering (catching the earlier parts.pop() mis-hoist), spawns a subprocess for the abort case so a regression fails the child rather than the runner, and covers all three namespace-member forms. The PR description documents each failure mode on main and the wider suites that pass. All prior review feedback (mine and CodeRabbit's) is addressed. No outstanding comments.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_parser/visit/visit_stmt.rs (1)
806-833: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
.expect(...)instead of bare.unwrap()for consistency with the new macro.The invariant here ("
mark_for_replaceimpliesreplace_exportshas a"default"entry") is identical to the one the newinject_default_replacement!macro documents via.expect("infallible: mark_for_replace implies an entry"). This restructured block still uses a bare.unwrap(), which loses the diagnostic message if the invariant is ever violated by a future refactor.♻️ Proposed fix
let entry = p .options .features .replace_exports .get_ptr(b"default") .cloned() - .unwrap(); + .expect("infallible: mark_for_replace implies an entry");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/visit/visit_stmt.rs` around lines 806 - 833, The `mark_for_replace` path in `visit_stmt` still uses a bare `.unwrap()` when reading the `"default"` entry from `replace_exports`, which is inconsistent with the new `inject_default_replacement!` invariant handling. Replace that unwrap with an `.expect(...)` message matching the documented invariant, and keep the logic in the `ReplaceableExport::Replace` branch aligned with the macro’s pattern so future violations fail with a clear diagnostic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/transpiler/transpiler.test.js`:
- Around line 1661-1675: The subprocess test in the hoistable `it.each(...)`
case drops captured `stderr` from the assertion and is not marked concurrent.
Update this test to use `.concurrent` like the other process-spawning cases, and
change the final expectation to assert the combined `{ stdout, stderr, exitCode
}` result from `Promise.all(...)` using the existing `proc.stdout.text()`,
`proc.stderr.text()`, and `proc.exited` values so failures include diagnostic
output.
---
Outside diff comments:
In `@src/js_parser/visit/visit_stmt.rs`:
- Around line 806-833: The `mark_for_replace` path in `visit_stmt` still uses a
bare `.unwrap()` when reading the `"default"` entry from `replace_exports`,
which is inconsistent with the new `inject_default_replacement!` invariant
handling. Replace that unwrap with an `.expect(...)` message matching the
documented invariant, and keep the logic in the `ReplaceableExport::Replace`
branch aligned with the macro’s pattern so future violations fail with a clear
diagnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 275014c5-4329-48ea-80a7-b506def258c2
📒 Files selected for processing (2)
src/js_parser/visit/visit_stmt.rstest/bundler/transpiler/transpiler.test.js
CI statusBuild 71849 on the rebased sha
Every code path this PR touches is gated on
Previously (pre-rebase)Build 68633 on Local verificationFail-before, with Notes for review
|
…arations
exports.eliminate and exports.replace were silently ignored for
`export function f() {}` and `export class f {}` unless the unrelated
deadCodeElimination option was enabled. s_function and s_class computed one
mark_as_dead flag that gated both the usage-count suppression and the
replacement itself, and it was ANDed with dead_code_elimination. The
`export const` and `export {}` paths already applied the replacement
unconditionally and only gated the dead marking.
Split the two: should_replace_export no longer looks at dead_code_elimination,
matching visit_decls and s_export_default. It also requires that we are not
inside a namespace, since a namespace member is a property of the namespace
object rather than a module export; marking one dead emptied its body while
still emitting the statement.
Two more replace_exports paths that depended on the same flag:
- `export default class X {}` with a replacement value kept the class, because
the class lowering below reassigned data.value after the replacement was
written. Push the replaced statement and skip the lowering.
- `export default` with an inject entry (`["__N_SSG", true]`) emitted nothing
when dead_code_elimination was on, because the dead check returned before the
injection ran.
Eliminating a hoistable export now happens with dead_code_elimination off too,
which reaches the parts.pop() in _parse that assumes append_part appended a
part. That fix is the same as #33376: append_part reports whether it appended.
s_local took the same shortcut s_function and s_class did: an `export const f` inside a namespace matched a replace_exports entry named `f` and was dropped, taking its initializer's side effects with it and never assigning `N.f`. Also make the fail-before for these tests a clean assertion failure rather than a process abort: every source now leads with two statements, which turns the hoisting bug into a visible reordering instead of a pop on an empty parts list. The bare hoistable shapes that do abort run in a spawned child. Add scan() assertions to the default-export cases and run the namespace case under both deadCodeElimination settings.
The three `mark_for_replace` branches in s_export_default each re-did the `get_ptr(b"default")` lookup with a bare unwrap. Hoist it into one macro next to the injection helper so the invariant is stated once. Assert stderr in the spawned transpiler test so an abort shows its panic trace, and run those two cases concurrently.
visit_decls handles a decl without an initializer in its own branch, and that
branch read `replace_decl_and_possibly_remove`'s return value with the opposite
polarity of the branch right above it. The helper returns false for `Delete`
(leaving the decl alone) and true for `Replace`/`Inject` (which write a value
into it), so `export let f;` with `exports.eliminate` fell into `visit_decl`,
which unwraps `decl.value`, and aborted:
panic: called `Option::unwrap()` on a `None` value
while `exports.replace` took the `continue` arm and dropped the rewritten decl,
silently removing the export instead of substituting it. Match the polarity of
the initialized branch.
e7fd535 to
897b7f6
Compare
There was a problem hiding this comment.
I didn't find any bugs, and the two issues I raised earlier (namespace guard on s_local, inverted polarity in visit_decls no-initializer branch) are both addressed with tests. That said, this reworks control flow across five sites in the parser visit pass (s_function, s_class, s_local, s_export_default, append_part) and changes when statements are emitted vs. dropped — worth a human look before merging.
Checked: should_replace_export split from mark_as_dead in s_function/s_class — the removed || was_export_inside_namespace disjunct is subsumed by the new enclosing_namespace_arg_ref.is_none() guard.
Checked: inject_default_replacement! on the three is_control_flow_dead early-return paths — mark_for_replace is only set for a non-replace entry, so Delete emits nothing and Inject emits the renamed var; Replace never reaches that branch.
Checked: append_part bool return — both SClass/SExportDefault callers gate the parts.pop() on it; the third caller (default arm) discards it, which is fine.
Checked: the swapped arms in visit_decls — visit_decl now only runs after Replace/Inject writes a value, so the decl.value.unwrap() in visit_decl is safe.
Extended reasoning...
Overview
This PR fixes Bun.Transpiler's exports.eliminate/exports.replace so they apply to export function/export class regardless of deadCodeElimination, plus four sibling bugs uncovered while auditing: namespace members were incorrectly matched, export let f; (no initializer) crashed on eliminate and dropped on replace, export default class + replace was undone by class lowering, and inject on export default emitted nothing under DCE. It also carries the append_part return-bool fix from #33376 since eliminating a hoistable export now reaches parts.pop() on an empty list. Five files: visit_stmt.rs (the bulk), visit/mod.rs (polarity swap), p.rs + parse_entry.rs (append_part), and ~180 lines of new tests in transpiler.test.js.
Security risks
None identified. This is transpiler output-shaping logic (which exports survive/get replaced), not auth/crypto/permissions. The replace_exports map is populated from Bun.Transpiler options controlled by the caller, not from parsed source. No new untrusted-input parsing.
Level of scrutiny
High. visit_stmt.rs is the core visitor pass — every JS/TS file Bun transpiles goes through s_function/s_class/s_local/s_export_default. The changes reorder control flow around statement emission, add early returns, and change when is_control_flow_dead gates behavior. A mistake here affects every transpiled file, not just those using exports.eliminate. The new test matrix is thorough (DCE on/off × 4 declaration forms × eliminate/replace/inject, plus default-export forms, uninitialized let/var, and namespace members), and the author verified 34 assertions fail on main for the right reasons. But the number of independent behavioral changes (five distinct bugs) and the subtlety of the mark_as_dead/should_replace_export split warrant a maintainer's read.
Other factors
- Both of my prior inline findings (
s_localnamespace guard,visit_declspolarity) were fixed in follow-up commits with test rows added. - All CodeRabbit nits are marked addressed and resolved.
- CI on the latest push is green except an unrelated darwin artifact-download timeout (per the author's CI-status comment; ASAN lanes covering
test/bundler/transpiler/passed). - The
append_parthunk overlaps #33376; whichever lands first, the other rebases away. - The one thing I'd want a human to sanity-check is the
s_export_defaultclass branch: it now pushes*stmtand returns before class lowering runs. That's the stated intent (lowering would overwritedata.value), and the test assertsexport default 42output forexport default class X {}+ replace, so it's covered — but skipping lowering entirely is the kind of change worth a second pair of eyes.
|
Heads-up from #38287: that PR routes every statement |
|
Cross reference: #38527 makes |
### Problem - `visit_decl` in `src/js_parser/visit/mod.rs` took three adjacent `bool` parameters (`was_anonymous_named_expr`, `could_be_const_value`, `could_be_macro`), and the second of its two callers passed them positionally: `visit_decl(decl, false, was_const && !is_after, false)`. Nothing at that call says which flag is which. - This is the one `bare_bool_args` finding recorded for this file in `mordant-baseline.toml`. - The shape has already bitten once: the Zig version of this call site passed `was_const and !is_after` in the `was_anonymous_named_expr` slot (`src/js_parser/visit/visit.zig` before d451445); the Rust port put it in the right slot. Two open PRs (#34089, #35548) each add a fourth positional bool to the same call. ### Fix - Adds `VisitDeclOpts` to `src/js_parser/parser.rs`, next to `VisitArgsOpts`, with one documented field per former parameter. Same shape as the existing `VisitArgsOpts` / `ParenExprOpts` option structs; an options struct rather than three enums because `was_anonymous_named_expr` is handed straight on to `maybe_keep_expr_symbol_name` as a `bool`, and `dylint.toml` already treats several-bool structs as this repo's option-bag style. - `visit_decl` now takes `opts: VisitDeclOpts` and destructures it on entry; the rest of its body is unchanged. Both callers build the struct with every field named, using the same expressions as before, in the same order. - Removes the `"bare_bool_args:src/js_parser/visit/mod.rs" = 1` line from `mordant-baseline.toml`. Running the baseline writer scoped to `bun_js_parser` (`MORDANT_BASELINE_WRITE=1 cargo dylint --all -p bun_js_parser`) rewrites the file byte for byte identical to what is committed. - No behavior change is intended. Verified with a debug build: - `bun bd test test/bundler/transpiler/`: 3870 pass, 0 fail. The only red was `jsx-production.test.ts`, whose 32 concurrent parent+child debug-build spawns exceed the 5s per-test timeout on an 8 core box; all 32 pass with `--timeout 120000`. - `bun bd test test/bundler/bundler_minify.test.ts test/bundler/esbuild/dce.test.ts test/regression/issue/26360.test.ts test/regression/issue/22656.test.ts` (keepNames, const inlining, macros): 128 pass, 0 fail. - `bun bd test test/bundler/bundler_edgecase.test.ts`: 134 pass, 0 fail. - `cargo dylint --all -p bun_js_parser -- --keep-going` with the baseline line removed: on the previous code it reports the finding quoted below as over the baseline; on this branch it reports nothing and `target/mordant/over-baseline.txt` is not written. - No test is added: a signature refactor has no observable behavior to pin, so there is nothing a new test could fail on before this change. The existing suites above are the coverage; the lint run is what distinguishes before from after. ### Background - `visit_decls` visits every `decl` of a `let`/`const`/`var` statement. For each one it visits the initializer and then calls `visit_decl`, which does the bookkeeping that depends on facts only the caller observed: whether the initializer was an anonymous function or class before visiting (so the binding's name can be attached to it via `maybe_keep_expr_symbol_name`), whether this is a `const` still inside the scope's leading run of `const`s (so the value may be recorded in `const_values` for inlining), and whether visiting the initializer ran a macro (so the macro result may be recorded or destructured into `const_values`). The three fields of `VisitDeclOpts` are those three facts. - The second caller runs on the `exports.replace` / `exports.eliminate` path of `Bun.Transpiler` for a declaration with no initializer. Its replace/delete branches are inverted today (`eliminate` on `export let x;` hits an `unwrap` on `None`); that is pre-existing and already addressed by #33378, so it is left alone here. - `mordant-baseline.toml` is a ratchet: it records the per (lint, file) counts of findings that predate the lint job, and the job fails only on findings above those counts. Fixing a finding means deleting its line so it cannot come back. <details> <summary>Lint output on the previous code with the baseline line removed</summary> ``` warning: `visit_decl` takes bools `was_anonymous_named_expr`, `could_be_const_value` and `could_be_macro`, and 1 of its 2 calls passes bare `true`/`false` for at least two of them. At `visit_decl(.., false, .., false)` nothing says which is which --> src/js_parser/visit/mod.rs:517:19 | 517 | pub(crate) fn visit_decl( | ^^^^^^^^^^ | note: one of those calls --> src/js_parser/visit/mod.rs:417:29 | 417 | ... self.visit_decl(decl, false, was_const && !is_after, false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = help: give `was_anonymous_named_expr`, `could_be_const_value` and `could_be_macro` a two-variant enum each, or an options struct, so every call names what it sets = note: `bare_bool_args` over the mordant baseline (0 recorded for src/js_parser/visit/mod.rs) warning: mordant: 1 finding(s) over the baseline in bun_js_parser ``` On this branch the same command finishes with no warnings. </details>
This branch has not been deployed
1 participant
Repro
export class f {}behaves the same.export const f = () => {}andexport { f }are eliminated correctly, and everything works oncedeadCodeEliminationis turned on.exports.replaceis ignored in exactly the same cases.exports.eliminate/exports.replaceis the API frameworks use to strip server-only exports out of client bundles, so the declaration form deciding whether the strip happens means functions and classes, the exports people most expect to strip, survive into the client artifact whenever a tool turns DCE off.Cause
s_functionands_classfold two unrelated decisions into onemark_as_deadflag:It decides both whether to suppress usage counts while visiting the body and whether to eliminate or replace the export at all, and it is
&&-ed withdead_code_elimination. Theexport constpath (visit_decls) ands_export_defaultalready keep those separate: the replacement is unconditional, and onlyis_control_flow_deadis gated on DCE.Fix
should_replace_exportno longer looks atdead_code_elimination;mark_as_deadbecomesshould_replace_export && dead_code_elimination.Auditing the sibling sites for the same assumption turned up four more bugs in
replace_exports, all fixed here.Namespace members
should_replace_exportalso requiresenclosing_namespace_arg_ref.is_none(), in all three sites (s_function,s_class,s_local). A declaration exported from a TypeScript namespace is a property of the namespace object, not a module export, and each site got this wrong in its own way when a member happened to share a name with an eliminate/replace target:s_function/s_classmarked the body dead while the namespace branch still emitted the statement, so the body came out empty.s_localdropped the declaration outright, taking the initializer with it.Declarations with no initializer
visit_declshandles a decl without an initializer in a separate branch, and that branch readreplace_decl_and_possibly_remove's return value with the opposite polarity of the branch right above it. The helper returnsfalseforDelete(leaving the decl alone) andtrueforReplace/Inject(which write a value into it), so:Deletefell intovisit_decl, which unwrapsdecl.value;Replace/Injecttook thecontinuearm and threw the rewritten decl away. Matching the polarity of the initialized branch fixes both.export defaultexport default class X {}with a replacement value kept the class.s_export_defaultwrites the replacement intodata.value, then the class lowering below reassignsdata.valueand undoes it. The replaced statement is now pushed and the lowering skipped, the same shape the function branch already uses.export defaultwith an inject entry (replace: { default: ["__N_SSG", true] }) emitted nothing whendeadCodeEliminationwas on, because theis_control_flow_deadcheck returns before the injection runs. The injection isDelete-safe, so it runs on that path too.Hoisting
Eliminating a hoistable export now happens with
deadCodeEliminationoff as well, which reaches theparts.pop().expect("unreachable")in_parsethat assumesappend_partappended a part. Without that,export class f {}+eliminate: ["f"]aborts the process, and a non-leading one silently steals an unrelated part and reorders the output. This PR carries the identical hunk as #33376 (append_partreports whether it appended) because the tests here reach that path; if #33376 lands first, this rebases away cleanly.Verification
bun bd test test/bundler/transpiler/transpiler.test.js— 229 pass, 0 fail. Onmain, 34 of the new assertions fail, each for the right reason:mainexport function f(){}+eliminate, DCE offfsurvivesexport class f {}+eliminate, DCE offfsurvivesexport class f {}+eliminatepanic: unreachable, child abortsconsole.log("a");console.log("b");export class f {}+eliminatebbeforeaexport let f;+eliminatepanic: called Option::unwrap() on a None valueexport let f;+replace/ injectexport default class X {}+replace: { default: 42 }export default X+replace: { default: ["__N_SSG", true] }, DCE onexport namespace N { export function f(){ g(); } }+eliminateexport namespace N { export const f = g(); }+eliminateg()goneThe new block asserts the full matrix:
deadCodeEliminationin[true, false]x 4 initialized declaration forms (function, class, const, export clause) xeliminate/replace/ inject, plus uninitializedlet/var, the threeexport defaultvalue forms, and the namespace members. Every source leads with two statements so the assertions pin statement order too; every cell'stransformSyncoutput andscan().exportsis now identical across bothdeadCodeEliminationsettings. The three shapes that abort onmainrun in a spawned child so the failure is reported rather than taking the runner down.Wider suites
test/bundler/transpiler/macro-test.test.tsis excluded from that count: on a debug build the whole-directory run trips a pre-existing JSC assertion (ASSERTION FAILED: !m_topGCOwnedDataScopeinHeap::clearConcurrentRetainedDataIfPossible), which reproduces on a pristinemaincheckout and is unrelated to this change. The file passes on its own (11/11).[review] gate passed · iteration 3 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 3
evidence per changed file