Preserve string-literal names in re-export clauses#29245
Preserve string-literal names in re-export clauses#29245
Conversation
|
Updated 9:20 PM PT - Apr 12th, 2026
❌ @autofix-ci[bot], your commit 6f510fb has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29245That installs a local version of the PR into your bun-29245 --bun |
|
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:
WalkthroughPrinter extended to emit Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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_printer.zig (1)
1933-1974:⚠️ Potential issue | 🔴 CriticalUse
item.original_namein the newexport_fromprinter path.For re-exports, the left side is the source module’s export name, not the renamed intermediate symbol.
ClauseItem.original_nameexists for exactly this case. As written, minification/collision renaming can still turnexport { foo as out } from "./mod"intoexport { a as out } from "./mod", which breaks the re-export.🐛 Suggested fix
fn printClauseItemAs(p: *Printer, item: js_ast.ClauseItem, comptime as: `@Type`(.enum_literal)) void { const name = p.renamer.nameForSymbol(item.name.ref.?); + const export_from_name = if (comptime as == .export_from and item.original_name.len > 0) + item.original_name + else + name; @@ } else if (comptime as == .export_from) { // In `export { x } from 'mod'`, the "name" on the left of `as` refers // to an export of the other module. ECMAScript allows this to be a // string literal (`export { "a b c" } from 'mod'`), so we must use // `printClauseAlias` rather than `printIdentifier` here. The symbol // does not participate in local renaming. - p.printClauseAlias(name); + p.printClauseAlias(export_from_name); - if (!strings.eql(name, item.alias)) { + if (!strings.eql(export_from_name, item.alias)) { p.print(" as "); p.addSourceMapping(item.alias_loc); p.printClauseAlias(item.alias); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js_printer.zig` around lines 1933 - 1974, The export-from branch in fn printClauseItemAs currently prints the locally renamed symbol (name) instead of the source module's export name; change the export_from path to use item.original_name (e.g., call p.printClauseAlias(item.original_name)) so re-exports emit the original export identifier/string literal rather than the renamed local symbol, and keep the existing alias handling (the subsequent if with item.alias and item.alias_loc) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29242.test.ts`:
- Around line 107-119: The exit-code assertion is placed too early, which hides
useful stdout failure details; move the expect(exitCode).toBe(0) so it runs
after the stdout content checks. Specifically, in the block using
proc.stdout/stderr/exited and variables stdout, stderr, exitCode and
mustContain, keep expect(stderr).not.toContain("SyntaxError") first, then assert
each mustContain fragment against stdout and the two not.toMatch checks for
unquoted patterns, and only after all stdout assertions call
expect(exitCode).toBe(0); preserve existing comments (e.g., the "No unquoted `a
b c`..." line) and ordering of stderr checks.
---
Outside diff comments:
In `@src/js_printer.zig`:
- Around line 1933-1974: The export-from branch in fn printClauseItemAs
currently prints the locally renamed symbol (name) instead of the source
module's export name; change the export_from path to use item.original_name
(e.g., call p.printClauseAlias(item.original_name)) so re-exports emit the
original export identifier/string literal rather than the renamed local symbol,
and keep the existing alias handling (the subsequent if with item.alias and
item.alias_loc) unchanged.
🪄 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: acc4a9ee-7054-42eb-9fbb-a63dcc092904
📒 Files selected for processing (2)
src/js_printer.zigtest/regression/issue/29242.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29242.test.ts (1)
95-98:⚠️ Potential issue | 🟡 MinorMove the exit-code assertion after stdout content checks (Line 95).
expect(exitCode).toBe(0)currently runs before thestdoutfragment assertions, which reduces failure usefulness when content mismatches occur.Suggested fix
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stderr).not.toContain("SyntaxError"); - expect(exitCode).toBe(0); for (const frag of mustContain) { expect(stdout).toContain(frag); } // No unquoted `a b c` or `x y z` anywhere. expect(stdout).not.toMatch(/(^|[^"])a b c(?!")/); expect(stdout).not.toMatch(/(^|[^"])x y z(?!")/); + expect(exitCode).toBe(0);As per coding guidelines, "Expect stdout assertions before exit code assertions - expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29242.test.ts` around lines 95 - 98, The test currently asserts expect(exitCode).toBe(0) before verifying stdout fragments; move the exit-code check to after the stdout checks so failures show the mismatched output first. Specifically, in the test that uses exitCode, stdout, and mustContain, iterate over mustContain and assert expect(stdout).toContain(frag) for each frag first, then call expect(exitCode).toBe(0) afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29242.test.ts`:
- Around line 95-98: The test currently asserts expect(exitCode).toBe(0) before
verifying stdout fragments; move the exit-code check to after the stdout checks
so failures show the mismatched output first. Specifically, in the test that
uses exitCode, stdout, and mustContain, iterate over mustContain and assert
expect(stdout).toContain(frag) for each frag first, then call
expect(exitCode).toBe(0) afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17ec920a-9f93-43cd-85b7-1171679882c2
📒 Files selected for processing (1)
test/regression/issue/29242.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix with good test coverage.
Extended reasoning...
Overview
This PR modifies src/js_printer.zig to add a new .export_from variant in printClauseItemAs, and introduces a corresponding printExportFromClauseItem helper. The call site at the export { ... } from 'mod' branch is switched from printExportClauseItem to the new printExportFromClauseItem. A regression test file is added.
Security Risks
None. This is a printer output correctness fix — it only affects what JavaScript text gets emitted, and strictly produces more spec-compliant output.
Level of Scrutiny
Low-to-medium. The change is well-contained: one new enum variant in a comptime switch, one new thin wrapper function, and one changed call site. The logic mirrors the existing .import branch (which also uses printClauseAlias for the foreign-module name side). The distinction between local exports (printIdentifier correct per spec) and re-exports (printClauseAlias required per spec) is sound.
Other Factors
The two inline bug reports are style nits (missing test.concurrent and assertion ordering in the test.each block) — neither affects test correctness or the fix itself. The three individual end-to-end tests follow the prescribed ordering correctly. Coverage of four printer variants plus three runtime variants is thorough for the scope of the fix.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/regression/issue/29242.test.ts:111-118— In thetest.eachblock (lines ~111-118),expect(exitCode).toBe(0)is checked before the stdout content assertions, violating the CLAUDE.md rule that stdout checks should come before exitCode. The three individual tests in the same file correctly follow the prescribed order (stderr → stdout → exitCode), making this an inconsistency that slightly degrades test failure diagnostics.Extended reasoning...
What the bug is and how it manifests
The
test.eachblock at the bottom oftest/regression/issue/29242.test.tscollectsstdout,stderr, andexitCodefrom the transpiler process, then checks them in this order:expect(stderr).not.toContain("SyntaxError"), thenexpect(exitCode).toBe(0), and only afterwards iteratesmustContainfragments withexpect(stdout).toContain(frag)plus the two regex checks. This inverts the recommended ordering forexitCodevs. stdout assertions.The specific code path that triggers it
In the async callback of
test.each(approximately lines 111-119 in the new file), afterproc.exitedresolves, the assertion order is: (1)expect(stderr).not.toContain("SyntaxError"), (2)expect(exitCode).toBe(0)— exitCode checked here, (3)for (const frag of mustContain) { expect(stdout).toContain(frag); }— stdout checked here, and (4) twoexpect(stdout).not.toMatch(...)regex checks.Why existing code does not prevent it
The three individual tests above ("re-export with string literal local name", "re-export aliasing from string literal to identifier", "re-export aliasing string literal to string literal") all correctly order their assertions as stderr → stdout → exitCode. There is no lint rule or automated check enforcing the CLAUDE.md ordering convention in the test.each block.
What the impact would be
If the printer emits incorrect output (e.g., unquoted
a b cinstead of"a b c"), the transpiler process will likely exit with code 1. With the current ordering, the failing test reports "expected exitCode to be 0 but got 1", which gives no information about why the process failed. If stdout were checked first, the test would instead report something like "expected stdout to contain "a b c"", immediately pointing to the broken printer output.How to fix it
Move
expect(exitCode).toBe(0)to after themustContainloop and the twonot.toMatchregex assertions, matching the order used in the three individual tests.Step-by-step proof
- Suppose the fix in js_printer.zig is accidentally reverted.
- The transpiler processes
export { "a b c" } from "./mod";and emitsexport { a b c } from "./mod";(unquoted). - Because the output is syntactically invalid,
bun buildexits with code 1. - With current ordering:
expect(exitCode).toBe(0)fires first, failure message: "Expected: 0, Received: 1". Developer must guess why. - With corrected ordering:
expect(stdout).toContain("\"a b c\"")fires first, failure message directly shows the malformed stdout. Root cause is immediately obvious.
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_printer.zig (1)
4213-4263:⚠️ Potential issue | 🟠 MajorKeep
ModuleInfoin sync with the printed re-export name.When
module_infois enabled, Line 4262 still records the indirect source name viap.renamer.nameForSymbol(item.name.ref.?). Forexport { "x y z" as a } from ..., the emitted code now correctly usesitem.original_name, but the metadata still stores the synthesized/local symbol name instead of"x y z". That leaves Bun's internal export graph wrong for the exact case this patch fixes.🛠️ Suggested fix
if (may_have_module_info) { if (p.moduleInfo()) |mi| { const irp_id = bun.handleOom(mi.str(irp)); bun.handleOom(mi.requestModule(irp_id, .none)); for (s.items) |item| { - const name = p.renamer.nameForSymbol(item.name.ref.?); - bun.handleOom(mi.addExportInfoIndirect(bun.handleOom(mi.str(item.alias)), bun.handleOom(mi.str(name)), irp_id)); + const name = p.renamer.nameForSymbol(item.name.ref.?); + const from_name = if (item.original_name.len > 0) item.original_name else name; + bun.handleOom(mi.addExportInfoIndirect( + bun.handleOom(mi.str(item.alias)), + bun.handleOom(mi.str(from_name)), + irp_id, + )); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js_printer.zig` around lines 4213 - 4263, The ModuleInfo export metadata is using the local/synthesized symbol via p.renamer.nameForSymbol(item.name.ref.?) instead of the printed re-export token (item.original_name) for re-exports; update the loop that calls bun.handleOom(mi.addExportInfoIndirect(...)) inside the .s_export_from arm to use item.original_name when present (fall back to p.renamer.nameForSymbol(...) only if item.original_name is null/empty), and ensure you still wrap values with bun.handleOom(mi.str(...)) and pass the same irp_id as before so the stored export name matches what p.printExportFromClauseItem emits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29242.test.ts`:
- Around line 99-100: The test uses regexes that only treat double quotes as
string delimiters in the expect(stdout).not.toMatch assertions for the tokens "a
b c" and "x y z" (and the similar assertion at line 126); update those regexes
in this test file so the unquoted-token guard is quote-style agnostic by
matching either single or double quotes (use a character class like ["'] or an
alternation) in the lookaround, keeping the same anchors and negative lookahead
logic so the assertions still reject unquoted tokens but accept tokens
surrounded by either single or double quotes.
---
Outside diff comments:
In `@src/js_printer.zig`:
- Around line 4213-4263: The ModuleInfo export metadata is using the
local/synthesized symbol via p.renamer.nameForSymbol(item.name.ref.?) instead of
the printed re-export token (item.original_name) for re-exports; update the loop
that calls bun.handleOom(mi.addExportInfoIndirect(...)) inside the
.s_export_from arm to use item.original_name when present (fall back to
p.renamer.nameForSymbol(...) only if item.original_name is null/empty), and
ensure you still wrap values with bun.handleOom(mi.str(...)) and pass the same
irp_id as before so the stored export name matches what
p.printExportFromClauseItem emits.
🪄 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: 9e585682-4784-45af-99ba-ef254876183f
📒 Files selected for processing (2)
src/js_printer.zigtest/regression/issue/29242.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29242.test.ts (1)
99-100: 🧹 Nitpick | 🔵 TrivialMake the "unquoted token" guard quote-style agnostic.
The regex patterns only treat double quotes as valid delimiters. If the printer formatting ever switches to single-quoted string literals, these assertions will fail even when the output is semantically correct.
♻️ Suggested tweak
- expect(stdout).not.toMatch(/(^|[^"])a b c(?!")/); - expect(stdout).not.toMatch(/(^|[^"])x y z(?!")/); + expect(stdout).not.toMatch(/(^|[^"'])a b c(?!["'])/); + expect(stdout).not.toMatch(/(^|[^"'])x y z(?!["'])/);Also applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29242.test.ts` around lines 99 - 100, The two regex assertions that detect unquoted tokens (the expect(stdout).not.toMatch checks for "a b c" and "x y z") only treat double quotes as delimiters; update both patterns so the surrounding-quote guard is agnostic by matching either single or double quotes (use a character class matching ' or " in place of the literal " in the left context and the negative lookahead), i.e., change the (^|[^"]) and (?!") parts to use a ['"] class so both expect(stdout).not.toMatch checks are resilient to single- or double-quoted output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29242.test.ts`:
- Around line 99-100: The two regex assertions that detect unquoted tokens (the
expect(stdout).not.toMatch checks for "a b c" and "x y z") only treat double
quotes as delimiters; update both patterns so the surrounding-quote guard is
agnostic by matching either single or double quotes (use a character class
matching ' or " in place of the literal " in the left context and the negative
lookahead), i.e., change the (^|[^"]) and (?!") parts to use a ['"] class so
both expect(stdout).not.toMatch checks are resilient to single- or double-quoted
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0ad0b92-12b8-4b16-b2f9-35c6a001aa7f
📒 Files selected for processing (1)
test/regression/issue/29242.test.ts
There was a problem hiding this comment.
LGTM — previous feedback addressed, fix is correct and well-tested.
Extended reasoning...
Overview
Touches two files: (printer logic) and (new regression test). The change adds an variant in that routes through for the left-side name in clauses, preserving string-literal quotes. The handler is updated to call the new helper.
Security risks
None. The change only affects how the JS printer serializes export-from clause items; it has no effect on parsing, execution, auth, crypto, or permissions.
Level of scrutiny
Low. This is a narrowly-scoped bug fix in a non-security-critical path (the transpiler printer). The logic is straightforward: string-literal export names need quoting when re-emitted, which already handles correctly. The regular path is intentionally unchanged.
Other factors
All reviewer feedback was addressed in commit d6d67f4: (1) tests switched to per test/CLAUDE.md guidelines, (2) assertion moved after stdout checks, (3) used instead of the renamer name to prevent minifier interference. No outstanding unresolved comments.
There was a problem hiding this comment.
LGTM — the printer fix is correct and all prior review feedback has been addressed; the test title nit is captured in the inline comment.
Extended reasoning...
Overview
The PR modifies src/js_printer.zig to add a dedicated printExportFromClauseItem helper and a new .export_from branch in printClauseItemAs. The fix ensures that in export { ... } from 'mod' clauses, the left-side name is printed via printClauseAlias (which quotes string literals) using item.original_name as the source of truth, rather than the renamer's symbol name which could be mangled or lose quotes. The call site in the s_export_from path is updated accordingly. A new regression test file covers four runtime and five printer-level variants.
Security risks
None. This is a pure code-generation correctness fix in the JS printer with no security surface.
Level of scrutiny
Low-to-medium. The change is narrowly scoped to a single code path (s_export_from in the no-bundle transpile path), does not affect the bundler/linker path, and the logic is straightforward. The fix correctly falls back to name when original_name is empty, preserving existing behavior for normal identifier exports.
Other factors
Both previous review concerns (sequential tests and exitCode assertion ordering) were addressed in commit d6d67f4. The only remaining finding is a cosmetic test title nit (first test.concurrent says 'x y z' but the fixture uses 'a b c'), which is captured as an inline comment and does not affect test correctness or the validity of the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29242.test.ts`:
- Around line 2-10: Remove the long explanatory prose at the top of the
regression test and replace it with a concise single-line reference to the issue
(e.g., the issue URL or number); specifically edit the header block in
test/regression/issue/29242.test.ts that contains the multi-line
reproduction/output narrative so it only includes the issue link or short
identifier and no extra commentary, leaving the actual test code (the export
string-literal case) unchanged.
🪄 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: acb95d43-7940-4fe9-81db-9ffaf32e840a
📒 Files selected for processing (1)
test/regression/issue/29242.test.ts
| // | ||
| // The parser handles string-literal names in `export { ... } from 'mod'` | ||
| // clauses, but when transpiling without bundling the printer dropped the | ||
| // quotes around the local name, producing invalid syntax that JSC then | ||
| // rejected: | ||
| // | ||
| // export { "a b c" } from './b.mjs'; // input | ||
| // export { a b c } from './b.mjs'; // old output — SyntaxError | ||
| // export { "a b c" } from './b.mjs'; // fixed output |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trim the top-of-file bug-context prose to keep regression tests concise.
Consider keeping just the issue URL and removing the long reproduction/output narrative block; this context already lives in the PR/issue and makes the test noisier to scan.
Based on learnings: In oven-sh/bun regression test files, do not suggest adding inline comments that explain bug context or issue number being tested; PR descriptions cover that context and test code is kept clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/29242.test.ts` around lines 2 - 10, Remove the long
explanatory prose at the top of the regression test and replace it with a
concise single-line reference to the issue (e.g., the issue URL or number);
specifically edit the header block in test/regression/issue/29242.test.ts that
contains the multi-line reproduction/output narrative so it only includes the
issue link or short identifier and no extra commentary, leaving the actual test
code (the export string-literal case) unchanged.
There was a problem hiding this comment.
LGTM — all previously raised nits have been addressed.
Extended reasoning...
Overview
The PR modifies src/js_printer.zig to add a dedicated printExportFromClauseItem path for export { ... } from 'mod' clauses, plus a new regression test test/regression/issue/29242.test.ts. The change is tightly scoped to the printer's export-from rendering.
Security risks
None. This is a pure printer/code-generation fix with no auth, crypto, or permission-sensitive code paths.
Level of scrutiny
Low. The change is small (~30 lines of Zig + a new test file), well-explained, and follows existing patterns in the printer. The distinction between .export (local binding, must be identifier) and .export_from (foreign-module name, may be a string literal) is correctly modeled.
Other factors
All three review threads (concurrent tests, exitCode assertion order, quote-agnostic regex, mismatched test title) were fully resolved before the current HEAD. The bug-hunting system found no new issues. Test coverage spans bare re-export, identifier alias, string-to-string alias, mixed clause, and minify-identifiers variants.
1 participant
Fixes #29242
Repro
bun main.mjsfails with:Cause
Bun's parser already accepts string-literal module export names in
export { ... } from 'mod'clauses, but the printer emitted the local name viaprintIdentifier. Forexport { "a b c" } from './b.mjs', this stripped the quotes and producedexport { a b c } from './b.mjs'which JSC then rejected. Bundled output was fine — only single-file transpile hit this path.Fix
Added an
.export_fromvariant ofprintClauseItemAsthat prints the local name viaprintClauseAlias(which quotes the string when it isn't a valid identifier). The regularexport { x }path is unchanged — per spec the local name there must be anIdentifierReference, soprintIdentifieris still correct.Verification
test/regression/issue/29242.test.tscovers four variants ({ 'x y z' },{ 'x y z' as a },{ 'x y z' as 'a b c' }, mixed) plus direct printer checks. All seven fail on main and pass with the fix.