Stage 2.7 Spec Review - Guy Bedford

[guybedford]

Thanks for the patience on this one, here's my review. Overall the spec text is clear and comprehensive. The semantics and design mostly seem correct, down to basic questions around naming and structure and then just two main questions - the one you are already bringing up at plenary around defer being needed on reexports, and then also a new question I have about the interaction between deferred namespace imports and these optional reexports, which I've posted separately in #11.

I do still have concerns about (a) implementation complexity and (b) ecosystem effects here. In particular, how much burden this will put on implementations to extend the module system, and if we truly want to encourage community convergence on these barrel file patterns as outstanding questions respectively. I would like to see further engagement / feedback on both of these questions before progressing to Stage 3.

Spec Review

Abstract Module Records

• Typo: should be [[DeferredModules]] with "s" in ResolveExport method on Abstract Methods table
• importedNames as a Link() arg needs to be marked as an addition

Cyclic Module Records

• What is the motivation for OptionalIndirectExports on CyclicModuleRecord over Source Text Module Record? It seems a little strange to have only this one separate, also not in the "Additional Module Fields" section. I would be interested to hear more about the justification here. If for synthetic module records, not clear how they would utilize this property? Wasm wouldn't use it either. I think I would prefer if we kept this to STMR only.

GraphLoading State Record

• I’m uneasy about [[Visited]] as containing “records with fields” [[Module]] and [[ImportedNames]]. I think we should rather explicitly state the top-level type list that this is rather, and formally define the OptionalImport Record as the other type with just those fields or whatever we want to call that.

Link

• It would be nice to see a better explanation for what importedNames is in the context of link and how it is used, the description doesn't say much about this. Partial linking is an important concept to describe, a short note could be useful.

BuildLinkingList

• Can we call remainingImportedNames newImportedNames for clarity? Took me a moment to follow this.
• Exclude followed by Merge can just be written as an append of remaining / new for simplicity, or turned into its own abstract operation since these are commonly done together. In fact all four steps together - find in visited, exclude, merge, get optional seem to always go together such that this could be a single abstract op on a list and remove duplication?

InnerModuleEvaluation

• I do have some concerns about execution ordering and BuildEvaluationList - would it help to open another issue to discuss this? Am I understanding correctly that we are doing a post order unrolling of the optional imports? By not tracking a stack for these doesn't that mean that strongly connected components within the optional graph are not being tracked?

Static Semantics: Module Requests

• ModuleRequestsEqual is now very much ModuleRequestsKeysEqual, could be worth a rename here
• Neither defer nor source phases have ImportedNames - would be nice to have an assert when phase checking that it is empty for defer phase

Static Semantics: Imported Names

• Could be much shorter to just inline ImportedDefaultBinding names as <<default>>? (EDIT @nicolo-ribaudo: not doing this, see Stage 2.7 Spec Review - Guy Bedford #12 (comment))

[nicolo-ribaudo]

• What is the motivation for OptionalIndirectExports on CyclicModuleRecord over Source Text Module Record? It seems a little strange to have only this one separate, also not in the "Additional Module Fields" section. I would be interested to hear more about the justification here. If for synthetic module records, not clear how they would utilize this property? Wasm wouldn't use it either. I think I would prefer if we kept this to STMR only.

Cyclic Module Record is the one that actually deals with graph traversal, so it needs this information. An alternative would be to store [[OptionalIndirectExports]] on STMR, and then make GetOptionalIndirectExportsModuleRequests be an abstract method that subclasses STMR need to implement. We could say that the default behavior of that method is to return an empty list. Does this sound better?

[nicolo-ribaudo]

• ModuleRequestsEqual is now very much ModuleRequestsKeysEqual, could be worth a rename here

I'll do this in the import defer spec text.

[nicolo-ribaudo]

• It would be nice to see a better explanation for what importedNames is in the context of link and how it is used, the description doesn't say much about this. Partial linking is an important concept to describe, a short note could be useful.

The idea was that hosts would call it either with ~all~, if then they expose the full namespace object (think of require("esm")), or with an empty list if there is no way to get the exports of the module without importing it also in some other way (think of node ./esm.js). It also accepts a list of strings, but that's meant for "internal ECMA-262 usage". I'll write a better description, and maybe see if I can avoid exposing the "internal thing" as part of the spec public interface.

[nicolo-ribaudo]

• I’m uneasy about [[Visited]] as containing “records with fields” [[Module]] and [[ImportedNames]]. I think we should rather explicitly state the top-level type list that this is rather, and formally define the OptionalImport Record as the other type with just those fields or whatever we want to call that.

This pattern is actually used in multiple places in the spec where we conceptually have a map (with a unique "key", in this case [[Module]], and an associated value, in this case [[ImportedNames]]).

See:

• https://tc39.es/ecma262/#sec-add-value-to-keyed-group 's groups and https://tc39.es/ecma262/#sec-groupby 's return value
• [[TemplateMap]] in https://tc39.es/ecma262/#table-realm-record-fields
• https://tc39.es/ecma262/#sec-hostgetimportmetaproperties 's return type
• https://tc39.es/ecma262/#sec-resolveexport 's resolveSet

For this type I took direct inspiration from that resolveSet.

[nicolo-ribaudo]

• I do have some concerns about execution ordering and BuildEvaluationList - would it help to open another issue to discuss this? Am I understanding correctly that we are doing a post order unrolling of the optional imports? By not tracking a stack for these doesn't that mean that strongly connected components within the optional graph are not being tracked?

Opened #13

[nicolo-ribaudo]

• Could be much shorter to just inline ImportedDefaultBinding names as <<default>>?

Not much, because then I would need to add a definition for ImportClause : ImportedDefaultBinding anyway that currently just is defined by default thanks to the ImportedDefaultBinding : ImportedBinding definition.

I could hard-code names1 in ImportClause : ImportedDefaultBinding , NamedImports without removing the case for the lone ImportedDefaultBinding, but I quite like that the logic for SS recursion on lists is just "get it for the right, get it for the left, and merge the result".

[guybedford]

Cyclic Module Record is the one that actually deals with graph traversal, so it needs this information. An alternative would be to store [[OptionalIndirectExports]] on STMR, and then make GetOptionalIndirectExportsModuleRequests be an abstract method that subclasses STMR need to implement. We could say that the default behavior of that method is to return an empty list. Does this sound better?

From a field consistency approach I do tend to prefer this, if it doesn't add too much additional complexity.

The idea was that hosts would call it either with all, if then they expose the full namespace object (think of require("esm")), or with an empty list if there is no way to get the exports of the module without importing it also in some other way (think of node ./esm.js). It also accepts a list of strings, but that's meant for "internal ECMA-262 usage". I'll write a better description, and maybe see if I can avoid exposing the "internal thing" as part of the spec public interface.

I mean just having a one line sentence in the description for Link about what this argument means, since this meaning isn't immediately obvious.

This pattern is actually used in multiple places in the spec where we conceptually have a map (with a unique "key", in this case [[Module]], and an associated value, in this case [[ImportedNames]]).

Could we make a distinction in the spec between "templated / generic abstract operations" and "specialized abstract operations"? In this framing it feels like we still want these cases to be specialized as there is no real "polymorphism"?

For resolveSet, isn't it always the same anonymous record, or does it actually have other entry types?

I could hard-code names1 in ImportClause : ImportedDefaultBinding , NamedImports without removing the case for the lone ImportedDefaultBinding, but I quite like that the logic for SS recursion on lists is just "get it for the right, get it for the left, and merge the result".

Point taken, algorithmic consistency makes sense.

[guybedford]

Cyclic Module Record is the one that actually deals with graph traversal, so it needs this information. An alternative would be to store [[OptionalIndirectExports]] on STMR, and then make GetOptionalIndirectExportsModuleRequests be an abstract method that subclasses STMR need to implement. We could say that the default behavior of that method is to return an empty list. Does this sound better?

Thinking about this some more, instead of subclassing, could we explicitly say "if it is a source text module record" and then explicitly read fields off the subclass without it being generic? I feel like it is STMR specific, but let me know if I'm missing something.

[nicolo-ribaudo]

I'm not sure about what approach I prefer for that STMR/Cyclic split, I'll try both and see.

The idea was that hosts would call it either with all, if then they expose the full namespace object (think of require("esm")), or with an empty list if there is no way to get the exports of the module without importing it also in some other way (think of node ./esm.js). It also accepts a list of strings, but that's meant for "internal ECMA-262 usage". I'll write a better description, and maybe see if I can avoid exposing the "internal thing" as part of the spec public interface.

I mean just having a one line sentence in the description for Link about what this argument means, since this meaning isn't immediately obvious.

This pattern is actually used in multiple places in the spec where we conceptually have a map (with a unique "key", in this case [[Module]], and an associated value, in this case [[ImportedNames]]).

Could we make a distinction in the spec between "templated / generic abstract operations" and "specialized abstract operations"? In this framing it feels like we still want these cases to be specialized as there is no real "polymorphism"?

For resolveSet, isn't it always the same anonymous record, or does it actually have other entry types?

👍

[nicolo-ribaudo]

Thinking about this some more, instead of subclassing, could we explicitly say "if it is a source text module record" and then explicitly read fields off the subclass without it being generic? I feel like it is STMR specific, but let me know if I'm missing something.

After talking with @joyeecheung, it sounds like Node.js would benefit from being allowed to define its own non-source-text internal modules that defer-reexport from other modules, with some special V8 API for it. I'd rather not disallow that at the spec level.

I'll go ahead with #12 (comment) for now.

[joyeecheung]

Specifically, see nodejs/node#59686