Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix reentrancy invariant #39

Merged
merged 12 commits into from
Apr 24, 2024
Merged

fix reentrancy invariant #39

Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4098f92
normative: handle reentrancy invariant
guybedford Apr 2, 2024
07b34ab
fixups
guybedford Apr 2, 2024
8826194
simplify
guybedford Apr 2, 2024
9ee06d5
readd note
guybedford Apr 2, 2024
35581c7
Update spec.emu
guybedford Apr 2, 2024
e9094d5
fixup evaluating-async handling
guybedford Apr 2, 2024
3de1cd6
fixups
guybedford Apr 2, 2024
0744c50
simplify diff
guybedford Apr 2, 2024
f1512b8
grammar
guybedford Apr 2, 2024
130ddd9
invert assertion to call out evaluating
guybedford Apr 2, 2024
3dd0025
remove evaluating-async stop condition for now
guybedford Apr 4, 2024
d93f0aa
Update spec.emu
guybedford Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 22 additions & 16 deletions spec.emu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ contributors: Nicolò Ribaudo
1. If _P_ is a Symbol, then
1. Return ! OrdinaryGet(_O_, _P_, _Receiver_).
1. <ins>Let _m_ be _O_.[[Module]].</ins>
1. <ins>If _m_ is not a Cyclic Module Record, or both _m_.[[Status]] is ~linked~ and AnyDependencyNeedsAsyncEvaluation(_m_) is *false*, then</ins>
1. <ins>If _m_ is not a Cyclic Module Record, then</ins>
1. <ins>Perform ? EvaluateSync(_m_).</ins>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EvaluateSync unconditionally calls module.Evaluate(). It will cause the target non Cyclic Module Record to be evaluated every time its namespace object is accessed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend posting this as a new issue as it is not specific to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evaluate() can be called multiple times and it's expected to cache it's result:

Returns a promise for the evaluation of this module and its dependencies, resolving on successful evaluation or if it has already been evaluated successfully, and rejecting for an evaluation error or if it has already been evaluated unsuccessfully.

https://tc39.es/ecma262/multipage/ecmascript-language-scripts-and-modules.html#table-abstract-methods-of-module-records

Dynamic import also calls Evaluate() every time, even if a module is already evaluated.

1. <ins>Otherwise if _m_.[[Status]] is not ~evaluated~ and ! ReadyForSyncExecution(_m_) is *true*, then</ins>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ReadyForSyncExecution(m) is false, then what will this AO return? The (possibly) uninitialized binding and lead to TDZ error (for export let)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the case of it not being ready, the namespace will expose TDZ or undefined for non-TDZ bindings, per the note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also what happens with cycles without defer:

// entrypoint
import "dep";
export let a = 1;
// dep
import * as ns from "entrypoint";
ns.a; // TDZ error

1. <ins>Perform ? EvaluateSync(_m_).</ins>
1. Let _exports_ be _O_.[[Exports]].
1. If _exports_ does not contain _P_, return *undefined*.
Expand All @@ -93,29 +95,32 @@ contributors: Nicolò Ribaudo
<p>ResolveExport is side-effect free. Each time this operation is called with a specific _exportName_, _resolveSet_ pair as arguments it must return the same result. An implementation might choose to pre-compute or cache the ResolveExport results for the [[Exports]] of each module namespace exotic object.</p>
</emu-note>
<emu-note>
<ins>AnyDependencyNeedsAsyncEvaluation can return *true* when this [[Get]] operation on a namespace coming from a deferred import is triggered during the evaluation of one of its dependencies, either synchronously or asynchronously. In that case, the module cannot be fully evaluated and some of its exports will not be initialized yet.</ins>
<ins>ReadyForSyncExecution can return *false* when this [[Get]] operation on a namespace coming from a deferred import is triggered during the evaluation of one of its dependencies, either synchronously or asynchronously. In that case, the module cannot be fully evaluated and some of its exports will not be initialized yet.</ins>
</emu-note>

<emu-clause id="sec-module-namespace-exotic-objects-get-p-receiver-AnyDependencyNeedsAsyncEvaluation" type="abstract operation">
<emu-clause id="sec-module-namespace-exotic-objects-get-p-receiver-ReadyForSyncExecution" type="abstract operation">
<h1>
<ins>
AnyDependencyNeedsAsyncEvaluation (
_module_: a Module Record,
ReadyForSyncExecution (
_module_: a Cyclic Module Record,
optional _seen_: a List of Module Records,
): a Boolean
</ins>
</h1>
<dl class="header"></dl>
<emu-alg>
1. If _seen_ is not provided, let _seen_ be a new empty List.
1. If _seen_ contains _module_, return *false*.
1. If _seen_ contains _module_, return *true*.
1. Append _module_ to _seen_.
1. If _module_ is not a Cyclic Module Record or _module_.[[Status]] is ~evaluated~, return *false*.
1. If _module_.[[HasTLA]] is *true*, return *true*.
1. If _module_.[[Status]] is ~evaluated~, return *true*.
1. If _module_.[[Status]] is ~evaluating~ or ~evaluating-async~, return *false*.
1. Assert: _module_.[[Status]] is ~linked~.
1. If _module_.[[HasTLA]] is *true*, return *false*.
1. For each ModuleRequest Record _required_ of _module_.[[RequestedModules]], do
1. Let _requiredModule_ be GetImportedModule(_module_, _required_.[[Specifer]]).
1. If AnyDependencyNeedsAsyncEvaluation(_requiredModule_, _seen_) is *true*, return *true*.
1. Return *false*.
1. If ! ReadyForSyncExecution(_requiredModule_, _seen_) is *false*, then
1. Return *false*.
1. Return *true*.
</emu-alg>
</emu-clause>
</emu-clause>
Expand Down Expand Up @@ -514,7 +519,7 @@ contributors: Nicolò Ribaudo
a List of <del>Strings</del><ins>ModuleRequest Records</ins>
</td>
<td>
A List of all the |ModuleSpecifier| strings used by the module represented by this record to request the importation of a module, <ins>alongside with their maximum specified import phase (~defer~ or ~evaluation~)</ins>. The List is in source text occurrence order.
A List of all the |ModuleSpecifier| strings used by the module represented by this record to request the importation of a module, <ins>along with their maximum specified import phase (~defer~ or ~evaluation~)</ins>. The List is in source text occurrence order.
</td>
</tr>
<tr>
Expand Down Expand Up @@ -874,7 +879,8 @@ contributors: Nicolò Ribaudo
</dl>

<emu-alg>
1. Assert: This call to Evaluate is not happening at the same time as another call to Evaluate within the surrounding agent.
1. <del>Assert: This call to Evaluate is not happening at the same time as another call to Evaluate within the surrounding agent.</del>.
1. <ins>Assert: None of _module_ or any of its recursive dependencies have [[Status]] set to ~evaluating~ or a status earlier than ~linked~.</ins>
1. Assert: _module_.[[Status]] is one of ~linked~, ~evaluating-async~, or ~evaluated~.
1. If _module_.[[Status]] is either ~evaluating-async~ or ~evaluated~, set _module_ to _module_.[[CycleRoot]].
1. If _module_.[[TopLevelCapability]] is not ~empty~, then
Expand Down Expand Up @@ -996,7 +1002,7 @@ contributors: Nicolò Ribaudo
</h1>
<dl class="header">
<dt>description</dt>
<dd>It synchronously evaluates _module_, which must not have asynchronous dependencies.</dd>
<dd>It synchronously evaluates _module_ provided it is ready for synchronous execution.</dd>
</dl>

<emu-alg>
Expand All @@ -1009,7 +1015,7 @@ contributors: Nicolò Ribaudo
</emu-alg>
</emu-clause>

<emu-clause id="sec-InnerAsyncSubgraphsEvaluation" type="abstract operation">
<emu-clause id="sec-GatherAsynchronousTransitiveDependencies" type="abstract operation">
<h1>
<ins>
GatherAsynchronousTransitiveDependencies (
Expand All @@ -1021,7 +1027,7 @@ contributors: Nicolò Ribaudo
</h1>
<dl class="header">
<dt>description</dt>
<dd></dd>
<dd>Collects the direct post-order list of asynchronous unexecuted transitive dependencies, stopping the depth-first search for a branch when an async dependency is found.</dd>
</dl>

<emu-alg>
Expand All @@ -1030,7 +1036,7 @@ contributors: Nicolò Ribaudo
1. Append _module_ to _seen_.
1. If _module_ is not a Cyclic Module Record, return ~unused~.
1. If _module_.[[Status]] is either ~evaluating~ or ~evaluated~, return ~unused~.
1. If _module_.[[HasTLA]] is *true*, then
1. If _module_.[[HasTLA]] is *true* or _module_.[[Status]], then
guybedford marked this conversation as resolved.
Show resolved Hide resolved
1. If _result_ does not contain _module_, append _module_ to _result_.
1. Return ~unused~.
1. For each ModuleRequest Record _required_ of _module_.[[RequestedModules]], do
Expand Down