Editorial: dedupe and isolate modules traversal logic

[nicolo-ribaudo]

This PR separates the logic to perform a depth-first traversal on module graph out of InnerModuleLinking/InnerModuleEvaluation. It has two advantages:

• it makes it obvious that traversal for linking and evaluation work the same way, since it's not copy&pasted code that needs to be kept in sync
• especially for evaluation, it disentangles the traversal logic from the "keeping track of top-level await consequences" logic.

Whether this is an improvement or not is subjective (it adds a layer of indirection), so I'm
curious to read the editors opinion about it.

From an editorial/formatting point of view, ModuleGraphDFS is an AO that takes a bunch of callbacks. I've declared them inline in the Link()/Evaluate() concrete methods as abstract closures, even though they don't actually close over anything. I think having them inline rather than as separate AOs makes the reading flow simpler. Ideally, my preferred approach would be something that lets us write

1. Perform ModuleGraphDFS(_module_, _initialStatus_, _pendingStatus_) with
     Action(_m_):
       1. ...
     SCCCompletion(_m_, _sccRoot_):
       1. ...
     PostErrorCleanup(_m_, _errorCompletion_):
       1. ...

I suggest reading this commit-by-commit. The first two commits just move logic around so that what would go in the same AC of the third commit is already all in the same place.

This PR preserves the bugs that would be fixed by #3613 and #3583, but by implementing this PR + those two in engine262 all the tests I have still pass.

The import defer proposal would add a new callback to ModuleGraphDFS, _getModuleDescendants_, that for linking just returns the RequestedModules while for Evaluation it uses import defer's step 11 of https://tc39.es/proposal-defer-import-eval/#sec-innermoduleevaluation (with GatherAsynchronousTransitiveDependencies).

[nicolo-ribaudo]

Marking as draft as:

• I actually think I prefer AOs to those inline abstract closures, I'll update accordingly
• I'd prefer Editorial: Use the dfs stack instead of [[DFSAncestorIndex]]  #3637 to be reviewed first, since it's the most tricky one and I'd prefer to not have to rebase it on top of this one.

[github-actions]

The rendered spec for this PR is available at https://tc39.es/ecma262/pr/3635.

[jmdyck]

Also, this PR un-defines the following ids:

• sec-InnerModuleLinking
• sec-innermoduleevaluation
• sec-innermoduleinstantiation

which should maybe be retained in oldids attributes.

[nicolo-ribaudo]

Any preference on whether the oldid for sec-InnerModuleLinking and sec-innermoduleevaluation should point to Link/Evaluate or to InnerModuleGraphDFS?

[jmdyck]

Any preference on whether the oldid for sec-InnerModuleLinking and sec-innermoduleevaluation should point to Link/Evaluate or to InnerModuleGraphDFS?

Me? Maybe a slight pref for InnerModuleGraphDFS.

BTW, just noticed that the PR leaves lots of dangling references to InnerModuleLinking and InnerModuleEvaluation, mostly in Example Cyclic Module Record Graphs but elsewhere too.

[michaelficarra]

Suggested change

<p>Now consider a case where _A_ has a linking error; for example, it tries to import a binding from _C_ that does not exist. In that case, the above steps still occur, including the early return from the second call to InnerModuleGraphDFS on _A_. However, once we unwind back to the original InnerModuleGraphDFS on _A_, it fails during _action_'s _A_.InitializeEnvironment, namely right after _C_.ResolveExport(). The thrown *SyntaxError* exception propagates up to _A_.Link, which resets all modules that are currently on its _stack_ (these are always exactly the modules that are still ~linking~) thourh ModuleGraphDFS's _postErrorCleanup_. Hence both _A_ and _B_ become ~unlinked~. Note that _C_ is left as ~linked~.</p>

<p>Now consider a case where _A_ has a linking error; for example, it tries to import a binding from _C_ that does not exist. In that case, the above steps still occur, including the early return from the second call to InnerModuleGraphDFS on _A_. However, once we unwind back to the original InnerModuleGraphDFS on _A_, it fails during _action_'s _A_.InitializeEnvironment, namely right after _C_.ResolveExport(). The thrown *SyntaxError* exception propagates up to _A_.Link, which resets all modules that are currently on its _stack_ (these are always exactly the modules that are still ~linking~) through ModuleGraphDFS's _postErrorCleanup_. Hence both _A_ and _B_ become ~unlinked~. Note that _C_ is left as ~linked~.</p>

[michaelficarra]

These Evaluate* AOs should be ACs.

[michaelficarra]

ModuleGraphDFS is only called in 2 places: here, where it passes only ACs that capture nothing, and in Evaluate below where it passes AOs (which obviously also capture nothing). That feels like it should just be a flag (a 2-state spec enum) to me.