Layering: Add HostLoadImportedModule hook by nicolo-ribaudo · Pull Request #2905 · tc39/ecma262

nicolo-ribaudo · 2022-09-15T14:35:44Z

This PR also removes the HostResolveImportedModule and HostImportModuleDynamically hooks.

A few comments/questions:

ContinueDynamicImport is kinda messy, with all the closures and PerformPromiseThen. Is it ok to extend AsyncBlockStart like in https://tc39.es/proposal-array-from-async/#sec-asyncblockstart so that I can use the Await macro?
ResolveExport is described as returning null or something, but it can also return a throw completion (if it's called on a module that re-exports from a dependency that hasn't been already loaded - NOTE: this can only happen if a host calls ResolveExport() even if LoadRequestedModules() doesn't succeed, and they don't have any good reason to do so). This was the behavior even before this PR.
- Is it ok?
- Should we make it return null?
- Should we require that it's called only after having loaded the dependencies?
I named the new Module Records method LoadRequestedModules() to match the name of the [[RequestedModules]] slot, but maybe LoadImportedModules() or LoadDependencies() are better?
I find the hostDefined parameter of LoadRequestedModules() to be incredibly ugly, but it's needed by HTML to pass some context only to modules imported in "the current loading process" and not in "future loading processes" (the .destination flag it sets on the Request object it creates to fetch modules). It's possible that one day HTML will align requests of static and dynamic imports, but today is not that day yet.
I tracked the loading state across the ecma262-host-ecma262 round trip using a ModuleLoadState record, which is overloaded for two usages that are distinguished using an [[Action]] enum field. Is it ok, or should I split it into two different record types (GraphLoadingState and DynamicImportState)? (related: Layering: Add HostLoadImportedModule hook #2905 (comment))

The HTML PR is whatwg/html#8253. Today's consensus was based on the fact that this refactor is purely editorial for the ECMA-262+(insert known host here) pair, so I would like to get a full review on the HTML side to check if we need any tweaks before moving on with this PR. EDIT: Got a 👍 from the HTML side.

spec.html

nicolo-ribaudo · 2022-10-03T17:33:27Z

The HTML side of this refactor is starting to get reviewed, I would love to receive a review for this PR too!

guybedford

It's possible that one day HTML will align requests of static and dynamic imports, but today is not that day yet.

Would be interested to hear more!

ResolveExport is described as returning null or something, but it can also return a throw completion (if it's called on a module that re-exports from a dependency that hasn't been already loaded - NOTE: this can only happen if a host calls ResolveExport() even if LoadRequestedModules() doesn't succeed, and they don't have any good reason to do so):

I don't think there is any real-world expectation that has ever relied upon ResolveExport throwing, so my preference would be turning this into an internal assertion and enforcing the behaviour that it is only ever called on loaded modules. Is that possible to do?

spec.html

guybedford · 2022-10-04T17:15:27Z

spec.html

+        <p>An implementation of HostLoadImportedModule must conform to the following requirements:</p>
+        <ul>
+          <li>
+            The host environment must perform FinishLoadingImportedModule(_referrer_, _specifier_, _payload_, _result_), where _result_ is either a normal completion containing the loaded Module Record or a throw completion, either synchronously or asynchronously.


In the case where a module has already been loaded, is it up to the host whether it resolves synchronously or asynchronously? I wonder if we should mandate already-loaded modules return synchronously to avoid unnecessary divergence?

Until last week the HTML spec was ambiguous and allowed loading of already-loaded modules to be asynchronous. I updated that spec to make it synchronous, but without first discussing it with implementers, and browsers don't currently agree.

I'm working on it in web-platform-tests/wpt#35949, but I prefer to keep it out of this PR (since the consensus was based on the fact that this was not observably normative in common hosts).

Glad to hear you're working on it!

guybedford · 2022-10-04T17:17:58Z

spec.html

+              </td>
+              <td>
+                This field is only used if [[Action]] is ~graph-loading~. It is true if the loading process has not finished yet, neither successfully nor with an error.
+              </td>


Do we really need a separate [[IsLoading]] state if it is equivalent to checking [[PendingModules]] > 0?

It's not equivalent if the host calls FinishLoadingImportedModule with a throw completion: there might still be pending modules, but we stop the loading process.

spec.html

jmdyck · 2022-10-07T04:29:07Z

Oh, I forgot to say: This PR undefines the ids:

sec-hostresolveimportedmodule
sec-hostimportmoduledynamically
sec-finishdynamicimport

which should maybe become oldids.

nicolo-ribaudo · 2022-10-07T09:05:24Z

Thanks for the review @jmdyck! I added sec-hostresolveimportedmodule/sec-hostimportmoduledynamically as oldids of HostLoadImportedModule, and sec-finishdynamicimport as oldids fo FinishLoadingImportedModule. However, I'm not sure if it's correct because those aren't just renames but completely new AOs.

spec.html

nicolo-ribaudo · 2022-10-19T16:47:23Z

@tc39/ecma262-editors is there anything I could do to help moving this PR forward? There are a bunch of proposals that needs to be updated on top of this, so having it reviewed&merged would be nice.

There are some editorial questions I have (in the issue description), and you can probably give your opinion on them just reading the new spec text without first doing a deep review, so that I can start resolving them :)

spec.html

syg · 2022-10-19T22:14:21Z

@nicolo-ribaudo We discussed some of your questions on the editor call, our preferences are:

We have plans eventually to basically have asynchronous Abstract Closures. For now, since you already wrote the manual promise-handler version, just keep it as is, we'll simplify this when we introduce the ability to have async ACs.
We don't quite grok the scenario you're describing. Are you saying the only reason this method would throw today is if a host calls it in a weird way that no host actually does? If so, please help us confirm if that's the case, and once confirmed, we should add assertions and make the method infallible.
Name sounds good.
More of a comment than a question? Let's keep the host-defined.
Please name and split them into two separate Record types. Get rid of [[Action]] and just query for if the record is one of the specific record types.

michaelficarra · 2022-11-29T09:52:50Z

@nicolo-ribaudo Can you try rebasing on main?

nicolo-ribaudo · 2022-11-29T10:08:15Z

Still failing, maybe because Tobie left google (or at least, it's not linked anymore in his GitHub profile)?

ljharb · 2022-11-29T15:44:35Z

@nicolo-ribaudo ill take a look at it

ljharb · 2022-11-29T18:38:47Z

It's very strange; the passing tests found 146 authors, in the last day or two it's found 147 including tobie. I'll fix it by adding them to the Emeriti team, since they were employed at Google at the time.

spec.html

nicolo-ribaudo · 2022-11-29T21:45:54Z

Thanks @ljharb :)

@jmdyck done!

addressed

This commit also removed the HostResolveImportedModule and HostImportModuleDynamically hooks * Layering: Add HostLoadImportedModule hook * fix(JWK review): some wrong variable names * html review: FinishLoadImportedModule -> FinishLoadingImportedModule * guybedford review * jmdyck review * Add missing _state_.[[IsLoading]] check * syg review + add missing "Return ~unused~" steps * Require LoadRequestedModules before ResolveExport and GetExportedNames

Follows tc39/ecma262#2905. See https://docs.google.com/presentation/d/1RVUE-MENQT8dj2wxvMLMDxg_VoMOwiwNQQged39QIEU/edit for an overview of the changes here. This is not a normative change, and in fact hosts can continue to have the same capabilities as before. But now, much of the graph-crawling work can be offloaded to the JavaScript specification.

…mport. r=jonco From the specification, ContinueDynamicImport is done inside the engine. https://tc39.es/ecma262/#sec-ContinueDynamicImport However, our implementation for EvaluateModuleInContext() has bytecode encoding related stuff, so for now I implement the ContinueDynamicImport in the host layer. Also, the updated spec has a slightly different error handling behavior when module.Link() fails. I list them below: Before HTML PR 8253 [whatwg/html#8253] fetch the descendants of and link a module script https://web.archive.org/web/20221130023614/https://html.spec.whatwg.org/#fetch-the-descendants-of-and-link-a-module-script onFetchDescendantsComplete Step 3.2. Perform record.Link(). If this throws an exception, set result's error to rethrow to that exception. HostImportModuleDynamically https://web.archive.org/web/20221130023614/https://html.spec.whatwg.org/#hostimportmoduledynamically(referencingscriptormodule,-modulerequest,-promisecapability) Step 6.3 Otherwise, set promise to the result of running a module script given result and true. run a module script https://web.archive.org/web/20221130023614/https://html.spec.whatwg.org/#run-a-module-script Step 5. If script's error to rethrow is not null, then set evaluationPromise to a promise rejected with script's error to rethrow. -------------------------------------------------------- After ECMA262 PR 2905 [tc39/ecma262#2905] ContinueDynamicImport Step 6.a Let link be Completion(module.Link()). Step 6.b If link is an abrupt completion, then Step 6.b.i. Perform ! Call(promiseCapability.[[Reject]], undefined, « link.[[Value]] »). Step 6.b.ii. Return unused. In short, * The old behavior: It catched the exception thrown during module.Link(), and set the module script's error to rethrow to the exception. Later in Evaluate(), if the module script's error to rethrow is not null, reject the evaluation promise with the error to rethrow. * The new behavior: It simply rejects the evaluation promise with the exception thrown during module.Link(). Differential Revision: https://phabricator.services.mozilla.com/D214577

nicolo-ribaudo added the layering affects the public spec interface and may require updates to integrating specs label Sep 15, 2022

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch 2 times, most recently from bac620f to 36635e0 Compare September 15, 2022 14:41

Jack-Works reviewed Sep 16, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

Jack-Works reviewed Sep 16, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

Jack-Works reviewed Sep 16, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

Jack-Works added a commit to DimensionDev/aot-secure-ecmascript that referenced this pull request Sep 16, 2022

chore: follow tc39/ecma262#2905

2a8c9f7

github-actions bot mentioned this pull request Sep 16, 2022

Version Packages DimensionDev/aot-secure-ecmascript#31

Merged

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch from e73116d to 407474b Compare September 20, 2022 11:00

nicolo-ribaudo mentioned this pull request Sep 20, 2022

Use the AOs introduced by the modules host hooks refactor tc39/proposal-module-expressions#75

Merged

nicolo-ribaudo mentioned this pull request Sep 28, 2022

Add tests for interaction between import() and microtask queue web-platform-tests/wpt#35949

Merged

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch from 407474b to 891949f Compare October 3, 2022 17:23

nicolo-ribaudo mentioned this pull request Oct 3, 2022

[ECMA-262 layering] Refactor import-related host hooks whatwg/html#8253

Merged

nicolo-ribaudo mentioned this pull request Oct 3, 2022

Use new host hooks for importing modules tc39/proposal-import-attributes#124

Merged

nicolo-ribaudo requested a review from a team October 4, 2022 15:42

guybedford reviewed Oct 4, 2022

View reviewed changes

guybedford reviewed Oct 5, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch from 3cc0cce to e5360da Compare October 5, 2022 18:17

guybedford reviewed Oct 5, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

jmdyck requested changes Oct 7, 2022

View reviewed changes

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch from eac7558 to 389bf4f Compare October 7, 2022 09:03

caridy approved these changes Oct 14, 2022

View reviewed changes

Jack-Works reviewed Oct 18, 2022

View reviewed changes

spec.html Show resolved Hide resolved

michaelficarra added the editor call to be discussed in the next editor call label Oct 19, 2022

syg reviewed Oct 19, 2022

View reviewed changes

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch from 6704c5e to f2ea1ad Compare November 29, 2022 09:56

jmdyck previously requested changes Nov 29, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

spec.html Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the refactor-import-host-hooks branch from f2ea1ad to 0413272 Compare November 29, 2022 21:45

syg added the editor call to be discussed in the next editor call label Dec 7, 2022

syg approved these changes Dec 7, 2022

View reviewed changes

syg removed the editor call to be discussed in the next editor call label Dec 7, 2022

bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Dec 7, 2022

ljharb force-pushed the refactor-import-host-hooks branch from 0413272 to 18d41aa Compare December 10, 2022 05:49

ljharb merged commit 18d41aa into tc39:main Dec 10, 2022

nicolo-ribaudo mentioned this pull request May 8, 2023

Normative: Add Import Attributes #3057

Merged

nicolo-ribaudo deleted the refactor-import-host-hooks branch May 8, 2023 12:59

krkk mentioned this pull request Oct 28, 2023

LibWeb: Product images don't load on twinings.co.uk SerenityOS/serenity#21009

Closed

stelar7 mentioned this pull request Nov 3, 2023

LibJS: "Dynamic Imports are not allowed" on https://polar.sh/ SerenityOS/serenity#21568

Closed

nicolo-ribaudo mentioned this pull request Jun 29, 2024

Only use phase: parse for errors that happens while parsing the test entry point tc39/test262#4124

Open

Conversation

nicolo-ribaudo commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo commented Oct 3, 2022

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guybedford Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

guybedford Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmdyck commented Oct 7, 2022

Uh oh!

nicolo-ribaudo commented Oct 7, 2022

Uh oh!

Uh oh!

nicolo-ribaudo commented Oct 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

syg commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelficarra commented Nov 29, 2022

Uh oh!

nicolo-ribaudo commented Nov 29, 2022

Uh oh!

ljharb commented Nov 29, 2022

Uh oh!

ljharb commented Nov 29, 2022

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

nicolo-ribaudo commented Sep 15, 2022 •

edited

Loading

nicolo-ribaudo Oct 5, 2022 •

edited

Loading

syg commented Oct 19, 2022 •

edited

Loading