Do not remember module instantiation errors. by GeorgNeis · Pull Request #1006 · tc39/ecma262

GeorgNeis · 2017-09-22T08:00:18Z

A while ago, we changed the specification of modules such that instantiation and evaluation errors are recorded and then re-thrown by later instantiation/evaluation attempts. We have since realized that, in the case of instantiation errors, this was not a good idea, as it does not give us the kind of determinacy that we want.

The problem has to do with instantiating multiple "root" modules in a non-deterministic order, which happens e.g. due to multiple <script type=module ...> elements or multiple import() expressions. What we want to guarantee is at least "blame determinism", i.e., no matter
at which point an instantiation happens, if it fails then the error always originates from the same importing statement.

In cyclic graphs, having a failed instantiation record the error is in conflict with this goal. Consider this example:

<!-- index.html -->
<script type="module" src="./A.js"></script>
<script type="module" src="./B.js"></script>

// A.js
import "./B.js"
import {something} from "./empty.js"

// B.js
import "./A.js";
import {something_else} from "./empty.js"

If A gets instantiated first, both script loads result in an error blaming B's import of "something_else". If B gets instantiated first, both loads result in an error blaming A's import of "something".

Our solution is simply to not record instantiation errors but instead reset a module to "uninstantiated" if instantiation fails. In the example above, the load of A will then result in an error blaming B's import of "something_else", and the load of B will result in an error blaming A's import of "something" --- no matter which one gets instantiated first. (The error objects are not identical. While it's possible to enforce even this, it would require significant extra complexity, which
is why we decided against it.)

The treatment of evaluation errors remains unchanged: unlike instantiation, evaluation must not happen more than once, and is not idempotent, so we definitely need to record the error there. (See launchTheMissiles() in #862.)

Summary of the changes proposed in this PR:

Main changes:

Instantiation errors are no longer remembered.
Instantiation failure resets module to status "uninstantiated".
Since the only recorded errors are now evaluation errors, instantiation can treat errored modules like instantiated modules.

Minor changes include:

Reverted ResolveExport to what it was originally.
Renamed [[ErrorCompletion]] to [[EvaluationError]].
Replace "errored" status by "evaluated" with non-undefined
[[EvaluationError]].
Moved [[Status]] and [[EvaluationError]] from Abstract MR to Source Text MR.
Renamed InnerModuleDeclarationInstantiation to InnerModuleInstantiation.

Related pull request for HTML: whatwg/html#2991

allenwb · 2017-09-22T16:22:12Z

😀

bterlson · 2017-10-10T19:26:27Z

Give me a little bit to read through the diff but the approach seems fine.

…iation errors Today, we are in somewhat stale state where new module tree fetching algorithm [1][2] is partially applied. This CL (temporarily) disables an assert which exists in the final algorithm, but doesn't hold today. Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently doesn't hold, as the instantiation may have failed for a module script node, and current V8 implementation records the instantiation error as an error. See the attached test case for an example. [1] whatwg/html#2991 [2] tc39/ecma262#1006 [3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier) Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html Bug: 772750, 763597 Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16

…iation errors Today, we are in somewhat stale state where new module tree fetching algorithm [1][2] is partially applied. This CL (temporarily) disables an assert which exists in the final algorithm, but doesn't hold today. Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently doesn't hold, as the instantiation may have failed for a module script node, and current V8 implementation records the instantiation error as an error. See the attached test case for an example. [1] whatwg/html#2991 [2] tc39/ecma262#1006 [3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier) Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html Bug: 772750, 763597 Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16 Reviewed-on: https://chromium-review.googlesource.com/708078 Reviewed-by: Yutaka Hirano <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#507888}

bterlson · 2017-10-12T20:49:49Z

This looks good. Especially appreciate the updated copy in 15.2.1.16.6 Example Source Text Module Record Graphs. I introduced a merge conflict - will attempt to resolve soon unless someone beats me to it.

bterlson · 2017-10-12T21:19:48Z

Alright I think I rebased correctly. @GeorgNeis @domenic could one of you take a quick look and make sure I didn't hose anything?

domenic · 2017-10-12T21:49:26Z

LGTM

…iation errors Today, we are in somewhat stale state where new module tree fetching algorithm [1][2] is partially applied. This CL (temporarily) disables an assert which exists in the final algorithm, but doesn't hold today. Specifically, the assert in HostResolveImportedModule (HRIM [3]) Step 7 currently doesn't hold, as the instantiation may have failed for a module script node, and current V8 implementation records the instantiation error as an error. See the attached test case for an example. [1] whatwg/html#2991 [2] tc39/ecma262#1006 [3] https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-specifier) Test: external/wpt/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html Bug: 772750, 763597 Change-Id: Ida600598b658c74cdbda5937219f1e62a41f2a16 Reviewed-on: https://chromium-review.googlesource.com/708078 Reviewed-by: Yutaka Hirano <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#507888}

…struction (tc39#1007) Fixes tc39#1006

chromium-wpt-export-bot mentioned this pull request Oct 11, 2017

[ES6 modules] HRIM may return errored module until V8 forgets instantiation errors web-platform-tests/wpt#7664

Merged

GeorgNeis and others added 3 commits October 12, 2017 14:15

Do not remember instantiation errors.

b0a4e4d

Update examples for these changes

225db2c

Revise example.

8bc7f46

bterlson force-pushed the modules-instantiation branch from 5408ef3 to 8bc7f46 Compare October 12, 2017 21:16

bterlson merged commit c0e07a8 into tc39:master Oct 12, 2017

libin1991 mentioned this pull request Jun 27, 2018

在浏览器中使用原生 JavaScript 模块（译） libin1991/libin_Blog#582

Open

ptomato pushed a commit to ptomato/ecma262 that referenced this pull request Feb 16, 2026

Editorial: Fix incorrect validation of variants for Intl.Locale con…

1f315d5

…struction (tc39#1007) Fixes tc39#1006

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not remember module instantiation errors.#1006

Do not remember module instantiation errors.#1006
bterlson merged 3 commits intotc39:masterfrom
GeorgNeis:modules-instantiation

GeorgNeis commented Sep 22, 2017

Uh oh!

allenwb commented Sep 22, 2017

Uh oh!

bterlson commented Oct 10, 2017

Uh oh!

bterlson commented Oct 12, 2017

Uh oh!

bterlson commented Oct 12, 2017

Uh oh!

domenic commented Oct 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

GeorgNeis commented Sep 22, 2017

Uh oh!

allenwb commented Sep 22, 2017

Uh oh!

bterlson commented Oct 10, 2017

Uh oh!

bterlson commented Oct 12, 2017

Uh oh!

bterlson commented Oct 12, 2017

Uh oh!

domenic commented Oct 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants