From d06155048e2a8e221b374391f8a9f23f37c50ae2 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 27 Oct 2026 19:06:41 -0700 Subject: [PATCH] Only bare binding getters are pure; compare getter exports for mutation A member read in an export getter (a.b or a[b]) can invoke another accessor that runs code on every read, so the recorder now accepts only a bare identifier as a plain binding getter. Everything else keeps its runtime read and is not snapshotted. The mutation check that makes an earlier island module eager copied only data properties of a module value, so an object exported through a binding getter could stay stale when a later load changed it. The shape now reads binding getters the same way the snapshot does, and compares them by reading again, so those modules no longer defer. Corpus fixtures pin both: a getter that reads a member counts every read, and a later load that mutates a getter-exported object keeps both modules eager. Co-Authored-By: Claude Opus 5.5 --- .../src/node-islands/probe-recorder.ts | 36 ++++++++++++------- .../deferral/getter-member/main.ts | 3 ++ .../node_modules/counter-pkg/index.d.ts | 2 ++ .../node_modules/counter-pkg/index.js | 7 ++++ .../node_modules/counter-pkg/package.json | 1 + .../deferral/getter-member/package.json | 1 + .../deferral/mutate-getter/main.ts | 4 +++ .../node_modules/pkg-a/index.d.ts | 1 + .../mutate-getter/node_modules/pkg-a/index.js | 4 +++ .../node_modules/pkg-a/package.json | 1 + .../node_modules/pkg-b/index.d.ts | 1 + .../mutate-getter/node_modules/pkg-b/index.js | 4 +++ .../node_modules/pkg-b/package.json | 1 + .../deferral/mutate-getter/package.json | 1 + tests/harness/node-island-deferral.test.ts | 16 +++++++++ 15 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 tests/fixtures/node-island/deferral/getter-member/main.ts create mode 100644 tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts create mode 100644 tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js create mode 100644 tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json create mode 100644 tests/fixtures/node-island/deferral/getter-member/package.json create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/main.ts create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json create mode 100644 tests/fixtures/node-island/deferral/mutate-getter/package.json diff --git a/packages/compiler/src/node-islands/probe-recorder.ts b/packages/compiler/src/node-islands/probe-recorder.ts index 1299c183f..499de3593 100644 --- a/packages/compiler/src/node-islands/probe-recorder.ts +++ b/packages/compiler/src/node-islands/probe-recorder.ts @@ -80,9 +80,11 @@ function internalCaller() { const fnToString = Function.prototype.toString; const hasOwn = Object.prototype.hasOwnProperty; // A getter that only reads a binding (esbuild's and tsc's export getters): -// reading it again reads the same binding. Any other getter can run code -// on every read, so its member is not snapshotted. -const BINDING_GETTER = /^(?:\(\)\s*=>\s*[\w$.]+|function\s*[\w$]*\s*\(\)\s*\{\s*return\s+[\w$.]+(?:\[[\w$]+\])?\s*;?\s*\}|get\s+[\w$]+\s*\(\)\s*\{\s*return\s+[\w$.]+\s*;?\s*\})$/; +// reading it again reads the same binding. A member read ('a.b', 'a[b]') +// can invoke another accessor that runs code on every read, so only a bare +// identifier qualifies. Any other getter can also run code on every read, +// so its member is not snapshotted. +const BINDING_GETTER = /^(?:\(\)\s*=>\s*[A-Za-z_$][\w$]*|function\s*[\w$]*\s*\(\)\s*\{\s*return\s+[A-Za-z_$][\w$]*\s*;?\s*\}|get\s+[\w$]+\s*\(\)\s*\{\s*return\s+[A-Za-z_$][\w$]*\s*;?\s*\})$/; function bindingGetter(fn) { let source = ""; try { source = fnToString.call(fn); } catch { return false; } @@ -463,20 +465,31 @@ exports.probe = function probe(input) { return [name, m]; }); + // What a member of a module value reads now: a data property's value, or + // a binding getter's (esbuild's and tsc's export getters). Any other + // accessor can run code, so it is not part of a shape. + const MISSING = {}; + const memberValue = (o, k) => { + const d = Object.getOwnPropertyDescriptor(o, k); + if (d === undefined) return MISSING; + if ("value" in d) return d.value; + if (typeof d.get !== "function" || !bindingGetter(d.get)) return MISSING; + try { return d.get.call(o); } catch { return MISSING; } + }; + // A module's exported values right after its load, at the depth the // snapshot covers (the module value's members, and theirs for objects). const exportShape = (v) => { const shape = []; if (v === null || (typeof v !== "object" && typeof v !== "function")) return shape; for (const k of Reflect.ownKeys(v)) { - const d = Object.getOwnPropertyDescriptor(v, k); - if (d === undefined || !("value" in d)) continue; - shape.push([v, k, d.value]); - const m = d.value; + const m = memberValue(v, k); + if (m === MISSING) continue; + shape.push([v, k, m]); if (m !== null && typeof m === "object") for (const k2 of Reflect.ownKeys(m)) { - const d2 = Object.getOwnPropertyDescriptor(m, k2); - if (d2 !== undefined && "value" in d2) shape.push([m, k2, d2.value]); + const m2 = memberValue(m, k2); + if (m2 !== MISSING) shape.push([m, k2, m2]); } } return shape; @@ -537,10 +550,7 @@ exports.probe = function probe(input) { // earlier snapshot no longer holds, and this load's change has to happen // where the program imports it). for (const [earlier, shape] of shapes) { - const changed = shape.some(([o, k, v]) => { - const d = Object.getOwnPropertyDescriptor(o, k); - return d === undefined || !("value" in d) || !Object.is(d.value, v); - }); + const changed = shape.some(([o, k, v]) => !Object.is(memberValue(o, k), v)); if (!changed) continue; delete earlier.root; earlier.reason = "a later island module's load changes its exports"; diff --git a/tests/fixtures/node-island/deferral/getter-member/main.ts b/tests/fixtures/node-island/deferral/getter-member/main.ts new file mode 100644 index 000000000..96223673c --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/main.ts @@ -0,0 +1,3 @@ +import counter from "counter-pkg"; + +console.log(counter.label, counter.count, counter.count); diff --git a/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts new file mode 100644 index 000000000..82afbe16d --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts @@ -0,0 +1,2 @@ +declare const counter: { readonly count: number; readonly label: string }; +export default counter; diff --git a/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js new file mode 100644 index 000000000..8ad752913 --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js @@ -0,0 +1,7 @@ +"use strict"; +let reads = 0; +// The export getter reads a member, not a binding: that member's getter +// counts the read, so it is not a plain export getter. +const state = { get count() { return ++reads; } }; +exports.label = "counter"; +Object.defineProperty(exports, "count", { enumerable: true, get: () => state.count }); diff --git a/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json new file mode 100644 index 000000000..50ce07f31 --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json @@ -0,0 +1 @@ +{ "name": "counter-pkg", "version": "1.0.0", "main": "index.js", "types": "index.d.ts" } diff --git a/tests/fixtures/node-island/deferral/getter-member/package.json b/tests/fixtures/node-island/deferral/getter-member/package.json new file mode 100644 index 000000000..6d5d7c46b --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/package.json @@ -0,0 +1 @@ +{ "name": "deferral-getter-member", "private": true, "type": "module", "scriptc": { "packages": { "counter-pkg": "node" } } } diff --git a/tests/fixtures/node-island/deferral/mutate-getter/main.ts b/tests/fixtures/node-island/deferral/mutate-getter/main.ts new file mode 100644 index 000000000..f7dbae8e2 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/main.ts @@ -0,0 +1,4 @@ +import { config } from "pkg-a"; +import { ok } from "pkg-b"; + +console.log(config.mode, ok); diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts new file mode 100644 index 000000000..8b403d629 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts @@ -0,0 +1 @@ +export declare const config: { mode: string }; diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js new file mode 100644 index 000000000..c46d72b3a --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js @@ -0,0 +1,4 @@ +"use strict"; +// pkg-a exports through a binding getter over its own object. +const config = { mode: "initial" }; +Object.defineProperty(exports, "config", { enumerable: true, get: () => config }); diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json new file mode 100644 index 000000000..e0270f674 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json @@ -0,0 +1 @@ +{ "name": "pkg-a", "version": "1.0.0", "main": "index.js", "types": "index.d.ts" } diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts new file mode 100644 index 000000000..f29ace298 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts @@ -0,0 +1 @@ +export declare const ok: boolean; diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js new file mode 100644 index 000000000..749c51d3c --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js @@ -0,0 +1,4 @@ +"use strict"; +// Its load changes what pkg-a exports, through pkg-a's binding getter. +require("pkg-a").config.mode = "set by pkg-b"; +exports.ok = true; diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json new file mode 100644 index 000000000..ea2784527 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json @@ -0,0 +1 @@ +{ "name": "pkg-b", "version": "1.0.0", "main": "index.js", "types": "index.d.ts", "dependencies": { "pkg-a": "1.0.0" } } diff --git a/tests/fixtures/node-island/deferral/mutate-getter/package.json b/tests/fixtures/node-island/deferral/mutate-getter/package.json new file mode 100644 index 000000000..0416d97bc --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/package.json @@ -0,0 +1 @@ +{ "name": "deferral-mutate-getter", "private": true, "type": "module", "scriptc": { "packages": { "pkg-a": "node", "pkg-b": { "run": "node", "evaluate": "lazy" } } } } diff --git a/tests/harness/node-island-deferral.test.ts b/tests/harness/node-island-deferral.test.ts index c17b847fc..93dce44cb 100644 --- a/tests/harness/node-island-deferral.test.ts +++ b/tests/harness/node-island-deferral.test.ts @@ -75,11 +75,27 @@ describe.skipIf(skip)("deferred Node island modules", () => { expect(await run(binary)).toBe("set by pkg-b true\n"); }, 300_000); + test("a load that changes an object exported through a getter defers neither", async () => { + const { binary, trace } = await build("mutate-getter"); + expect(trace.some((l) => /eager .*pkg-a.*a later island module's load changes/.test(l))).toBe( + true, + ); + expect( + trace.some((l) => /eager .*pkg-b.*changes another island module's exports/.test(l)), + ).toBe(true); + expect(await run(binary)).toBe("set by pkg-b true\n"); + }, 300_000); + test("an export getter that runs code runs on every read", async () => { const { binary } = await build("getter"); expect(await run(binary)).toBe("counter 1 2\n"); }, 300_000); + test("an export getter that reads a member runs on every read", async () => { + const { binary } = await build("getter-member"); + expect(await run(binary)).toBe("counter 1 2\n"); + }, 300_000); + test("a package file missing when the build probes keeps the module eager", async () => { const { binary, trace } = await build("missing-file"); expect(trace.some((l) => /eager .*probe-pkg.*existsSync/.test(l))).toBe(true);