Conversation
… export A JSON, JSONC, JSON5, TOML, YAML or XML file whose only top-level key is "default" gets no named export. `parse_data_loader` still built `var` with zero declarations, and `print_decls` aborts on that. Emit only `export default` when no key gets a named export.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 12 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 11:47 AM PT - Sep 18th, 2026
✅ @robobun, your commit 6a45fae716ea03b2ab9114d3d5225739b7fc267f passed in 🧪 To try this PR locally: bunx bun-pr 43330That installs a local version of the PR into your bun-43330 --bun |
|
Status: ready for review. Reproduced on 1.4.3-canary.1 (b52d513), linux-x64: $ printf '{"default": 1}' > d.json && bun build d.json --no-bundle
panic: internal error: entered unreachable code
Crashed while printing d.jsonWith this change the command prints |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 findings, I also checked the symbols.truncate(count) addition in src/bundler/transpiler.rs: every Ref the loop creates is Ref::init(count, ...) for count in 0..count, the trimmed entries were never named by a Ref, and the only consumer is the printer via ast.symbols, so the truncate changes no output. The count == 0 fall-through leaves symbols empty, the same state the scalar/array branch reaches, and prints the untouched object literal, so a file with only default keys (including duplicates) emits export default { default: ... } — consistent with what the bundler's lazy-export linker path produces at runtime.
Extended reasoning...
The core change is a small, mechanically correct guard: the three-statement var/export {}/export default construction is now skipped when no property produced a named export, falling through to the existing export default <expr> branch. I traced symbols from its Vec::new() at line 1944 through resize_with(n), the dense symbols[count] = ... writes, the new truncate(count), and its consumption at ast.symbols (line 2123) and symbol::Map::init_with_one_list in the print path; no code indexes past count, so the truncate is safe and the fall-through's empty symbol list matches the non-object path. The inline findings are pre-existing sibling issues in the same function (non-string YAML/JSON5 keys still hitting .expect, reserved-word identifiers, scan() not populating named_exports) plus a test-concurrency nit; they are worth a human's judgement on scope, which is why this is a defer rather than an approve.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/bundler/transpiler.rs— Users runningbun build --no-bundleorBun.Transpileron a YAML or JSON5 file with a non-string key still get a process abort after this merge, the same crash class this PR claims to fix. The named-export loop at src/bundler/transpiler.rs:2000 calls.expect("infallible: variant checked")on the key, but the YAML parser builds mapping keys throughparse_node, sotrue: t,null: xor1: aarrive as boolean/null/number keys and panic. Fix: skip or stringify non-e_stringkeys in this loop (falling into the newcount == 0path) instead of relying on the unmerged #41827; also correct the false SAFETY comment at 1993-1994.Extended reasoning...
The dismissal rests on the PR text saying #41827 handles it; that PR is not in this checkout, and the loop still panics here. The PR's stated goal is that a data file whose keys yield no named export prints
export defaultinstead of aborting; a YAML filetrue: tis exactly such a file and still aborts. src/parsers/yaml.rs around 2640-2650: a flow-mapping key isself.parse_node(...), which returns E::Boolean / E::Null / E::Number for plain scalars, not E::String. Block mappings use the same key parser. Step 1: user writesports.yamlcontaining8080: web(numeric keys are common in YAML port/id maps). Step 2:bun build ports.yaml --no-bundleornew Bun.Transpiler().transformSync(src, "yaml"). Step 3: parse_data_loader reaches the loop at src/bundler/transpiler.rs:1991 with keep_json_and_toml_as_one_statement=false. Step 4: line 1997-2000key.data.e_string_mut().expect(...)panics on the E::Number key; bun prints a crash report and exits. Step 5: the newif count > 0guard at 2065 never runs. Runtimeimportof the same file is unaffected (jsc_hooks.rs:2547 passes keep=true…Verification: pre-existing. Trigger:
bun build --no-bundleorBun.Transpileron a YAML file with a plain-scalar non-string key (true: t,null: x,1: a) or a JSON5 file with atrue/nullkey. Mechanism verified: /home/claude/bun/src/bundler/transpiler.rs:1995-2001 doesprop.key.as_mut().unwrap()then.data.e_string_mut().expect("infallible: variant checked")with a comment claiming… -
🟣
src/bundler/transpiler.rs— Users runningbun build --no-bundleorBun.Transpileron a JSON/TOML/YAML file with a key such asclass,if,new,importorthisget output that is a SyntaxError when imported. The symbol name at src/bundler/transpiler.rs:2023 comes fromensure_valid_identifier, which only remaps the 9 strict-mode words (let,static,yield...) and returns every other reserved word unchanged, so the printer emitsvar class = 1; export { class };. Fix: reject or prefix every JS reserved word (not only strict-mode ones) when building the declaration symbol, e.g. reuse the printer/renamer reserved-word check, and add a test for{"class": 1}.Extended reasoning...
The finder argued this away as living in ensure_valid_identifier and being pre-existing without opening it. src/bun_core/string/MutableString.rs:141-157: when every char is an identifier char the function returns
strict_mode_reserved_word_remap(str).unwrap_or(str). src/bun_core/string/mod.rs:1719-1729: that table holds exactly implements, interface, let, package, private, protected, public, static, yield.class,var,if,for,new,this,true,null,import,export,return,function,delete,typeof,await,enumare all absent. Step 1: config.json is{"class": "btn"}(a very common key in UI configs, alsoif/thenin rule files,importin tsconfig-like files). Step 2:bun build config.json --no-bundle. Step 3: the loop at 2022 stores original_nameclass; 2039-2053 build a var decl and export clause with that ref; the printer uses NoOpRenamer (js_printer/lib.rs:7819) so the name is printed verbatim. Step 4: output isvar class = "btn"; export { class }; export default { class }. Step 5: importing that file throws SyntaxError. The…Verification: pre-existing. Triggering condition: a JSON/JSONC/JSON5/TOML/YAML/XML object with a top-level key that is a JS reserved word other than the 9 strict-mode words (e.g.
class,if,new,import,this), transpiled viabun build --no-bundleorBun.Transpiler(the data-loader path, not the linker'sgenerateCodeForLazyExport). Mechanism verified:src/bundler/transpiler.rs:2022-2035sets… -
🟣
src/bundler/transpiler.rs— Callers ofBun.Transpiler.scan()on a JSON, TOML, YAML, JSON5 or XML source getexports: []even thoughtransform()of the same source emitsexport { a, b }.parse_data_loaderbuilds the export clause at src/bundler/transpiler.rs:2074-2080 but never fillsast.named_exports, and JSTranspiler.rs:1331 reads only that map. Fix: populateast.named_exports(andast.symbolsalready set at 2123) with one entry per counted key, so scan() reports the same names transform() prints; thecount == 0path added here should report none.Extended reasoning...
The finder called this pre-existing and out of scope; that is true, but this PR is the one that now defines the count == 0 contract for data loaders and re-touches the same function, and the two entry points disagree on the same input. Step 1:
const t = new Bun.Transpiler(); t.scan('{"a":1}', ...)with loader json (scan accepts a loader argument through get_parse_result at JSTranspiler.rs:1310). Step 2: parse_data_loader returns an Ast built byAst::from_partsat 2122; onlysymbolsis assigned at 2123;named_exportsstays the default empty map. Step 3: JSTranspiler.rs:1331named_exports_to_js(global, &mut parse_result.ast.named_exports)yields[]. Step 4: the same transpiler'stransformSyncprintsvar a = 1; export { a }; export default { a }. Tools that use scan() to decide whether a data module has named exports (dev servers, HMR export tracking, tree-shaking heuristics) get a wrong answer for every data-loader file on every scan call. Remedy: after the loop, insert each counted name intoast.named_exportswith itsref_, mirroring what the JS parser does for…Verification: pre-existing — the base fails the same way by the same route; this PR touches
parse_data_loaderbut does not changenamed_exportshandling. Triggering condition: any caller ofBun.Transpiler.scan(code, "json" | "jsonc" | "json5" | "toml" | "yaml" | "xml")on an object source with non-defaultkeys. Mechanism verified in code: -src/runtime/api/JSTranspiler.rs:1279-1282—…
It spawns a subprocess and shares no state, like the data loader cases in the describe.concurrent block before it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reply to the review above. Both inline threads are addressed and resolved (f088b53, 6a45fae). The three findings outside the diff are real. Each one exists on
|
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.
LGTM — small, well-scoped fix for the empty-var abort, and the concurrency nit from the earlier pass is addressed (the new Bun.Transpiler test is now it.concurrent).
Beyond the inline note, I also checked: symbols.truncate(count) is safe because every emitted Ref is minted from the dense count index; the count == 0 fall-through reuses expr before any prop.value was rewritten to an identifier (that only happens after a key gets a declaration); and adding xml to expectBundled's supported loader list unmasks no other test — no existing bundler test passes an .xml loader map.
Extended reasoning...
Overview
The PR changes one block in parse_data_loader (src/bundler/transpiler.rs:2059-2097): it adds symbols.truncate(count) alongside the existing decls.truncate(count) and wraps the var / export {} / export default three-statement emission in if count > 0, so a data-file object whose only keys are "default" falls through to the pre-existing single export default <expr> path. Tests are added in test/bundler/bundler_loader.test.ts (six --no-bundle itBundled cases across json/jsonc/json5/toml/yaml/xml), test/bundler/transpiler/transpiler.test.js (one it.concurrent subprocess test covering transformSync and transform), and test/bundler/expectBundled.ts gains "xml" in supportedLoaderTypes.
Security risks
None. The change only affects which AST statements are produced for a data file with no named exports; it introduces no new parsing, allocation, or user-controlled indexing. The crash it removes was a reachable unreachable!() in the printer (src/js_printer/lib.rs:2223) on user input, so the PR reduces a DoS-class panic rather than adding risk.
Level of scrutiny
Moderate, and it was met by reading the full loop. The two things worth proving were (1) that symbols.truncate(count) cannot drop a symbol some Ref still names — it cannot, since Ref::init(count as u32, ...) at line 2039 is the only ref constructor and it always uses the dense index; and (2) that the fall-through export default expr is correct when count == 0 — it is, because prop.value = Some(Expr::init_identifier(ref_, ..)) at line 2055 executes only on the path that also increments count, so with count == 0 no property was rewritten. Borrow-wise, decls and export_clauses are only moved/borrowed inside the branch and simply dropped or left in the arena otherwise, which compiles fine and leaks nothing (the arena owns export_clauses). The single inline finding attached to this PR is a pre-existing identifier-collision issue in the same loop that this change neither introduces nor worsens, and it is not a reason to hold a crash fix.
Other factors
The prior review's only comment (make the subprocess test concurrent) was addressed in commit f088b53; no third-party CHANGES_REQUESTED review is on the timeline, and no CODEOWNERS entry covers the changed files. I confirmed the bunRun harness helper returns exactly {stdout, stderr, exitCode, signalCode} with trimmed output, matching the new test's toEqual, and that no other test in test/bundler passes an xml entry in a loader map, so widening expectBundled's supported list only affects the new case. I could not execute the debug build in this environment, so the fail-before/pass-after claim rests on the PR description plus the printer's unreachable!() mechanism, which I verified in source.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/bundler/transpiler.rs— pre-existing: users ofbun build --no-bundleorBun.Transpileron a data file with two keys that normalize to one identifier get the wrong value for one key.ensure_valid_identifierat src/bundler/transpiler.rs:2023 maps"a-b"and"a_b"both toa_b, whileduplicate_key_checkerat :2007 dedupes on the raw key, so two symbols share one name andvar a_b = 1, a_b = 2is printed. Fix: make the generated names unique across the loop (numbered suffixes like the bundler renamer, or skip non-identifier keys as generateCodeForLazyExport.rs:440 does) so every key keeps its own value. This differs from the reserved-word finding: remapping keywords does not dedupe.Extended reasoning...
Input
{"a-b": 1, "a_b": 2}as data.json (same for toml/yaml/json5/jsonc/xml; also"my key"/"my_key",""/"_","1"/"_1"). Loop at src/bundler/transpiler.rs:1991 runs for both keys.duplicate_key_checker.get_or_put(name)at :2007 uses the raw key, so neither isfound_existing.symbols[count].original_nameat :2022-2033 becomesa_bfor both (ensure_valid_identifier replaces-with_, MutableString.rs:173-179). Two refs 0 and 1, two decls, two clause items with aliasesa-banda_b; eachprop.valueis replaced by an identifier of its ref at :2055. The transform path usesNoOpRenamer(js_printer/lib.rs:7819), whosename_for_symbol(renamer.rs:97-107) returnsoriginal_name, so both refs print asa_b. Output:var a_b = 1, a_b = 2; export { a_b as "a-b", a_b }; export default { "a-b": a_b, a_b };(the"a-b"key is not an identifier so lib.rs:5013 disables shorthand). At runtimea_bis 2, soimport { "a-b" as x }anddata["a-b"]both give 2 instead of 1: silent data loss. The bundler path is not affected:…Verification: pre-existing. Triggering condition: a data file (json/jsonc/json5/toml/yaml/xml) with two top-level keys that
ensure_valid_identifiermaps to the same identifier, e.g.{"a-b": 1, "a_b": 2}, transpiled viabun build --no-bundleorBun.Transpiler(the non-bundlingparse_data_loaderpath). Mechanism verified in /home/claude/bun/src/bundler/transpiler.rs: the loop at :1991-2057 dedupes…
|
Reply to the second review. The finding about two keys that map to one identifier ( |
This branch has not been deployed
1 participant
Problem
bun build --no-bundleaborts on a JSON, JSONC, JSON5, TOML, YAML or XML file whose only top-level key isdefault:panic: internal error: entered unreachable code,Crashed while printing d.json.new Bun.Transpiler().transformSync('{"default": 1}', "json")aborts the same way.parse_data_loader(src/bundler/transpiler.rs:2059). It gives adefaultkey no named export, then always builds avarfor the named ones. With no other key thevarhas zero declarations.var ;is not valid JavaScript, soprint_decls(src/js_printer/lib.rs:2223) hasunreachable!()for it.Fix
parse_data_loaderemits onlyexport default <value>, as it already does for{}, an array or a scalar. It also cuts the symbol list to the declared symbols.export default { default: 1 };. A runtimeimportand the bundler give the same singledefaultexport.test/bundler/bundler_loader.test.ts(6 new--no-bundlecases) andtest/bundler/transpiler/transpiler.test.js(1 newBun.Transpilercase). All 7 abort without the change. Also rantest/bundler/cli.test.ts.Bun.Transpiler). Rejected: a differential harness for this route, which is a separate test-only change.Background
bun build --no-bundleandBun.Transpiler,parse_data_loaderturns the parsed value into statements.varwith one declaration per top-level key,export { ... }for those names, andexport defaultof the whole object.defaultgets no declaration, becauseexport { x as default }would collide withexport default.Notes
Repro on 1.4.3-canary.1 (b52d513), linux-x64:
The same abort happens for
default = 1(.toml),default: 1(.yaml),{default: 1}(.json5), a.jsoncfile,<default>1</default>with--loader .xml:xml, and{"default": 1, "default": 2}.Output with this change:
generateCodeForLazyExport.rs) and never had this problem:bun build d.jsonprintsexport { d_default as default }.{"a": 1, "default": 2}is unchanged:var a = 1; export { a }; export default { a, default: 2 };.unreachable!()inprint_declsstates a real invariant, and parser: drop dead-branchvarwith zero identifiers instead of emittingvar;#31003 fixed an earlier emptyvarthe same way, in the code that built it.true: t) then reaches this same state. A review comment on that PR names this root cause. This change is in the block after the loop, a different hunk, so the two merge cleanly.loader: { ".xml": "xml" }.bun build --no-bundlemaps.xmlto thefileloader unless--loader .xml:xmlis given, becauseDEFAULT_LOADER_EXTinsrc/bundler/options.rshas no.xml. That is a separate gap and this change does not touch it.test/bundler/expectBundled.tslistedxmlas a loader thatbun builddoes not implement and registered such a test as a todo, soxmlis now in its list.symbols.truncate(count): the list had one entry per property. The entries pastcountwere default symbols that noRefnames. Refs are0..count, so the cut changes no output.src/atorigin/main: 7 fail, each with the panic above. With the change: 7 pass.test/bundler/bundler_loader.test.ts(65 pass),test/bundler/transpiler/transpiler.test.js(223 pass),test/bundler/cli.test.ts(38 pass).[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file