fix(ses): cyclic star export with renaming reexport (issue #59) - refresh for #3276 feedback

.changeset/fix-ses-star-export-cycle-rename.md

@@ -0,0 +1,7 @@
+---
+'ses': patch
+---
+
+Fix a star-export cycle defect where a module reached more than once via `export *` and a renaming reexport with a different exported name (`export { y as x } from ...`) raised a spurious `SyntaxError: ... does not provide an export named 'X'` (latterly `TypeError: notify is not a function`).
+The reexport wire-up now installs a deferred forwarding notifier that resolves through the upstream's notifier table on first subscription, so cyclic star-export fixed-points converge.
+Resolves endojs/endo#59.

packages/compartment-mapper/test/_cycle-cjs-reexporter-assertions.js

@@ -0,0 +1,55 @@
+/**
+ * Shared assertion logic for the cyclic CommonJS reexporter scenario.
+ * Both the Node.js parity test and the Compartment Mapper test import from
+ * this module so the expected values live in exactly one place. If both
+ * tests pass, parity with Node.js is verified by construction.
+ *
+ * The fixture under fixtures-cycle-cjs-reexporter/node_modules/app/ exercises
+ * this arrangement, all three modules being CommonJS:
+ *
+ *   star-reexporter.cjs: Object.assign(exports, require('./export-renamer.cjs'));
+ *   export-renamer.cjs:  require('./star-reexporter.cjs');
+ *                        Object.defineProperty(exports, 'x', {
+ *                          get() { return module.exports.y; }, enumerable: true });
+ *                        exports.y = 45;
+ *   main.js:             const reexp = require('./star-reexporter.cjs');
+ *                        const ren = require('./export-renamer.cjs');
+ *                        exports.captured = reexp.x;
+ *                        exports.namespace1 = { x: reexp.x, y: reexp.y };
+ *                        exports.namespace2 = { x: ren.x, y: ren.y };
+ *
+ * In a pure-CommonJS cycle, the reexporter's `Object.assign` reads the
+ * renamer's `x` getter after the renamer has set `y = 45`, so the copied
+ * value is 45. Both namespaces project { x: 45, y: 45 }. Node.js and the
+ * compartment mapper agree on this shape, so the same assertions apply to
+ * both layers.
+ *
+ * The companion divergence scenario (ESM module participating in a cycle
+ * with a CommonJS module) is exercised by fixtures-cycle-esm-in-cjs and
+ * its tests; Node.js rejects that topology with ERR_REQUIRE_CYCLE_MODULE
+ * while SES allows it.
+ *
+ * @module
+ */
+
+/** @import {ExecutionContext} from 'ava' */
+
+export const expectedCaptured = 45;
+export const expectedNamespace1 = { x: 45, y: 45 };
+export const expectedNamespace2 = { x: 45, y: 45 };
+
+/**
+ * @param {ExecutionContext} t
+ * @param {object} namespace
+ */
+export const assertCycleCjsReexporter = (t, namespace) => {
+  t.is(namespace.captured, expectedCaptured);
+  t.deepEqual(
+    { x: namespace.namespace1.x, y: namespace.namespace1.y },
+    expectedNamespace1,
+  );
+  t.deepEqual(
+    { x: namespace.namespace2.x, y: namespace.namespace2.y },
+    expectedNamespace2,
+  );
+};

packages/compartment-mapper/test/_cycle-rename-assertions.js

@@ -0,0 +1,45 @@
+/**
+ * Shared assertion logic for the cyclic star-export with renaming reexport
+ * regression (endojs/endo#59). Both the Node.js parity test and the
+ * Compartment Mapper test import from this module so the expected values
+ * live in exactly one place. If both tests pass, parity with Node.js is
+ * verified by construction.
+ *
+ * The fixture under fixtures-cycle-rename/node_modules/app/ exercises this
+ * arrangement:
+ *
+ *   star-reexporter.js: export * from './export-renamer.js';
+ *   export-renamer.js:  export { y as x } from './star-reexporter.js';
+ *                       export var y = 45;
+ *   main.js:            import { x } from './star-reexporter.js';
+ *                       import * as ns1 from './star-reexporter.js';
+ *                       import * as ns2 from './export-renamer.js';
+ *                       export const captured = x;
+ *                       export const namespace1 = { x: ns1.x, y: ns1.y };
+ *                       export const namespace2 = { x: ns2.x, y: ns2.y };
+ *
+ * Before the fix, the SES linker visited star-reexporter while its
+ * star-imported notifier for `y` had not yet been wired. The synchronous
+ * wireUp at the cycle's back-edge then passed `undefined` as the upstream
+ * notifier, manifesting as `TypeError: notify is not a function`. Node.js
+ * does not exhibit the defect, so the parity test pinned both layers to a
+ * single expected shape.
+ *
+ * @module
+ */
+
+/** @import {ExecutionContext} from 'ava' */
+
+export const expectedCaptured = 45;
+export const expectedNamespace1 = { x: 45, y: 45 };
+export const expectedNamespace2 = { x: 45, y: 45 };
+
+/**
+ * @param {ExecutionContext} t
+ * @param {object} namespace
+ */
+export const assertCycleRename = (t, namespace) => {
+  t.is(namespace.captured, expectedCaptured);
+  t.deepEqual(namespace.namespace1, expectedNamespace1);
+  t.deepEqual(namespace.namespace2, expectedNamespace2);
+};

packages/compartment-mapper/test/_cycle-rename-tdz-assertions.js

@@ -0,0 +1,106 @@
+/**
+ * Shared assertion logic for the TDZ-observation matrix of the cyclic
+ * star-export and named-reexport scenarios from endojs/endo#59. Each fixture
+ * exercises one cell of the matrix and exports a `probe` value captured by
+ * the star-reexporter (or named-reexporter) during its top-level evaluation:
+ * the probe reads the renamer's binding `y` through a namespace import (`r.y`)
+ * inside a try block and records either the value (when the binding is
+ * already initialized) or the error name (when the read raises during the
+ * temporal dead zone window).
+ *
+ * The matrix axes are:
+ *
+ *   1. Which module main.js imports first (the export-renamer or the
+ *      star-reexporter or the named-reexporter). The first-imported module
+ *      starts evaluating first; depth-first traversal of the cycle then
+ *      determines which module's body runs while the other is on the
+ *      evaluation stack with bindings not yet initialized.
+ *   2. The renamer's binding form for `y` (`const`, `let`, or `var`). Under
+ *      ECMA-262 semantics, `const` and `let` create a binding that is in
+ *      the temporal dead zone until its declaration is evaluated, so a read
+ *      raises ReferenceError; `var` is hoisted and reads `undefined` until
+ *      the assignment runs.
+ *   3. Whether the upstream side of the cycle is reached through `export *`
+ *      (the six star-reexport cells) or through `export { y } from` (the
+ *      single named-reexport cell).
+ *
+ * After the TDZ-enforcement fix landed on endojs/endo-but-for-bots#379
+ * (commit 94c88465d, plus the named-reexport coverage in 53d8662a7), SES
+ * enforces the same TDZ on
+ * the cross-module namespace path that Node.js enforces natively. Each
+ * fixture's parity test pins the compartment mapper's behavior to Node.js's
+ * reference behavior by importing from this module so the expected values
+ * live in exactly one place; if both tests pass, parity is verified by
+ * construction.
+ *
+ * The companion in-process scenarios live in
+ * `packages/ses/test/import-gauntlet.test.js` as the seven matrix cells
+ * starting at "cyclic star export with renaming reexport, renamer imported
+ * first, const binding observes ReferenceError during temporal dead zone".
+ *
+ * @module
+ */
+
+/** @import {ExecutionContext} from 'ava' */
+
+// Renamer imported first: depth-first traversal evaluates the
+// star-reexporter's body while the renamer is on the evaluation stack with
+// its binding `y` not yet initialized. Under ECMA-262, `const` and `let`
+// raise ReferenceError on read; `var` reads undefined (the hoisting
+// preamble clears the upstream TDZ before the downstream observes).
+
+export const expectedProbeStarConstRenamerFirst = {
+  kind: 'error',
+  name: 'ReferenceError',
+};
+
+export const expectedProbeStarLetRenamerFirst = {
+  kind: 'error',
+  name: 'ReferenceError',
+};
+
+export const expectedProbeStarVarRenamerFirst = {
+  kind: 'value',
+  value: undefined,
+};
+
+// Star-reexporter imported first: depth-first cycle resolution evaluates
+// the renamer's body to completion before the star-reexporter's body runs,
+// so the probe captures the assigned value for every binding form. The
+// "star reexporter imported first" cases therefore have no TDZ window to
+// observe; they are the expected non-observation that completes the matrix.
+
+export const expectedProbeStarConstStarFirst = {
+  kind: 'value',
+  value: 42,
+};
+
+export const expectedProbeStarLetStarFirst = {
+  kind: 'value',
+  value: 42,
+};
+
+export const expectedProbeStarVarStarFirst = {
+  kind: 'value',
+  value: 42,
+};
+
+// Named-reexport variant with renamer imported first: the cycle is reached
+// through `export { y } from` rather than `export *`. The TDZ semantics
+// live with the binding, not with the reexport form, so the const cell
+// raises ReferenceError just as it does for the star-reexport cell. This
+// confirms the gap is not specific to `export *`.
+
+export const expectedProbeNamedConstRenamerFirst = {
+  kind: 'error',
+  name: 'ReferenceError',
+};
+
+/**
+ * @param {ExecutionContext} t
+ * @param {object} namespace
+ * @param {object} expectedProbe
+ */
+export const assertCycleRenameTdz = (t, namespace, expectedProbe) => {
+  t.deepEqual(namespace.probe, expectedProbe);
+};

packages/compartment-mapper/test/_cycle-rename-unused-assertions.js

@@ -0,0 +1,48 @@
+/**
+ * Shared assertion logic for the unused-live-binding shape of the cyclic
+ * star-export with renaming reexport regression (endojs/endo#59). This is
+ * the companion to the populated shape exercised by
+ * `_cycle-rename-assertions.js`; the only difference is that the renamer's
+ * `export var y` here has no initializer, so the live binding is declared
+ * but never updated. Every projection of the cycle therefore reads
+ * `undefined`. Both the Node.js parity test and the Compartment Mapper test
+ * import from this module so the expected values live in exactly one place;
+ * if both tests pass, parity with Node.js is verified by construction.
+ *
+ * The fixture under fixtures-cycle-rename-unused/node_modules/app/ exercises
+ * this arrangement:
+ *
+ *   star-reexporter.js: export * from './export-renamer.js';
+ *   export-renamer.js:  export { y as x } from './star-reexporter.js';
+ *                       export var y;
+ *   main.js:            import { x } from './star-reexporter.js';
+ *                       import * as ns1 from './star-reexporter.js';
+ *                       import * as ns2 from './export-renamer.js';
+ *                       export const captured = x;
+ *                       export const namespace1 = { x: ns1.x, y: ns1.y };
+ *                       export const namespace2 = { x: ns2.x, y: ns2.y };
+ *
+ * The deferring closure introduced by the issue #59 fix queues subscribers
+ * until the upstream notifier resolves, then forwards them. With no
+ * initializer the upstream's value never updates, so every read is
+ * `undefined`. Node.js exhibits the same shape, so the parity test pins
+ * both layers to a single expected projection.
+ *
+ * @module
+ */
+
+/** @import {ExecutionContext} from 'ava' */
+
+export const expectedCaptured = undefined;
+export const expectedNamespace1 = { x: undefined, y: undefined };
+export const expectedNamespace2 = { x: undefined, y: undefined };
+
+/**
+ * @param {ExecutionContext} t
+ * @param {object} namespace
+ */
+export const assertCycleRenameUnused = (t, namespace) => {
+  t.is(namespace.captured, expectedCaptured);
+  t.deepEqual(namespace.namespace1, expectedNamespace1);
+  t.deepEqual(namespace.namespace2, expectedNamespace2);
+};

packages/compartment-mapper/test/cycle-cjs-reexporter-node-parity.test.js

@@ -0,0 +1,25 @@
+/**
+ * Node.js parity test for the cyclic CommonJS reexporter scenario. This
+ * test runs the same three-module pure-CommonJS fixture under plain Node.js
+ * (no SES, no compartment mapper) and asserts the same expected values
+ * asserted in cycle-cjs-reexporter.test.js. Parity is verified by
+ * construction: if both tests pass, the compartment mapper's CommonJS
+ * cycle behavior matches Node.js for this case.
+ */
+
+import test from 'ava';
+import { assertCycleCjsReexporter } from './_cycle-cjs-reexporter-assertions.js';
+
+test('cyclic CommonJS reexporter - node parity', async t => {
+  t.plan(3);
+  // Dynamic ESM import of a CommonJS module: Node exposes the module's
+  // module.exports as the namespace's default export. Re-use the shared
+  // assertion module by projecting through `default`.
+  const moduleNamespace = await import(
+    new URL(
+      'fixtures-cycle-cjs-reexporter/node_modules/app/main.js',
+      import.meta.url,
+    ).href
+  );
+  assertCycleCjsReexporter(t, moduleNamespace.default);
+});

packages/compartment-mapper/test/cycle-cjs-reexporter.test.js

@@ -0,0 +1,43 @@
+/**
+ * Cyclic CommonJS reexporter scenario exercised through the
+ * compartment-mapper test scaffold. The companion Node.js parity test in
+ * cycle-cjs-reexporter-node-parity.test.js imports the same fixture under
+ * Node.js and asserts the same expected values; together the two tests
+ * teach the compartment mapper's CommonJS cycle behavior and pin it to
+ * Node.js's reference behavior.
+ *
+ * This is the pure-CommonJS counterpart to the ESM-in-CJS-cycle divergence
+ * exercised by cycle-esm-in-cjs.test.js and
+ * cycle-esm-in-cjs-node-parity.test.js, where Node.js rejects the topology
+ * with ERR_REQUIRE_CYCLE_MODULE but SES allows it.
+ */
+
+/** @import {ExecutionContext} from 'ava' */
+
+import 'ses';
+import test from 'ava';
+import { scaffold } from './scaffold.js';
+import { assertCycleCjsReexporter } from './_cycle-cjs-reexporter-assertions.js';
+
+const fixture = new URL(
+  'fixtures-cycle-cjs-reexporter/node_modules/app/main.js',
+  import.meta.url,
+).toString();
+
+const fixtureAssertionCount = 3;
+
+/**
+ * @param {ExecutionContext} t
+ * @param {{namespace: object}} result
+ */
+const assertFixture = (t, { namespace }) => {
+  assertCycleCjsReexporter(t, namespace);
+};
+
+scaffold(
+  'cycle-cjs-reexporter (issue #59 follow-up)',
+  test,
+  fixture,
+  assertFixture,
+  fixtureAssertionCount,
+);

packages/compartment-mapper/test/cycle-esm-in-cjs-node-parity.test.js

@@ -0,0 +1,40 @@
+/**
+ * Node.js parity test for the ESM-in-CommonJS-cycle divergence scenario.
+ * This test runs the same fixture under plain Node.js (no SES, no
+ * compartment mapper) and asserts that Node.js rejects the topology with
+ * ERR_REQUIRE_CYCLE_MODULE. The companion compartment-mapper / SES test
+ * in cycle-esm-in-cjs.test.js asserts the divergent behavior: SES allows
+ * the same fixture to load and exposes the cycle's snapshot / live-binding
+ * shape on the namespace. Together the two tests verify the divergence
+ * programmatically rather than narratively.
+ */
+
+import test from 'ava';
+import process from 'process';
+import { spawnSync } from 'child_process';
+import { fileURLToPath } from 'url';
+
+test('ESM-in-CJS-cycle - node parity (rejects with ERR_REQUIRE_CYCLE_MODULE)', t => {
+  t.plan(2);
+  const fixture = new URL(
+    'fixtures-cycle-esm-in-cjs/node_modules/app/main.mjs',
+    import.meta.url,
+  );
+  // Spawn a fresh Node process to execute the fixture. The expected outcome
+  // is a non-zero exit with the ERR_REQUIRE_CYCLE_MODULE error code printed
+  // on stderr. Spawning isolates the failure from the test runner's own
+  // module graph and keeps the rest of the suite running.
+  const result = spawnSync(process.execPath, [fileURLToPath(fixture)], {
+    encoding: 'utf8',
+  });
+  t.not(
+    result.status,
+    0,
+    `Expected Node to reject ESM-in-CJS-cycle, got exit ${result.status}`,
+  );
+  t.regex(
+    result.stderr,
+    /ERR_REQUIRE_CYCLE_MODULE/,
+    `Expected ERR_REQUIRE_CYCLE_MODULE in stderr, got:\n${result.stderr}`,
+  );
+});