Reviews for Stage 2.7

[guybedford]

Issue to track Stage 2 reviewers feedback.

Spec: https://tc39.es/proposal-esm-phase-imports/

Reviewers:

• @nicolo-ribaudo
• @legendecas
• @kriskowal

Editors:

• @bakkot
• @syg
• @michaelficarra

[ljharb]

to clarify, reviewers are within stage 2, they're just a prerequisite for stage 2.7.

[michaelficarra]

1. EvaluateImportCall step 9.b.i: don't say "is not equal to", just say "is not"; see https://github.com/tc39/ecma262/wiki/Editorial-Conventions#comparisons.
2. GetModuleSourceModuleRecord: wouldn't it be more appropriate to name this parameter specifier? Isn't it still conceptually a specifier?
3. GetModuleSourceModuleRecord step 5: "is equal to" -> "is"
4. Source Text Module Records [[SourceText]] field: "ECMAScript source text" is probably a better type. Also I wouldn't include the optimisation note for hosts.
5. Source Text Module Records [[ModuleSource]] field: like the above rows, we should describe when it's expected to be empty
6. ModuleSourcesEqual step 2: "is equal to" -> "is"
7. The ModuleSource constructor: I don't think this is needed: "will throw an error when invoked, where support for dynamic construction may be added in future"
8. Where is ModuleSourcesEqual called? Why is it on abstract module records and not just source text module records (or better yet, just an AO) if we know it'll only be called on them?
9. "For Module Records that implement a normal completion" -> "For Module Records that return a normal completion"
10. I don't love the not-a-source enum name, but we can bikeshed that during integration

[michaelficarra]

LGTM

[syg]

My normative review:

• Why does EvaluateImportCall get a ModuleSource without consulting the phase argument? That doesn't seem right. If it's intentional then I don't understand what step 9 is doing at all.

My editorial review:

• This is layered on top of source phase imports but I don't see necessary changes needed to that spec draft (e.g. EvaluateImportCall callsites).
• GetModuleSource Step 3, the call to OrdinaryObjectCreate needs to pass « [[SourceTextModuleRecord]] » as an additional internal slot
• ModuleSourcesEqual is a weird AO; is it needed? It seems like it's only used by embedder specs? If so, can the embedder compare identity of the value in [[SourceText]] instead of calling this AO?

[guybedford]

Why does EvaluateImportCall get a ModuleSource without consulting the phase argument? That doesn't seem right. If it's intentional then I don't understand what step 9 is doing at all.

Will do my best to explain further, and happy to discuss more. The import(moduleSource) use case is the ability to import the module instance at the key of a module source.

This is the required primitive for module expressions since a module expression would allow both import(module { export var p = 5 }) and postMessage, while having its JS object representation be the module source object. In turn we would also enable import(wasmModule) with the same behaviour, and in particular for source-phase-imported Wasm.

The registry effectively consists of a key - (module source, module Instance) 1 - 1 "canonical" source / instance pair relation. We get the pair today since a module record just is both things. This is the phasing model - url -> source -> instance all building off the same registry key. Since the module source associates directly with its URL, this allows them to be used as a proxy for their canonical instance, hence being supported in dynamic import() in a well-defined way as an import capability for the module in a sense.

Supporting new Worker(modSource), postMessage(modSource) and import(modSource) in this proposal get us the basic primitives to then follow-up with the module harmony features from there, and including in the Wasm world the ability to have module handles as well. The most pressing next step right now though being module expressions and module declarations.

I can work on more clearly motivating the above if it would help. The module full primitive goal is a crucial one to get right here though, we have to solve all of representation, postMessage and registry identity together, it's no use solving one without the other.

This is layered on top of source phase imports but I don't see necessary changes needed to that spec draft (e.g. EvaluateImportCall callsites).

Can you clarify what you mean by this? The latest source phase spec it is based to is the one here - tc39/ecma262#3492, not the version in the spec repo, which is now out of date. Does that help?

GetModuleSource Step 3, the call to OrdinaryObjectCreate needs to pass « [[SourceTextModuleRecord]] » as an additional internal slot

Thanks, added in #39.

ModuleSourcesEqual is a weird AO; is it needed? It seems like it's only used by embedder specs? If so, can the embedder compare identity of the value in [[SourceText]] instead of calling this AO?

The benefit of defining this was that Wasm could define it in the same way for WebAssembly module records. I found myself writing this out in HTML explicitly, and it felt unnecessary there. Formally defining the meaning for equality for module sources needs to be done somewhere - either in HTML or ECMA-262. @michaelficarra also brought this one up though, maybe we should just cut it until it's needed to be called directly from ECMA-262 though?

[syg]

Oh I see, I completely misunderstood the use case then. I'll give it a second read tomorrow.

[nicolo-ribaudo]

1. In EvaluateImportCall step 9.b, I'd prefer to swap steps i and ii: first we "read" the arguments, and then we do validation.
2. In EvaluateImportCall step 9.b.ii, we need to validate that options is an object before reading "with" from it. See steps 10.d.i.
3. (Not really a review comment: I wish GetModuleSourceName of Source Text Modules could return "JavaScript Module Source")
4. I don't understand the change in step 1 of FinishLoadingImportedModule. Why if we are importing a string specifier we don't want anymore to add it to the loaded map?

I finished going through the spec -- consider my review as ✔ when 1/2/4 are solved (or explicitly not).

[guybedford]

Added #40 for points 1 & 2 above.

For (3), yeah that's what we mean...

I don't understand the change in step 1 of FinishLoadingImportedModule. Why if we are importing a string specifier we don't want anymore to add it to the loaded map?

In the case where you do import(moduleSource) we can't add anything to the map, therefore we gate step one of FinishLoadingImportedModule on the module request being a string, and not a module source (which results in the module request being passed as a module record). That is, import(moduleSource) is a module request for a module record, not a module request record, and that is reflected in the types of HostLoadImportedModule and FinishLoadingImportedModule. Let me know if I'm missing something here though.

[syg]

Will do my best to explain further, and happy to discuss more. The import(moduleSource) use case is the ability to import the module instance at the key of a module source.

Thanks for the explanation, the use case makes sense to me.

Step 9.b.i of EvaluateImportCall doesn't seem to allow import(moduleSourceObj) where options is undefined. Is that intentional?

The latest source phase spec it is based to is the one here - tc39/ecma262#3492, not the version in the spec repo, which is now out of date. Does that help?

Ack. That helps and addresses my previous question.

With the explanation, editorially LGTM.

[guybedford]

Thanks, added #42.

[legendecas]

Editorial: In EvaluateImportCall 10.a Let moduleSource be GetModuleSourceModuleRecord(specifier), the moduleSource is a Module Record, can we name it module to not confuse it with ModuleSource?

[bakkot]

Apologies for the delay. Here's my editorial comments:

• In EvaluateImportCall, it's weird to have the following sequence: "let attributes be options.with; check module.[[Realm]]; check attributes != undefined". Seems like it would make more sense to consolidate the "reading options.with" and the "checking that options.with is undefined" steps; i.e., to pull 10.b.iii up to after 10.b.i.3
  • This is maybe contrary to @nicolo-ribaudo's suggestion above, but I disagree with that suggestion. We generally validate as we read options from options bags and similar. Compare reading/validating the "alphabet" and "omitPadding" options in Uint8Array.prototype.toBase64.
• In the intro of GetModuleSourceModuleRecord, the sentence beginning "Provides" should begin "It provides". We use complete sentence in the prose parts of the spec, as a rule.
• Ditto the sentence in the intro of ModuleSourcesEqual beginning "Checks".
• The Agoric folks have requested that all new "hidden" (non-global) intrinsics have a documented way to reach them, which I think would include %ModuleSource%. This can just be in the readme but you might also want to call it out when presenting.
• This introduces ModuleSourcesEqual but does not use it, as far as I can tell. I see the other editors have noticed this as well. Personally I'd just leave this out; we don't usually define machinery for use by hosts. If it's essential to have it, it should be explicitly noted that it is not used in this spec but is expected to be used by hosts, and under what circumstances we expect that.
• s/@@toStringTag/%Symbol.toStringTag%/ throughout, as of Editorial: Convert @@ notation to intrinsics notation for well-known Symbols ecma262#1314
• The description of GetModuleSourceName says "... to be provided in the strongly branded return value of the @@toStringTag getter on %AbstractModuleSource%". But the return value of that getter is a String; strings, being primitives, cannot be branded. I think just "to be used as the return value of the @@toStringTag getter on %AbstractModuleSource%" would be fine.
• I was very confused about what "GetModuleSourceName" did because I thought it was returning the name of a specific module source object, and I didn't know what that would be. I would much prefer to call it "GetModuleSourceKind".
• Is there a reason [[ModuleSource]] needs to be initially ~empty~? It would not be observable if it were populated eagerly, would it? If so, I'd prefer that, for simplicity. You can add a note to engines saying that it need not be manifested until observed, if you like, though engines are generally aware of that sort of thing already. ([[ImportMeta]] works the same way, but that's "observable" in the sense that it calls a host hook when first manifested, which could in principle do something observable. Manifesting [[ModuleSource]] doesn't call any host hooks.)