Repository navigation
Conversation
Bun.Transpiler's exports.eliminate and exports.replace are keyed on the
exported name, which is the module's public surface and what
Bun.Transpiler.scan() reports. Export clauses matched the local name
instead, so `export { q as QA }` was missed by eliminate: ["QA"] and
removed by eliminate: ["q"]. Re-export clauses already matched the
exported name, giving the same build two opposite conventions.
Match the alias in both clause forms, and name the var that a replace
entry injects after the alias too: `export { rr as RR } from "./dep"`
replaced by { RR: expr } emitted `export var rr` rather than RR.
The injected binding is a var, so declare it hoisted; that also lets a
local of the same name merge with it instead of reporting a spurious
"already been declared".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR changes ChangesExported-name matching implementation
Related PRs: None identified Suggested labels: bug, javascript, javascript-transpiler Suggested reviewers: Jarred-Sumner, paperclover 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:05 PM PT - Jul 5th, 2026
❌ @robobun, your commit 16e091c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33386That installs a local version of the PR into your bun-33386 --bun |
StatusReproduced on new Bun.Transpiler({ loader: "ts", exports: { eliminate: ["QA"] } })
.transformSync(`const q = 1; export { q as QA };`);
// "QA" survives, because the clause matched the local "q"
Verified
Only CI: the diff is green, two unrelated lanes are red Build 68671 is the first to finish rather than be superseded: 284 jobs passed, 2 failed, and neither failure is reachable from this diff. 1.
2.
I have not pushed a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
exports.replace keys are validated with is_identifier, which accepts every
keyword, so `replace: { default: 9 }` constructs fine. A renamed export then
reached declare_symbol with the alias and printed `export var default = 9`,
which is a syntax error. The same holds for `class`, `let`, `eval` and the
rest.
Only take the rename path when the alias can spell a strict-mode binding.
Reserved-word aliases keep their export clause, which is what main already
did for `export { q as default }`.
eliminate is unaffected: it deletes the clause item and never declares a
binding, so `export { q as default }` and `export { q as class }` are still
removed by their exported name.
|
Good catch, and the analysis is right: Verified the claim before fixing, with the released binary (i.e.
So the two clause forms this PR rewires would indeed have started printing What changedRather than guard only fn can_bind_exported_name(name: &[u8]) -> bool {
js_ast::lexer_tables::keyword(name).is_none()
&& !js_lexer::is_strict_mode_reserved_word(name)
&& name != b"eval"
&& name != b"arguments"
}
Every form the PR touches now produces parseable output, checked by re-transpiling each result: Six more tests cover this ( Deliberately left for a follow-upRouting the replacement to |
![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 `@src/js_parser/visit/visit_stmt.rs`:
- Around line 196-221: The rename/bindability handling duplicated in
s_export_clause and s_export_from should be extracted into a shared helper
instead of repeated multi-line logic. Create a small named helper around the
common pattern in visit_stmt.rs that computes rename, checks
can_bind_exported_name, chooses between declare_symbol(Kind::Hoisted, alias_loc,
alias) and the original ref, and then calls inject_replacement_export. Update
both call sites to use that helper so the replacement export behavior stays
identical in both paths.
In `@test/bundler/transpiler/transpiler.test.js`:
- Around line 1601-1605: Add a test case alongside the existing s_export_from
rename coverage in transpiler.test.js to exercise export { rr as RR } from
"./dep" with replace using the Inject array form, mirroring the s_export_clause
matrix case. Keep the same test style as the existing transform assertions, and
verify the renamed re-export path in s_export_from correctly handles replace: {
RR: ["INJ", true] } so the bindability/rename branch is covered for both scalar
and array-form replacements.
🪄 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: 0f6ad47c-1436-40df-b0c1-5fbe4e74fb2d
📒 Files selected for processing (3)
packages/bun-types/bun.d.tssrc/js_parser/visit/visit_stmt.rstest/bundler/transpiler/transpiler.test.js
s_export_clause and s_export_from resolved the ref to bind identically: compute whether the export is renamed, gate on the alias being bindable, then declare a hoisted symbol for the alias or reuse the local ref. Move it next to inject_replacement_export as replacement_export_ref, which returns None when the alias has no binding spelling so the caller keeps the clause item. can_bind_exported_name moves alongside it. Also cover the renamed re-export with the inject array form, mirroring the export-clause case.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/visit/visit_stmt.rs:210-215— TheKind::Hoistedmerge covers a pre-existingvarof the alias name, but notconst/let/class:const QA = 5; const q = 1; export { q as QA };underreplace: { QA: 9 }now throws a spurious"QA" has already been declared(pointing at the alias inside the export clause, which declares nothing). Pre-PR this input was a silent no-match, so it's a valid-input → hard-error regression, though on a very contrived intersection. It'd be consistent with the reserved-word policy to fall through and leave the export alone when the alias already has a lexical binding (checkingp.current_scope().membersbeforedeclare_symbol), rather than lettingdeclare_symbollog the error — same applies to thes_export_frompath at :324-329.Extended reasoning...
What the bug is. When
exports.replacematches a renamed export clause and the alias happens to collide with an existing lexical binding in the module (const/let/class), the newdeclare_symbol(Kind::Hoisted, alias_loc, alias)call fails the merge and logs"<alias>" has already been declared.transformSyncthen throws on syntactically valid input. The PR explicitly choseKind::Hoistedso a pre-existingvar <alias>merges (and tests it at transpiler.test.js:1610), but that only covers hoisted siblings — a lexical sibling still hitsSymbolMergeResult::Forbidden.Code path. In
visit_stmt.rs:210-215(and the mirroreds_export_fromblock at :324-329), whenrenameis true andcan_bind_exported_name(alias)is true, the code callsp.declare_symbol(js_ast::symbol::Kind::Hoisted, items[i].alias_loc, alias). At visit time the module scope already contains the parse-phase declaration ofconst QA(kindConstant).declare_symbol(p.rs:4911-4933) finds the existing member, callscan_merge_symbol_kinds::<true>(Entry, Constant, Hoisted), and walks scope.rs:153-238:existing != Unbound; not the TS-import branch; the hoisted-or-function branch at :200-208 requires both kinds to satisfyis_hoisted_or_function(symbol.rs:391-395: onlyHoisted | HoistedFunction | GeneratorOrAsyncFunction), andConstantis not — so it falls through toForbiddenat :238. TheForbiddenarm callslog().add_symbol_already_declared_error(source, alias, alias_loc, existing.loc)and returnsOk(existing.ref_), so the?doesn't short-circuit; the error surfaces later whentransformSynccheckslog.errors > 0and throws.Why nothing prevents it.
can_bind_exported_nameonly checks whether the alias is a keyword / strict-mode reserved word /eval/arguments; it cannot know about existing scope members. Anddeclare_symbol'sForbiddenbranch is designed to log (not propagate via?), so the caller has no way to observe the collision and fall through instead.Impact. Pre-PR,
replace: { QA: 9 }onconst QA = 5; const q = 1; export { q as QA };keyed on the localq, never matched, and passed through unchanged. Post-PR the same input hard-errors. The diagnostic is also misleading: it points at theQAinsideexport { q as QA }, which does not declare anything in the user's source. This is inconsistent with the policy this PR itself introduces for the analogous "cannot emitexport var <alias>" case (reserved-word aliases), which falls through and leaves the export alone. It's also inconsistent with the unrenamed path:const foo = 1; export { foo };+replace: { foo: 9 }reusesref_without redeclaring, so no error there. That said, the input is highly contrived (nicheBun.TranspilerAPI × renamed export × unrelated lexical binding whose name equals the alias), pre-PR behavior was also wrong (silent miss), and there is no obviously-correct output —export var QA = 9alongsideconst QA = 5would itself be a SyntaxError — so erroring is arguably no worse than emitting broken JS; only the message is bad.Step-by-step proof.
new Bun.Transpiler({ loader: "ts", exports: { replace: { QA: 9 } } }).transformSync("const QA = 5; const q = 1; export { q as QA };").- Parse:
const QA = 5→parse_and_declare_decls(symbol::Kind::Constant, ...)(parse_stmt.rs:180) putsQA: Constantin the module scope. - Visit
export { q as QA }:alias = b"QA",name = b"q";replace_exports.get_ptr(b"QA")matches;entry.is_replace() && alias != name→rename = true;can_bind_exported_name(b"QA")→true(not a keyword/reserved/eval/arguments). p.declare_symbol(Kind::Hoisted, alias_loc, b"QA")→ existing member found →can_merge_symbol_kinds(Entry, Constant, Hoisted)→Forbidden→ logsadd_symbol_already_declared_error("QA", alias_loc, <const QA loc>), returnsOk(existing.ref_).transformSyncseeslog.errors > 0and throws"QA" has already been declared.
Same forlet QA(Kind::Other) /class QA {}(Kind::Class), and forconst QA = 5; export { rr as QA } from "./d";via thes_export_frombranch.
Fix. Before calling
declare_symbol, look upaliasinp.current_scope().members; if it exists with a non-hoisted-or-function kind, fall through and leave the export clause alone (same policy as the reserved-word case). Alternatively, declare a fresh generated symbol and emitexport { <tmp> as <alias> }instead ofexport var <alias>, which sidesteps both this collision and the reserved-word case in one go. -
🟡
src/js_parser/visit/visit_stmt.rs:322-332— Thecan_bind_exported_nameguard added in 4901f1c only fires whenrenameis true, so it misses the unrenamed re-export spelling:export { default } from "./d"underreplace: { default: 9 }hasalias == original_name,rename = false, and still emitsexport var default = 9;. Output is byte-identical to pre-PR (not a regression), but the PR now tests thatexport { rr as default } fromis left alone while the semantically-equivalent unrenamed form is not — ins_export_fromthe guard should gate oncan_bind_exported_name(alias)wheneverentry.is_replace(), keepingrenameonly for thedeclare_symboldecision.Extended reasoning...
What the bug is. Commit 4901f1c added
can_bind_exported_nameand applied it in both clause forms behind the condition!rename || can_bind_exported_name(alias), whererename = entry.is_replace() && alias != original_name. Ins_export_from, an unrenamed re-export of a keyword is legal syntax —export { default } from "./d"— and therealias == original_name == b"default", sorename = false, the||short-circuits without ever consultingcan_bind_exported_name,export_ref = old_ref(whose stored name isdefault), andinject_replacement_exportstill emitsexport var default = 9;, a SyntaxError.Step-by-step.
parse_export_clauseonexport { default } from "./d"setsalias = b"default"and, with noas,original_name = alias(parse_import_export.rs:301).name.ref_ = store_name_in_ref(b"default"). The keyword check at ~448 only fires whenfromis absent, so this parses asS::ExportFrom.- In
s_export_from(visit_stmt.rs:322),entry.is_replace()is true forreplace: { default: 9 }, andalias != items[i].original_name.slice()isb"default" != b"default"→ false, sorename = false. !rename || can_bind_exported_name(alias)→true || …→ true. The guard is bypassed.export_ref = old_ref(namedefault), andinject_replacement_export'sReplacearm buildsS::Local { is_export: true, decls: [B::Identifier(old_ref) = 9] }→ printsexport var default = 9;.
Same for
export { class } from "./d",export { if } from "./d", etc.Why nothing catches it. The guard was written to protect the newly introduced
declare_symbol(Hoisted, …, alias)path, which only runs whenalias != original_name. But ins_export_fromtheold_ref's stored name is the keyword too (it came fromstore_name_in_ref(alias)), so the unrenamed branch has the same emission problem the guard was added to prevent.s_export_clausecannot hit this —export { default }withoutfromis a parse error, so the local name there is always a valid binding — which is why the gap iss_export_from-specific.Impact / severity. Pre-existing: the pre-PR code passed
old_refunconditionally on this exact path, so output forexport { default } from "./d"underreplace: { default: 9 }is byte-for-byte the same before and after this PR. Filing as a nit rather than pre_existing only because 4901f1c addedcan_bind_exported_namein this PR specifically for this defect class in response to review, and the PR now tests thatexport { rr as default } from "./d"is left alone — leaving the unrenamed spelling of the same re-export inconsistent with the tested renamed spelling. Extremely niche trigger (undocumented API × reserved-word key × specific spelling), so not worth blocking merge over.Fix. In
s_export_from, decouple the guard fromrename: gate on!entry.is_replace() || can_bind_exported_name(alias), and keeprenameonly for choosing betweendeclare_symbolandold_ref.s_export_clauseneeds no change (the unrenamed case there cannot carry a keyword). Optionally addexport { default } from "./d"alongside the existing "leaves a renamed re-export of a reserved word alone when replacing" test. -
🟡
src/js_parser/visit/visit_stmt.rs:40-45—can_bind_exported_namemissesawait: it is neither inKEYWORDSnor the strict-mode set, but in a module (which any source containingexport {…}is) it is a reserved BindingIdentifier — soreplace: { await: 9 }onexport { q as await }now emitsexport var await = 9;, a SyntaxError. The near-identical siblingcan_be_class_binding_name(src/js_parser/lower/lower_decorators.rs:134-140) already carries the explicit&& name != b"await"clause; add the same here and drop"await"into theit.each(["default", "class", "let", "eval"])list.Extended reasoning...
What happens. Commit 4901f1c added
can_bind_exported_nameto stopReplacefrom emittingexport var <keyword> = …when the alias is a reserved word. It checkskeyword(),is_strict_mode_reserved_word(),eval, andarguments.awaitpasses all four: it has no entry injs_ast::lexer_tables::KEYWORDS(there is noTAwaittoken variant —awaitis lexed contextually), it is not one of the nine strict-mode reserved words (implements,interface,let,package,private,protected,public,static,yield), and it is neitherevalnorarguments. Socan_bind_exported_name(b"await")returnstrue.Why that is wrong here. The doc comment on the function asks "can
namespell the binding of anexport var <name> = ...in a module". Per ES2024 §13.1.1, a BindingIdentifier whose StringValue is"await"is an early SyntaxError when the goal symbol is Module. Any source that reaches this branch contains anexport { … }clause and is therefore a Module, soexport var await = 9;is rejected by every engine.Step-by-step.
new Bun.Transpiler({ loader: "ts", exports: { replace: { await: 9 } } })— key"await"passesJSLexer::is_identifier, entry stored.- Input
const q = 1; export { q as await };reaches thes_export_clausearm;alias = b"await",name = b"q". replace_exports.get_ptr(alias)matches;rename = true(is_replace() && alias != name).can_bind_exported_name(b"await"):keyword(b"await").is_none()→ true;!is_strict_mode_reserved_word(b"await")→ true;!= b"eval"→ true;!= b"arguments"→ true. Result:true.declare_symbol(Kind::Hoisted, loc, b"await")succeeds (its guard atp.rs:4877-4879only checks the strict-mode subset).inject_replacement_exportemitsS::Local { is_export: true, kind: Var, decls: [B::Identifier(await_ref) = 9] };NoOpRenamerprintsexport var await = 9;.
The same applies to the
s_export_frombranch forexport { rr as await } from "./d".Regression shape. Pre-PR,
export { q as await }underreplace: { await: 9 }did not match at all (keyed on localq), so the output stayed valid. Post-PR it matches and prints a SyntaxError. For theexport { rr as await } from "./d"form, pre-PR emittedexport var rr = 9;(wrong exported name, but syntactically valid); post-PR emitsexport var await = 9;(syntax error). So on both paths this moves from "wrong-but-parses" to "does-not-parse".Not a duplicate of the earlier comment. The existing inline comment on this file flagged the absence of any reserved-word guard (leading with
default); 4901f1c addedcan_bind_exported_namein response and theit.each(["default", "class", "let", "eval"])test to cover it. This is a residual gap in that new guard — the one contextual reserved word that falls between thekeyword()table and the strict-mode set.Fix. One line: add
&& name != b"await"tocan_bind_exported_name, matching the siblingcan_be_class_binding_nameatsrc/js_parser/lower/lower_decorators.rs:134-140(whose doc comment names"await"explicitly). Adding"await"to theit.eachlist at test/bundler/transpiler/transpiler.test.js covers it. Alternatively, share one helper between the two files — per CLAUDE.md "grep for the in-tree helper before hand-writing anything" — since they now differ only by theis_identifierprefix check.Severity: nit. The trigger is the intersection of a niche API (
Bun.Transpilerexports.replace), areplacekey of exactly"await", and theexport { x as await }spelling — vanishingly rare in real code. But the fix is one line the codebase already has elsewhere, and the guard was added in this PR precisely to prevent this class of syntax-error output, so it is worth closing the gap here rather than in a follow-up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/js_parser/p.rs`:
- Around line 6262-6266: The alias reuse in the export lowering path currently
lets alias == local_name return local_ref before checking
can_bind_exported_name, which can produce invalid output for Replace aliases.
Update the logic in the export resolution flow around the local_ref fast-path so
bindability is checked first, and only reuse local_ref when the alias is valid
to bind; keep the same behavior in the surrounding replacement handling and
can_bind_exported_name branch.
🪄 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: 9c56e36a-7bf7-4b26-9902-3616ba9aad71
📒 Files selected for processing (3)
src/js_parser/p.rssrc/js_parser/visit/visit_stmt.rstest/bundler/transpiler/transpiler.test.js
can_bind_exported_name was a hand-rolled copy of can_be_class_binding_name
in lower_decorators.rs, minus its `await` clause. `await` is lexed
contextually, so it is in neither the keyword table nor the strict-mode set,
and `export { q as await }` under replace: { await: 9 } printed
`export var await = 9`, a syntax error in a module. Share the one predicate
as can_be_binding_identifier and delete the copy.
Check it before the `alias == local_name` fast path, not after. An unrenamed
re-export carries the exported name too, so `export { default } from "./d"`
under replace: { default: 9 } skipped the guard and asserted in
inject_replacement_export on a ref that is not a symbol.
The injected `export var <alias>` merges with a `var` of the same name but
not with a lexical one, so `const QA = 5; var q = 1; export { q as QA }`
reported a redeclaration the source never wrote. Ask can_merge_symbol_kinds
first and leave the clause alone when declare_symbol would refuse, matching
the reserved-word policy.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1636-1657: The reserved-word coverage for re-exports is incomplete
compared with export clauses. Update the `it.each` cases in `transpiler.test.js`
around the `transform` assertions for `export { rr as %s } from "./d";` and
`export { default } from "./d";` to exercise the same reserved-word matrix as
the `export { q as %s }` test, using the shared bindability behavior in the
transpiler paths for `s_export_clause` and `s_export_from`.
🪄 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: b0558b50-ff8b-4965-bce9-5204782787f0
📒 Files selected for processing (4)
src/js_parser/lower/lower_decorators.rssrc/js_parser/p.rssrc/js_parser/parser.rstest/bundler/transpiler/transpiler.test.js
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/p.rs:9682-9687—can_bind_exported_namemissesawait: it is in neither thekeyword()table nor the strict-mode reserved-word set, yet the ES spec makes it a BindingIdentifier early error when the goal symbol is Module. Soreplace: { await: 9 }onexport { q as await }(orexport { rr as await } from "./d") passes the guard and printsexport var await = 9;, which is unparseable in a module. The siblingcan_be_class_binding_nameinsrc/js_parser/lower/lower_decorators.rs:134-140solves the identical problem and already carries&& name != b"await"— add the same clause here (and"await"to theit.eachat transpiler.test.js:1624).Extended reasoning...
What the bug is.
can_bind_exported_name(src/js_parser/p.rs:9682-9687) is the guard 4901f1c added to stopreplace: { <reserved>: ... }from emittingexport var <reserved> = .... It checkskeyword(name).is_none(),!is_strict_mode_reserved_word(name), andname != b"eval" / b"arguments".awaitpasses all four: it is not in theKEYWORDStable (src/ast/lexer_tables.rs:178-215 —awaitis a contextual keyword, not a lexer token), andSTRICT_MODE_RESERVED_WORDSis exactly the nine ES §12.7.2 words (implements/interface/let/package/private/protected/public/static/yield). Socan_bind_exported_name(b"await")returnstrue.Step-by-step trigger.
new Bun.Transpiler({ loader: "ts", exports: { replace: { await: 9 } } })— the key"await"passesJSLexer::is_identifier(pure ID_Start/ID_Continue), so the entry is stored.- Input
var q = 1; export { q as await };reaches thes_export_clausearm;alias = b"await",name = b"q". replace_exports.get_ptr(alias)matches;replacement_export_refseesis_replace() && alias != local_name, callscan_bind_exported_name(b"await")→true, thendeclare_symbol(Kind::Hoisted, ..., b"await")(which only guards the nine strict-mode words, so it succeeds).inject_replacement_exportbuildsS::Local { is_export: true, kind: Var, decls: [B::Identifier(ref) = 9] };NoOpRenamerprints the symbol's original name verbatim →export var await = 9;.- Per the ES spec (13.1.1 Static Semantics: Early Errors — "It is a Syntax Error if the goal symbol of the syntactic grammar is Module and the StringValue of BindingIdentifier is "await""), that output is unparseable in a module.
The
s_export_frompath is identical:export { rr as await } from "./d"underreplace: { await: 9 }now emitsexport var await = 9;(pre-PR it emittedexport var rr = 9;— wrong exported name, but syntactically valid).Why nothing catches it. The PR description says the rename path is gated on "not a keyword, not a strict-mode reserved word, not
eval/arguments", and that is exactly what the code does — butawaitlives in neither lookup table. It is only reserved by the Module-goal early error, the same wayyieldis reserved only in strict mode.is_strict_mode_reserved_wordpicks upyield; nothing picks upawait.In-tree precedent.
can_be_class_binding_nameat src/js_parser/lower/lower_decorators.rs:134-140 answers the identical question ("can this name be a strict-mode/module binding?") and, after the samekeyword().is_none() && !is_strict_mode_reserved_word()checks, explicitly adds&& name != b"await". Its doc comment (line 131) namesawaitalongsidedefaultandletas a word that would turn the emitted binding into a syntax error.Newly reachable. Pre-PR,
s_export_clausekeyed on the localq, soreplace: { await: 9 }onexport { q as await }never matched and the input passed through unchanged. This PR makes it match, and 4901f1c's guard was meant to close exactly this class —awaitis the one case the two tables it consults do not cover. This is the same bug class as thedefault/class/let/evalcases already fixed and tested.Fix. Add
&& name != b"await"tocan_bind_exported_name(or replace both helpers with one shared predicate), and add"await"to theit.each(["default", "class", "let", "eval"])at test/bundler/transpiler/transpiler.test.js:1624.
s_export_clause and s_export_from share the bindability check, so the re-export cases should exercise the same keywords the export-clause cases do, renamed and unrenamed.
|
All three of the findings above are real and are fixed in 6017970, and the last two What changed
pub(crate) fn can_be_binding_identifier(name: &[u8]) -> bool {
js_lexer::is_identifier(name)
&& js_lexer::keyword(name).is_none()
&& !js_lexer::is_strict_mode_reserved_word(name)
&& name != b"await"
&& !is_eval_or_arguments(name)
}The lexical collision. The The crash this was hidingWidening the re-export reserved-word matrix to the full five keywords (3e35644) turned up the real severity. Each input is individually harmless on released bun, just wrong: Run back to back in one process, released bun segfaults: Same root cause: a name ref used where a symbol ref was expected corrupts the symbol table. With the guard those clauses are left untouched and the crash is gone. VerificationThe describe block is 30 tests and covers every branch of On released bun the suite does not merely fail, it takes the process down: 12 fail, 11 pass, then the segfault ends the run. On this branch |
exports.replace was typed Record<string, string>, but JSTranspiler takes any
of string, number, boolean, null or undefined, plus a [name, value] pair that
exports the value under a different name. The transpiler tests already rely on
both, so a TypeScript caller writing the documented Next.js config
({ getStaticProps: ["__N_SSG", true] }) was told number is not assignable to
string.
Nothing type-checked a non-string replace value, so add a fixture that pins
the accepted space and rejects objects.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-types/bun.d.ts (1)
2445-2454: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify that
replacekeys must be ECMAScript identifierspackages/bun-types/bun.d.ts:2445
exports.replacerejects non-identifier keys at runtime, including the first element of[name, value]. Call out that this is narrower thaneliminate, so string-named or reserved-word exports won’t work here.🤖 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 `@packages/bun-types/bun.d.ts` around lines 2445 - 2454, Clarify the `replace` option in the export replacement docs: its keys in `ExportReplacement` are validated as ECMAScript identifiers at runtime, including the first element of any `[name, value]` pair, so update the `replace?: Record<string, ExportReplacement | [string, ExportReplacement]>` comment near `replace` to explicitly state this constraint and note that it is narrower than `eliminate`, meaning string-named or reserved-word exports are not supported here.
🤖 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.
Outside diff comments:
In `@packages/bun-types/bun.d.ts`:
- Around line 2445-2454: Clarify the `replace` option in the export replacement
docs: its keys in `ExportReplacement` are validated as ECMAScript identifiers at
runtime, including the first element of any `[name, value]` pair, so update the
`replace?: Record<string, ExportReplacement | [string, ExportReplacement]>`
comment near `replace` to explicitly state this constraint and note that it is
narrower than `eliminate`, meaning string-named or reserved-word exports are not
supported here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4ba89a7-7560-4884-9ec3-3a364c9a74bb
📒 Files selected for processing (2)
packages/bun-types/bun.d.tstest/integration/bun-types/fixture/transpiler.ts
A replace key, and the name in a [name, value] pair, are validated as
ECMAScript identifiers, so a string-named export throws. eliminate has no
such restriction. A replacement is also emitted as `export var <name>`, which
a reserved word cannot spell, so { default: 1 } replaces `export default x`
but leaves `export { x as default }` alone.
|
Good catch, documented in a2d7d80. Verified both halves against the released binary first (the key validation is untouched by this PR, so it is a valid oracle): One nuance worth being precise about, since "reserved-word exports won't work here" is only half true. A reserved word is accepted as a key, because and Every sentence there is backed by a test already in this PR:
|
replacement_export_ref reused local_ref whenever the export was not renamed.
In s_export_from that ref is the parse-time name ref from store_name_in_ref,
never a symbol, so `export { RR } from "./dep"` under replace: { RR: 9 } sent
it to inject_replacement_export and tripped r#ref.is_symbol(), the same
assertion the reserved-word reordering fixed for `export { default } from`.
A re-export has no local binding, so Replace has to declare the alias itself.
Gate the fast path on local_ref.is_symbol() and let the rest fall through to
can_declare_hoisted, which also means a colliding lexical binding leaves the
clause alone exactly as it does when the export is renamed.
can_declare_hoisted rejected only Forbidden merges, but TypeScript merges a
`var` into an import rather than refusing it, on the grounds that the import
may be type-only. Both still bind the name, so scan_imports rejects the file
once the import is used as a value: `import { QA } from "./d"; console.log(QA);
var q = 1; export { q as QA }` under replace: { QA: 9 } reported "QA" has
already been declared, where main passed the input through untouched.
Reject Kind::Import and leave the clause alone, as with const/let/class. Keep
the Forbidden check for everything else, since an Unbound name is not a
declaration and the injected `var` should still bind it.
can_declare_hoisted sat behind the `alias == local_name` fast path, so it only
ran when the clause renamed the export. An unrenamed one reused the local ref
whatever its kind, and `const foo = 1; export { foo }` under replace: { foo: 9 }
printed `export var foo = 9` beside the const. `import { foo } from "./d";
export { foo }` did the same, which is the ordinary re-export-an-import shape.
Hoist the check above the fast path so it gates both forms. The collision rules
are now the same everywhere: `var` and function declarations merge, const/let/
class/import leave the clause alone, an unbound name binds.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-05, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Bun.Transpiler'sexports.eliminate/exports.replaceare keyed on a module's exported names: that is the public surface, and it is what the siblingBun.Transpiler.scan()reports. Export clauses matched the local name instead, and re-export clauses matched the exported name, so the same build had two opposite conventions and no single name worked for both forms. The miss is silent.Repro
Cause
s_export_clauselooked the entry up underload_name_from_ref(item.name.ref_), which is the local name being exported, notitem.alias.s_export_fromalready useditem.alias, hence the split.Keying on the alias then exposed a second half of the same defect: a
replaceentry injectsexport var <name> = value, and that name came from the local symbol.export { rr as RR } from "./dep"replaced by{ RR: 9 }emittedexport var rr = 9on main, i.e. it matchedRRbut exportedrr.Fix
Match
item.aliasin both clause forms, and name the injectedReplacebinding after the alias when it differs from the local name.replacement_export_refresolves that ref for both forms.The injected binding is a
var, so it is declaredKind::Hoistedrather thanKind::Other. Without this,var QA = 5; var q = 1; export { q as QA };underreplace: { QA: 9 }would report a spurious"QA" has already been declaredfor valid input; as a hoistedvarit merges, exactly like thevarthat gets printed. Aconst/let/classof the same name cannot merge, soreplacement_export_refaskscan_merge_symbol_kindswhatdeclare_symbolwould decide and leaves the clause alone onForbidden. An import is rejected too: TypeScript merges avarinto it rather than refusing it, since the import may be type-only, but both still bind the name andscan_importsthen rejects the file. An unbound name is not a declaration, so the injectedvarstill binds it. The same check gates the unrenamed clause, where it lands on the local itself:const foo = 1; export { foo };andimport { foo } from "./d"; export { foo };used to printexport var foo = 9beside the existing binding.Emitting
export var <alias>also needs the alias to be spellable as a strict-mode binding, andexports.replacekeys are only checked withis_identifier, which accepts every keyword. The parser already had that predicate ascan_be_class_binding_nameinlower_decorators.rs; it now lives inparser.rsascan_be_binding_identifierand both callers share it. A reserved-word alias keeps its export clause, which is what main does forexport { q as default }.That guard runs before the
alias == local_namefast path, because an unrenamed re-export carries the exported name too. The fast path itself only reuseslocal_refwhen itis_symbol(): ins_export_fromthat ref is always the parse-time name ref fromstore_name_in_ref, so a re-export has no local binding to reuse andReplacehas to declare the alias. Without both,export { default } from "./d"andexport { RR } from "./dep"underreplacereachedinject_replacement_exportwith a non-symbol ref:assertion failed: r#ref.is_symbol()on a debug build, and on release a symbol table indexed by a name ref, which is what segfaults released bun once several such clauses run in one process.Delete(eliminate) declares nothing, so reserved-word and string-named exports (export { q as default },export { q as "a-b" }) are still eliminated by their exported name.exports.replacewas typedRecord<string, string>whileJSTranspileracceptsstring | number | boolean | null | undefined, plus a[name, value]pair. The transpiler tests rely on both, so the documented Next.js config ({ getStaticProps: ["__N_SSG", true] }) did not type-check. Widened, with abun-typesfixture that pins the accepted space. The JSDoc also records thatreplacekeys (and thenameof a pair) are identifier-validated whileeliminateis not, soexport { q as "a-b" }can only be eliminated.Declaration forms (
export const/function/class),export default, andexport * as nsalready keyed on the exported name and are unchanged.Verification
test/bundler/transpiler/transpiler.test.js,describe("exports.eliminate and exports.replace match the exported name"): 38 tests covering renamed and unrenamed clauses, renamed and unrenamed re-exports, scalar and array-formreplace,as default, string-named exports, the local-name non-match, the full collision matrix (varandfunctionmerge,const/let/class/import leave the clause alone, unbound binds), and every branch ofcan_be_binding_identifier(default,class,let,eval,await).18 of them fail on released bun, and the unrenamed re-export case asserts on any debug build without the
is_symbol()guard, which I confirmed by reverting just that line. On this branch the describe block is 38 pass / 0 fail, andtest/bundler/transpiler/+bundler_decorator_metadata.test.ts+bundler_esm.test.ts+bundler_edgecase.test.tsare green at 4317 passing (the decorator suites cover thelower_decorators.rsdedup).test/integration/bun-types/bun-types.test.tsis 12 pass, 0 fail, and fails on the oldRecord<string, string>annotation, so the new fixture is not vacuous.Only
Bun.Transpilerpopulatesreplace_exports; the bundler always passes an empty map, so nothing else changes behavior.Known gaps, not touched here
All pre-existing, none reachable from the forms this PR rewires:
s_export_stardeclares the alias itself, soexport * as default from "./d"underreplace: { default: 9 }still emits an invalidexport var default = 9, andvar ns = 5; export * as ns from "./d"underreplace: { ns: 9 }still reports a redeclaration. Routing it throughreplacement_export_refwould fix both, but it is an untouched path with no reported bug.inject_replacement_exportandreplace_decl_and_possibly_removedeclare theirInjectname withKind::Other, soreplace: { foo: ["default", true] }emits an invalidexport var default, and two entries injecting the same name (thegetStaticProps/getStaticPaths→__N_SSGconfig in this repo's own tests) error. Making those merge raises a separate question about whether a duplicateInjectshould dedupe.