fix(ses): cyclic star export with renaming reexport (issue #59) - refresh for #3276 feedback - #379
fix(ses): cyclic star export with renaming reexport (issue #59) - refresh for #3276 feedback#379kriscendobot wants to merge 24 commits into
Conversation
|
@naugtur Thanks for the careful read on upstream endojs/endo#3276. Disposition: verified, no source change. Tracing the deferring closure at Companion regression Bot mirror PR #379 carries the refresh (closed mirror #336 was force-pushed beyond GitHub's reopen tolerance). |
| } | ||
| pendingUpdaters.length = 0; | ||
| upstreamNotify(update); | ||
| }; |
There was a problem hiding this comment.
Naugtur and I agree there is a likely opportunity for refactoring, here. The notifier system would benefit from a synchronous variant on Promise.withResolvers, e.g., makeNotifierWithResolver, that can be used here and in makeVirtualModule, to reduce the likelihood that these parallel implementations will drift.
Naugtur also asked for a test that verifies similar behavior if the reexporting module is CommonJS. I would like a test that at least verifies that the behavior is consistent with Node.js in that scenario.
There was a problem hiding this comment.
Applied makeNotifierWithResolver to the wireUpExportNotifier deferred branch in this commit. The Naugtur-asked CommonJS reexporter test landed in packages/ses/test/import-cjs.test.js, pinned to Node.js parity.
For makeVirtualModuleInstance the builder's design judgment was to leave the existing pattern in place: that site is a live-cell fan-out where set is called repeatedly and every subscriber must receive every future value, while makeNotifierWithResolver is a one-shot redirect (Promise.withResolvers shape). Unifying would either need a "repeat-resolve" semantic or a current-value protocol — either direction seemed to obscure local semantics. Happy to revisit if you'd like the unification forced or want a different abstraction shape (e.g., a separate makeLiveNotifier for the fan-out site).
|
Addressed all three asks in commits 4d4953d..f89afdb (head 4d4953d):
|
…59 follow-up) Add the parity test pair kriskowal asked for on #379 (review comment 3338677487): the unused-live-binding shape of the cyclic star-export with renaming reexport (endojs/endo#59) needs a parity test substantiating the Node.js parity claim, ideally with a shared fixture as with cycle-rename parity. The companion populated-binding shape was already covered by cycle-rename.test.js and cycle-rename-node-parity.test.js through _cycle-rename-assertions.js; this commit lands the analogous trio for the unused-live-binding shape: fixtures-cycle-rename-unused/node_modules/app/ - three modules; the renamer's `export var y` has no initializer. _cycle-rename-unused-assertions.js - shared assertion module; expected projections are { x: undefined, y: undefined } and captured: undefined. cycle-rename-unused.test.js - compartment-mapper scaffold exercise. cycle-rename-unused-node-parity.test.js - Node.js exercise of the same fixture; parity is verified by construction when both pass. Also update the in-process SES regression's prose in packages/ses/test/import-gauntlet.test.js to reference the new parity pair, matching the cross-reference pattern the populated-binding test already uses for cycle-rename. Refs: endojs/endo#3276 (naugtur), #379 (kriskowal)
Audit summary: feedback completeness on endojs/endo#3276 + #379This is an audit-only sweep across naugtur's three inline asks on endojs/endo#3276 and kriskowal's five mirror-side asks on this PR. Each item is verified against the current head ( naugtur, endojs/endo#3276 inline asks
kriskowal, #379 inline asks
CIPost-rebase CI status will refresh as runs queue. The expected red category is |
Audit of naugtur's 3 inline asks on endojs/endo#3276 and kriskowal's 5 mirror-side asks on endojs/endo-but-for-bots#379. Seven of eight asks are genuinely addressed by prior commits; one ask (kriskowal #3 on import-gauntlet.test.js) had a missing shared-fixture parity pair for the unused-live-binding shape, closed with new compartment-mapper head f1a7dfb60. The eighth ask (kriskowal's gardener-meta follow-up) is queued as a message-fixer-40ac9b entry for the next gardener dispatch. Top-level PR audit comment posted at endojs/endo-but-for-bots#379-issuecomment-4609535322.
…59 follow-up) Add the parity test pair kriskowal asked for on endojs/endo-but-for-bots#379 (review comment 3338677487): the unused-live-binding shape of the cyclic star-export with renaming reexport (#59) needs a parity test substantiating the Node.js parity claim, ideally with a shared fixture as with cycle-rename parity. The companion populated-binding shape was already covered by cycle-rename.test.js and cycle-rename-node-parity.test.js through _cycle-rename-assertions.js; this commit lands the analogous trio for the unused-live-binding shape: fixtures-cycle-rename-unused/node_modules/app/ - three modules; the renamer's `export var y` has no initializer. _cycle-rename-unused-assertions.js - shared assertion module; expected projections are { x: undefined, y: undefined } and captured: undefined. cycle-rename-unused.test.js - compartment-mapper scaffold exercise. cycle-rename-unused-node-parity.test.js - Node.js exercise of the same fixture; parity is verified by construction when both pass. Also update the in-process SES regression's prose in packages/ses/test/import-gauntlet.test.js to reference the new parity pair, matching the cross-reference pattern the populated-binding test already uses for cycle-rename.
|
Mirror of endojs/endo#3276 (head e3f111d19). |
…59 follow-up) Add the parity test pair kriskowal asked for on endojs/endo-but-for-bots#379 (review comment 3338677487): the unused-live-binding shape of the cyclic star-export with renaming reexport (#59) needs a parity test substantiating the Node.js parity claim, ideally with a shared fixture as with cycle-rename parity. The companion populated-binding shape was already covered by cycle-rename.test.js and cycle-rename-node-parity.test.js through _cycle-rename-assertions.js; this commit lands the analogous trio for the unused-live-binding shape: fixtures-cycle-rename-unused/node_modules/app/ - three modules; the renamer's `export var y` has no initializer. _cycle-rename-unused-assertions.js - shared assertion module; expected projections are { x: undefined, y: undefined } and captured: undefined. cycle-rename-unused.test.js - compartment-mapper scaffold exercise. cycle-rename-unused-node-parity.test.js - Node.js exercise of the same fixture; parity is verified by construction when both pass. Also update the in-process SES regression's prose in packages/ses/test/import-gauntlet.test.js to reference the new parity pair, matching the cross-reference pattern the populated-binding test already uses for cycle-rename.
kriskowal
left a comment
kriskowal
left a comment
There was a problem hiding this comment.
@kriscendobot Please create additional tests that vary by whether the renamer or star exporter are imported first from main.js, and also vary by whether the binding is a const, let, or var. In all cases that it is possible, attempt to observe the value after it has been linked with its cyclic module but before it has been assigned or bound. This may be an observation of an exception while the binding is temporally dead (TDZ).
|
@kriscendobot I believe I now understand ZB’s comment endojs/endo#3276 (comment): Please consider whether we can create all of the notifiers for a module instance at time of construction instead of time of link. This may require a more deliberate separation of the instantiation and link phases, but I suspect we already have a hard enough line. This would obviate deferred notifier linkage, since notifiers would always be available up front. If not, please explain why such an arrangement is not possible or how the calling convention for precompiled ModuleSource instances would need to change. It might be that the notifiers need to be partially applied before full initialization. Please also write a test that demonstrates the same failure mode of the cyclic export, but with a named reexport rather than a star reexport. |
|
@kriskowal Added the 6-cell TDZ-observation matrix in Two axes:
The star-reexporter reads Matrix outcomes:
The two The four star-reexporter-first cells have no TDZ window to observe because depth-first cycle resolution evaluates the renamer's body to completion before the star-reexporter's body runs; they record the bound-value observation that completes the matrix. |
|
@kriskowal Addressed both asks in two commits on top of the prior fixer's TDZ matrix. (1) Construction-time notifiers considerationDisposition: feasible-and-documented, deferred to follow-up PR. Wrote the analysis as a standalone design document at
Recommendation in the document: land the redesign as a follow-up PR rather than fold the refactor into this regression-fix PR. The redesign touches two large surfaces, shifts the instantiate/link line, and benefits from being reviewed against the parity baseline this PR's test matrix establishes. (2) Named-reexport variant testAdded in commit 0c46da9 to The failure mode reproduces in the named-reexport shape. Node.js raises Verified out-of-band with a minimal native-ESM fixture across the same const/let/var matrix; the named-reexport variant matches Node.js's TDZ enforcement on Verification
|
kriskowal
left a comment
kriskowal
left a comment
There was a problem hiding this comment.
The follow-up design looks good. I find it preferable to fix the defects we’ve discovered in this exploration, with the added tests introduced without the .failing caveats. Please dispatch an agent to address both issues. Address each issue in separate commits.
|
@kriskowal addressed both TDZ defects in two separate commits per your directive:
Root causeThe cross-module read Three targeted fixes
Also updated Test status
Re-requesting review. |
There was a problem hiding this comment.
@kriscendobot Please delete, but capture a copy in your journal.
There was a problem hiding this comment.
Archived as commit b76ab1a and removed from the PR tree. The verbatim analysis now lives in the garden journal at https://github.com/kriskowal/garden/blob/journal/projects/endo/drafts/construction-time-notifiers.md with an index row in https://github.com/kriskowal/garden/blob/journal/projects/endo/drafts/README.md
There was a problem hiding this comment.
Please double-check that each of these scenarios is also represented as a compartment mapper test fixture with a Node.js parity check.
There was a problem hiding this comment.
Verified and extended in commit cc23fca. The previously-covered scenarios (cycle-rename and cycle-rename-unused) already had compartment-mapper fixtures with Node.js parity tests. The seven remaining scenarios from the TDZ-observation matrix and the named-reexport variant now have matching fixtures:
- packages/compartment-mapper/test/fixtures-cycle-rename-tdz-const-renamer-first/ (probe ReferenceError)
- packages/compartment-mapper/test/fixtures-cycle-rename-tdz-let-renamer-first/ (probe ReferenceError)
- packages/compartment-mapper/test/fixtures-cycle-rename-tdz-var-renamer-first/ (probe undefined)
- packages/compartment-mapper/test/fixtures-cycle-rename-tdz-const-star-first/ (probe value 42)
- packages/compartment-mapper/test/fixtures-cycle-rename-tdz-let-star-first/ (probe value 42)
- packages/compartment-mapper/test/fixtures-cycle-rename-tdz-var-star-first/ (probe value 42)
- packages/compartment-mapper/test/fixtures-cycle-named-reexport-tdz-const-renamer-first/ (probe ReferenceError)
Each fixture has a SES-side test (cycle-rename-tdz--.test.js or cycle-named-reexport-tdz-const-renamer-first.test.js) and a Node.js parity sibling with the -node-parity.test.js suffix. The expected probe values live in _cycle-rename-tdz-assertions.js so both layers compare against exactly one set of expectations; parity is verified by construction. The import-gauntlet.test.js header comment now links each matrix cell to its fixture path.
… kriskowal Maintainer review 4474269526 on endojs/endo-but-for-bots#379 asked to delete packages/ses/designs/construction-time-notifiers.md from the PR tree and capture a copy in the journal. The analysis explored moving SES reexport notifier creation from the link phase into the construction phase. The recommendation was to land the redesign as a follow-up PR; the in-PR fix for issue #59 enforces TDZ on cross-module namespace reads without requiring the construction-time-notifiers redesign. Archived under projects/endo/drafts/ per the existing draft-design convention. The drafts/README.md index gains a row pointing to the new file. Fixer dispatch 4ab426.
|
Addressed both inline asks from review 4474269526. Ask 1: archive then delete construction-time-notifiers.md Ask 2: compartment-mapper parity for each gauntlet scenario
Each fixture has both a SES-side test (loaded via the compartment-mapper scaffold) and a Node.js parity sibling (-node-parity.test.js suffix) that runs the same fixture under plain Node.js. Expected probe values live in _cycle-rename-tdz-assertions.js so both layers compare against exactly one set of expectations. The import-gauntlet.test.js header comment now links each matrix cell to its fixture path so future maintainers can find both layers from either side. Verification
CI @kriskowal re-requesting your review. |
kriskowal
left a comment
kriskowal
left a comment
There was a problem hiding this comment.
Please consolidate all of the new compartment mapper tests into a table test generator that makes evident which assertions correspond to each fixture, and that for every fixture, the corresponding Node.js parity test passes.
There was a problem hiding this comment.
Please update to reflect sum of changes.
There was a problem hiding this comment.
Addressed in 39b880d: the changeset now describes both user-visible changes the cycle-rename branch carries: the original deferred forwarding notifier that resolves the spurious cycle-rename SyntaxError, and the ECMA-262 temporal-dead-zone enforcement for cross-module namespace reads during cycle evaluation (covering both export * and export { y } from reexport forms).
| }; | ||
|
|
||
| scaffold( | ||
| 'cycle-rename-tdz var renamer-first (issue #59)', |
There was a problem hiding this comment.
The issue number will be invalid upstream. Remove issue numbers.
There was a problem hiding this comment.
Addressed in b7e77cf: swept the issue-number references (#59 and endojs/endo#59) from the consolidated table-driven test pair (cycle-rename-tdz-matrix.test.js + cycle-rename-tdz-matrix-node-parity.test.js) and from the shared assertions module's preamble. The 14 individual per-cell files that originally carried the references no longer exist after consolidation in 7d601a2.
…driven test pair Replace the 14 individual per-cell test files (seven for the SES side through the compartment-mapper scaffold, seven for the Node.js parity side) with a single SCENARIOS table in the shared assertions module and two iterate-and-register test files that walk it. Each scenario's `fixture` field names its directory under packages/compartment-mapper/test/; the `expectedProbe` field carries the matrix's expected observation; the assertion module's preamble holds the matrix's framing and per-cell rationale. The diff is shape-only: every scenario from the prior individual files appears as one SCENARIOS row, and both layers (SES under scaffold and plain Node.js) walk the same table so per-scenario parity is verified by construction. Test counts match the prior split layout (7 Node parity + 7 * 11 scaffold variants); cycle-rename-tdz cells all pass on both layers. Update the import-gauntlet.test.js cross-reference block to name the consolidated SCENARIOS table and the two table-driven files rather than the prior per-cell file pattern. Per kriskowal review 4491014140 on PR #379.
…cle-rename defect fix
The original changeset described only the deferred forwarding notifier
that resolved the spurious cycle-rename `SyntaxError`. The branch has
since grown a second user-visible change: SES now enforces ECMA-262
temporal-dead-zone semantics for cross-module reads through a module
namespace import while the upstream's body is still mid-evaluation,
for both the `export *` and `export { y } from` reexport forms. Update
the changeset prose to name both behaviors so consumers reading the
release notes see the full surface affected by the cycle-rename work.
Per kriskowal review 4491014140 on PR #379.
…-rename-tdz matrix files The bot-fork-side review on PR #379 noted that the issue numbers (#59 and the qualified endojs/endo#59) will not translate when the cycle-rename branch lands on the upstream endojs/endo repository. Sweep the issue-number references from the consolidated table-driven test pair (cycle-rename-tdz-matrix.test.js and the node-parity sibling) and the shared assertions module so the scenario titles and module-level prose stand on their own description of the matrix's framing. The bot-fork-side PR number itself (#379) and the per-commit SHAs that lived in the assertions module's preamble go away with the same sweep; the module continues to point readers at the SCENARIOS table and at the import-gauntlet.test.js sibling for the in-process scenarios. Per kriskowal review 4491014140 on PR #379.
|
@kriskowal, addressed the three asks from review 4491014140:
CI status at head b7e77cf: |
When a module re-exports `*` from another module, and that other module
re-exports a binding from the first under a *different* exported name
(`export { y as x } from './mod1.js'`), the linker visited the first
module 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, which manifested as `TypeError: notify is not
a function` at `packages/ses/src/module-instance.js`. The original 2019
issue described the failure as `SyntaxError: ... does not provide an
export named 'y'`; the surface symptom evolved as the linker matured,
but the underlying defect is the same.
`wireUpExportNotifier` now installs a deferred forwarding notifier for
re-exports whose upstream notifier is not yet present. The forwarding
queues subscribers until the upstream resolves, then drains them
through. By the time any module downstream subscribes to the re-export,
the upstream module has completed its candidate-all walk and the
upstream notifier exists, so the chain converges.
Two regression surfaces accompany the fix. In SES, `import-gauntlet`
adds `cyclic star export with renaming reexport (issue #59)` exercising
the exact reproducer from the issue against the SES linker directly. In
the compartment mapper, a three-module fixture (`star-reexporter` re-
exports `*` from `export-renamer`; `export-renamer` re-exports `y as x`
from `star-reexporter`) drives the same shape through `loadLocation`,
`importLocation`, the archive round-trip pair, and `makeArchiveFromMap`,
plus a Node.js parity test that imports the same fixture under plain
Node.js (no SES, no compartment mapper) and asserts identical expected
values from a shared assertions module. Pinning the compartment
mapper's behavior to Node.js's reference behavior keeps the expected
values defined in one place and teases linker behavior out of SES
rather than asserting it against itself.
Reverting the `wireUpExportNotifier` change while keeping either test
surface reproduces `TypeError: notify is not a function` (nine of the
eleven compartment-mapper import-path variants fail; the two archive-
integrity variants pass because they do not import the fixture; the
Node.js parity test is unaffected). Re-applying the fix restores all
twelve passing tests.
Fixes #59
Companion regression for endojs/endo#59 addressing review feedback on endojs/endo#3276: a sibling fixture where the live binding `y` is declared but never assigned. Node.js reads every projection of the cycle as `undefined` for this shape (verified directly with `node`), so the SES linker must match. The deferring closure introduced by the fix either resolves (when a wireUp higher in the chain re-references the binding) or stays pending; the namespace reads agree with Node.js in either case. This commit adds executable evidence; no source change.
Introduce a synchronous variant of Promise.withResolvers. The helper
returns { notify, resolve }: subscribers attached via notify(update)
before resolve(targetNotify) is called are queued; once resolve is
called, queued updaters are replayed to the target notifier and
subsequent notify(update) calls forward directly through. resolve is
one-shot (idempotent for repeat calls), which lets a caller invoke it
lazily on each notify and have only the first invocation take effect.
Apply the helper to the cycle-resolver branch of wireUpExportNotifier
in module-instance.js, replacing the inline pendingUpdaters[] +
resolvedUpstreamNotify state machine. The two patterns are now
syntactically separated from their call site, reducing the chance that
the local state machine will drift away from any future second use.
Refs: endojs/endo#3276 (kriskowal review)
Add a regression test in import-cjs.test.js verifying that a cyclic star-export topology in which the "reexporter" is a CommonJS module behaves consistent with Node.js. Node.js rejects ESM-in-CJS-cycle outright (ERR_REQUIRE_CYCLE_MODULE), so the parity comparison is against the pure-CJS cycle: snapshot-at-call-time semantics for the CJS side (property capture sees whatever the renamer had assigned by the re-entry instant), live-binding semantics for the ESM side. Both namespaces project the same shapes the test pins. Addresses naugtur's review feedback on endojs/endo#3276 asking for a test of the cyclic star-export with a CommonJS reexporting module. Refs: endojs/endo#3276 (naugtur review)
… tests (#59 follow-up) Mirror the cycle-rename parity-test layout for the pure-CommonJS cyclic reexporter scenario, in which Node.js and SES agree. The fixture under fixtures-cycle-cjs-reexporter/node_modules/app/ expresses the star-reexporter and renaming reexporter as on-disk .cjs modules with a live getter for the renamed export. cycle-cjs-reexporter.test.js runs the fixture through the compartment-mapper scaffold; cycle-cjs-reexporter-node-parity.test.js runs the same fixture under plain Node.js. Both tests assert through the shared _cycle-cjs-reexporter-assertions.js module, so parity is verified by construction: if both tests pass, the compartment mapper's CommonJS cycle behavior matches Node.js for this case.
…follow-up) Add a parity-test pair that programmatically verifies the divergence between Node.js and SES for the ESM-in-CommonJS-cycle topology. The fixture under fixtures-cycle-esm-in-cjs/node_modules/app/ has a CJS bridge module that require()s an ESM peer module that imports back from the bridge. cycle-esm-in-cjs.test.js asserts SES allows the topology and the namespace projects the live binding (bridgeValue === 42). cycle-esm-in-cjs-node-parity.test.js spawns a fresh Node.js process on the same fixture and asserts Node rejects with ERR_REQUIRE_CYCLE_MODULE. Together the two tests pin the divergence as a verified property rather than narrative prose.
…tment-mapper parity Rewrite the JSDoc on the cyclic CommonJS reexporter test in import-cjs.test.js and the companion unused-live-binding test in import-gauntlet.test.js so each block is primarily an explanation of what the test verifies (the shapes of the projected namespaces, the specific snapshot vs live-binding distinction, the parity property), not the procedural history of how the test came to be. The in-process SES regression is retained on the import-cjs.test.js side because it exercises the module-instance linker directly through the Compartment API with inline ModuleSources, a path the compartment-mapper parity suite does not cover. The prose now points at the parity suite for the parity-with-Node substantiation: packages/compartment-mapper/test/cycle-cjs-reexporter.test.js and its node-parity sibling for the pure-CommonJS agreement case; packages/compartment-mapper/test/cycle-esm-in-cjs.test.js and its node-parity sibling for the ESM-in-CJS-cycle divergence (ERR_REQUIRE_CYCLE_MODULE on Node, allowed on SES).
…59 follow-up) Add the parity test pair kriskowal asked for on #379 (review comment 3338677487): the unused-live-binding shape of the cyclic star-export with renaming reexport (endojs/endo#59) needs a parity test substantiating the Node.js parity claim, ideally with a shared fixture as with cycle-rename parity. The companion populated-binding shape was already covered by cycle-rename.test.js and cycle-rename-node-parity.test.js through _cycle-rename-assertions.js; this commit lands the analogous trio for the unused-live-binding shape: fixtures-cycle-rename-unused/node_modules/app/ - three modules; the renamer's `export var y` has no initializer. _cycle-rename-unused-assertions.js - shared assertion module; expected projections are { x: undefined, y: undefined } and captured: undefined. cycle-rename-unused.test.js - compartment-mapper scaffold exercise. cycle-rename-unused-node-parity.test.js - Node.js exercise of the same fixture; parity is verified by construction when both pass. Also update the in-process SES regression's prose in packages/ses/test/import-gauntlet.test.js to reference the new parity pair, matching the cross-reference pattern the populated-binding test already uses for cycle-rename. Refs: endojs/endo#3276 (naugtur), #379 (kriskowal)
…g reexport (issue #59 follow-up) Add a 6-cell test matrix that varies the cyclic star-export fixture along two axes: the order in which main.js imports the export-renamer and the star-reexporter, and the binding form (const, let, var) the renamer uses for its local y. The star-reexporter reads r.y through a namespace import of the renamer at the top of its body and captures the result, so when the renamer is imported first from main.js the observation lands while the renamer is on the evaluation stack with y not yet initialized. Four cells assert SES's current behavior because it already matches Node.js: renamer-first with var (reads undefined), and all three star-reexporter-first orderings (read the assigned value 42). The two remaining cells (renamer-first with const and let) are marked test.failing: under ECMA-262 semantics, the cross-module read through the namespace import should raise ReferenceError because y is in the temporal dead zone, and Node.js confirms this; SES's current module-instance machinery returns undefined instead. The .failing markers pin the Node.js reference behavior as the desired outcome and surface the SES divergence as a known gap to address separately.
follow-up) Adds packages/ses/designs/construction-time-notifiers.md analyzing the maintainer's proposal to create all module-instance notifiers at construction time instead of link time, in response to naugtur's inline-comment ask on upstream review and kriskowal's follow-up. The analysis sets out the present four-phase lifecycle (load, instantiate, construct, link), enumerates what is and is not available at construction time, sketches a two-pass redesign that constructs reexport-notifier stubs in instantiate and resolves them in a pass-2 wire step, and addresses the precompiled-ModuleSource calling-convention question (no schema change required; imports(updateRecord) becomes wire-only). The recommendation is to land the redesign as a follow-up PR rather than fold the refactor into the present regression-fix PR: the redesign touches two large surfaces (makeModuleInstance and makeVirtualModuleInstance), shifts the instantiate/link line, and benefits from being reviewed against the parity baseline this PR's test matrix establishes. The analysis also notes a separable observation: the redesign does not by itself close the SES-against-Node cross-module TDZ divergence the test matrix pins as test.failing. Closing that gap requires a separate change to the exported-getter contract (consult the upstream's own-binding getter rather than caching the propagated value), which the redesign enables but does not require.
…#59 follow-up) Adds one test to packages/ses/test/import-gauntlet.test.js that mirrors the renamer-first plus const star-reexport cell of the prior fixer's TDZ matrix but with a named reexport (export { y } from './export-renamer.js') instead of a star reexport (export * from './export-renamer.js'). The cycle has the same shape as the star case (the named reexporter and the export renamer reference each other), and the observation of r.y through the namespace import lands during the same linked-but-not-yet-bound window when main.js imports the renamer first. Node.js raises ReferenceError for const y = 42 in this case, matching the star-reexport behavior, because the temporal dead zone semantics live with the binding, not with the reexport form. SES's current module-instance machinery returns undefined instead, the same gap the star-reexport test.failing cells pin. The new test is marked test.failing per the prior fixer's discipline: it asserts the Node.js reference behavior (ReferenceError) as the desired outcome and surfaces the SES divergence as a known gap to address separately. The named-reexport variant confirms the gap is not specific to export *, which the maintainer asked to verify in issue-comment 4675471286.
…star reexport) Closes the SES-against-Node cross-module TDZ divergence pinned by the renamer-first × const and renamer-first × let cells of the cyclic star-reexport matrix in packages/ses/test/import-gauntlet.test.js. Diagnosis. The cross-module read `r.y` through a namespace import (the `'*'` notifier) propagated the raw `exportsTarget` object, not the `exportsProxy`. `exportsTarget` had no property defined for the binding until the late `defineProperty` pass at the end of `imports()`; a missing property reads as `undefined` rather than throwing. Even after the property landed, the `wireUpExportNotifier` helper installed an exported getter that returned the last propagated value (initially `undefined`) with no TDZ tracking. Fix, three targeted changes that do not require the full construction-time-notifiers redesign documented at `packages/ses/designs/construction-time-notifiers.md`: - `packages/ses/src/module-instance.js`: define `exportsTarget[name]` eagerly during construction for each own fixed and live export, using the existing TDZ-aware getter from `localGetNotify`. The late `arrayForEach(arraySort, defineProperty)` pass at the end of `imports()` redefines the same descriptor as a no-op, preserving the ECMA-262 sorted enumeration order without changing the eager TDZ-aware behavior. `wireUpExportNotifier` tracks its own TDZ state so the downstream's exported getter throws `ReferenceError` until the upstream binding propagates a value through the notifier chain, and defines the property on `exportsTarget` eagerly too. - `packages/module-source/src/transform-analyze.js`: reorder the preamble so hoisted declarations (function declarations and `var` initializers) run before the imports call. This matches the ECMA-262 model: function/var bindings are created and initialized to undefined during `InitializeEnvironment`, which precedes dependency evaluation in `Module.Evaluate`. Without this reorder, a hoisted `var y` in an upstream module would still be in the live-binding TDZ when a downstream's body read `r.y`, and the eager TDZ-aware getter would throw `ReferenceError` instead of returning `undefined` (the var cells of the matrix). - `packages/module-source/test/fixtures/format-preserved.txt`: update the format-preserved expected output to reflect the new hoisted-before-imports preamble order. The star-reexport `.failing` cells (renamer-first × const, renamer-first × let) now pass without the `.failing` modifier. The four converging cells of the matrix continue to pass unchanged. The renamer-first × var cell continues to read `undefined` because the upstream's hoisting preamble clears its TDZ before the downstream observes; this matches Node.js. The design document updates explain how the fix relates to the prior construction-time-notifiers redesign analysis (the redesign remains orthogonal to the TDZ fix and is still appropriate as a follow-up PR). Refs endojs/endo#59.
The cyclic-named-reexport TDZ matrix cell (`export { y } from
'./export-renamer.js'` reading `r.y` from the renamer's namespace during
the linked-but-not-yet-bound window) is now ECMA-262-compliant and reads
ReferenceError, matching Node.js.
The preceding commit's fix to `wireUpExportNotifier` in
`packages/ses/src/module-instance.js` already covered both star reexports
and named reexports because both wire through the same helper from
`imports()` (the named-reexport path at line ~462 calls
`wireUpExportNotifier(exportedName, importNotifiers[localName],
specifier, localName)` with the same TDZ-tracking semantics as the
star-reexport candidate-all walk at line ~473). This commit therefore
ships the test-only change that converts the named-reexport `.failing`
cell to a passing assertion, confirming the gap is closed in the
named-reexport shape too.
Refs endojs/endo#59.
… journal The construction-time-notifiers analysis written for the issue #59 follow-up review described a possible redesign of the SES module-instance machinery (move reexport notifier creation from link phase into construction phase) but recommended deferring implementation to a separate PR. Per kriskowal's CHANGES_REQUESTED review on this PR, the analysis belongs in the garden's journal rather than the package tree: the artifact records how the exploration informed the in-PR TDZ fix without persisting a design document that the PR does not implement. The verbatim text now lives at journal/projects/endo/drafts/construction-time-notifiers.md in the garden's journal branch and is indexed from drafts/README.md there.
…TDZ matrix
The seven new scenarios that the prior fixers added to
packages/ses/test/import-gauntlet.test.js (a six-cell TDZ matrix of binding
form against import order for cyclic star-export plus one named-reexport
variant) now have matching compartment-mapper test fixtures, each with a
Node.js parity sibling. The same fixture is loaded by both: by the
compartment-mapper scaffold in the SES-side test, and by Node.js's native
loader in the parity-side test. The expected probe value lives in
_cycle-rename-tdz-assertions.js so the two layers compare against exactly
one set of expectations. Parity is verified by construction: if both tests
pass, SES enforces the same TDZ semantics on the cross-module namespace
path that Node.js enforces natively.
Fixtures:
- fixtures-cycle-rename-tdz-const-renamer-first (probe: ReferenceError)
- fixtures-cycle-rename-tdz-let-renamer-first (probe: ReferenceError)
- fixtures-cycle-rename-tdz-var-renamer-first (probe: value undefined)
- fixtures-cycle-rename-tdz-const-star-first (probe: value 42)
- fixtures-cycle-rename-tdz-let-star-first (probe: value 42)
- fixtures-cycle-rename-tdz-var-star-first (probe: value 42)
- fixtures-cycle-named-reexport-tdz-const-renamer-first
(probe: ReferenceError)
The renamer-first const and let cells observe ReferenceError during the
temporal-dead-zone window when the renamer is on the evaluation stack with
y not yet initialized. The renamer-first var cell observes undefined
because the hoisting preamble clears the upstream TDZ before the downstream
observes. The star-first cells have no TDZ window to observe; 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 42
for every binding form. The named-reexport variant confirms the TDZ gap is
not specific to export *.
The import-gauntlet.test.js header comment now links each matrix cell to
its parity fixture path so future maintainers can find both layers from
either side.
…driven test pair Replace the 14 individual per-cell test files (seven for the SES side through the compartment-mapper scaffold, seven for the Node.js parity side) with a single SCENARIOS table in the shared assertions module and two iterate-and-register test files that walk it. Each scenario's `fixture` field names its directory under packages/compartment-mapper/test/; the `expectedProbe` field carries the matrix's expected observation; the assertion module's preamble holds the matrix's framing and per-cell rationale. The diff is shape-only: every scenario from the prior individual files appears as one SCENARIOS row, and both layers (SES under scaffold and plain Node.js) walk the same table so per-scenario parity is verified by construction. Test counts match the prior split layout (7 Node parity + 7 * 11 scaffold variants); cycle-rename-tdz cells all pass on both layers. Update the import-gauntlet.test.js cross-reference block to name the consolidated SCENARIOS table and the two table-driven files rather than the prior per-cell file pattern. Per kriskowal review 4491014140 on PR #379.
…cle-rename defect fix
The original changeset described only the deferred forwarding notifier
that resolved the spurious cycle-rename `SyntaxError`. The branch has
since grown a second user-visible change: SES now enforces ECMA-262
temporal-dead-zone semantics for cross-module reads through a module
namespace import while the upstream's body is still mid-evaluation,
for both the `export *` and `export { y } from` reexport forms. Update
the changeset prose to name both behaviors so consumers reading the
release notes see the full surface affected by the cycle-rename work.
Per kriskowal review 4491014140 on PR #379.
…-rename-tdz matrix files The bot-fork-side review on PR #379 noted that the issue numbers (#59 and the qualified endojs/endo#59) will not translate when the cycle-rename branch lands on the upstream endojs/endo repository. Sweep the issue-number references from the consolidated table-driven test pair (cycle-rename-tdz-matrix.test.js and the node-parity sibling) and the shared assertions module so the scenario titles and module-level prose stand on their own description of the matrix's framing. The bot-fork-side PR number itself (#379) and the per-commit SHAs that lived in the assertions module's preamble go away with the same sweep; the module continues to point readers at the SCENARIOS table and at the import-gauntlet.test.js sibling for the in-process scenarios. Per kriskowal review 4491014140 on PR #379.
…s into single module per kriskowal review Consolidate the cycle-rename-tdz matrix's SES treatment (cycle-rename-tdz-matrix.test.js) and Node.js parity treatment (cycle-rename-tdz-matrix-node-parity.test.js) into a single test module. Each scenario now registers two tests back-to-back in the same module: the SES treatment through the compartment-mapper scaffold and the Node.js parity treatment through a direct dynamic import. The paired registration makes the shared fixture coverage legible at a glance: a fixture appears in the module exactly twice with the same expected probe. The SCENARIOS table (and the assertCycleRenameTdz helper) move inline into the consolidated module; the prior shared module (_cycle-rename-tdz-assertions.js) had only one consumer remaining after the merge and is deleted. The split node-parity sibling (cycle-rename-tdz-matrix-node-parity.test.js) is deleted with the same sweep. Update the prose in packages/ses/test/import-gauntlet.test.js that pointed readers at the prior split-file shape to point at the consolidated module instead. Per kriskowal review 4492610183 on PR #379.
) Per boneskull's review on endojs/endo#3276 (review 4489675443): - Add a `@module` docstring to `packages/ses/src/notifier-with-resolver.js`. - Drop the brittle prose backreference to `module-instance.js` `wireUpExportNotifier` from the helper's JSDoc, and instead reference `{@link makeNotifierWithResolver}` from `wireUpExportNotifier`'s own inline narrative, so the cross-module link is owned by the consumer rather than the producer.
…-with-resolver (#379) Per boneskull's style suggestion on endojs/endo#3276 (review 4489675443): restructure `notify` and `resolve` in `packages/ses/src/notifier-with-resolver.js` so the conditional uses strict equality (`resolvedTargetNotify === undefined`) and an `else` branch rather than an `!== undefined` test with an early return. Same observable behavior; reads as one decision per call rather than two.
…do#3220 (#379) Per boneskull's note on endojs/endo#3276 (review 4489675443): the local `CjsModuleSource` helper in `packages/ses/test/import-cjs.test.js` is a heuristic-regex stand-in for the AST-based parser that endojs/endo#3220 will expose from `@endo/module-source`. Add a TODO at the mock's definition so the swap is discoverable from a grep when #3220 lands. (#3220 is still open; addressing the note-to-self in place rather than swapping the implementation.)
…single modules per kriskowal review Extend the consolidation pattern from cycle-rename-tdz-matrix.test.js (commit ca17e11) to the five remaining parity-test pairs in packages/compartment-mapper/test/. Each pair now lives as one module that registers a (ses) treatment through the compartment-mapper scaffold and a (node parity) treatment through plain Node.js (dynamic import or spawnSync) back-to-back. The paired registration makes the shared fixture coverage legible at a glance: a fixture appears in the module once for the SES treatment and once for the Node.js parity treatment, asserting the same expected values through the shared assertion module. Pairs consolidated (parity file merged into sibling and deleted): - cycle-cjs-reexporter-node-parity.test.js -> cycle-cjs-reexporter.test.js - cycle-esm-in-cjs-node-parity.test.js -> cycle-esm-in-cjs.test.js - cycle-rename-node-parity.test.js -> cycle-rename.test.js - cycle-rename-unused-node-parity.test.js -> cycle-rename-unused.test.js - subpath-patterns-node-parity.test.js -> subpath-patterns.test.js For subpath-patterns, the two sides do not pair test-for-test (SES has archive-shape and policy tests with no Node analog; Node has multi-star / globstar exclusions with no SES analog). Scenarios with a direct counterpart on both sides are registered back-to-back as "<title> (ses)" and "<title> (node parity)"; scenarios on only one side are registered once. For cycle-esm-in-cjs, the Node parity treatment spawns a Node process to assert ERR_REQUIRE_CYCLE_MODULE rather than dynamic-importing the fixture, so the test runner's own module graph is not exposed to the rejected topology. The SES treatment loads the same fixture through the scaffold and asserts the divergent SES-side success behavior. Drop bare "(issue #59 ...)" markers from test titles touched by this consolidation per pre-push-gates (no-pull-citations probe); the qualified "endojs/endo#59" references in docstrings are preserved. Update prose references in packages/ses/test/import-cjs.test.js, packages/ses/test/import-gauntlet.test.js, and packages/compartment-mapper/designs/subpath-pattern-replacement.md that pointed at the prior split-file shape to point at the consolidated modules instead. Per kriskowal review on PR #379 (2026-06-15T06:35:31Z, inline on cycle-esm-in-cjs-node-parity.test.js:1): "I would, for example, like this module's test to be moved into cycle-esm-in-cjs.test.js so that it is evident at a glance that the same test passes both Node.js parity and with Endo. I would like this principled generally to the other new parity tests."
… module The _cycle-rename-unused-assertions.js module still had two references that the no-pull-citations gate catches: endojs/endo#59 in the module preamble and "issue #59 fix" in the body. Rewrite prose to stand on its own description (cyclic-star-export regression, deferring closure) without citing the bot-fork or upstream issue number. This continues the sweep begun in b7e77cf which swept the same references from the cycle-rename-tdz-matrix files.
c0044e4 to
cb597b2
Compare
Rebased onto current
|
|
Superseded by #779 (retargeted to frozen base |
2 participants
Refs: endojs/endo#59, endojs/endo#3276
Supersedes: closed mirror PR #336 (branch force-pushed to address review feedback).
Description
Mirror of upstream endojs/endo#3276 (fix for the 2019 issue endojs/endo#59, cyclic star export with renaming reexport), refreshed for the bot-pushable fork so the gauntlet can re-run against the current upstream PR head.
This refresh sets the mirror head to upstream PR head
f4aad15aand adds a single companion regression test that addresses inline review feedback from naugtur on upstream pullrequestreview-4388440170.Naugtur's question
Disposition: verified, no source change
The "all calls happen before upstreamNotify is obtained" case is reachable in theory (a cyclic re-export of an unused live binding in which no further wireUp re-references the deferred notifier), but it is benign. Tracing the closure at
packages/ses/src/module-instance.js:379-396:notify(update)call at line 403 always fires once duringwireUpExportNotifier. IfupstreamInstance.notifiers[deferredImportName]is undefined at that moment, the call is queued.export *or another renaming reexport), or (b) a downstream consumer subscribes through the mainimports()updateRecord loop at line 449.valuestaysundefined. The export's getter at line 404-411 then returnsundefined.export var y;declared but never assigned) is exactly this shape under Node.js too: every projection of the cycle reads asundefined. Verified directly withnode.The new test in
packages/ses/test/import-gauntlet.test.js(cyclic star export with renaming reexport, unused live binding) pins SES's behavior to Node.js's reference behavior for this case.Why this is the right shape
The deferring closure's contract is "forward to the upstream's notifier once it exists, otherwise queue". The contract is well-defined whether or not a second call ever arrives; the queue is bounded by the number of subscribers (zero in the unused case). The "all calls before upstream" hypothetical is just the zero-subscriber subcase; it does not mis-route any update.
Verification
yarn workspace ses test503 pass + 2 known failures + 2 skipped (includes the new gauntlet test).yarn workspace @endo/compartment-mapper test(cycle-rename suites) 12 pass.Security Considerations
None. Test-only addition; no source change.
Scaling Considerations
None.
Documentation Considerations
None.
Testing Considerations
The new test exercises the unused-live-binding subcase and pins it to Node.js parity. Removing the deferring branch makes the original (issue #59) test fail with
TypeError: notify is not a function; the new test passes with or without the deferring branch (it is a parity assertion, not a defect reproducer).Compatibility Considerations
None.
Upgrade Considerations
None.