Merge remote-tracking branch 'origin/ni/i10-js-island-requires' into ni/i11-cross-heap-gc

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";

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);

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;

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 });

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" }

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" } } }

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);

tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts

@@ -0,0 +1 @@
+export declare const config: { mode: string };

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 });

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" }

tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts

@@ -0,0 +1 @@
+export declare const ok: boolean;

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;

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" } }

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" } } } }

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);