Bug 1233109 - Alias fewer bindings at module toplevel r=shu

js/src/builtin/ModuleObject.cpp

@@ -906,18 +906,18 @@ ModuleBuilder::ModuleBuilder(JSContext* cx, HandleModuleObject module)
 {}
 
 bool
-ModuleBuilder::initModule()
+ModuleBuilder::buildTables()
 {
     for (const auto& e : exportEntries_) {
         RootedExportEntryObject exp(cx_, e);
         if (!exp->moduleRequest()) {
             RootedImportEntryObject importEntry(cx_, importEntryFor(exp->localName()));
             if (!importEntry) {
-                if (!appendLocalExportEntry(exp))
+                if (!localExportEntries_.append(exp))
                     return false;
             } else {
                 if (importEntry->importName() == cx_->names().star) {
-                    if (!appendLocalExportEntry(exp))
+                    if (!localExportEntries_.append(exp))
                         return false;
                 } else {
                     RootedAtom exportName(cx_, exp->exportName());
@@ -942,6 +942,12 @@ ModuleBuilder::initModule()
         }
     }
 
+    return true;
+}
+
+bool
+ModuleBuilder::initModule()
+{
     RootedArrayObject requestedModules(cx_, createArray<JSAtom*>(requestedModules_));
     if (!requestedModules)
         return false;
@@ -972,19 +978,6 @@ ModuleBuilder::initModule()
     return true;
 }
 
-bool
-ModuleBuilder::appendLocalExportEntry(HandleExportEntryObject exp)
-{
-    if (!module_->initialEnvironment().lookup(cx_, AtomToId(exp->localName()))) {
-        JSAutoByteString str;
-        str.encodeLatin1(cx_, exp->localName());
-        JS_ReportErrorNumber(cx_, GetErrorMessage, nullptr, JSMSG_MISSING_EXPORT, str.ptr());
-        return false;
-    }
-
-    return localExportEntries_.append(exp);
-}
-
 bool
 ModuleBuilder::processImport(frontend::ParseNode* pn)
 {

js/src/builtin/ModuleObject.h

@@ -282,6 +282,12 @@ class MOZ_STACK_CLASS ModuleBuilder
 
     bool hasExportedName(JSAtom* name) const;
 
+    using ExportEntryVector = TraceableVector<ExportEntryObject*>;
+    const ExportEntryVector& localExportEntries() const {
+        return localExportEntries_;
+    }
+
+    bool buildTables();
     bool initModule();
 
   private:
@@ -289,8 +295,7 @@ class MOZ_STACK_CLASS ModuleBuilder
     using RootedAtomVector = JS::Rooted<AtomVector>;
     using ImportEntryVector = TraceableVector<ImportEntryObject*>;
     using RootedImportEntryVector = JS::Rooted<ImportEntryVector>;
-    using ExportEntryVector = TraceableVector<ExportEntryObject*> ;
-    using RootedExportEntryVector = JS::Rooted<ExportEntryVector> ;
+    using RootedExportEntryVector = JS::Rooted<ExportEntryVector>;
 
     JSContext* cx_;
     RootedModuleObject module_;
@@ -310,8 +315,6 @@ class MOZ_STACK_CLASS ModuleBuilder
 
     bool maybeAppendRequestedModule(HandleAtom module);
 
-    bool appendLocalExportEntry(HandleExportEntryObject exp);
-
     template <typename T>
     ArrayObject* createArray(const TraceableVector<T>& vector);
 };

js/src/frontend/Parser.cpp

@@ -305,8 +305,6 @@ ParseContext<FullParseHandler>::define(TokenStream& ts,
             if (!checkLocalsOverflow(ts))
                 return false;
         }
-        if (atModuleScope())
-            dn->pn_dflags |= PND_CLOSED;
         if (!decls_.addUnique(name, dn))
             return false;
         break;
@@ -316,8 +314,6 @@ ParseContext<FullParseHandler>::define(TokenStream& ts,
         // See FullParseHandler::setLexicalDeclarationOp.
         dn->setOp(dn->pn_scopecoord.isFree() ? JSOP_INITGLEXICAL : JSOP_INITLEXICAL);
         dn->pn_dflags |= (PND_LEXICAL | PND_BOUND);
-        if (atModuleLevel())
-            dn->pn_dflags |= PND_CLOSED;
         if (atBodyLevel()) {
             if (!bodyLevelLexicals_.append(dn))
                 return false;
@@ -335,7 +331,7 @@ ParseContext<FullParseHandler>::define(TokenStream& ts,
         break;
 
       case Definition::IMPORT:
-        dn->pn_dflags |= PND_LEXICAL | PND_CLOSED;
+        dn->pn_dflags |= PND_LEXICAL;
         MOZ_ASSERT(atBodyLevel());
         if (!decls_.addShadow(name, dn))
             return false;
@@ -945,9 +941,9 @@ Parser<ParseHandler>::checkStrictBinding(PropertyName* name, Node pn)
     return true;
 }
 
-template <typename ParseHandler>
-typename ParseHandler::Node
-Parser<ParseHandler>::standaloneModule(HandleModuleObject module, ModuleBuilder& builder)
+template <>
+ParseNode*
+Parser<FullParseHandler>::standaloneModule(HandleModuleObject module, ModuleBuilder& builder)
 {
     MOZ_ASSERT(checkOptionsCalled);
 
@@ -981,6 +977,28 @@ Parser<ParseHandler>::standaloneModule(HandleModuleObject module, ModuleBuilder&
         return null();
     }
 
+    if (!builder.buildTables())
+        return null();
+
+    // Check exported local bindings exist and mark them as closed over.
+    for (auto entry : modulebox->builder.localExportEntries()) {
+        JSAtom* name = entry->localName();
+        MOZ_ASSERT(name);
+
+        Definition* def = modulepc.decls().lookupFirst(name);
+        if (!def) {
+            JSAutoByteString str;
+            if (!str.encodeLatin1(context, name))
+                return null();
+
+            JS_ReportErrorNumber(context->asJSContext(), GetErrorMessage, nullptr,
+                                 JSMSG_MISSING_EXPORT, str.ptr());
+            return null();
+        }
+
+        def->pn_dflags |= PND_CLOSED;
+    }
+
     if (!FoldConstants(context, &pn, this))
         return null();
 
@@ -2832,6 +2850,10 @@ Parser<FullParseHandler>::functionArgsAndBody(InHandling inHandling, ParseNode*
 {
     ParseContext<FullParseHandler>* outerpc = pc;
 
+    // Close over top level function definitions in modules.
+    if (pc->sc->isModuleBox())
+        pn->pn_dflags |= PND_CLOSED;
+
     // Create box for fun->object early to protect against last-ditch GC.
     FunctionBox* funbox = newFunctionBox(pn, fun, pc, inheritedDirectives, generatorKind);
     if (!funbox)
@@ -5057,7 +5079,7 @@ Parser<FullParseHandler>::namedImportsOrNamespaceImport(TokenKind tt, Node impor
         // const variables which are initialized during the
         // ModuleDeclarationInstantiation step. Thus we don't set the PND_IMPORT
         // flag on the definition.
-        bindingName->pn_dflags |= PND_CONST;
+        bindingName->pn_dflags |= PND_CONST | PND_CLOSED;
         BindData<FullParseHandler> data(context);
         data.initLexical(HoistVars, JSOP_DEFLET, nullptr, JSMSG_TOO_MANY_LOCALS);
         handler.setPosition(bindingName, pos());

js/src/frontend/Parser.h

@@ -331,11 +331,6 @@ struct MOZ_STACK_CLASS ParseContext : public GenericParseContext
         return atBodyLevel() && sc->isModuleBox();
     }
 
-    // True if the current lexical scope is the topmost level of a module.
-    bool atModuleScope() {
-        return sc->isModuleBox() && !innermostScopeStmt();
-    }
-
     // True if this is the ParseContext for the body of a function created by
     // the Function constructor.
     bool isFunctionConstructorBody() const {

js/src/jit-test/modules/defaultClass.js

@@ -0,0 +1,5 @@
+export default class {
+    triple(x) {
+        return x * 3;
+    }
+}

js/src/jit-test/modules/defaultFunction.js

@@ -0,0 +1,3 @@
+export default function(x) {
+    return x * 2;
+}

js/src/jit-test/tests/modules/debugger-vars-function.js

@@ -0,0 +1,37 @@
+// Test debugger access to aliased and unaliased bindings work correctly.
+
+load(libdir + "asserts.js");
+
+var g = newGlobal();
+var dbg = Debugger(g);
+dbg.onDebuggerStatement = function (frame) {
+    let env = frame.environment.parent;
+    assertEq(env.getVariable('a'), 1);
+    assertEq(env.getVariable('b'), 2);
+    assertEq(env.getVariable('c'), 3);
+    assertEq(env.getVariable('d'), 4);
+    assertEq(env.getVariable('e'), 5);
+};
+
+g.eval(
+`
+    let moduleRepo = {};
+    setModuleResolveHook(function(module, specifier) {
+        if (specifier in moduleRepo)
+            return moduleRepo[specifier];
+        throw "Module '" + specifier + "' not found";
+    });
+
+    let m = parseModule(
+    \`
+        var a = 1;
+        let b = 2;
+        export var c = 3;
+        export let d = 4;
+        let e = 5;
+        function f() { debugger; return e; }
+    \`);
+    m.declarationInstantiation();
+    m.evaluation();
+`);
+

js/src/jit-test/tests/modules/debugger-vars-toplevel.js

@@ -0,0 +1,38 @@
+// Test debugger access to aliased and unaliased bindings work correctly.
+
+load(libdir + "asserts.js");
+
+var g = newGlobal();
+var dbg = Debugger(g);
+dbg.onDebuggerStatement = function (frame) {
+    let env = frame.environment;
+    assertEq(env.getVariable('a'), 1);
+    assertEq(env.getVariable('b'), 2);
+    assertEq(env.getVariable('c'), 3);
+    assertEq(env.getVariable('d'), 4);
+    assertEq(env.getVariable('e'), 5);
+};
+
+g.eval(
+`
+    let moduleRepo = {};
+    setModuleResolveHook(function(module, specifier) {
+        if (specifier in moduleRepo)
+            return moduleRepo[specifier];
+        throw "Module '" + specifier + "' not found";
+    });
+
+    let m = parseModule(
+    \`
+        var a = 1;
+        let b = 2;
+        export var c = 3;
+        export let d = 4;
+        let e = 5;
+        function f() { return e; }
+        debugger;
+    \`);
+    m.declarationInstantiation();
+    m.evaluation();
+`);
+

js/src/jit-test/tests/modules/export-declaration.js

@@ -78,6 +78,12 @@ program([
 ]).assert(parseAsModule("export {}"));
 
 program([
+    letDeclaration([
+        {
+            id: ident("a"),
+            init: lit(1)
+        }
+    ]),
     exportDeclaration(
         null,
         [
@@ -89,9 +95,15 @@ program([
         null,
         false
     )
-]).assert(parseAsModule("export { a }"));
+]).assert(parseAsModule("let a = 1; export { a }"));
 
 program([
+    letDeclaration([
+        {
+            id: ident("a"),
+            init: lit(1)
+        }
+    ]),
     exportDeclaration(
         null,
         [
@@ -103,9 +115,15 @@ program([
         null,
         false
     )
-]).assert(parseAsModule("export { a as b }"));
+]).assert(parseAsModule("let a = 1; export { a as b }"));
 
 program([
+    letDeclaration([
+        {
+            id: ident("as"),
+            init: lit(1)
+        }
+    ]),
     exportDeclaration(
         null,
         [
@@ -117,9 +135,15 @@ program([
         null,
         false
     )
-]).assert(parseAsModule("export { as as as }"));
+]).assert(parseAsModule("let as = 1; export { as as as }"));
 
 program([
+    letDeclaration([
+        {
+            id: ident("a"),
+            init: lit(1)
+        }
+    ]),
     exportDeclaration(
         null,
         [
@@ -131,9 +155,19 @@ program([
         null,
         false
     )
-]).assert(parseAsModule("export { a as true }"));
+]).assert(parseAsModule("let a = 1; export { a as true }"));
 
 program([
+    letDeclaration([
+        {
+            id: ident("a"),
+            init: lit(1)
+        },
+        {
+            id: ident("b"),
+            init: lit(2)
+        }
+    ]),
     exportDeclaration(
         null,
         [
@@ -149,9 +183,19 @@ program([
         null,
         false
     )
-]).assert(parseAsModule("export { a, b }"));
+]).assert(parseAsModule("let a = 1, b = 2; export { a, b }"));
 
 program([
+    letDeclaration([
+        {
+            id: ident("a"),
+            init: lit(1)
+        },
+        {
+            id: ident("c"),
+            init: lit(2)
+        }
+    ]),
     exportDeclaration(
         null,
         [
@@ -167,7 +211,7 @@ program([
         null,
         false
     )
-]).assert(parseAsModule("export { a as b, c as d }"));
+]).assert(parseAsModule("let a = 1, c = 2; export { a as b, c as d }"));
 
 program([
     exportDeclaration(

js/src/jit-test/tests/modules/import-default-class.js

@@ -0,0 +1,5 @@
+// |jit-test| module
+
+import c from "defaultClass.js";
+let o = new c();
+assertEq(o.triple(14), 42);

js/src/jit-test/tests/modules/import-default-function.js

@@ -0,0 +1,4 @@
+// |jit-test| module
+
+import f from "defaultFunction.js";
+assertEq(f(21), 42);

js/src/jit-test/tests/modules/import-namespace.js

@@ -19,8 +19,17 @@ function testHasNames(names, expected) {
     });
 }
 
-let a = moduleRepo['a'] = parseModule("export var a = 1; export var b = 2;");
-let b = moduleRepo['b'] = parseModule("import * as ns from 'a'; var x = ns.a + ns.b;");
+let a = moduleRepo['a'] = parseModule(
+    `export var a = 1;
+     export var b = 2;`
+);
+
+let b = moduleRepo['b'] = parseModule(
+    `import * as ns from 'a';
+     export { ns };
+     export var x = ns.a + ns.b;`
+);
+
 b.declarationInstantiation();
 b.evaluation();
 testHasNames(getModuleEnvironmentNames(b), ["ns", "x"]);

js/src/jit-test/tests/modules/module-environment.js

@@ -9,17 +9,26 @@ function testInitialEnvironment(source, expected) {
     });
 }
 
+// Non-exported bindings: only top-level functions are present in the
+// environment.
 testInitialEnvironment('', []);
-testInitialEnvironment('var x = 1;', ['x']);
-testInitialEnvironment('let x = 1;', ['x']);
+testInitialEnvironment('var x = 1;', []);
+testInitialEnvironment('let x = 1;', []);
 testInitialEnvironment("if (true) { let x = 1; }", []);
-testInitialEnvironment("if (true) { var x = 1; }", ['x']);
+testInitialEnvironment("if (true) { var x = 1; }", []);
 testInitialEnvironment('function x() {}', ['x']);
+testInitialEnvironment("class x { constructor() {} }", []);
+
+// Exported bindings must be present in the environment.
 testInitialEnvironment('export var x = 1;', ['x']);
 testInitialEnvironment('export let x = 1;', ['x']);
 testInitialEnvironment('export default function x() {};', ['x']);
 testInitialEnvironment('export default 1;', ['*default*']);
 testInitialEnvironment('export default function() {};', ['*default*']);
-testInitialEnvironment("class x { constructor() {} }", ['x']);
+testInitialEnvironment("export class x { constructor() {} }", ['x']);
 testInitialEnvironment('export default class x { constructor() {} };', ['x']);
 testInitialEnvironment('export default class { constructor() {} };', ['*default*']);
+
+// Imports: namespace imports are present.
+testInitialEnvironment('import { x } from "m";', []);
+testInitialEnvironment('import * as x from "m";', ['x']);

js/src/jsscript.cpp

@@ -173,7 +173,6 @@ Bindings::initWithTemporaryStorage(ExclusiveContext* cx, MutableHandle<Bindings>
 
     uint32_t slot = CallObject::RESERVED_SLOTS;
     for (BindingIter bi(self); bi; bi++) {
-        MOZ_ASSERT_IF(isModule, bi->aliased());
         if (!bi->aliased())
             continue;
 

js/src/jsscript.h

@@ -1280,23 +1280,23 @@ class JSScript : public js::gc::TenuredCell
 
     void setColumn(size_t column) { column_ = column; }
 
-    // The fixed part of a stack frame is comprised of vars (in function code)
-    // and block-scoped locals (in all kinds of code).
+    // The fixed part of a stack frame is comprised of vars (in function and
+    // module code) and block-scoped locals (in all kinds of code).
     size_t nfixed() const {
-        return function_ ? bindings.numFixedLocals() : bindings.numBlockScoped();
+        return isGlobalCode() ? bindings.numBlockScoped() : bindings.numFixedLocals();
     }
 
     // Number of fixed slots reserved for vars.  Only nonzero for function
-    // code.
+    // or module code.
     size_t nfixedvars() const {
-        return function_ ? bindings.numUnaliasedVars() : 0;
+        return isGlobalCode() ? 0 : bindings.numUnaliasedVars();
     }
 
     // Number of fixed slots reserved for body-level lexicals and vars. This
     // value minus nfixedvars() is the number of body-level lexicals. Only
-    // nonzero for function code.
+    // nonzero for function or module code.
     size_t nbodyfixed() const {
-        return function_ ? bindings.numUnaliasedBodyLevelLocals() : 0;
+        return isGlobalCode() ? 0 : bindings.numUnaliasedBodyLevelLocals();
     }
 
     // Calculate the number of fixed slots that are live at a particular bytecode.

js/src/tests/ecma_6/extensions/keyword-unescaped-requirement-modules.js

@@ -43,16 +43,18 @@ if (typeof parseModule === "function")
 if (typeof Reflect.parse === "function")
 {
   var twoStatementAST =
-    Reflect.parse(`export { x } /* ASI should trigger here */
+    Reflect.parse(`let x = 0;
+                  export { x } /* ASI should trigger here */
                   fro\\u006D`,
                   { target: "module" });
 
   var statements = twoStatementAST.body;
-  assertEq(statements.length, 2,
+  assertEq(statements.length, 3,
            "should have two items in the module, not one ExportDeclaration");
-  assertEq(statements[0].type, "ExportDeclaration");
-  assertEq(statements[1].type, "ExpressionStatement");
-  assertEq(statements[1].expression.name, "from");
+  assertEq(statements[0].type, "VariableDeclaration");
+  assertEq(statements[1].type, "ExportDeclaration");
+  assertEq(statements[2].type, "ExpressionStatement");
+  assertEq(statements[2].expression.name, "from");
 
   var oneStatementAST =
     Reflect.parse(`export { x } /* no ASI here */