Skip to content

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 27, 2022

After #63 I realized that the current proposal spec does not define how import(moduleBlock) works.

I don't think that we should go through host hooks to import a module block, because:

  1. There is no path/url to resolve, we already have all the necessary code.
  2. We know for sure that module blocks are ECMAScript modules, and not some other language that can be imported using ESM.

This PR fills this gap, by moving all the logic to import module blocks to the proposal spec rather than leaving it host-defined. I mostly modified existing ecma262 algorithms/AOs, and I kept the same emu-clause structure as in ecma262.

There are two open questions:

  1. It's possible that we might need a new host hook to make sure that import.meta works, or we might just have to change where HostInitializeModuleBlock is called. I think that this will be clear when working on the HTML integration.
  2. I arbitrary decided to cache the evaluation of module blocks per-realm (I had to pick some semantics in order to be able to specify module blocks evaluation): two await import(aModuleBlock) calls in the same realm always return the same object, and they return different objects if done in different realms. This might not be the best choice, but I'd like to keep the "how caching exactly should works" discussion for Cache-by-position or reinit each time #45.

If this PR gets merged, please keep it as separate commits: I hope to then extract Move ResolveImportedModule cache from host requirements to new AO (and maybe Extract finishing steps from HostImportModuleDynamically) and propose it to ecma262 ahead of this proposal, since it would solve tc39/ecma262#2803.

@surma
Copy link
Member

surma commented Jun 28, 2022

That’s brilliant work. Thanks so much! <3

@surma surma merged commit 64e09fb into tc39:master Jun 28, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the define-evaluation branch June 28, 2022 09:40
Copy link

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work indeed!

<emu-clause id="sec-source-text-module-records">
<h1>Source Text Module Records</h1>

<emu-clause id="sec-parsemodule" type="abstract operation">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the expectation that this will be used by other host functions as a refactoring? If so, is it worth doing that refactoring in this spec or is the plan to do it later on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParseModule is already an existing abstract operation, I only modified it to accept a Parse Node (since module blocks are already "parsed") so that I can use to initialize module blocks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you meant. I didn't think about that, but I feel like it should be done in parallel regardless of this proposal.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually misread this initially, sorry for the confusion!

</emu-alg>

<emu-note type="editor">
Replace all the references to HostResolveImportedModule in the ecma262 speification with ResolveImportedModule.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "speification".

Comment on lines +336 to +339
1. If there exists a Record _record_ in _currentRealm_.[[ModuleMap]] such that _record_.[[ReferencingScriptOrModule]] is _referencingScriptOrModule_ and _record_.[[Specifier]] is _specifier_, then
1. Return NormalCompletion(_record_.[[ModuleRecord]]).
1. Assert: Type(_specifier_) is String.
1. Let _completion_ be Completion(HostResolveImportedModule(_referencingScriptOrModule_, _specifier_)).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could also tighten the async requirements here. There's an implicit assumption that HostResolveImportedModule can do promise work. Now that the cache case is reified, we could even refactor the calls that are known to be cached (ie in evaluation) to clearly never result in userland hooks and never result in promise work.

Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it? It's specified to be synchronous (it either throws, or returns a module record): https://tc39.es/ecma262/#sec-hostresolveimportedmodule. The async stuff is done in HostImportModuleDynamically, which obviously needs to be asynchronous not only to evaluate (which could be moved to the spec, yes) but also to load the file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HostResolveImportedModule is async in instantiate (by definition - you cannot resolve a module until you've loaded the parent, and you cannot return a module record until you've loaded the resolved module which is on the web an async network operation) and sync in execute, and this is an implicit assumption in the spec that has never been formalized. HostImportModuleDynamcially is only for dynamic import(), although I was never quite sure why it needed its own host hook.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we are implicitly assuming in ECMA-262 today that HostResolveImportedModule in instantiate is sync because the fetch maps are already populated. But that basically assumes that HostResolveImportedModule is not actually a resolver, but a reified resolver to begin with just representing another resolver that actually already did the work.

Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly what's happening. And I don't think that it's implicit: HostResolveImportedModule explicitly returns a Completion<Module Record> and not Completion<Promise<Module Record>>.

As an example, the HTML spec guarantees that the whole graph has been loaded before any call to HostResolveImportedModule: step 6 of https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingscriptormodule,-modulerequest)

But that basically assumes that HostResolveImportedModule is not actually a resolver

It's as much a resolver as import.meta.resolve is on the web: both are synchronous pure transforms that depend only on the referrer, the specifier and the module map.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, all loaders have an initial phase to gather all the modules before initiating the algorithm. This is interesting though - effectively all module loaders are two module loaders!

<p><ins>For every Record _record_ it contains, if Type(_record_.[[Specifier]]) is Object then:</ins></p>
<ul>
<li>
<ins>_record_.[[Specifier]] has a [[ModuleBlockBody]] internal slot.</ins>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one way to do it, but another way might be to actually just go ahead and split up the module map into the two separate maps for compilation and instances. Eg if all resolutions are based on compiled module returns, then the import of a module block is not the cached resolution of a module block specifier, but rather just the instance lookup for a compiled module. Would something like that make sense? Not tied to either way just trying to work through where this goes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially yes, but I first want to figure out exactly how caching should work. For example, currently the proposal does not guarantee that the two imports in this file evaluate to the same module:

import * as ns from "./file.js";

await import(module {
  import * as ns from "./file.js";
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #68 (comment) for discussion on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants