tc39 / proposal-defer-import-eval

A proposal for introducing a way to defer evaluate of a module
https://tc39.es/proposal-defer-import-eval
MIT License
208 stars 12 forks source link

Note shortpath for gather transitive async dependencies #36

Closed guybedford closed 4 months ago

guybedford commented 4 months ago

Given the complexity of TLA and the invariants, I thought it might be worth noting this one. I had to verify the logic quite carefully myself, and would value a second opinion if the short path does truly work. Feel free to ignore if it feels inappropriate to bring into the spec though.

nicolo-ribaudo commented 4 months ago

In which case module.[[Status]] is ~evaluating-async~ and [[AsyncEvaluation]] is not true?

Also, does this short-circuiting also work in case of modules that error? e.g.

// a
import defer * as b from "b";
// b
import "c";
throw new Error("Err!");
// c
await promise;

assume that:

If GatherAsynchronousTransitiveDependencies returned [ b ], then a would transition to an error state.

guybedford commented 4 months ago

In which case module.[[Status]] is ~evaluating-async~ and [[AsyncEvaluation]] is not true?

During a cycle async evaluation - [[AsyncEvaluation]] will flip one-by-one, while evaluating-async is a state change over the entire strongly connected component at the end, during the InnerModuleEvaluation calls.

This was to maintain the invariant of cycle transitions happening together. Deferred evaluation may be an opportunity to reconsider these invariants as well.

For failure, we have the current failure behaviours when importing a deferred module X:

  1. X fails sync -> throws on namespace access
  2. X failed sync earlier -> does not throw on import and does not throw on namespace access as the evaluation already completed
  3. X fails async -> throws on deferred import
  4. X failed async earlier -> throws on deferred import

Therefore in the transitive handler for a dynamic import, we must make sure to propagate (3) and (4) only, so yes the short path would only apply to [[AsyncEvaluation]] true and in the case of [[AsyncEvaluation]] being false would still need to iterate the graph, and to then propagate the error (although in theory there is an error short path as well here).

I've updated the PR for the [[AsyncEvaluation]] truthy case only for now. It would only be a one-liner to add it next to the [[HasTLA]] check actually, but let me know your thoughts with respect to the editorial nature of this.

nicolo-ribaudo commented 4 months ago

while evaluating-async is a state change over the entire strongly connected component

I don't think so. If you have a graph A->B where B has TLA, there are no strongly connected components (or, there are two strongly connected components with one node each), but A also transitions to ~evaluating-async~ while it's waiting for B to evaluate.

NOTE 1 in https://tc39.es/ecma262/#sec-innermoduleevaluation also mentions that ~evaluating-async~ is for "modules that have TLA or asynchronus dependencies" -- it's not about cycles.

guybedford commented 4 months ago

I don't think so. If you have a graph A->B where B has TLA, there are no strongly connected components (or, there are two strongly connected components with one node each), but A also transitions to ~evaluating-async~ while it's waiting for B to evaluate.

The "Tarjan guarantee" in the ES module system spec is the stack walk at the end of InnerModuleEvaluation where state transitions for a strongly connected component happen together for that entire component. When evaluating-async was implemented, it followed this same guarantee as evaluating -> evaluated, so that this property was maintained. That is, all transitions out of the evaluating state (into evaluating or evaluating-async) are done over the entire strongly connected component. If there is no strongly connected component, the module is effectively alone as its own strongly connected component, and the transitions happen separately of course.

This is also the reason for having both evaluating-async and AsyncEvaluation.

guybedford commented 4 months ago

I realised I forgot to actually push the update here - I've done that now.

guybedford commented 4 months ago

The change in https://github.com/tc39/proposal-defer-import-eval/pull/39 actually subsumes this short path anyway now.