Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Layer 0 Spec: Module & Module Source #72

Merged
merged 22 commits into from
Aug 18, 2022
Merged

Layer 0 Spec: Module & Module Source #72

merged 22 commits into from
Aug 18, 2022

Conversation

caridy
Copy link
Collaborator

@caridy caridy commented Jul 25, 2022

This PR define the specs for layer 0.

The readable version of this PR can be found here.

Details

  1. Introduces Module
  2. Introduces ModuleSource
  3. Modifies Runtime Semantics for import() to accept a Module instance

@caridy caridy marked this pull request as draft July 25, 2022 22:11
0-module-and-module-source.emu Show resolved Hide resolved
<emu-clause id="sec-modulesource-abstracts">
<h1>ModuleSource Abstract Operations</h1>

<emu-clause id="sec-parsemodulesource" type="abstract operation">
Copy link
Member

Choose a reason for hiding this comment

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

This algr looks very similar to ParseModule. Can you use <ins> and <del> to indicate the changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added a note below this operation related to this. I don't it will be a modifcation, but a refactor that splits ParseModule into two operations.

0-module-and-module-source.emu Outdated Show resolved Hide resolved
0-module-and-module-source.emu Outdated Show resolved Hide resolved
1. Assert: _module_ is a Module Record.
1. Assert: _module_.[[ImportMeta]] is ~empty~.
1. Let _importMeta_ to OrdinaryObjectCreate(*null*).
1. Let _keys_ be ? <emu-meta effects="user-code">_from_.[[OwnPropertyKeys]]()</emu-meta>.
Copy link
Member

Choose a reason for hiding this comment

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

Can we do ? Call (%Object.assign%, *null*, < _importMeta_, _meta_ >)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

being more conservative here since we don't use Object.assign anywhere.

1. If _desc_ is not *undefined* and _desc_.[[Enumerable]] is *true*, then
1. Let _nextValue_ be ? Get(_from_, _nextKey_).
1. Perform ! CreateDataPropertyOrThrow(_importMeta_, _nextKey_,_nextValue_).
1. Perform HostFinalizeImportMeta(_importMeta_, _module_).
Copy link
Member

Choose a reason for hiding this comment

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

Why it will call the host hook? This will prevent virtualization if the host did something observable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HostFinalizeImportMeta: https://tc39.es/ecma262/#sec-hostfinalizeimportmeta

Most hosts will be able to simply define HostGetImportMetaProperties, and leave HostFinalizeImportMeta with its default behaviour. However, HostFinalizeImportMeta provides an "escape hatch" for hosts which need to directly manipulate the object before it is exposed to ECMAScript code.

Basically, the creation of the meta object involves two steps, one of them needs virtualization (HostGetImportMetaProperties), the other one HostFinalizeImportMeta doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

No, since it does not call the HostGetImportMetaProperties, it should not call HostFinalizeImportMeta either.

0-module-and-module-source.emu Outdated Show resolved Hide resolved
1. Let _importHookPromise_ be ? PromiseResolve(%Promise%, _completion_.[[Value]]).
1. Let _fulfilledClosure_ be a new Abstract Closure with parameters (_result_) that captures _moduleRecord_ and _promiseCapability_ and performs the following steps when called
1. If _result_ is an Object that has a [[ImportHook]] internal slot, then
1. Let _moduleRecord_ be _result_.[[Module]].
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo @Jack-Works, I have two issues here:

  1. if the result of calling [[ImportHook]] function must always be a Module Instance, then how can we resolve to fs or some other module instance that delegates the resolution to the browser? That seems like a problem. I don't know if the issue is here, or somewhere else, but definitely needs to be addressed.
  2. where do we go into the recursive resolution? in HostImportModuleRecordDynamically we only talk about the "unique ModuleSpecifier associated to the moduleRecord", never about the transitive dependencies. any suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One option here might be to support having import hook return a ModuleSource which is then instanced against its primary default instance for the host context? Would need to think through the edge cases though....

Recursive resolution happens through HostResolveImportedModule calls in InnerModuleLinking and InnerModuleEvaluation. I really liked @nicolo-ribaudo's approach in https://nicolo-ribaudo.github.io/modules-import-hooks-refactor/ of having this being a sync lookup against the resolution list on the module record itself though to more clearly separate the sync and async iterations.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jul 27, 2022

Choose a reason for hiding this comment

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

  1. My hope is that import reflection could also give Modules other than ModuleSources (the syntax is just an idea):

    import module jsM from "x.js";
    import source jsS from "x.js";
    import module wasmM from "y.wasm";
    import source wasmS from "y.wasm";
    
    jsM instanceof Module;
    jsS instanceof ModuleSource;
    wasmM instanceof Module;
    wasmS instanceof WebAssembly.Module; // WebAssembly.Module respects the "module source interface", whatever that means
    
    jsM.source === jsS;
    wasmM.source === wasmS;

    Then, an import hook might be implemented like this:

    function importHook(specifier, importMeta) {
      if (specifier === "fs") return import("fs", { reflection: "module" });
      
      const source = // get the module source
      const innerImportMeta = // get the import.meta
      return new Module(source, importHook, innerImportMeta);
    }

    @guybedford The problem is that, at least for built-in modules, we might not want a ModuleSource (module.source should be null) because you wouldn't want to be able to, for example, create multiple instances of a "fs" module.

  2. As @guybedford said, the current ECMA-262, we explicitly handle the recursive resolution: we hide the fact that "real resolution" can be asynchronous, and we pretend that the resolution happens in HostResolveImportedModule. We call it recursively for every transitive dependency in InnerModuleLinking, which is triggered by an (implicit) call to .Link() of the top-level imported module in HostImportModuleDynamically.
    We probably can keep hand waving in this proposal spec (we need a "complete spec text" only when asking for stage 3, and this PR is already very good for asking for stage 2), but a proper solution will look a lot like https://nicolo-ribaudo.github.io/modules-import-hooks-refactor/#sec-LoadRequestedModules (or, I push for merging https://nicolo-ribaudo.github.io/modules-import-hooks-refactor without waiting for a proposal, and then you just edit that in this proposal to call [[ImportHook]] instead of HostLoadImportedModule)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nicolo-ribaudo on (1), this is an interesting proposal although does introduce more complexity. That said it very much resolves some current concerns through the separation. Would the module instance reflection be unexecuted if it hasn't already been executed? But would the dependencies be fully preloaded? If so, that may well make it suitable for deferred evaluation mechanisms....

Furthermore, passing an instance to another realm in this way with linkage might enable an easy solution to resolution sharing without having to re-instrument a loader on the other realm...

Would be interested to hear more details if it might be able to address the above.

Copy link
Collaborator

@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.

Seems roughly on the right track to me.

1. <ins>Else,</ins>
1. Let _specifierString_ be Completion(ToString(_specifierOrModule_)).
1. IfAbruptRejectPromise(_specifierString_, _promiseCapability_).
1. Perform HostImportModuleDynamically(_referencingScriptOrModule_, _specifierString_, _promiseCapability_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible, to overload HostImportModuleDynamically to accept either a module source or a string specifier as it's "specifier"?

0-module-and-module-source.emu Show resolved Hide resolved
An instance of a %Module% constructor or *undefined*.
</td>
<td>
A Module Instance or *undefined* if no Module Instance is associated to this Record.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add something like a note here on the topic that the Instance constructor will create a bi-directional mapping between an instance object and its corresponding SourceTextModule record, while internal SourceTextModule construction leaves this undefined forming a distinction between user-instrumented and non-user-instrumented code?

0-module-and-module-source.emu Show resolved Hide resolved
1. <ins>Let _specifierOrModule_ be ? GetValue(_argRef_).</ins>
1. Let _promiseCapability_ be ! NewPromiseCapability(%Promise%).
1. <ins>If Type(_specifierOrModule_) is Object that has a [[ModuleSourceInstance]] internal slot, then</ins>
1. <ins>Let _moduleRecord_ be _specifierOrModule_.[[Module]].</ins>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be [[Module]] or [[ModuleSourceInstance]]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should we also be looking up if _referencingScriptOrModule_ is itself a SourceTextModule (pretty sure we can assume it always be?) that we should look up its [[Instance]] slot in turn for the parent reference if there is one? Would that be useful information for an import hook even at the top level?

Copy link
Member

Choose a reason for hiding this comment

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

Pardon this late response; I’m sure our shared understanding has evolved in 22 days. I believe we’re expecting Module instances to represent not just modules constructed by the Module constructor, but any reflected module and any module block, which suggests they may or may not have a [[ModuleSourceInstance]], that they will always have a [[Module]] Module Record. We can only be sure that [[Module]] is a Source Text Module Record if the Module Instance has a [[ModuleSourceInstance]].

We may want to make that invariant explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be [[Module]] or [[ModuleSourceInstance]]?

It should be [[Module]] because the instance provided must already have a qualifying module record associate to it, and that's the currency of all these operations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may want to make that invariant explicit.

Any suggestion on how to make that more explicit?

0-module-and-module-source.emu Show resolved Hide resolved
1. Let _importHookPromise_ be ? PromiseResolve(%Promise%, _completion_.[[Value]]).
1. Let _fulfilledClosure_ be a new Abstract Closure with parameters (_result_) that captures _moduleRecord_ and _promiseCapability_ and performs the following steps when called
1. If _result_ is an Object that has a [[ImportHook]] internal slot, then
1. Let _moduleRecord_ be _result_.[[Module]].
Copy link
Collaborator

Choose a reason for hiding this comment

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

One option here might be to support having import hook return a ModuleSource which is then instanced against its primary default instance for the host context? Would need to think through the edge cases though....

Recursive resolution happens through HostResolveImportedModule calls in InnerModuleLinking and InnerModuleEvaluation. I really liked @nicolo-ribaudo's approach in https://nicolo-ribaudo.github.io/modules-import-hooks-refactor/ of having this being a sync lookup against the resolution list on the module record itself though to more clearly separate the sync and async iterations.

Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

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

Should https://raw.githack.com/tc39/proposal-compartments/e897e4a5707154f4af67ac89ec7bf7cfef966636/0-module-and-module-source.html specify that ModuleSource.prototype.constructor is the ModuleSource constructor? Or is that taken care of by a generic preamble?

@erights
Copy link
Collaborator

erights commented Jul 28, 2022

Step 3

  1. If Type(importHook) is not Callable, throw a TypeError exception.

seems redundant with step 6

  1. If IsCallable(importHook) is false, throw a TypeError exception.

0-module-and-module-source.emu Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Member

@erights

If 7.b completes abruptly, shouldn't 7.c be skipped?

IfAbruptRejectPromise has an early return if the completion is an abrupt completion, so 7.c is skipped.

@caridy
Copy link
Collaborator Author

caridy commented Jul 28, 2022

@erights

Should https://raw.githack.com/tc39/proposal-compartments/e897e4a5707154f4af67ac89ec7bf7cfef966636/0-module-and-module-source.html specify that ModuleSource.prototype.constructor is the ModuleSource constructor? Or is that taken care of by a generic preamble?

it was missing for both, I just added.

Step 3

  1. If Type(importHook) is not Callable, throw a TypeError exception.

seems redundant with step 6

  1. If IsCallable(importHook) is false, throw a TypeError exception.

Fixed.

@caridy caridy requested a review from bmeck July 28, 2022 20:38
@caridy caridy marked this pull request as ready for review July 28, 2022 20:39
@naugtur
Copy link
Collaborator

naugtur commented Aug 1, 2022

I made an attempt to visualize this change for the benefit of my own mental model. Sharing in case it's useful:
https://gist.github.com/naugtur/daf2aaf415e03e6c3ae5030e06f7b20e

0-module-and-module-source.emu Outdated Show resolved Hide resolved
@caridy
Copy link
Collaborator Author

caridy commented Aug 1, 2022

I made an attempt to visualize this change for the benefit of my own mental model. Sharing in case it's useful: https://gist.github.com/naugtur/daf2aaf415e03e6c3ae5030e06f7b20e

Very nice, I added a note in the gist.

</ul>

<emu-clause id="sec-module">
<h1>Module ( _moduleSource_, _referral_, _importHook_ [, _importMeta_ ] )</h1>
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Aug 15, 2022

Choose a reason for hiding this comment

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

We have always discussed about a string referrer, but this spec text allows referral to be any ECMAScript value. I very much prefer this behavior, rather than restricting to strings (I was going to open a new issue proposing to relax the restriction, before noticing that it's already not restricted).

I realized that passing the referrer to importHook (and thus passing referral to new Module) is not necessary, but it's just an ergonomic improvement. If the spec didn't call importHook passing the referrer, I could just do this:

function buildImportHook(referrer) {
  return async function importHook(specifier) {
    const url = new URL(specifier, referrer);
    const sourceText = await fetch(url).then(r => r.text());
    const source = new ModuleSource(sourceText);
    return new Module(source, buildImportHook(url));
  };
};

const entryModule = new Module(entrySource, buildImportHook(entryURL));
await import(entryModule);

(or I could use .bind rather than manual currying)

Passing the referrer through the Module constructor to importHook is just an ergonomic/performance improvement to avoid creating closures that capture context: it's similar to the thisArg parameter of all the array methods that take a function.

Since we are "just" improving ergonomics, we can look at other use cases for providing some context to the importHook function. One example (taken from the HTML spec) is to provide some "fetch options" that describe how the dependencies should be fetched:

async function importHook(specifier, { baseURL, referrerPolicy }) {
  const url = new URL(specifier, baseURL);
  const sourceText = await fetch(url, { referrerPolicy }).then(r => r.text());
  const source = new ModuleSource(sourceText);
  return new Module(source, {
    baseURL: url,
    referrerPolicy,
  }, importHook);
}

const entryModule = importHook("./main.js", { baseURL: document.URL, referrerPolicy: "same-origin" });
await import(entryModule);

// That is roughly equivalent to
//   <script type="module" src="./main.js" referrerpolicy="same-origin" />

Allowing any object aligns this "context parameter" with the current [[HostDefined]] slot on Module Records: it's a general-purpouse place used to share loader-level information between modules.

So 👍 to not checking that Type(_referral_) is string, and maybe we could rename it to _context_ (or _thisArg_ and pass it as this to importHook, like array methods do).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo in principle I agree with this. Question: in the example above I suspect you are missing importHook argument when calling new Module() right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, fixed!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolution: We will look into this in another PR.

</ul>

<emu-clause id="sec-module">
<h1>Module ( _moduleSource_, _referral_, _importHook_ [, _importMeta_ ] )</h1>
Copy link
Member

Choose a reason for hiding this comment

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

importHook should be optional argument. Inherit the current evaluator's import hook.

const x = new Module(source, 'spec') // uses host
const ev = new Evaluators({ importHook: myHook })
new ev.Module(source, 'spec') // uses myHook

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolution: we will look into the default hook in another PR/issue.

@caridy caridy requested review from Jack-Works and erights August 17, 2022 17:54
@caridy
Copy link
Collaborator Author

caridy commented Aug 17, 2022

@kriskowal feel free to merge this. It seems that we are all in agreement according to the last SES meeting.

<emu-alg>
1. If NewTarget is *undefined*, throw a *TypeError* exception.
1. Let _O_ be ? OrdinaryCreateFromConstructor(NewTarget, *"%ModuleSource.prototype%"*, « [[ModuleSource]] »).
1. Let _body_ be ParseText(_sourceText_, |Module|).
Copy link
Member

Choose a reason for hiding this comment

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

It should call ? HostEnsureCanCompileStrings(_currentRealm_) before ParseText.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the wrong place to do so... creating an instance of module source should be possible even when CSP prevents evaluation of code. Perhaps the host hook has the wrong name... it is not really about compilation, but evaluation. I'm fixing this in #76.

<emu-alg>
1. If NewTarget is *undefined*, throw a *TypeError* exception.
1. Let _O_ be ? OrdinaryCreateFromConstructor(NewTarget, *"%ModuleSource.prototype%"*, « [[ModuleSource]] »).
1. Let _body_ be ParseText(_sourceText_, |Module|).
Copy link
Member

Choose a reason for hiding this comment

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

This step should do a coherent to convert sourceText to a String.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed by #76

1. <ins>Let _specifierOrModule_ be ? GetValue(_argRef_).</ins>
1. Let _promiseCapability_ be ! NewPromiseCapability(%Promise%).
1. <ins>If Type(_specifierOrModule_) is Object that has a [[ModuleSourceInstance]] internal slot, then</ins>
1. <ins>Let _moduleRecord_ be _specifierOrModule_.[[Module]].</ins>
Copy link
Member

Choose a reason for hiding this comment

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

Pardon this late response; I’m sure our shared understanding has evolved in 22 days. I believe we’re expecting Module instances to represent not just modules constructed by the Module constructor, but any reflected module and any module block, which suggests they may or may not have a [[ModuleSourceInstance]], that they will always have a [[Module]] Module Record. We can only be sure that [[Module]] is a Source Text Module Record if the Module Instance has a [[ModuleSourceInstance]].

We may want to make that invariant explicit.

@kriskowal
Copy link
Member

I believe “referrer” is the more correct word-choice than “referral”, but that is not important yet.

@kriskowal kriskowal merged commit e3d74c8 into master Aug 18, 2022
@kriskowal kriskowal deleted the caridy/layer-0 branch August 18, 2022 03:55
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.

7 participants