Store and rethrow module instantiation/evaluation errors

[domenic]

This addresses #862 by ensuring that repeated calls to ModuleDeclarationInstantiation() and ModuleEvaluation(), for Source Text Module Records, rethrow any exceptions they previously threw, instead of silently succeeding. This allows host environments to do top-down instantiation/evaluation of module graphs, instead of having to do bottom-up instantiation/evaluation in order to record individual failures and thus prevent future instantiation/evaluation.

In the process, this helps formalize some of the invariants previously stated in a vague way, such as "ModuleDeclarationInstantiation must have completed successfully", replacing them instead with an explicit [[Status]] field whose contents can be asserted against.

For background on the trouble caused by the previous approach of silent success, see:

• Module records should be be discarded on link failure whatwg/html#1545
• Importing modules which failed evaluation previously #862
• Circular module graphs instantiate too early whatwg/html#2629
• Fix error cases of <script type=module> whatwg/html#2595 (comment)

---

I've verified this works in some detail via extensive case analysis.

The corresponding HTML change is at whatwg/html#2674

• ES spec preview with these changes
• HTML spec preview with the corresponding changes

---

I'd be thrilled if the editor/committee felt comfortable accepting this without needing to wait for the in-person meeting; that would certainly help us implement modules in Chrome and write valid web platform tests for them. We'd rather avoid a two-week delay in velocity, so we'll probably proceed assuming something with the same observable effects as this is acceptable, as I hope it will be.

The only potentially-opinionated thing here is rethrowing the same error, instead of some other way of "not completing successfully", but hosts who wanted a different error signal could easily do so by wrapping the calls to ModuleDeclarationInstantiation()/ModuleEvaluation(), so I don't think this limits expressive power. We also have strong signals from both Node and browsers that this is the behavior we'd prefer, and I think standardizing it across environments would be beneficial.

---

The alternative to this PR is to add more host hooks to allow hosts to accomplish the same thing. In particular, that would consist of something like:

• A hook called at the end of Source Text Module Records' ModuleDeclarationInstantiation() and ModuleEvaluation(), passed the completion value of the instantiation/evaluation process, which allows us to store thrown exceptions in [[HostDefined]] for later retrieval
• A hook called in the beginning of Source Text Module Records' ModuleDeclarationInstantiation() and ModuleEvaluation(), which allows us to throw the stored-in-[[HostDefined]] exceptions

Alternately, both Node.js and browsers could collaborate on a new spec for "For Realz Source Text Module Records" which are new implementations of the Abstract Module Records concept, wrapping Source Text Module Records and adding the appropriate before/after behavior.

Both of these options seem less good than having the behavior in 262. Especially given the nice bonus of the explicit [[Status]] field that this PR gives; I'd be sad to see 262 go back to vaguely asserting "ModuleDeclarationInstantiation must have completed successfully".

As such, I'd love a signal from the editor/committee as to whether I need to invest time in such alternate approaches, or if they think it's likely the strategy in this PR will be acceptable.

[ljharb]

Having explicit ways to describe the state and the completion record of a failure seems strictly superior to an [[Evaluated]] boolean 👍

[ljharb]

Is there a benefit to having both "status" and "errorCompletion", as opposed to having something where "evaluated" would be a NormalCompletion, and "errored" an AbruptCompletion?

Given that Status has 5 states, and only three of them would be obvious in this model (undefined, ?, ?, NormalCompletion, AbruptCompletion), I'm not convinced this is the better path, but I wanted to raise the question - it seems odd to me to have a field that's only meaningful based on the value of another field.

[domenic]

I don't think you can distinguish undefined and NormalCompletion(undefined).

I don't think there's a way to mash this into one field unless we make it multi-typed, e.g. "uninstantiated", "instantiating", "instantiated", "evaluated", or an abrupt completion. Which seems worse to me.

This kind of thing is a reasonably-common pattern in stateful systems. If you end up having too many such fields you'll want to refactor to something polymorphic, but just one seems fine.

[ljharb]

Sounds good; no strong opinion here, but i wanted to ask :-)

[rossberg]

To smart-ass around a bit: if you end up having such fields then that's due to proper variant datatypes (a.k.a. disjoint unions) missing from the language, which no amount of refactoring can really make up for. :)

[GeorgNeis]

It looks like it's possible to have a module end up in state "instantiated" even though one of its dependencies ends up in state "errored". Example:

A: import "B"; import {x} from "C";
B: import "A";
C: // empty

Instantiation of A will trigger instantiation of B while A is in state "instantiating". This instantiation of B will leave B in state "instantiated", even though, later on, instantiation of A completes with a failure due to x being unresolvable.

[domenic]

@GeorgNeis great catch; cyclic graphs always manage to trip this sort of stuff up.

I'll try to figure out a solution, but do you have any ideas yourself on the right way to handle such issues?

[erights]

It seems we need to treat the dependency graph as an acyclic graph of strongly connected components (SCC), as is often done with directed graphs. Then an SCC as a whole makes an atomic transition to state "instantiated" or "erroneous".

In this case, A+B is an SCC dependent on the SCC consisting solely of C. Neither A nor B become instantiated or erroneous until both A and B make such a transition together.

[allenwb]

This shouldn't be a problem. ModuleInstantiation is not supposed to have any observable side-effects (the specified forms of ModuleDeclarationEvaluation do not evaluate any ECMAScript code). In fact, the design intend is that it can be done ahead of time by a "linker"

You can see this in TopLevelEvaluationJob steps 5-7. (and I don't care that "no browser does this". It's what all root-level module imports were supposed to be modeled after). If there were any errors during module instantiations then we never reach step 7, which is where all observable evaluations of this root-level load start.

It's up to an implementation whether it keeps or discards ModuleRecords (and associated artifacts) that are produced as part a ModuleDeclarationInstantiation that failed. It's all unobservable internal state.

[domenic]

@allenwb, the problem is we need to know which ModuleDeclarationInstantiation errored, and remember that error. TopLevelModuleEvaluationJob doesn't allow for that.

I don't see you addressing that in your post; indeed your post seems unrelated to this PR as far as I can tell.

I'd strongly recommend (re-)reading whatwg/html#1545 for background.

[allenwb]

This seems central to why rethrowing (and propagating) internal generated exception objects to ES requesters of a root module load (eg, dynamic import) may be problematic. Such exception objects provide an observable window into module load processing and is state that can be compared between distinct attempts to load dependency related modules.

The "rethrow model" seems predicated on the assumptions that internal module load processing is generating ES exception objects to report internally discovered inconsistencies (missing modules, unresolvable imports, etc.). But, the internal error processing model should be an implementation details. For example, consider an ahead of module linker written in, for example, Rust).

In the ES6 module spec. I used abrupt completions within the specification of the module semantics as a convenient way to propagate error conditions through the algorithms. However, I did not intend to imply that those errors were reported via ES exception objects. You see a bit of this in the way that ParseModule errors are handled in steps 3 and 4 of TopLevelModuleEvaluationJob. Arguably, something similar should have been done for step 5 and possibly step 7 errors abrupt completions. But for the ES6 spec, it didn't really make any difference because the only root-level module load operation was TopLevelModuleEvaluationJob and there was no ES specified semantics that allowed for the observation of exception objects that might be produced by steps 5 and 7.

My position, is that additional root-level module load APIs such as dynamic import should be specific such that they throw (from their internal top-level) unique (to each invocation) exception objects reporting the failure. They can carry as much payload as they wish to identify the kind and cause of a failure, in user meaningful terms. But they should not propagate or rethrow internal exceptions used within the implementation of the load process.

Leaking internal exceptions to API clients is a classic layering bug. We should be mandating it.

[domenic]

It is crucial for browsers (and, so I've heard, for Node) that errors are cached so that telemetry can be used to identify repeatedly-encountered problems in a page.

I understand if you were implementing a host you would not do that. But I don't think the spec should prohibit such host behavior, and I do think it should enable it to be possible. Whether that is via mechanisms such as this PR, or via the alternatives I discuss in the OP (such as browsers not using source text module records at all) is up for discussion, but I am hopeful the committee can cooperate fruitfully with host environments on the module feature, given how dependent on hosts it is in general.

[allenwb]

Des cache error information === caching and rethrowing exception objects? Does some telemetry package depend upon object identity of exceptions? Certainly exception identity is lost as soon as you serialize error information.

I don't see what in the spec. currently precludes caching any sort of error information you desire.

[domenic]

Des cache error information === caching and rethrowing exception objects? Does some telemetry package depend upon object identity of exceptions?

Yes; errors are often aggregated on the client side, using object identity.

I don't see what in the spec. currently precludes caching any sort of error information you desire.

I feel that I've explained this a few times, as do the threads I've linked to, but I'll try again...

If you call ModuleDeclarationInstantiation(), or ModuleEvaluation(), on a root Source Text Module Record, then you cannot tell which module failed to instantiate or evaluate. You can only tell that something in the graph failed. This means you cannot properly associate the instantiation/evaluation error with the module that failed.

You can work around this, as we did in whatwg/html#1545, by doing "bottom up" instantiation/evaluation instead of "top down": that is, instead of a single instantiate/evaluate per graph (similar to TopLevelModuleEvaluationJob), you instantiate/evaluate each individual node in the graph appropriately, once its dependencies are instantiated/evaluated. This allows you to determine which node caused the failure. You can indeed do this with the current spec; it's what HTML currently does, for instantiation at least, although not (yet?) evaluation.

However, this is hard to do correctly, as the links in the OP show. (They are bugs in the current HTML spec text which attempts the bottom-up approach.)

So it'd be easier for hosts if we could make top-down evaluation work for hosts' needs. This could be done in a variety of ways, as discussed in the OP: we could record errors for the hosts as this PR does; we could insert host hooks before/after every instantiation/evaluation to allow hosts to record errors themselves; or hosts could just not use source text module records.

Hope this helps...

[allenwb]

Yes; errors are often aggregated on the client side, using object identity.

In what existing circumstances do ECMAScript semantics result in distinct actions at distinct times yield throwing === exception objects? Curious minds want to know

If you call ModuleDeclarationInstantiation(), or ModuleEvaluation(), on a root Source Text Module Record, then you cannot tell which module failed to instantiate or evaluate.

The specs. for MDI and associated operations such as ResolveExport explicitly throw exceptions (usually SyntaxError) for various situations. Implementations are free to associate appropriate host/implementation specific information with such exceptions, just like they can for any specified abrupt completion. They can do so via the message property of the exception object, or by adding non-standard properties to the exceptions, or by any other mechanism they devise. Similarly, operations like ModuleEvaluation can add additional information to exceptions that propagate through them.

I guess I don't see why this is different from the association of sourcefile/linenumber or stacktrace info with spec generated exceptions that is routinely done by implementations.

The novel concept seems to be reusing exceptions. How is this case fundamentally different from:

let badstr=`
   var foo=1;  //good syntax
   this is not valid JS source code
   `;
let e1,e2;
try {eval(badstr)} catch(e) {e1=e};
try {eval(badstr)} catch(e) {e2=e};
console.log(e1===e2);  //specified to be false, but e1&e2 might report same source position of error

[benjamn]

I suppose you could argue that

try {eval(badstr)} catch(e) {e1=e};
try {eval(badstr)} catch(e) {e2=e};

differs from the module case because you're evaluating badstr as two distinct script fragments, even though it happens to be the same string of code both times. If badstr has any side effects, you could observe those effects happen twice.

In the module case, we expect instantiation to have no observable side effects, and evaluation to happen only once (idempotence). The module's code should never be reevaluated when you import the module again. In some sense, then, there's only ever one eval(moduleSource) behind the scenes (pardon my informality here), and thus at most one exception object.

The top-down evaluation strategy that @domenic is proposing assumes module evaluation is idempotent, so I think you have to take that into account when comparing to eval. Though it's a bit more involved, I believe this toy code more accurately reflects what's happening with modules:

function instantiateModule(source) {
  let result, error, status;

  return function evaluateModule() {
    if (status === "evaluated") return result;
    if (status === "errored") throw error;
    try {
      result = eval(source);
      status = "evaluated";
    } catch (e) {
      error = e;
      status = "errored";
    }
    return evaluateModule();
  }
}

const evaluateModule = instantiateModule(badstr);

let e1, e2;
try { evaluateModule() } catch (e) { e1 = e }
try { evaluateModule() } catch (e) { e2 = e }

If you agree that this code captures the essence of module instantiation/evaluation (and I would be happy to know where it goes wrong, if you disagree), then it I hope it's now a little easier to accept that e1 === e2 is the desired outcome.

[jonco3]

Setting the module status to "evaluated" at this point will cause an infinite loop if we have a cycle in the module graph. We either need to set this earlier or set an 'evaluating' status while a module is evaluating.

[domenic]

In what existing circumstances do ECMAScript semantics result in distinct actions at distinct times yield throwing === exception objects?

Promises are the most common example; they vend the same rejection reason every time you call .then()/.catch().

Streams, similarly, whenever they error, vend the same exception through their various API surfaces. (Not sure if web platform APIs are included in "ECMAScript semantics", but since we're talking about existing circumstances that cause libraries to do such error aggreggation, I thought it was a helpful example.)

The novel concept seems to be reusing exceptions. How is this case fundamentally different from:

@benjamn makes it pretty clear through his example. To repeat, module instantiation/evaluation is meant to be idempotent.

---

As a status update, @GeorgNeis has a work-in-progress patch around the cyclic graph issues that I am hoping to get posted here soon. Right now we're struggling with graphs that involve both source text and non-source text module records. In any case, I anticipate a good committee discussion on this subject over the next few days.

[domenic]

Probably should rename "default" to "normal" to avoid confusion with default export.

[domenic]

No underscores around ResolutionFailure (here and elsewhere)

[bterlson]

What is "strongly-connected"

[domenic]

https://en.wikipedia.org/wiki/Strongly_connected_component

[bterlson]

Now that there are so many abstract ops I'd appreciate some intro prose description of their purpose and invariants.

[domenic]

Done. I think the invariants are largely covered by the table for Abstract Module Records.

[bterlson]

I think object equality usually calls "SameValue" or some such?

[domenic]

This is a completion value though, so SameValue doesn't quite apply. Cf. "Assert: module.[[Realm]] is not undefined."

[bterlson]

First, sorry about being so late to review this. Overall I like it and still agree with the committee consensus to merge this PR.

I agree with what @allenwb is saying above: nothing in the current spec text precludes an implementation from implementing exactly this (AFAICT you could consider your implementation to either be another subclass of AMR with a different top-level evaluation job, or perhaps a subclass of STMR itself?). However, it seems very reasonable that caching of errors is something that hosts will want to do frequently (our two biggest have already said as much). Further, I think regardless of what choices you would make as a host, the changes in this PR help clarify how module resolution works. And from a layering perspective, and modulo "needless" complexity, the more implementations can make use of built-in algorithms as-is or with trivial changes, the better for everyone.

That said, on the complexity front, this PR adds quite a bit. It's not as bad as the diff makes it seem but it's still more to page in and there are specific problem areas (notably, the resolveMode/newResolveMode song and dance in the new ResolveExport). I propose we mitigate this new complexity in at least three ways:

1. Refactor ResolveExport to not take a resolveMode and simply return the resolved module or a record that gives the caller information about what module failed to resolve.
2. Prose descriptions before every STMR method describing what it does and the state transitions it makes under various initial conditions.
3. A high level description of how STMR resolution proceeds (possibly drawing on Domenic's massive doc in OP of the various scenarios). This need not block the PR.

I'm going to continue thinking through other aspects of this PR throughout the weekend so don't be surprised if I pop back in :)

[domenic]

I've completed points (2) and (3); please take a look at the preview.

I also threw in a bonus change of renaming ModuleDeclarationInstantiation to Instantiate and ModuleEvaluation to Evaluate, but we can easily revert that if it's not desired.

[GeorgNeis]

I don't see how to remove ResolveExport's resolveMode argument.

What I can do is make ResolveExport return (in the error case) the exception and the culprit module, as suggested, but only remove the "namespace" mode. The distinction between "normal" and "star" mode is still necessary for knowing whether a failure to resolve the name is actually an error or not. Here's an example for illustration (we want to load module A):

A: import {x} from "B"
B: export * from "C"; export * from "D"
C: // empty
D: export {x} from "C"

When trying to resolve x in B, we try to resolve x in C. This fails but does not constitute an error. Then, when trying to resolve x in D, we again try to resolve x in C and fail, but this time it does constitute an error.

Note that by returning a single culprit module and having InnerModuleDeclarationInstantiation record the error there, we'd be recording it in less modules than in the current version, where we record it also in all modules that ResolveExport visited on the way to the culprit module. This difference is arguably insignificant, though, since it cannot be observed by the user (at least when the host is HTML).

Best,
Georg

[bterlson]

@GeorgNeis Alright I see the difficulty. I still think removing one possible value of the parameter will be helpful. What do you think though?

Can you give an example how this change will impact where the error metadata gets stored? I think it's conceptually good that SCCs transition state together and hold the same metadata - will that still be the case?

@domenic your recent changes are possibly the best editorial copy in the spec now! I might add a description of how index, [[DFSIndex]] and [[DFSAncestorIndex]] are used (yeah it's numbering each instantiated module in a depth-first manner (?) but description is warranted IMO). I'll probably make editorial updates for brevity/clarify after accepting the PR rather than go back and forth here.

[GeorgNeis]

@GeorgNeis Alright I see the difficulty. I still think removing one possible
value of the parameter will be helpful. What do you think though?

I'm fine with making this change.

Can you give an example how this change will impact where the error metadata
gets stored? I think it's conceptually good that SCCs transition state
together and hold the same metadata - will that still be the case?

Let me first comment on the second question. Already in the current proposal
it's generally not the case that all modules of an SCC transition together: in
the error case, some transition to being 'errored' while others may remain
'uninstantiated'. This is simply because we abort instantiation when we hit an
error and at that point we may not have visited all modules of the affected
SCCs.

Simple example:

A: import "B"
B: import "C"; import "D"
C: import {x} from "C"
D: import "A"

There are two SCCs: {A, B, D}, {C}

Assuming all modules are 'uninstantiated', the effect of
A.ModuleDeclarationInstantiation() is:
A, B, C become 'errored'.
D remains 'uninstantiated'.

The suggested change to ResolveExport does not affect the example above. So,
regarding the first question, here's a different example:

A: import "B"; export {x} from "C"
B: import {x} from "A"
C: export {x} from "D"
D: export {x} from "D"

There are three SCCs: {A, B}, {C}, {D}

Assuming all modules are 'uninstantiated', the effect of
A.ModuleDeclarationInstantiation() is:
A, B, C, D become 'errored'.

With the suggested change to ResolveExport, the effect of
A.ModuleDeclarationInstantiation() would be:
A, B, D become 'errored'.
C remains 'uninstantiated'.

Now, there's the general question of what happens when later on we try to
instantiate (another graph containing) one of those modules that remained
'uninstantiated' during a failed instantiation and now have a direct or indirect
'errored' dependency (D in the first example, C in the second example after the
change).

I'd like there to be the property that if a graph already contains 'errored'
modules, then its instantiation will fail with the "first" such error, and it
seems reasonable to make "first" mean "first in DFS order".

The current proposal by itself does not enforce this: instantiation of such a
graph can result in a different error, even one that doesn't yet exist when
instantiation starts.

In the HTML proposal, however, the property is enforced by refusing to call
ModuleDeclarationInstantiation when one of the dependencies is already errored
(and reporting that error). Consequently, HTML's HostResolveImportedModule
callback never throws.

It was my suggestion to do this on the host side because it is a small change to
the fetching procedure in HTML, whereas in ES it would require an extra DFS
traversal separate from and prior to the actual instantiation pass, blowing up
the size and complexity of ModuleDeclarationInstantiation even more.

By now, though, I feel that adding this additional traversal to ES is the right
thing to do. Besides enforcing the first-error-property for all hosts, this
also means that the host doesn't need to know about the [[Status]] and
[[ErrorCompletion]] fields. If these fields are encapsulated, then the suggested
change to ResolveExport becomes unobservable to the host.

[GeorgNeis]

I've changed ResolveExport such that if resolution fails, it returns the culprit module or null if none exist. I've also realized that knowing whether or not there is a culprit lets me get rid of resolveMode altogether, so I did that.

Mostly independent of that, I have corrected some of the existing explanations and examples.

[bterlson]

I like the resolveMode change, personally, so thanks for doing that! Also appreciate the editorial updates and corrections. I'm going to talk to a few people in person about this change here and then pull in!

[bterlson]

Note to self: make sure squash takes @GeorgNeis's commit metadata not Domenic (as I think would be the default).

[bterlson]

@GeorgNeis @domenic thank you for your tireless efforts here.