Repository navigation
Conversation
Fixes #9874. When a mock.module factory returns an object with accessor properties (`{ get foo() { ... } }`), the exports were being evaluated once at module creation time and frozen into the synthetic module's environment slots, so subsequent reads never saw the getter again. For require(), the mock object now becomes module.exports directly, so property access goes through the original getters. For await import(), generateObjectModuleSourceCode detects accessor properties and passes the factory object to SyntheticModuleRecord as a live-exports backing source (via a trailing exportValue with no matching exportName). JSModuleNamespaceObject::getOwnPropertySlotCommon then forwards namespace reads through that object instead of returning the raw environment slot. The module-namespace IC is not installed for these properties, so DFG/FTL do not lower the read to a GetClosureVar raw slot load. The environment slots still hold the first-read snapshot, so static `import { x }` bindings (which every tier reads as a raw variableAt(offset) load) are unaffected. Requires oven-sh/WebKit#380. WEBKIT_VERSION is pinned to its preview build; swap to the merged main sha before landing.
WalkthroughChangesThe module mock implementation now preserves live accessor exports for ESM and CommonJS modules, handles property access exceptions, and validates re-mocking and caching behavior. The WebKit build dependency now uses an autobuild preview identifier. Module mock live exports
WebKit preview update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Updated 7:28 PM PT - Aug 1st, 2026
❌ @robobun, your commit 433b575 has 2 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.a2e1dd5f-5a67-4472-a980-32ba4ae19fb0
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.203c192b-ccfa-4e0f-9fad-af739b4327f4
🧪 To try this PR locally: bunx bun-pr 36677That installs a local version of the PR into your bun-36677 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
On the related-issue suggestions:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
overrideExportValue now writes through to SyntheticModuleRecord's live-exports backing object (WebKit side), and JSMock__jsModuleMock swaps the source to the new factory object when re-mocking a synthetic record, so spyOn(ns, key) and a subsequent mock.module on an accessor-backed namespace are observed by namespace reads. Bumps WEBKIT_VERSION to the updated preview build.
There was a problem hiding this comment.
The re-mock / spyOn regression I flagged earlier looks addressed in 1a5eb0a + oven-sh/WebKit@1ee717ba, and the new tests cover both paths. I didn't find further issues, but this needs a human look: it bumps WEBKIT_VERSION (still pinned to the preview tag, and the range also pulls in oven-sh/WebKit#333 CodeBlock aging), and the fix hinges on new JSC-side semantics (m_liveExportsSource, overrideExportValue write-through) that I can't verify from this repo alone.
Checked: the re-mock setLiveExportsSource call is inside the object branch so the non-object defaultKeyword path is unaffected; the new wasModuleMock CJS branch in handleVirtualModuleResult is ordered after the __esModule check so that path still wins; the trailing exportValues.append(object) only fires when an own accessor is present, so plain-object modules keep the old snapshot behavior.
Extended reasoning...
Overview
This PR makes accessor properties on mock.module / loader: "object" factory results behave as live bindings instead of one-time snapshots. It touches src/jsc/modules/ObjectModule.cpp (detect accessors, append the factory object as a trailing name-less export value), src/jsc/bindings/ModuleLoader.cpp (assign the factory object directly as module.exports for the CJS require() path when wasModuleMock), and src/jsc/bindings/BunPlugin.cpp (swap SyntheticModuleRecord::liveExportsSource to the new factory object on re-mock). It bumps WEBKIT_VERSION to a preview autobuild tag for oven-sh/WebKit#380, which introduces m_liveExportsSource on SyntheticModuleRecord and makes JSModuleNamespaceObject::getOwnPropertySlotCommon / overrideExportValue respect it. Five new tests cover ESM/CJS live bindings, mixed data+accessor exports, accessor→data→accessor re-mock, and spyOn + mock.restore() on an accessor-backed namespace; one plugin test covers the build.module path.
Security risks
None identified. This is test-runner / module-mock plumbing; no untrusted-input parsing, auth, or network surface is touched.
Level of scrutiny
High. This is a coordinated Bun + WebKit change to core module-loading semantics in JSC bindings. The correctness of the fix depends entirely on the WebKit-side implementation of m_liveExportsSource (GC rooting via WriteBarrier, precedence vs. env-slot reads across all JIT tiers, overrideExportValue write-through), none of which is reviewable from this diff. The WebKit range also carries oven-sh/WebKit#333 (execution-count CodeBlock aging, gated off) and #377 — unrelated changes a maintainer should be aware are riding along. Per the repo guidance, dependency bumps and JSC GC-adjacent changes warrant maintainer review.
Other factors
WEBKIT_VERSIONis currently a preview tag (autobuild-preview-pr-380-1ee717ba); the PR description explicitly says it must be swapped to the mergedmainsha before landing, so this cannot merge as-is regardless.- My earlier finding (re-mock and
spyOnwriting only to the env slot while reads forward through the stale live source) was addressed on both sides and is now covered by the two new tests. This run's finder also raised "re-mock unconditionally setsliveExportsSource" — verifiers ruled it out, and I agree: the call is guarded byobject != nullptrand bydynamicDowncast<SyntheticModuleRecord>, so non-synthetic (real ESM) records and non-object re-mocks are untouched. - CI for 1a5eb0a was still building at the time of the last robobun update; results should be checked before merge.
JSObject::get() on a module namespace object was always throwable (the TDZ path), and now additionally forwards through a live-exports source. The validator caught that the result was used (via copyNameAndLength) without a RETURN_IF_EXCEPTION in between.
There was a problem hiding this comment.
The follow-up commits address my earlier concern — setLiveExportsSource on re-mock, overrideExportValue write-through on the WebKit side, and the added RETURN_IF_EXCEPTION in spyOn for the ASAN validateExceptionChecks failure all look right, and the new re-mock/spyOn tests cover them. Deferring to a human because this is a coordinated Bun+WebKit change (new SyntheticModuleRecord::m_liveExportsSource routing namespace reads past the JIT's module-namespace IC) pinned to a preview WebKit tag that the description says must be swapped before landing.
What was reviewed:
ObjectModule.cppaccessor detection viagetOwnPropertySlotand the trailing-value protocol; falls through toobject->getwhen the own slot is absent, matching prior behavior.BunPlugin.cppre-mock now swaps the record's live source only forSyntheticModuleRecord(realJSModuleRecords are left alone, so #30242 stays unfixed as stated).ModuleLoader.cppwasModuleMockshort-circuit for CJS is placed after the existing__esModulehandling and only when a CJS target exists.- The WebKit range also pulls in oven-sh/WebKit#333 (execution-count CodeBlock aging, gated off) and #377 — worth a maintainer glance.
Extended reasoning...
Overview
The PR makes accessor properties on mock.module / plugin loader: "object" factory results behave as live bindings instead of one-time snapshots. On the Bun side it touches ObjectModule.cpp (detect accessors, append the factory object as a trailing export value), ModuleLoader.cpp (assign the factory object directly as module.exports for the require() path when the source was a module mock), BunPlugin.cpp (swap the synthetic record's live-source on re-mock), and JSMockFunction.cpp (an exception check after reading the spy target's value). The behavior depends on oven-sh/WebKit#380, which adds SyntheticModuleRecord::m_liveExportsSource / setLiveExportsSource, makes JSModuleNamespaceObject::getOwnPropertySlotCommon forward reads through it, and makes overrideExportValue write through to it. WEBKIT_VERSION is pinned to autobuild-preview-pr-380-1ee717ba.
Since my previous review
I previously flagged that once a namespace was backed by m_liveExportsSource, the re-mock loop and spyOn/clearSpy (which only wrote via overrideExportValue) would be silently ignored. That was addressed in 1a5eb0a (Bun) + 1ee717ba (WebKit): overrideExportValue now writes through to the live source, and JSMock__jsModuleMock calls synthetic->setLiveExportsSource(vm, object) after the override loop so the new factory object replaces the old one. New tests cover accessor→data→accessor re-mock and spyOn+mock.restore() on an accessor-backed namespace. The subsequent ASAN-lane failure (unchecked exception at copyNameAndLength triggered by the new spyOn test) is what 3138133 fixes by adding RETURN_IF_EXCEPTION after slot.getValue/object->get in JSMock__jsSpyOn — now that namespace reads can forward through a user object, that read is observably fallible.
Security risks
None identified. This is test-runner / module-loader plumbing; the trailing-value protocol is between Bun-authored C++ and Bun's WebKit fork, not user-controllable in a way that widens attack surface.
Level of scrutiny
High. This is a cross-repo design change to how synthetic module namespaces resolve property reads, with explicit reasoning about why the DFG/FTL ModuleNamespaceAccessCase IC won't cache stale values (setValueModuleNamespace is never called for live-source-backed keys). I can't review the WebKit half from here, and the correctness of the Bun half (particularly spyOn write-through and re-mock semantics for mixed data/accessor keys) hinges on exactly what overrideExportValue does on the WebKit side. The PR also bumps WebKit across a range that includes an unrelated CodeBlock-aging change (#333, gated off by default).
Other factors
The PR description explicitly flags that WEBKIT_VERSION is pinned to a preview tag and must be swapped to the merged main sha before landing — that alone means a maintainer needs to coordinate the merge. CI on 1a5eb0a showed the mock-module ASAN failure that 3138133 targets, plus three unrelated failures (a Windows bun-upgrade flake, a Windows regression test, and two agent-creation infra errors); a fresh run on 3138133 hasn't been reported in the thread yet.
There was a problem hiding this comment.
Beyond the inline findings: the RETURN_IF_EXCEPTION added at JSMockFunction.cpp:1526 is the fix for the ASAN validateExceptionChecks CI failure on 1a5eb0a — slot.getValue() on a namespace now forwards through the live source's user getter, which can throw, so the check is necessary and correctly placed. Also confirmed the new wasModuleMock CJS branch in ModuleLoader.cpp does not affect the plugin loader: "object" path (only mock.module sets wasModuleMock), so require() of a builder.module object still returns a namespace as before.
Extended reasoning...
The fix to my earlier comment (swap liveExportsSource on re-mock) over-applies — it now installs a live source on every re-mock of a SyntheticModuleRecord, which changes data-only partial re-mocks of already-imported builtins from patch-per-key to replace-wholesale. That's covered by the inline finding. Separately I traced the JSMockFunction.cpp:1526 exception check back to the ASAN failure in build 87111 and confirmed it's the right fix for the new throwing path, and verified the ModuleLoader.cpp CJS short-circuit is scoped to mock.module only. WEBKIT_VERSION is still on the preview tag, which the description already flags as a pre-land blocker.
The unconditional setLiveExportsSource on re-mock was replacing the backing object even for data-only re-mocks, so a partial mock.module of an already-imported synthetic module (including builtins) made every un-named export read as undefined. Two halves: - WebKit: getOwnPropertySlotCommon only forwards through the live source for properties the source actually owns; others fall through to the module-environment slot. - Bun: JSMock__jsModuleMock only installs/swaps the live source when the new factory has an accessor or a source was already set, so a data-only re-mock keeps the module-namespace IC fast path. Also pins the new require() contract with a test: a captured require() result is the factory object (matching Node/Jest), so it does not observe a subsequent re-mock; a fresh require() does.
![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
🤖 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 `@scripts/build/deps/webkit.ts`:
- Line 6: Update the WEBKIT_VERSION constant to the merged WebKit `#380` revision
and release identifier, replacing the temporary
autobuild-preview-pr-380-3cf0ca7b value so release builds no longer use the
preview artifact.
🪄 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: 7382f855-de47-4044-8b36-bc1714117e43
📒 Files selected for processing (8)
scripts/build/deps/webkit.tssrc/jsc/bindings/BunPlugin.cppsrc/jsc/bindings/JSMockFunction.cppsrc/jsc/bindings/ModuleLoader.cppsrc/jsc/modules/ObjectModule.cpptest/js/bun/plugin/module-plugins.tstest/js/bun/plugin/plugins.test.tstest/js/bun/test/mock/mock-module.test.ts
A data-only first mock leaves the SyntheticModuleRecord with no live source, so namespace reads install ModuleNamespaceAccessCase / lower to GetClosureVar (raw env-slot load). A later accessor re-mock left those compiled sites reading the stale slot while the interpreter forwarded through the live source. WebKit side: SyntheticModuleRecord carries an InlineWatchpointSet that both the access case and the DFG lowering watch; setLiveExportsSource fires it on the first install, which resets the stubs / jettisons that code. getOwnPropertySlotCommon also returns the env-slot fallthrough as uncacheable once the watchpoint has fired so the IC is never re-installed on a record that has ever had a live source. New test tiers up a single call site on a data-only mock, then re-mocks with an accessor and asserts the same site observes the live value.
overrideExportValue writes through to the record's current live-exports source. The re-mock loop ran with the previous factory object still installed, so each per-key write landed on that object (which, since require() of a mock returns it directly, can be a user-captured reference), overwriting its accessors with snapshot values. Clear the live source before the overrideExportValue loop and install the new factory object after. setLiveExportsSource now accepts null (setMayBeNull) and fires the IC watchpoint only on the first-ever install, so the clear/set pair does not re-fire it.
There was a problem hiding this comment.
The latest revision (aa89898 + oven-sh/WebKit@1b4afe55) addresses the write-through-to-previous-factory issue; I didn't find anything new this pass. Deferring to a maintainer for the cross-repo landing — WEBKIT_VERSION is still on the preview tag and the WebKit-side watchpoint/IC changes should get human eyes alongside oven-sh/WebKit#380.
What was reviewed:
BunPlugin.cppre-mock loop now clears the live source beforeoverrideExportValueand reinstalls after; verified this preserves partial-re-mock semantics for both data-only and accessor-backed cases and no longer mutates a capturedrequire()result.generateObjectModuleSourceCodetrailing-value protocol and thewasModuleMockshort-circuit inModuleLoader.cpp; the__esModulebranch above it still takes precedence.- Checked the
spyOnwrite-through path — it still mutates the live-source object (the factory result), which is consistent withspyOnon the same object obtained viarequire(). - The new
RETURN_IF_EXCEPTIONafterslot.getValueinJSMock__jsSpyOnis correct for the now-forwarded getter path.
Extended reasoning...
Overview
This PR makes mock.module factory accessors behave as live bindings on both the ESM namespace and the require() result, and applies the same to plugin loader: "object" exports. The Bun-side diff is ~60 lines of C++ across ObjectModule.cpp, BunPlugin.cpp, ModuleLoader.cpp, JSMockFunction.cpp, plus ~200 lines of new tests. The load-bearing half is in the paired oven-sh/WebKit#380: a new SyntheticModuleRecord::m_liveExportsSource slot, forwarding in JSModuleNamespaceObject::getOwnPropertySlotCommon, write-through in overrideExportValue, and an InlineWatchpointSet that ModuleNamespaceAccessCase and DFGByteCodeParser::handleModuleNamespaceLoad observe so a data-only → accessor re-mock invalidates already-compiled namespace ICs.
Review history
I've reviewed four prior revisions of this PR and each surfaced a real correctness issue (re-mock/spyOn ignored on accessor-backed namespaces; unconditional live-source install regressing partial builtin mocks; missing IC invalidation on the null→object transition; write-through mutating the previous factory object). All four are now addressed with dedicated tests, and this pass — over aa89898 / cf25198 — found nothing new.
Security risks
None. This is test-runner mocking machinery; the accessor forwarding runs user-provided getters on user-provided objects, which is the intended behavior.
Level of scrutiny
High, and specifically needs a maintainer, for two reasons this bot can't discharge:
- Cross-repo coordination.
WEBKIT_VERSIONis pinned toautobuild-preview-pr-380-1b4afe55; the PR description states it must be swapped to the mergedmainsha before landing. oven-sh/WebKit#380 is still open. - JIT-level design review. The correctness of the null→object watchpoint (fires once, observed by both the polymorphic IC via
additionalSetImpland the DFGGetClosureVarlowering, and permanently disables caching on the env-slot fallthrough) lives entirely in the WebKit diff. I've reasoned about it from the PR thread's description and the tests exercise it in-process, but I can't read the WebKit source here — a JSC-familiar reviewer should confirm the watchpoint plumbing on that side.
Other factors
- Test coverage is thorough for the shapes that broke across iterations: ESM/CJS live bindings, mixed data/accessor, re-mock (accessor→data→accessor), spyOn+restore on an accessor-backed namespace, partial re-mock (both data-only and accessor-backed), IC invalidation after a hot data-only read, captured-
require()contract, and re-mock non-mutation of the previous factory. The pluginloader: "object"path is covered too. - The 500k-iteration IC-invalidation test is the one place I'd flag for a maintainer to eyeball wall-time under debug+ASAN, though it's a tight integer loop.
- Existing
mock.moduleregression tests (6874, 6879, mock-module-*) andtest-vm-module-synthetic.jsare stated unchanged; I did not re-run them.
The non-object re-mock branch (reached when a factory returns an already-fulfilled Promise resolving to a primitive) also calls overrideExportValue, so clear the previous live source before both arms.
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)
src/jsc/bindings/BunPlugin.cpp (1)
658-689: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate accessor failures.
Line 675 and Line 684 clear exceptions from
getOwnPropertySlot()andpropSlot.getValue(). A Proxy trap or accessor can throw. The code then replaces the export withundefinedand letsmock.module()succeed.Propagate the exception to the caller. Add a regression test with a throwing accessor.
As per coding guidelines, calls that can run user code must propagate errors and must not clear exceptions.
🤖 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 `@src/jsc/bindings/BunPlugin.cpp` around lines 658 - 689, Update the property inspection loop in the mock.module export handling around `getOwnPropertySlot()` and `PropertySlot::getValue()` so exceptions from Proxy traps or accessors are propagated immediately to the caller instead of being cleared and replaced with undefined. Remove the exception-clearing fallback branches while preserving normal export override behavior, and add a regression test using a throwing accessor to verify mock.module() fails with that exception.Source: Coding guidelines
🤖 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 `@src/jsc/bindings/BunPlugin.cpp`:
- Around line 658-689: Update the property inspection loop in the mock.module
export handling around `getOwnPropertySlot()` and `PropertySlot::getValue()` so
exceptions from Proxy traps or accessors are propagated immediately to the
caller instead of being cleared and replaced with undefined. Remove the
exception-clearing fallback branches while preserving normal export override
behavior, and add a regression test using a throwing accessor to verify
mock.module() fails with that exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e25601ef-7fd2-47f6-9bb6-b7f3b3e8ab3f
📒 Files selected for processing (3)
scripts/build/deps/webkit.tssrc/jsc/bindings/BunPlugin.cpptest/js/bun/test/mock/mock-module.test.ts
|
On the outside-diff-range comment about clearing exceptions from accessor reads in the |
|
CI status: build #87475 on
Ready for review. |
…s ESM (#37525) Builtins implemented in `src/js` are handed to ES module importers by `generateInternalModuleSourceCode` (`src/jsc/bindings/ModuleLoader.cpp`), which snapshots the builtin's CommonJS exports object into a synthetic module record. It did that with `object->get()` on every own enumerable property, so every accessor on the exports object ran at import time. Those accessors are the builtins' lazy-loading mechanism, which means they only ever helped `require()` callers: - `node:fs`: `ReadStream`, `WriteStream`, `FileReadStream`, `FileWriteStream`, `Utf8Stream` each `require("internal/fs/streams")`, which loads the whole `node:stream` stack. - `node:tls`: `rootCertificates` parses the bundled CA store, `DEFAULT_CIPHERS` queries BoringSSL. - `node:http`: `globalAgent` instantiates the agent. `node:timers` / `node:stream`: `promises` load `timers/promises` and `stream/promises`. `node:assert`: `AssertionError` loads `internal/assert/assertion_error`. `node:repl`, `node:events`, `node:buffer`, `node:os` have a few more. #35541 is about to turn more of these into accessors (`fs.promises` among them), which this path would immediately defeat for the most common import style there is. ## Repro ```js // probe.mjs, run as `bun --expose-internals probe.mjs esm` vs `... require` const mode = process.argv[2]; const { createRequire } = await import("node:module"); let t = performance.now(); if (mode === "esm") await import("node:fs"); else createRequire(import.meta.url)("node:fs"); const fsMs = performance.now() - t; t = performance.now(); createRequire(import.meta.url)("internal/fs/streams"); console.log(mode, "node:fs", fsMs.toFixed(1), "ms; internal/fs/streams afterwards", (performance.now() - t).toFixed(1), "ms"); ``` bun 1.4.0 (release): `esm node:fs 13.3 ms; internal/fs/streams afterwards 0.2 ms` (already loaded) vs `require node:fs 7.8 ms; internal/fs/streams afterwards 5.0 ms`. On a debug build the difference is ~370ms per process, and `test/harness.ts` does `import fs from "node:fs"`, so every test file paid it. The directly observable form, which the tests use: the `node:fs` getters replace themselves with data properties when they run, so after `import fs from "node:fs"`, `Object.getOwnPropertyDescriptor(fs, "ReadStream")` had a `value` instead of a `get`. ## Fix An ES module binding cannot be made lazy from the embedder side: once the record exists, named imports read the exporting environment's slot directly (`ModuleVar`) and namespace reads go through `getValue()` in `JSModuleNamespaceObject`, so the slot has to hold the value before anything binds to it. The engine half is oven-sh/WebKit#408, merged as 7b763944f0ec. `WEBKIT_VERSION` moves from 09e477744721 (what main pins) to that sha; it is the only commit between the two: - `SyntheticModuleRecord::tryCreateWithExportNamesAndValues()` gets an overload taking a source object; an export whose value is the empty `JSValue` is declared but left in TDZ. - `materializeLazyExport()` fills such a slot from `source[name]` the first time something binds to it: `CyclicModuleRecord::initializeEnvironment` when an importer links a named import that resolves to it (directly or through `export *` / `export { x } from` chains), `JSModuleNamespaceObject::getOwnPropertySlotCommon` when a namespace read finds the slot empty (it then re-reads and installs the value the same way as before, so the namespace IC still applies), and `WebAssemblyModuleRecord::initializeImports`, which snapshots imported bindings directly (unreachable in bun today, reachable once #35587 lands). It is a no-op for a slot that already has a value, so `overrideExportValue` (what `mock.module` and `spyOn` use) keeps winning, `*namespace*` is skipped, a getter that re-enters keeps the first value that landed, and an exception from the getter propagates and leaves the slot for the next attempt. The write uses the same `symbolTablePutTouchWatchpointSet` the eager path used for its one write, so IC / DFG behaviour after materialization is identical to an eagerly built record; the baseline namespace IC already bails to the slow path on an empty slot and the DFG only folds loads that went through an installed IC. - `SyntheticSourceProvider::createWithLazyExports()` takes a generator that returns the source object, and `makeModule` passes it through. Records built any other way (JSON modules, `vm.SyntheticModule`, every existing `create()` caller) have no source object and take a pointer test on the new paths. The bun half is `generateInternalModuleSourceCode`: it now looks each property up with `getOwnPropertySlot`, snapshots data properties exactly as before, declares everything else (accessors) as a lazy export, and returns the exports object as the source. A builtin without accessors (most of them) produces a record identical to the old one. `require()` of a builtin never went through this function (`fetchCommonJSModule` returns the registry object), so it is unaffected. The engine change is inert without this opt-in: building the new WebKit with the old `ModuleLoader.cpp` still fails the new tests the same way the release does. ### Why this is the right behaviour Node builds the ESM facade of a builtin by reading the getters too, so the old behaviour was not wrong, just expensive, and there is no compat reason to keep it. The only observable difference is *when* an accessor export is sampled: at import before, at the point something first binds to it now. Data properties still snapshot at import. A namespace export or named import of an accessor is a snapshot either way (that is what `module.syncBuiltinESMExports()` exists for), and the later sample is never staler than the old one. Everything an importer can do with the export resolves to the same object `require()` would hand out, which is what the tests check for each binding path. `Object.keys(ns)`, `hasOwnProperty`, spread, and `console.log(ns)` still materialize what they touch, because `[[GetOwnProperty]]` has to produce the value; `in` ([[HasProperty]]) does not. Plugin `loader: "object"` modules and user CommonJS imported from ESM are deliberately not touched: those expose user objects whose getter timing is user-visible (#36677 is about that), whereas builtins' accessors are our own lazy-loading idiom. ## Tests `test/js/bun/resolve/builtin-esm-lazy-exports.test.ts`, next to the other module-system behaviour tests in that directory. Each case runs in its own process (a binding can only be materialized once) and checks both that importing left the `node:fs` accessors untouched and that the binding is the real class once used, for: default + named data import, named import of an accessor (exactly that one materializes), `import * as`, `import defer * as`, dynamic `import()` including `Object.keys` and that the export list is unchanged, `export *` and `export { x as y } from` re-exports (named import through the star and a read off the re-exporter's namespace, which exercises materializing on a record other than the namespace's own), accessors on `node:assert` (a function exports object), `node:timers` and `node:stream`, and `spyOn` / `mock.module` on an already imported builtin. 7 of the 8 cases fail on the current eager code (everything reports `value`); the `assert`/`timers`/`stream` identity case is a regression guard. With the fix, the probe above reports `internal/fs/streams` as not loaded after `import("node:fs")` (debug build: 338ms to load it afterwards in both modes, versus 10ms before because the import had already loaded it). Also run on the new build: `test/js/node/fs/fs.test.ts` (covers the existing `export { ReadStream, WriteStream } from "node:fs"` and `export * from "node:fs"` fixtures), the `test/js/bun/resolve` module tests, `test/js/bun/test/mock/mock-module*.test.ts`, `node-module-module.test.js`, `buffer-inspectmaxbytes.test.ts` (which pins that a named import of an accessor is a snapshot), and the events / os / timers / assert / tls-internals / node-http suites, all green apart from two failures that are identical on the released binary in this environment (`os.userInfo`, the http proxy test). <!-- robobun:evidence:begin --> --- **[decide:webkit]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 7 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/resolve/builtin-esm-lazy-exports.test.ts bun test v1.4.0 (435641a) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: 132 | afterKeys: kinds(), 133 | keysMatch: namespaceKeys.sort().join() === [...exportsKeys, "default"].sort().join(), 134 | allAreTheClasses: STREAMS.every(name => typeof ns[name] === "function" && ns[name] === fs[name]), 135 | }); 136 | `); 137 | expect(result).toEqual({ ^ error: expect(received).toEqual(expected) { "afterImport": { - "FileReadStream": "accessor", - "FileWriteStream": "accessor", - "ReadStream": "accessor", - "Utf8Stream": "accessor", - "WriteStream": "accessor", + "FileReadStream": "value", + "FileWriteStream": "value", + "ReadStream": "value", + "Utf8Stream": "value", + "WriteStream": "value", }, "afterKeys": { "FileReadStream": "value", "FileWriteStream": "value", "ReadStream": "value", "Utf8Stream": "value", "WriteStream": "value", }, "after ... (truncated) release without fix: 7 FAILED bun test v1.4.0-canary.1 (9008ae7) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: 68 | const result = await runEntry(` 69 | import { ReadStream } from "node:fs"; 70 | import { fs, kinds, print } from "./helper.mjs"; 71 | print({ kinds: kinds(), isTheClass: typeof ReadStream === "function" && ReadStream === fs.ReadStream }); 72 | `); 73 | expect(result).toEqual({ kinds: kinds("ReadStream"), isTheClass: true }); ^ error: expect(received).toEqual(expected) { "isTheClass": true, "kinds": { - "FileReadStream": "accessor", - "FileWriteStream": "accessor", + "FileReadStream": "value", + "FileWriteStream": "value", "ReadStream": "value", - "Utf8Stream": "accessor", - "WriteStream": "accessor", + "Utf8Stream": "value", + "WriteStream": "value", }, } - Expected - 4 + Received + 4 at <anonymous> (/workspace/bun/test/js/bun/resolve/builtin-esm-lazy-exports.test.ts:73:18) 89 | afterRead: kinds(), 90 | isTheClass: typeof WriteStream === "function" && WriteStream === fs.WriteStream, 91 | secondReadIsStable: ns.WriteStream === WriteStream, 92 | }) ... (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/mechgate.xml" test/js/bun/resolve/builtin-esm-lazy-exports.test.ts bun test v1.4.0 (435641a) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: (pass) importing the module does not run its accessors [1746.65ms] (pass) a named import materializes exactly the binding it links [2145.14ms] (pass) a namespace export materializes when it is read, not on import or `in` [2175.43ms] (pass) a deferred namespace (import defer) materializes on read as well [2720.80ms] (pass) import() namespace: same export list as before, enumerating it materializes everything [2873.44ms] (pass) re-exports bind through to the builtin's own binding [2232.58ms] (pass) spyOn and mock.module on an imported builtin [2125.50ms] (pass) accessors on other builtins bind to what require() returns [2735.47ms] 8 pass 0 fail 24 expect() calls Ran 8 tests across 1 file. [8.97s] __F:0:S:0 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 435641a features baseline 22 deps, 107 codegen, 1176 objects in 1540ms ninja: Entering directory `/workspace/bun/build/release' [1/1238] gen ErrorCode+*.h [2/1238] gen bindgenv2 [3/1238] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [4/1238] fetch zlib [zlib] up to date [5/1238] fetch libjpeg-turbo [libjpeg-turbo] up to date [6/1238] fetch tinycc [tinycc] up to date [7/1237] fetch picohttpparser [picohttpparser] up to date [8/1237] gen .bind.ts → GeneratedBindings.cpp [9/1237] gen ProcessBindingBuffer.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingBuffer.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingBuffer.cpp [10/1237] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp [11/1237] gen ProcessBindingHTTPParser.lut.h Generating /workspac ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/build/deps/webkit.ts | 2 +- src/jsc/bindings/ModuleLoader.cpp | 28 ++- .../bun/resolve/builtin-esm-lazy-exports.test.ts | 232 +++++++++++++++++++++ 3 files changed, 254 insertions(+), 8 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/build/deps/webkit.ts 3 3 0 src/jsc/bindings/ModuleLoader.cpp 1 3 0 test/js/bun/resolve/builtin-esm-lazy-exports.test.ts 0 0 0 ``` </details> <!-- robobun:evidence:end -->
|
Closing this PR because it has been inactive for more than 90 days. |
|
#44833 supersedes this PR. It landed live exports for module mocks on top of oven-sh/WebKit#781. I checked main at 2b0ace8 with a local debug build (plus one missing
oven-sh/WebKit#380 is closed too. |
Fixes #9874. Fixes #7128.
Repro
Cause
generateObjectModuleSourceCodecallsobject->get(globalObject, entry)for each own property of the factory result and appends the value to the synthetic module'sexportValues. For an accessor that evaluates the getter once;SyntheticModuleRecord::tryCreateWithExportNamesAndValuesthen stores the result in aJSModuleEnvironmentslot, andJSModuleNamespaceObject::getOwnPropertySlotCommonreads the slot directly on every subsequent access.fetchCommonJSModuleroutes therequire()path through the same code, so bothawait import()andrequire()observe a snapshot. The pluginbuild.module/onLoadloader: "object"path (#7128) sharesgenerateObjectModuleSourceCode, so it has the same limitation.Fix
require(): when the virtual module came from
mock.module,handleVirtualModuleResultassigns the factory object asmodule.exportsdirectly (matching whatJSMock__jsModuleMockalready does for a re-mock of an already-required module). Property access goes through the original getters. A capturedrequire()result therefore does not observe a later re-mock (Node/Jest semantics); a freshrequire()does.await import():
generateObjectModuleSourceCodestill snapshots each property into the module environment (so staticimport { x }bindings, which every tier reads as a rawvariableAt(offset)load, see a plain value), but when any own property is an accessor it also appends the factory object as a trailingexportValuewith no matchingexportName. On the WebKit side (oven-sh/WebKit#380)SyntheticModuleRecord::tryCreateWithExportNamesAndValuespicks that up asm_liveExportsSource, andJSModuleNamespaceObject::getOwnPropertySlotCommonforwards namespace reads through it via an uncacheableslot.setValuefor properties the source actually owns (others fall back to the env slot, so partial re-mocks keep un-overridden keys). BecausesetValueModuleNamespaceis never called for those properties, the JIT'sModuleNamespaceAccessCaseIC (and the DFG/FTL lowering toGetClosureVarthat hangs off it) is never installed.re-mock / spyOn:
JSModuleNamespaceObject::overrideExportValuealso writes through to the backing object when one is set, sospyOn(ns, key)andmock.restore()are observed by namespace reads; andJSMock__jsModuleMockswaps the backing object to the new factory on re-mock when the new factory has an accessor or a source was already set (so a data-only partial re-mock of a builtin keeps the module-namespace IC).Verification
New tests in
test/js/bun/test/mock/mock-module.test.ts(ESM + CJS live bindings, mixed data/accessor, re-mock of an accessor-backed namespace,spyOnon one, partial re-mock of data-only and accessor-backed, captured-require contract) andtest/js/bun/plugin/plugins.test.ts(loader: "object"accessor export):The existing
mock.moduletests (6874/,6879/,mock-module-*) andtest-vm-module-synthetic.jsare unchanged.Also in this WebKit range (34c01d1339..)
USE(BUN_JSC_ADDITIONS), defaults off)Dockerfile.windowsxwin hardeningNote
WEBKIT_VERSIONis pinned to the preview tagautobuild-preview-pr-380-1b4afe55while oven-sh/WebKit#380 is open. It must be swapped to the mergedmainsha before this lands.[decide:webkit] gate passed · iteration 2 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 0 rejected · iteration 2
evidence per changed file