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.

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

Normative: Top-level Await #2408

Merged
merged 1 commit into from
Aug 15, 2021
Merged

Conversation

codehag
Copy link
Contributor

@codehag codehag commented May 14, 2021

This PR introduces the necessary changes to the spec for top-level await

I hit a couple of conflicts (you will notice while reviewing) and I am not sure if that is the new style of the spec. I tried to resolve most of them but I sometimes wasn't sure. If the original is intended, let me know and I will update the spec accordingly.

Since the last meeting, one editorial change was merged in, introducing prose around what happens when an error is thrown during async evaluation. We are discussing a further clarification with @guybedford , but it doesn't affect the normative spec text. It will hopefully make it more readable. What is the best way to do this? Should we introduce that after this proposal is merged in?

@codehag codehag added has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels May 14, 2021
@codehag codehag requested review from guybedford and a team May 14, 2021 08:11
@codehag codehag changed the title Normative: Top Level Await Normative: Top-level Await May 14, 2021
@codehag codehag removed the request for review from guybedford May 14, 2021 08:59
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Rather than make a large number of 'suggested changes', I've submitted a PR against codehag:proposal-top-level-await.

@codehag
Copy link
Contributor Author

codehag commented May 15, 2021

We have a pr ready to help readability of the spec, should we just merge that in with this batch? tc39/proposal-top-level-await#179

@codehag
Copy link
Contributor Author

codehag commented May 17, 2021

@jmdyck I am unable to find your pr, against my fork or the main repo; do you have a link?

@nicolo-ribaudo
Copy link
Member

@codehag I think it's codehag#4

@codehag
Copy link
Contributor Author

codehag commented May 17, 2021

@nicolo-ribaudo ahhhh thanks!

@jmdyck
Copy link
Collaborator

jmdyck commented May 17, 2021

@codehag I think it's codehag#4

Yup!

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@codehag
Copy link
Contributor Author

codehag commented May 19, 2021

I haven't gotten a response here about the editorial change, so I will layer it in.

@ljharb ljharb requested a review from jmdyck May 19, 2021 22:14
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

First pass review. I haven't fully worked through the example yet.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
1. Perform ! Call(_module_.[[TopLevelCapability]].[[Resolve]], *undefined*, « *undefined* »).
1. Let _execList_ be a new empty List.
1. Perform ! GatherAsyncParentCompletions(_module_, _execList_).
1. Let _sortedExecList_ be a List whose elements are the elements of _execList_, in the order in which they had their [[AsyncEvaluation]] fields set to *true* in InnerModuleEvaluation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm leery of this, as I'm pretty sure there's nowhere in the current spec that behavior depends on the order in which fields were set to some value.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this briefly in the editor call today. I also don't love it, but haven't yet thought of an alternative I like more, and there's nothing technically wrong with this approach. (A global counter would work, for example, but isn't exactly cleaner.)

The closest thing we currently have is the step in OrdinaryOwnPropertyKeys which reads

For each own property key P of O such that Type(P) is String and P is not an array index, in ascending chronological order of property creation, do

Choose a reason for hiding this comment

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

It's a novel construct yes, but it might be ok. If there are further concerns here, an alternative to explicitly calling out field ordering might be to more generally reference "the original DFS visitor ordering via InnerModuleEvaluation that initiated the execution of this module". I've posted a possible adjustment to this wording in tc39/proposal-top-level-await#181. I can see either approach being fine here though. Further feedback welcome.

Copy link
Contributor Author

@codehag codehag May 20, 2021

Choose a reason for hiding this comment

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

I prefer the current wording or something similar, as it gets the intention across more clearly.

I agree -- it is the first time we do this and I was a bit worried about it when we first introduced it. I am open to ways we might do this while still maintaining a clear model for readers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the main reason I'm okay with this current wording is because as @bakkot says: an explicit "implementation" of the intent via e.g. a global counter just isn't any clearer.

spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

Second pass. I think mostly fine, but the example is confused in some parts.

img/module-graph-cycle-async.svg Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated

<p>Calling _A_.Evaluate() triggers InnerModuleEvaluation on _A_, _B_, and _D_, which all transition to ~evaluating~. Then InnerModuleEvaluation is called on _A_ again, which is a no-op because it is already ~evaluating~. At this point, _D_.[[PendingAsyncDependencies]] is 0, so ExecuteAsyncModule(_D_) is called and triggers the execution promise for _D_. We unwind back to the InnerModuleEvaluation on _B_, setting _B_.[[PendingAsyncDependencies]] to 1 and _B_.[[AsyncEvaluation]] to *true*. We unwind back to the original InnerModuleEvaluation on _A_, setting _A_.[[PendingAsyncDependencies]] to 1. In the next iteration of the loop over _A_'s dependencies, we call InnerModuleEvaluation on _C_ and thus on _D_ (again a no-op) and _E_. As _E_ has no dependencies, ExecuteAsyncModule(_E_) is called, which triggers its execution promise. Because _E_ is not part of a cycle, it is immediately removed from the stack and transitions to ~evaluated~. We unwind once more to the original InnerModuleEvaluation on _A_, setting _C_.[[AsyncEvaluation]] to *true*. Now we finish the loop over _A_'s dependencies, set _A_.[[AsyncEvaluation]] to *true*, and remove the entire strongly connected component from the stack, transitioning all of the modules to ~evaluating-async~ at once. At this point, the fields of the modules are as given in <emu-xref href="#table-module-graph-cycle-async-fields-1"></emu-xref>.</p>

<emu-table id="table-module-graph-cycle-async-fields-1" caption="Module fields after the initial Evaluate() call">
Copy link
Contributor

Choose a reason for hiding this comment

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

These tables are pretty wide and I can't get a horizontal scrollbar in my local rendering on Chrome for some reason.

spec.html Outdated
</table>
</emu-table>

<p>Let us assume that _E_ finishes executing first. When that happens, AsyncModuleExecutionFulfilled is called, _E_.[[Status]] is set to ~evaluated~ and _C_.[[PendingAsyncDependencies]] is decremented to become 1. The fields of the updated modules are as given in <emu-xref href="#table-module-graph-cycle-async-fields-2"></emu-xref>.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

The example is confused. The first paragraph talks about E transitioning to evaluated, suggesting it's not async. But here it's being async evaluated. I guess the first paragraph is wrong?

Copy link
Contributor Author

@codehag codehag Jun 14, 2021

Choose a reason for hiding this comment

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

Yes, I think the previous paragraph was wrong and E should have been evaluating-async at that point. It only transitions here.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@syg syg added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Jun 2, 2021
@ljharb ljharb force-pushed the master branch 2 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@bakkot
Copy link
Contributor

bakkot commented Jul 18, 2021

Force-pushed to resolve conflicts, mostly with #545, and also added a commit adopting the format introduced by #545.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 19, 2021

Now that #2439 has merged, should ExecuteAsyncModule and FinishDynamicImport use abstract closures (to pass to CreateBuiltinFunction)?

@bakkot
Copy link
Contributor

bakkot commented Jul 19, 2021

Yeah, probably. I'll do that shortly after discussing my review comments, to avoid conflicts.

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

I think this works. Just had some comments on the details.

spec.html Outdated
@@ -26064,7 +26682,8 @@ <h1>
1. Append _ee_ to _starExportEntries_.
1. Else,
1. Append _ee_ to _indirectExportEntries_.
1. Return Source Text Module Record { [[Realm]]: _realm_, [[Environment]]: *undefined*, [[Namespace]]: *undefined*, [[Status]]: ~unlinked~, [[EvaluationError]]: *undefined*, [[HostDefined]]: _hostDefined_, [[ECMAScriptCode]]: _body_, [[Context]]: ~empty~, [[ImportMeta]]: ~empty~, [[RequestedModules]]: _requestedModules_, [[ImportEntries]]: _importEntries_, [[LocalExportEntries]]: _localExportEntries_, [[IndirectExportEntries]]: _indirectExportEntries_, [[StarExportEntries]]: _starExportEntries_, [[DFSIndex]]: *undefined*, [[DFSAncestorIndex]]: *undefined* }.
1. Let _async_ be _body_ Contains `await`.
Copy link
Contributor

@bakkot bakkot Jul 19, 2021

Choose a reason for hiding this comment

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

I'm not 100% convinced this is sensible according to the current definition of Contains, but I see that we do something similar with "Contains super" in the Early Errors for script and module bodies, so I guess there's precedent.

(Note to self: if we decide this is sensible, it can probably be adopted by class static blocks as well.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a worthwhile shorthand to define. I'm leaning towards allow this here and elsewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that we do something similar with "Contains super" in the Early Errors for script and module bodies, so I guess there's precedent.

E.g. It is a Syntax Error if |ModuleItemList| Contains `super`. But that's an example of omitting the is *true*, so isn't a precedent for the usage here. I.e.
Let _async_ be _body_ Contains `await`.
isn't an abbreviation of:
Let _async_ be _body_ Contains `await` is *true*.


If you're talking about a Contains-invocation supplying the value for a Let, then the only precedent is in the default definition for Contains:
Let _contained_ be the result of _child_ Contains _symbol_.
I think the result of is there to prevent a garden-path parse. E.g., if it were
Let _contained_ be _child_ Contains _symbol_.
then someone might read
Let _contained_ be _child_
as a unit, and then be confused when they hit Contains. But if you think there's a low risk of confusion, then dropping the result of is okay. (Most Contains-invocations don't have it.)

spec.html Outdated
</td>
<td>
Initially ~unlinked~. Transitions to ~linking~, ~linked~, ~evaluating~, ~evaluated~ (in that order) as the module progresses throughout its lifecycle.
Initially ~unlinked~. Transitions to ~linking~, ~linked~, ~evaluating~, possibly ~evaluating-async~, ~evaluated~ (in that order) as the module progresses throughout its lifecycle. ~evaluating-async~ indicates this module is queued to execute on completion of its async dependencies or it is an async module that has been executed and is pending top-level completion.
Copy link
Contributor

Choose a reason for hiding this comment

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

"async module" isn't defined, and it's not 100% clear what it means here. There's also a use of "asynchronous module" below.

We should maybe dfn the definition of the [[Async]] field which says something like A module for which this field is *true* is called an <dfn>async module</dfn>. (assuming that's an accurate definition, anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

In this particular instance "async module" seems to mean individually async, i.e. a module whose [[Async]] field is true. Colloquially "async module" probably doesn't mean just that but also means if there's a module whose [[Async]] field is true in the transitive closure of dependencies.

For here I'd replace "async module" with either "individually async module" or "module whose [[Async]] field is true".

Choose a reason for hiding this comment

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

There is definitely room for confusion between an individually async module, and a module with async dependencies.

Perhaps we can avoid the term entirely and use something like "or it is a module containing a top-level await that has been executed and is pending top-level completion".

Copy link
Contributor

@syg syg Aug 4, 2021

Choose a reason for hiding this comment

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

Concrete suggestion after chatting on the editor call. The root cause here is that [[Async]] is a poorly named field. Let's rename it to [[HasTLA]] (and expand the TLA acronym in the definition), and replace this use of "async module" with "module whose [[HasTLA]] field is true (i.e. containing a top-level await)".

There's another occurrence of "async module", not yet clear what to replace that with.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Jul 19, 2021

Just to check my understand, are all of these statements true?

  • All of the modules in a given strongly connected component have the same [[CycleRoot]]
  • All of the modules in a given strongly connected component have the same [[DFSAncestorIndex]]
  • Said [[DFSAncestorIndex]] is equal to [[CycleRoot]].[[DFSIndex]]
  • m.[[TopLevelCapability]] being non-empty implies m.[[CycleRoot]] = m (though the reverse implication does not hold)

@guybedford
Copy link

@bakkot yes those statements are all correct.

@bakkot bakkot added the editor call to be discussed in the next editor call label Jul 21, 2021
@codehag
Copy link
Contributor Author

codehag commented Jul 22, 2021 via email

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jul 22, 2021
@bakkot
Copy link
Contributor

bakkot commented Jul 22, 2021

Pushed a few commits resolving all of my comments except the "async module" one, because I didn't have a wording I liked to use there.

@codehag
Copy link
Contributor Author

codehag commented Aug 10, 2021

@bakkot looks good to me

@bakkot bakkot added the editor call to be discussed in the next editor call label Aug 10, 2021
spec.html Outdated
@@ -25313,7 +25313,7 @@ <h1>Cyclic Module Records</h1>
~unlinked~ | ~linking~ | ~linked~ | ~evaluating~ | ~evaluating-async~ | ~evaluated~
</td>
<td>
Initially ~unlinked~. Transitions to ~linking~, ~linked~, ~evaluating~, possibly ~evaluating-async~, ~evaluated~ (in that order) as the module progresses throughout its lifecycle. ~evaluating-async~ indicates this module is queued to execute on completion of its async dependencies or it is an async module that has been executed and is pending top-level completion.
Initially ~unlinked~. Transitions to ~linking~, ~linked~, ~evaluating~, possibly ~evaluating-async~, ~evaluated~ (in that order) as the module progresses throughout its lifecycle. ~evaluating-async~ indicates this module is queued to execute on completion of its async dependencies or it is a module whose [[HasTLA]] field is *true* that has been executed and is pending top-level completion.
Copy link
Contributor

Choose a reason for hiding this comment

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

"async dependencies" -> "asynchronous dependencies"

@bakkot bakkot removed the editor call to be discussed in the next editor call label Aug 13, 2021
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Aug 13, 2021
Co-authored-by: codehag <yulia.startsev@gmail.com>
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
Co-authored-by: Kevin Gibbons <bakkot@gmail.com>
@ljharb ljharb dismissed jmdyck’s stale review August 15, 2021 19:50

changes addressed

@ljharb ljharb merged commit 7edbc7f into tc39:master Aug 15, 2021
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
Co-authored-by: codehag <yulia.startsev@gmail.com>
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
Co-authored-by: Kevin Gibbons <bakkot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants