Repository navigation
js_parser: fix exports.eliminate/replace on a declaration with no initializer and on namespace members - #44179
Conversation
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I traced the inverted return handling against replace_decl_and_possibly_remove (src/js_parser/p.rs:6832): Delete returns false and the decl is dropped; Replace/Inject set decl.value = Some(..) before returning true, so the fall-through into visit_decl no longer unwraps a None. The .expect("unreachable") on declare_symbol for an Inject name (p.rs:6854) was already reached by the old no-initializer arm and by the with-initializer arm, so this change does not make it newly reachable.
Extended reasoning...
The diff is a small control-flow fix in the exported-declaration branch of visit_decls in src/js_parser/visit/mod.rs plus three new tests in test/bundler/transpiler/transpiler.test.js; it touches no security-sensitive surface. The inline finding about the sibling with-initializer arm lacking the namespace guard is the reason a human should weigh the scope, so this note only records what else was checked and ruled out.
|
Status: open at e87d32d, on top of #38541 (a90fada). Waits for CI and review. Reproduced on new Bun.Transpiler({ loader: "ts", exports: { eliminate: ["A"] } }).transformSync("export let A");
// panic: called `Option::unwrap()` on a `None` value, exit 134Verification with When #38541 merges, I rebase this PR onto |
…ith no initializer The arm of visit_decls for a declaration with no initializer read the result of replace_decl_and_possibly_remove the wrong way round. An eliminated export went to visit_decl, which unwraps the missing value and aborts the process. A replaced export was dropped from the module. The arm now does what the arm for a declaration with an initializer does: drop the declaration when the helper returns false, visit it otherwise. A namespace member with no value is left alone, so that it prints what it prints today.
…namespace alone
s_local selected the visit that applies the entries for each exported
declaration, and a member of a TypeScript namespace is one. So
`namespace NS { export let A = 1 }` lost `NS.A` with
`eliminate: ["A"]`, and got `NS.A = 2` with `replace: { A: 2 }`. The
visitors of a function and of a class skip a member of a namespace.
s_local now selects that visit at module level only. The guard in the
arm for a declaration with no initializer is not necessary any more.
82db491 to
1eed084
Compare
|
Updated 10:50 PM PT - Sep 28th, 2026
✅ @robobun, your commit e87d32d6a829bbba3a6aefb6ec548a8967df0fdc passed in 🧪 To try this PR locally: bunx bun-pr 44179That installs a local version of the PR into your bun-44179 --bun |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the polarity fix itself: replace_decl_and_possibly_remove already runs visit_expr on the Replace/Inject value and visit_decl only does const-value/name bookkeeping, so the no-initializer arm does not visit the value twice, and a Delete entry now hits continue 'outer before the decl.value.unwrap() in visit_decl. Moving the namespace exclusion into the visit_decls::<true> selection in s_local covers both arms of visit_decls, and the namespace assignment loop below it still sees the decls untouched.
Extended reasoning...
The change flips the boolean read of replace_decl_and_possibly_remove in the no-initializer arm of visit_decls and gates the replace/eliminate-aware path in s_local on not being inside a TypeScript namespace, plus table-driven transpiler tests; no security-sensitive surface. Not approved because a verified finding on the s_function/s_class siblings is being posted inline and further verified findings were dropped from the post.
…that is a member of a namespace
s_function and s_class visited a member of a namespace as dead code
when its name was in the entries, and then kept the statement. So
`namespace NS { export function A() { return f() } }` with
`eliminate: ["A"]` printed `function A() {}`.
Both now look at the entries at module level only, as s_local does.
The tests of the declaration with no initializer run in one child
process.
The test in s_local, where the visit is selected, cost s_local 3 instructions for each statement with no exports option. visit_decls now does the test once, and only in the instantiation that applies the entries.
Problem
exports.eliminateaborts the process for an exported declaration with no initializer.export let Awitheliminate: ["A"]ends inpanic: called `Option::unwrap()` on a `None` value.exports.replacedrops the export:export let Awithreplace: { A: 1 }prints an empty module.visit_declsfor a declaration with no value (src/js_parser/visit/mod.rs:601) reads the result ofreplace_decl_and_possibly_removethe wrong way round, andvisit_declunwraps the missing value.Fix
visit_decls,s_functionands_classno longer apply an entry to it.test/bundler/transpiler/transpiler.test.js, 4 new tests. All 4 fail without thesrc/change. The file passes.Background
exports.eliminateremoves a named export.exports.replacegives it another value.visit_decls::<true>runs only for an exported declaration when the option is set.export let Areads the value of the entry, and a string value is a freed node after another parse.Downsides
mainchange for callers that rely on them:replace: { A: 1 }onexport let Aprintsexport let A = 1;, and an entry no longer changes a member of a namespace.exportsoption: release.texthas the same size, and 0 instructions are added.Notes
Found by fuzzing. There is no issue and no user report.
Repro
eliminate: ["A"]replace: { A: 1 }Segmentation fault at address 0x8maina4f1429panic: called `Option::unwrap()` on a `None` value, exit 134export let A = 1;The Zig source had the same arm, so the logic was never correct, and the tests are not in
test/regression/issue.Outputs that change (27 inputs, ts loader, release builds of the base a90fada and of e87d32d)
9 inputs abort on the base and print a module now. 12 print other text. 6 print the same text.
export let Aeliminate: ["A"]export let A, B = 1eliminate: ["A"]export let B = 1;export let A; A = 1;eliminate: ["A"]A = 1;namespace NS { export let A }eliminate: ["A"]export let Areplace: { A: 1 }export let A = 1;export let Areplace: { A: "bar" }export let A = "bar";export let Areplace: { A: ["N", 1] }export let N = 1;export let A, B, Creplace: { B: 1 }export let A;andexport let C;export let A;,export let B = 1;andexport let C;export let A; A = 2;replace: { A: 1 }A = 2;export let A = 1;andA = 2;namespace NS { export let A = 1 }eliminate: ["A"]NS.A = 1namespace NS { export let A = 1 }replace: { A: 2 }NS.A = 2NS.A = 1namespace NS { export let A = 1 }replace: { A: ["N", 2] }NS.N = 2NS.A = 1namespace NS { export function A() { return f() } }eliminate: ["A"]function A() {}namespace NS { export class K { m() { return f() } } }replace: { K: 1 }m() {}mA member of a namespace
s_localselectsvisit_decls::<true>for a member of a namespace too, because its gate testsis_exportand the size of the map only.s_functionands_classvisited such a member as dead code and then kept the statement, so its body was empty. A member is a property of the namespace object. It is not an export of the module, so no entry applies to it.visit_declsdoes the test once, and only in the instantiation fortrue. The first version had the test ins_local, where the visit is selected. That costs_local3 instructions for each statement with noexportsoption (188,400 to 192,000 for 1100 statements), so it moved.The test with a string and #38541
replace with a string, after the load of another moduleloads a CommonJS file of 50 lines betweennew Bun.TranspilerandtransformSync. Without #38541 the string value of the entry is a freed node at that time. Onmainthe path with no initializer never read the value, because it dropped the export. With this PR it reads the value, so this PR is on top of #38541.Measurements (release builds of the base a90fada and of e87d32d, linux x64)
.textllvm-size -A)s_localnm -S)s_function,s_classexportsoptions_local: 188,400 on both for 1100 statements (ts), 164,091 on both for 1003 (js).s_function: 10,284 on both (ts), 6700 on both (js).s_class: 6430 on both (ts), 3750 on both (js) (gdb step count, JIT off)visit_decl,visit_binding,replace_decl_and_possibly_removeobjdump, sha256)valgrind,straceandperfare not on the machine that ran this, so the instruction counts come from gdb.#33378
#33378 has the same swap among other changes: it applies the entries to function and class declarations when dead code elimination is off. It conflicts with
mainsince August. This PR is the part for a declaration with no initializer. The hunk of #33378 invisit_declsbecomes empty when it rebases over this PR.Left for later
export class A {}witheliminate: ["A"]:panic: unreachable, js_parser: fix panic when exports.eliminate removes a hoistable export #33376.replace: { A: ["default", 1] }: Bun.Transpiler: exports.replace prints a module that does not parse when the module cannot declare the export name #44138.