Repository navigation
bundler: writes through a lifted CommonJS module's namespace assign the bindings - #41182
Merged
Merged
Conversation
…he bindings `import config from "./config.js"; config.debug = true` landed on the namespace object's setter while every read of `config.debug` was the lifted `$debug` binding, so the write was lost. The namespace of a lifted module stands in for `module.exports`, so its properties now get setters that assign the binding (`__exportCjs`), and every importer sees the new value.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughSummaryLifted CommonJS namespaces now use ChangesLifted CommonJS namespace writes
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes writes through lifted CommonJS namespace objects update the underlying exported bindings. No actionable merge-blocking risk remains, so the PR is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes bundler output semantics for every lifted CommonJS module and threads new state through the parallel step-4/step-5 linker path, a human look at the linker and runtime pieces would still be worthwhile.
What was reviewed:
- Step 4 creates
lifted_setter_parambefore step 5's parallel run; the unsafe SoA row read indo_step5matches the surrounding per-task-row pattern and the SAFETY comment is accurate. __exportCjswithset: setters[name]— when an alias has no setter (re-export of an import),set: undefinedyields a getter-only accessor, matching the stated intent.- The
__export→__exportCjsassertion update in the existingcjs2esmtest is justified: that fixture is a lifted CJS file, so it now hits the new helper. - Variant coverage checked: minifyIdentifiers (shared param symbol across all setter arrows),
deprecatedNamespaceObjectSetters: false, code-splitting, and a non-exported extra key write.
Extended reasoning...
Overview
The PR makes writes through a lifted CommonJS module's namespace object (its default import) assign the underlying lifted bindings, so config.debug = true is observed by named imports and import * as it was before lifting. It adds a per-file lifted_setter_param: Ref column to LinkerGraph's js_meta SoA, populates it in scanImportsAndExports step 4 (serial), reads it in doStep5 (parallel per file) to emit a third { alias: (value) => $binding = value } argument to a new __exportCjs runtime helper, and wires the runtime-import edge in step 4 to point at __exportCjs instead of __export/__exportGetters for lifted files. Docs and five itBundled cases in bundler_cjs2esm.test.ts accompany the change.
Security risks
None. This is bundler codegen and a small runtime.js helper; no untrusted input parsing, auth, crypto, or filesystem paths are involved. The new __exportCjs uses the already-captured __defProp primordial and iterates the compiler-generated getters object with for..in, identical in shape to the existing __export.
Level of scrutiny
Medium-high. The change is not mechanical: it alters emitted code for an entire class of inputs (every module the CJS→ESM lifter converts), threads new state between two linker phases where step 5 is task-parallel and cannot allocate symbols, and adds an unsafe raw-pointer read. The step-4 symbol creation is placed alongside the existing needs_exports_variable/wrapper-part logic and gates on COMMONJS_LIFTED_TO_ESM with the same export_kind/wrap guards used nearby, and the step-5 raw read follows the exact row_mut!/.cast().add(id) SoA convention already in the function. Still, this is the linker — a reviewer familiar with LinkerContext should confirm the declared_symbols treatment of the shared setter-param ref (declared once, is_top_level: true, reused as the binding of every setter arrow) is correct for renaming/scope-hoisting, and that not adding lifted_setter_param to ns_export_symbol_uses is intentional.
Other factors
Test coverage is good and follows REVIEW.md's variant-matrix guidance (plain, minified with identifier mangling, deprecatedNamespaceObjectSetters: false, the React SSR patching idiom, and cross-chunk splitting). The one weakened assertion (__export → __exportCjs) is a direct consequence of the change and the test's run.stdout still asserts behavior. The __exportCjs helper is appended at the end of runtime.js per the PR's stated intent of not shifting existing part ranges. No CODEOWNERS entries cover the touched paths. Given the scope — every lifted-CJS namespace in every bundle — I'm deferring rather than approving.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 3, 2026
…ace as default (#41231) ### Problem - On canary, with `--splitting`, an `import()` of a lifted CommonJS module (`exports.x = ...`) loads a chunk whose `default` is a getter-only object. Since #41162 the static default import is the namespace object. So `(await import("./lib.cjs")).default !== lib`, and `m.default.x = v` throws `TypeError: Attempted to assign to readonly property`. Bun 1.4.1 gives one object. No release has the bug. - The cause is the synthetic default export in `generate_entry_point_tail_js` (`src/bundler/linker_context/postProcessJSChunk.rs:1089`). Before #41162, the chunk printed `export default require_lib()`. ### Fix - The chunk of a lifted module prints `export default exports_lib`, the namespace object. A write through `m.default` assigns the lifted binding (#41182), so `lib` and `import { x }` see it. - Step 1 of `scan_imports_and_exports` sets `needs_synthetic_default_export` only when an `import()` can read `default`. Step 6 then keeps the namespace part alive. - `exports.default` is a property of `module.exports`, as in Node and `bun run`. A module that also sets `__esModule` keeps `exports.default` as the chunk's `default`, as before. - Verified: `test/bundler/bundler_cjs2esm.test.ts` (9 new tests, 8 fail on main), and the suites in the notes. ### Background - Lifting turns `exports.foo = x` into `var $foo = x; export { $foo as foo }`. `exports_ref` names the namespace object. Its part is tree-shaken unless a live part depends on it. - With code splitting, each `import()` target is an entry point with its own chunk. - The parser records the properties read on an `import()` result. An untracked use can read any export. <details><summary>Notes</summary> Suites run: `bundler_cjs2esm`, `bundler_cjs`, `bundler_splitting`, `bundler_dynamic_import_dce`, `esbuild/splitting`, `esbuild/importstar`, `esbuild/importstar_ts`, `esbuild/default`, `esbuild/dce`, `bundler_edgecase`, `bundler_npm`, `bundler_minify`. On the first version of this branch, also `bundler_barrel`, `bundler_jsx`, `bundler_browser`, `bundler_bun` and `bun-build-api` (two bytecode tests time out at 5s in the debug build, unrelated). Repro from the report, `bun build ./entry.mjs --splitting --target=bun --outdir=s`: ```js // lib.cjs exports.createElement = function (t) { return "<" + t + ">"; }; exports.version = "19.x"; // entry.mjs import lib from "./lib.cjs"; const m = await import("./lib.cjs"); m.default.createElement = () => "PATCHED"; console.log(m.default === lib, lib.createElement("q")); ``` Canary: `TypeError: Attempted to assign to readonly property.` With this change: `true PATCHED`, the same as `bun entry.mjs` and Node. The chunk is `export default exports_lib;`, with `exports_lib` imported from the shared chunk. With #41186 (on main), both changes read the properties that the parser records for an `import()` result. Two tests pin how they combine. A static default import and a named import of a lifted module bind `$useState` directly in both. If the split `import()` only destructures, the chunk has no `default` and no namespace object. If it reads `default`, the chunk exports `exports_lib`, and a write through `m.default` reaches both static importers. When to emit the namespace default. The flag is set per `import()` site, when both hold: - the site can read `default`: its uses are not all tracked, it reads `default`, or the target is a user's entry point. - the site's `default` is `module.exports`: the module has no `default` export, or it is lifted, is not a user's entry point, and does not set `__esModule`. A chunk that no importer reads `default` from now has no `default` export and no namespace object. For `const { version } = await import("./lib.cjs")` with `--minify`, the output goes from 237 bytes on canary to 166. A lifted module with no exports gets `export default exports_x` too. On canary its chunk had no `default`. `import()` with splitting, for a module with `exports.default = "d"` and `exports.x = 1`, dynamic import only, `typeof m.default`: | | `bun run` | 1.4.1 | canary | this PR | |---|---|---|---|---| | no `__esModule`, `.js` or `.mjs` importer | object | string | string | object | | `__esModule`, `.js` or `.mjs` importer | string | string | string | string | With a static default import in the same `.js` or `.mjs` file and no `__esModule`, `m.default === lib` is true on 1.4.1, false on canary and true with this change. Known cases this PR does not change: - A module that sets `__esModule` and `exports.default`, with a static default import and a split `import()` in the same file. From an `.mjs` file, the static import is the namespace (Node's rule) and the `import()` gives `exports.default`. From a `.js` file the module keeps its `__commonJS` wrapper, and the next case applies. - A split `import()` of a lifted module that is in a `__commonJS` wrapper, because of a `require()` of it or the rule above, gets no `__toESM` on the importer side. So its named exports are `undefined`. This is the same on 1.4.1. It is tracked separately. - A lifted user entry point that no `import()` reaches has no `default` export. The `default` is decided per `import()` site, not per entry point, as before this change. - Open PR #40914 changes the same tail code in `postProcessJSChunk.rs`. The two need a rebase against each other. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [881.80ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [415.54ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [372.29ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [439.71ms] (pass) bundler > cjs2esm/ExportsFunction [362.42ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [447.58ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [379.65ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [491.09ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [441.26ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [420.72ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [418.84ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [745.78ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [704 ... (truncated) release without fix: 35 FAILED bun test v1.4.1-canary.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [24.40ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [13.68ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [15.04ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [13.59ms] (pass) bundler > cjs2esm/ExportsFunction [13.89ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [14.70ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [16.19ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [16.52ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [16.27ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [24.98ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [18.70ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [14.39ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [15.65ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRuntimeCondition [13.84ms] (pass) bundler > cjs2esm/UnwrappedModuleRequireAssigned [16.11ms] (pass) bundler > cjs2esm/Unwrap ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [959.21ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [409.94ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [587.69ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [379.52ms] (pass) bundler > cjs2esm/ExportsFunction [365.54ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [394.43ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [474.62ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [497.44ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [547.38ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [527.60ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [456.15ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [843.87ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [650 ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 586ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/86] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/brotli) �[1m�[92m Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output) �[1m�[92m Compiling�[0m bun_clap v0.0.0 (/workspace/bun/src/clap) �[1m�[92m Compiling�[0m b ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/bundler/index.mdx | 2 +- src/bundler/LinkerContext.rs | 10 + .../computeCrossChunkDependencies.rs | 11 +- src/bundler/linker_context/postProcessJSChunk.rs | 41 +++- .../linker_context/scanImportsAndExports.rs | 45 +++-- test/bundler/bundler_cjs2esm.test.ts | 215 ++++++++++++++++++++- 6 files changed, 301 insertions(+), 23 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/bundler/index.mdx 0 0 28 src/bundler/LinkerContext.rs 2 5 29 …bundler/linker_context/computeCrossChunkDependencies.rs 2 5 28 src/bundler/linker_context/postProcessJSChunk.rs 8 5 28 src/bundler/linker_context/scanImportsAndExports.rs 9 14 28 test/bundler/bundler_cjs2esm.test.ts 3 8 28 ``` </details> <!-- robobun:evidence:end -->
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 14, 2026
…default import as a value (#42569) ### Problem - Since #41162 (1.4.1), `import lib from "./lib.cjs"` of a lifted CommonJS module is a namespace object with a getter and a setter per export. `Object.defineProperty(lib, "x", ...)` and `delete lib.y` change that object, but `lib.x` reads the lifted `$x` binding. A write after `Object.freeze(lib)` still runs the setter. - Node, `bun run` and 1.4.0 print `defineProperty -> 65`, `delete -> true undefined false`, `write to frozen -> 3 true`. The bundle prints `1`, `true 2 false`, `66 true`. - Source: a fuzz ledger that compares bundles with `bun run`. No user issue reports this. ### Fix - The parser sets a new symbol flag, `import_used_as_value`, on an import that some use holds as a value. Exempt: a read, call or assignment of a property (`X.a`, `X[k]`, `X.a = 1`) and a destructuring declaration. `delete X.a` counts. - Step 1 of `scan_imports_and_exports` keeps the `__commonJS` wrapper of a lifted module when a default import of it has the flag or is re-exported. The importer gets the real `module.exports`, as in 1.4.0. - `React.useState()` and `React.x = y` stay lifted: react 18.3.1 `--production --minify` is 1423 bytes, same as main. With `Object.keys(React)` it is 7219 bytes (main 7794). - Verified: `test/bundler/bundler_cjs2esm.test.ts` (11 new cases fail on main), plus the suites in the Notes. ### Background - Lifting turns `exports.foo = x` into `var $foo = x` plus an ES module export. - The namespace object `exports_lib` stands in for `module.exports`. `__exportCjs` (#41182) gives it a getter and a setter per export. - `defineProperty` and `delete` only replace or remove the accessor. A frozen accessor still calls its setter. - Step 1 picks the CommonJS files before imports are matched, so it has parser facts only. A re-export counts as held. <details><summary>Notes</summary> Output for the report's repro is now byte-identical to what 1.4.0 emits (`var import_lib = __toESM(require_lib(), 1); Object.defineProperty(import_lib.default, "x", ...)`). How the flag is computed: `e_dot` and `e_index` visit their target with `ExprIn::is_property_access_target` (false for a delete target), `visit_decls` does the same for the initializer of an object pattern, and `handle_identifier` sets the flag when it turns an identifier into an `EImportIdentifier` without it. Two places build an import identifier outside the visitor and pass the bit themselves: the classic JSX factory (`React.createElement`) and `emitDecoratorMetadata` (`design:type` gets the import itself, so it sets the flag). Dead control flow does not set the flag. The flag is per file: a use in a part that tree shaking later removes still counts. Routes to `module.exports` of a module that stays lifted that this PR does not close. 1.4.0 kept the wrapper for every default import, so when the bundle also has a static default import of the module, these gave the real object in 1.4.0 and give the namespace object here: - `this` in a method call. `lib.self()` with `exports.self = function () { return this; }` still gets the namespace object (#41162 chose that for `this._helper()` style code). - `(await import(m)).default`, same chunk or split. - `ns.default` on `import * as ns` (new in 1.4.1, #41820 reworks it). `require()` of a lifted module outside the react family already keeps the wrapper. A react-family file of the shape `sideEffect(); module.exports = require("./impl")` (react-dom/index.js) is lifted to `export * from "./impl"`. When it keeps its wrapper it prints `module.exports = exports_impl`, the namespace object of `./impl`, as 1.4.0 did. Every read then goes through that object, so `defineProperty` and `delete` work (react-dom 18.3.1 `--production`: `Object.defineProperty(ReactDOM, "version", { value: "dp" })` prints `dp`, as 1.4.0 does, and main prints the old version). A write after `Object.freeze` still reaches the binding there. An earlier commit of this PR also wrapped `./impl`. That gave `module.exports = __toESM(require_impl())`, whose getters are not configurable, so `defineProperty` threw. It is reverted. The real fix for that shape is to print the raw `require_impl()` (handed off, #35722 had the approach). #41820 is open and edits the same step 1 lines, the same docs paragraph and the same tests. This PR is the smaller one and restores 1.4.0 output, so it is simpler to land it first and rebase #41820. Test expectations that changed, all toward Node: - `cjs/__toESM_mixed_import_styles` is back to its expectation from before #41162 (the namespace has a `default` key). - `ReExportDefaultAsNameFromLiftedCommonJS` and `ExportDefaultOfLiftedCommonJSDefaultImport` now keep the wrapper (a re-export can reach an importer that holds the object) and also check `Object.freeze` through the barrel. A third form, `export { React as R }`, is added. - `DefaultImportEscapingKeepsAssignmentOrder` became `DefaultImportPassedAsValueKeepsAssignmentOrder` and expects the wrapper. - Ten tests compared the default import by identity (`m.default === React`) or passed it to `Object.keys`. They now compare a property (`m.default.useState === React.useState`) so that they keep testing the lifted path. Suites run with the debug build: `bundler_cjs2esm`, `bundler_cjs`, `esbuild/importstar`, `esbuild/importstar_ts`, `esbuild/default`, `esbuild/dce`, `esbuild/splitting`, `esbuild/packagejson`, `esbuild/ts`, `bundler_splitting`, `bundler_edgecase`, `bundler_jsx`, `bundler_npm`, `bundler_barrel`, `bundler_minify`, `bundler_browser`, `bundler_bun`, `bundler_plugin`, `bundler_loader`, `bundler_string`, `bundler_promiseall_deadcode`, `transpiler/transpiler`, `regression/issue/03844`, `bun-build-api` (two bytecode stress tests time out at 5 s under the debug build, they bundle no imports). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 10 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 12 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_cjs.test.ts "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.3 (b993710) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [1254.52ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [548.64ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [554.77ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [472.17ms] (pass) bundler > cjs2esm/ExportsFunction [398.00ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [505.22ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [490.55ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [561.72ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [617.95ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [686.37ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [551.89ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [704.70ms] (pass) bundler > cjs2esm/ModuleExp ... (truncated) release without fix: 12 FAILED bun test v1.4.3-canary.1 (b993710) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [29.48ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [14.24ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [13.88ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [13.68ms] (pass) bundler > cjs2esm/ExportsFunction [14.09ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [13.74ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [13.08ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [16.07ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [17.11ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [15.81ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [15.04ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [17.65ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [15.90ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRuntimeCondition [16.27ms] (pass) bundler > cjs2esm/UnwrappedModuleRequireAssigned [15.69ms] (pass) bundler > cjs2esm/Unwrap ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_cjs.test.ts "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.3 (b993710) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [1115.42ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [521.71ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [536.01ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [594.49ms] (pass) bundler > cjs2esm/ExportsFunction [501.50ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [632.29ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [418.24ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [456.28ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [591.53ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [603.73ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [546.68ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [667.62ms] (pass) bundler > cjs2esm/ModuleExp ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision b9ce20b features baseline 23 deps, 131 codegen, 1176 objects in 676ms ninja: Entering directory `/workspace/bun/build/release' [1/1248] install /workspace/bun bun install v1.4.3-canary.1 (b993710) Checked 22 installs across 61 packages (no changes) [14.00ms] [2/1248] gen ErrorCode+*.h [3/1248] gen bindgenv2 [4/1248] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (b993710) Checked 1 install across 2 packages (no changes) [2.00ms] [5/1248] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (b993710) Checked 111 installs across 104 packages (no changes) [5.00ms] [6/1248] fetch zlib [zlib] up to date [7/1248] gen node-fallbacks/react-refresh.js Bundled 1 module in 10ms react-refresh.js 4.81 KB (entry point) [8/1248] fetch libjpeg-turbo [libjpeg-turbo] up to date [9/1248] fetch tinycc [tinycc] up to date [10/1247] gen .bind.ts → GeneratedBindings.cpp [11/1247] gen bake.{client,server,error}.js -> bake.client.js, bake.serve ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/bundler/index.mdx | 2 +- src/ast/symbol.rs | 4 + .../linker_context/scanImportsAndExports.rs | 28 +- src/js_parser/fold.rs | 3 + src/js_parser/p.rs | 19 +- src/js_parser/parser.rs | 14 + src/js_parser/visit/mod.rs | 2 + src/js_parser/visit/visit_expr.rs | 18 +- test/bundler/bundler_cjs.test.ts | 8 +- test/bundler/bundler_cjs2esm.test.ts | 318 +++++++++++++++------ 10 files changed, 318 insertions(+), 98 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/bundler/index.mdx 2 2 25 src/ast/symbol.rs 1 2 25 src/bundler/linker_context/scanImportsAndExports.rs 6 10 25 src/js_parser/fold.rs 1 1 25 src/js_parser/p.rs 7 6 25 src/js_parser/parser.rs 2 4 25 src/js_parser/visit/mod.rs 1 1 25 src/js_parser/visit/visit_expr.rs 5 3 25 test/bundler/bundler_cjs.test.ts 1 2 9 test/bundler/bundler_cjs2esm.test.ts 11 18 20 ``` </details> <!-- robobun:evidence:end -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…default import as a value (oven-sh#42569) ### Problem - Since oven-sh#41162 (1.4.1), `import lib from "./lib.cjs"` of a lifted CommonJS module is a namespace object with a getter and a setter per export. `Object.defineProperty(lib, "x", ...)` and `delete lib.y` change that object, but `lib.x` reads the lifted `$x` binding. A write after `Object.freeze(lib)` still runs the setter. - Node, `bun run` and 1.4.0 print `defineProperty -> 65`, `delete -> true undefined false`, `write to frozen -> 3 true`. The bundle prints `1`, `true 2 false`, `66 true`. - Source: a fuzz ledger that compares bundles with `bun run`. No user issue reports this. ### Fix - The parser sets a new symbol flag, `import_used_as_value`, on an import that some use holds as a value. Exempt: a read, call or assignment of a property (`X.a`, `X[k]`, `X.a = 1`) and a destructuring declaration. `delete X.a` counts. - Step 1 of `scan_imports_and_exports` keeps the `__commonJS` wrapper of a lifted module when a default import of it has the flag or is re-exported. The importer gets the real `module.exports`, as in 1.4.0. - `React.useState()` and `React.x = y` stay lifted: react 18.3.1 `--production --minify` is 1423 bytes, same as main. With `Object.keys(React)` it is 7219 bytes (main 7794). - Verified: `test/bundler/bundler_cjs2esm.test.ts` (11 new cases fail on main), plus the suites in the Notes. ### Background - Lifting turns `exports.foo = x` into `var $foo = x` plus an ES module export. - The namespace object `exports_lib` stands in for `module.exports`. `__exportCjs` (oven-sh#41182) gives it a getter and a setter per export. - `defineProperty` and `delete` only replace or remove the accessor. A frozen accessor still calls its setter. - Step 1 picks the CommonJS files before imports are matched, so it has parser facts only. A re-export counts as held. <details><summary>Notes</summary> Output for the report's repro is now byte-identical to what 1.4.0 emits (`var import_lib = __toESM(require_lib(), 1); Object.defineProperty(import_lib.default, "x", ...)`). How the flag is computed: `e_dot` and `e_index` visit their target with `ExprIn::is_property_access_target` (false for a delete target), `visit_decls` does the same for the initializer of an object pattern, and `handle_identifier` sets the flag when it turns an identifier into an `EImportIdentifier` without it. Two places build an import identifier outside the visitor and pass the bit themselves: the classic JSX factory (`React.createElement`) and `emitDecoratorMetadata` (`design:type` gets the import itself, so it sets the flag). Dead control flow does not set the flag. The flag is per file: a use in a part that tree shaking later removes still counts. Routes to `module.exports` of a module that stays lifted that this PR does not close. 1.4.0 kept the wrapper for every default import, so when the bundle also has a static default import of the module, these gave the real object in 1.4.0 and give the namespace object here: - `this` in a method call. `lib.self()` with `exports.self = function () { return this; }` still gets the namespace object (oven-sh#41162 chose that for `this._helper()` style code). - `(await import(m)).default`, same chunk or split. - `ns.default` on `import * as ns` (new in 1.4.1, oven-sh#41820 reworks it). `require()` of a lifted module outside the react family already keeps the wrapper. A react-family file of the shape `sideEffect(); module.exports = require("./impl")` (react-dom/index.js) is lifted to `export * from "./impl"`. When it keeps its wrapper it prints `module.exports = exports_impl`, the namespace object of `./impl`, as 1.4.0 did. Every read then goes through that object, so `defineProperty` and `delete` work (react-dom 18.3.1 `--production`: `Object.defineProperty(ReactDOM, "version", { value: "dp" })` prints `dp`, as 1.4.0 does, and main prints the old version). A write after `Object.freeze` still reaches the binding there. An earlier commit of this PR also wrapped `./impl`. That gave `module.exports = __toESM(require_impl())`, whose getters are not configurable, so `defineProperty` threw. It is reverted. The real fix for that shape is to print the raw `require_impl()` (handed off, oven-sh#35722 had the approach). oven-sh#41820 is open and edits the same step 1 lines, the same docs paragraph and the same tests. This PR is the smaller one and restores 1.4.0 output, so it is simpler to land it first and rebase oven-sh#41820. Test expectations that changed, all toward Node: - `cjs/__toESM_mixed_import_styles` is back to its expectation from before oven-sh#41162 (the namespace has a `default` key). - `ReExportDefaultAsNameFromLiftedCommonJS` and `ExportDefaultOfLiftedCommonJSDefaultImport` now keep the wrapper (a re-export can reach an importer that holds the object) and also check `Object.freeze` through the barrel. A third form, `export { React as R }`, is added. - `DefaultImportEscapingKeepsAssignmentOrder` became `DefaultImportPassedAsValueKeepsAssignmentOrder` and expects the wrapper. - Ten tests compared the default import by identity (`m.default === React`) or passed it to `Object.keys`. They now compare a property (`m.default.useState === React.useState`) so that they keep testing the lifted path. Suites run with the debug build: `bundler_cjs2esm`, `bundler_cjs`, `esbuild/importstar`, `esbuild/importstar_ts`, `esbuild/default`, `esbuild/dce`, `esbuild/splitting`, `esbuild/packagejson`, `esbuild/ts`, `bundler_splitting`, `bundler_edgecase`, `bundler_jsx`, `bundler_npm`, `bundler_barrel`, `bundler_minify`, `bundler_browser`, `bundler_bun`, `bundler_plugin`, `bundler_loader`, `bundler_string`, `bundler_promiseall_deadcode`, `transpiler/transpiler`, `regression/issue/03844`, `bun-build-api` (two bytecode stress tests time out at 5 s under the debug build, they bundle no imports). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 10 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 12 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_cjs.test.ts "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.3 (b993710) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [1254.52ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [548.64ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [554.77ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [472.17ms] (pass) bundler > cjs2esm/ExportsFunction [398.00ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [505.22ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [490.55ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [561.72ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [617.95ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [686.37ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [551.89ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [704.70ms] (pass) bundler > cjs2esm/ModuleExp ... (truncated) release without fix: 12 FAILED bun test v1.4.3-canary.1 (b993710) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [29.48ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [14.24ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [13.88ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [13.68ms] (pass) bundler > cjs2esm/ExportsFunction [14.09ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [13.74ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [13.08ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [16.07ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [17.11ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [15.81ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [15.04ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [17.65ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [15.90ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRuntimeCondition [16.27ms] (pass) bundler > cjs2esm/UnwrappedModuleRequireAssigned [15.69ms] (pass) bundler > cjs2esm/Unwrap ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_cjs.test.ts "test/bundler/bundler_cjs2esm.test.ts" bun test v1.4.3 (b993710) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [1115.42ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [521.71ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [536.01ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [594.49ms] (pass) bundler > cjs2esm/ExportsFunction [501.50ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [632.29ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [418.24ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [456.28ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [591.53ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [603.73ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [546.68ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [667.62ms] (pass) bundler > cjs2esm/ModuleExp ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision b9ce20b features baseline 23 deps, 131 codegen, 1176 objects in 676ms ninja: Entering directory `/workspace/bun/build/release' [1/1248] install /workspace/bun bun install v1.4.3-canary.1 (b993710) Checked 22 installs across 61 packages (no changes) [14.00ms] [2/1248] gen ErrorCode+*.h [3/1248] gen bindgenv2 [4/1248] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (b993710) Checked 1 install across 2 packages (no changes) [2.00ms] [5/1248] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (b993710) Checked 111 installs across 104 packages (no changes) [5.00ms] [6/1248] fetch zlib [zlib] up to date [7/1248] gen node-fallbacks/react-refresh.js Bundled 1 module in 10ms react-refresh.js 4.81 KB (entry point) [8/1248] fetch libjpeg-turbo [libjpeg-turbo] up to date [9/1248] fetch tinycc [tinycc] up to date [10/1247] gen .bind.ts → GeneratedBindings.cpp [11/1247] gen bake.{client,server,error}.js -> bake.client.js, bake.serve ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/bundler/index.mdx | 2 +- src/ast/symbol.rs | 4 + .../linker_context/scanImportsAndExports.rs | 28 +- src/js_parser/fold.rs | 3 + src/js_parser/p.rs | 19 +- src/js_parser/parser.rs | 14 + src/js_parser/visit/mod.rs | 2 + src/js_parser/visit/visit_expr.rs | 18 +- test/bundler/bundler_cjs.test.ts | 8 +- test/bundler/bundler_cjs2esm.test.ts | 318 +++++++++++++++------ 10 files changed, 318 insertions(+), 98 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/bundler/index.mdx 2 2 25 src/ast/symbol.rs 1 2 25 src/bundler/linker_context/scanImportsAndExports.rs 6 10 25 src/js_parser/fold.rs 1 1 25 src/js_parser/p.rs 7 6 25 src/js_parser/parser.rs 2 4 25 src/js_parser/visit/mod.rs 1 1 25 src/js_parser/visit/visit_expr.rs 5 3 25 test/bundler/bundler_cjs.test.ts 1 2 9 test/bundler/bundler_cjs2esm.test.ts 11 18 20 ``` </details> <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
X.fooreads the lifted$foobinding. A write went the other way:import config from "./config.js"; config.debug = true; console.log(config.debug)printedfalse, becauseconfig.debug = truelanded on the namespace object's setter (which only shadows the getter) while the read was$debug. Bun 1.4.1 and Node printtrue.React.useLayoutEffect = React.useEffecthad the same gap, and with--no-deprecated-namespace-object-settersthe write threw at startup.Fix
module.exports, so each of its local exports now gets a setter that assigns the lifted binding:__exportCjs(exports_config, { debug: () => $debug }, { debug: (value) => $debug = value }). A write through the default import, or throughimport * as nsreached via a re-export, is then seen by every reader, named imports included.scan_imports_and_exportscreates the setters' parameter symbol for each lifted module (step 5 runs in parallel and cannot create symbols).create_exports_for_fileemits the setter arrows. Exports that are re-exports of imports keep a getter only.deprecatedNamespaceObjectSetterssays: that option is about ES module namespaces, which are read-only. A CommonJSmodule.exportsis writable.test/bundler/bundler_cjs2esm.test.ts(5 new tests, 3 fail on main and on the released bun, all pass with the change). Alsobundler_cjs,esbuild/importstar,esbuild/importstar_ts,esbuild/default,esbuild/dce,bundler_splitting,bundler_jsx,bundler_npm,bundler_edgecase,bundler_minify,bundler_barrel,bundler_browser,bundler_bun,bun-build-api,bundler_promiseall_deadcode,bundler_bytecode_portable,regression/issue/cyclic-imports-async-bundler.Background
exports.foo = xintovar $foo = x; export { $foo as foo }, and bundler: bind the default import of a lifted CommonJS module to its namespace #41162 bindsimport X fromthat module to itsexports_ref, the namespace objectvar exports_x = {}; __export(exports_x, {...}), which exists only whenXescapes.X.fooreads compile to$foo.__exportdefines a getter per export plus a setter that replaces the getter's value on the namespace object without touching the binding.__exportGetters(--no-deprecated-namespace-object-setters) defines no setter. Both are right for an ES module namespace, which is read-only in the spec.__exportCjshelper is appended at the end ofruntime.js, so the part ranges of the existing runtime statements, and with them thedebugIdandbun-build-apihash snapshots, do not move.Notes
Writes are more live than in Node: Node's named imports of a CommonJS module are snapshots, so there
config.debug = trueis seen byconfig.debugbut not byimport { debug }. Bun 1.4.1 made every reader see it (everything went through the CommonJS object), and this change keeps that.import * as ns from "./lifted"; ns.debug = trueis a build error (Cannot assign to import), as in esbuild. A namespace reached through a re-export (export * as ns,export { ns }) takes the new setters like the default import does.A write of a key the module does not export (
config.extra = 1) creates a plain data property on the namespace object.config.extrareads stay property reads, so they see it.The docs paragraph in
docs/bundler/index.mdxno longer listsreact-domamong lifted packages: its entry files have thecheckDCE(); module.exports = require(...)shape, which the lifter does not convert today (handed off separately).[human-review] gate passed · iteration 0 · 6 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