Proposed specification changes incomplete

[spectranaut]

Hello @bterlson and @jdalton,

This proposal's spec change contains normative additions to the production and static semantics, but no modifications of the methods for the instantiation of modules. We will need a modification of these methods as presently the new syntax will always result in a SyntaxError during instantiation. Here's a walk through of instantiation of modules with the new production, to see the SyntaxError.

Consider the following three modules (with the relevant fields set in ParseModule):

---

a.js

export var x = 262;

a.js.[[LocalExportEntries]]

[[ModuleRequest]]	[[ImportName]]	[[LocalName]]	[[ExportName]]
null	null	"x"	"x"

---

b.js

export * as ns from 'a.js';

b.js.[[StarExportEntries]]:

[[ModuleRequest]]	[[ImportName]]	[[LocalName]]	[[ExportName]]
"a.js"	"*"	null	"ns"

---

c.js

import {ns} from 'b.js';

c.js.[[ImportEntries]]:

[[ModuleRequest]]	[[ImportName]]	[[LocalName]]
"b.js"	"ns"	"ns"

---

During the instantiation of module c.js, we arrive at ModuleDeclarationEnvironmentSetup for module c.js with the [[ImportEntries]] listed above. At this point, modules b.js and a.js have been instantiated.

ModuleDeclarationEnvironmentSetup
[...]

8. For each ImportEntry Record in in module.[[ImportEntries]], do
   a. Let importedModule be ! HostResolveImportedModule(module, in.[[ModuleRequest]]).
   b. [...]
   c. If in.[[ImportName]] is "*", then
     i. Let namespace be ? GetModuleNamespace(importedModule).
     ii. Perform ! envRec.CreateImmutableBinding(in.[[LocalName]], true).
     iii. Call envRec.InitializeBinding(in.[[LocalName]], namespace).
   d. Else,

     // in.[[ImportName]] is not "*", so we come here to resolve import.

     i. Let resolution be ? importedModule.ResolveExport(in.[[ImportName]], « »).

     // *b.js*.ResolveExport('ns') returns null, see below

     ii. If resolution is null or "ambiguous", throw a SyntaxError exception.

From line 8.d.i above, we end up in ResolveExport with:

exportName = 'ns'
module='b.js'

ResolveExport
[...]

7. Let starResolution be null.
8. For each ExportEntry Record e in module.[[StarExportEntries]], do

    // e: [ ExportName: 'ns', ModuleRequest: 'a.js', ImportName: '*', LocalName: null) ]

    a. Let importedModule be ? HostResolveImportedModule(module, e.[[ModuleRequest]]).
    b. Let resolution be ? importedModule.ResolveExport(exportName, resolveSet).

    // exportName: 'ns', importedModule: 'a.js'
    // resolution will be set to null, as we cannot find 'ns' in 'a.js'

    c. If resolution is "ambiguous", return "ambiguous".
    d. If resolution is not null, then
        i. Assert: resolution is a ResolvedBinding Record.
        ii. If starResolution is null, set starResolution to resolution.
        iii. Else,
            1. Assert: There is more than one * import that includes the requested name.
            2. If resolution.[[Module]] and starResolution.[[Module]] are not the same Module Record or SameValue(resolution.[[BindingName]], starResolution.[[BindingName]]) is false, return "ambiguous".
9. Return starResolution. 

Conclusion:

ResolveExport will always return null as we will never find "ns" in a.js. We go looking for "ns" in a.js because b.js's export record with [[ImportName]]='*' will trigger a look for the binding for "ns" in a.js. In result, ModuleDeclarationEnvironmentSetup will always throw a SyntaxError when instantiating c.js.

In addition to fixing this particular misdirection, we need to create a module namespace object for "ns", as is done in ModuleDeclarationEnvironmentSetup line 8.c. There is no code path for this happening from an export statement -- only import * as ns will trigger the creation of a module namespace object.

So, how can we handle export * as ns from 'a.js'?

Perhaps we can add logic to ModuleDeclarationInstantiation to process all [[StarImportEntries]] with an [[ExporName]] after line 7. This will cause the creation of the module namespace object while instantiating b.js.

However, we'd also have to edit ResolveExport to handle these special [[StarImportEntries]] cases. It might makes more sense to add this export entry to the [[IndirectExportEntrie]] in ParseModule -- instead of the [[StarExporEntries]] -- as they are logically more similar. Still, both methods will have to be edited.

(Thanks to @jugglinmike for originally helping me though the module logic!)

[spectranaut]

@dherman -- this is the problem I was asking you about on irc, if you are curious! :)

[leobalter]

cc @allenwb

[jugglinmike]

Hey @spectranaut! I've reviewed your write up, and it seems logical to me. I'm not the champion of this proposal, but I'd be happy to review a patch for you. Having an example of a fix (rather than a description of the problem) might make it easier for the other reviewers to comment. As you're now well-aware, this stuff is very tricky, especially if you've been away for a few months.

[spectranaut]

Closing - the consensus PR was merged: tc39/ecma262#1174 🎉