At the moment we alias all bindings at the top level of a module and store them in the module environment object. This is unnecessary and we could get away with storing them on the stack if they are not exported or not otherwise aliased.

This required a bit of refactoring first, so it's split into several patches. The idea is this: - instantiate ModuleBuilder earlier and use it to gather data about imports and exports while we are parsing, rather than doing this at the end - use this data to mark exported local bindings as aliased before we generate the bindings - update the parser to mark other declarations aliased as necessary (e.g. module namespace imports) We can also get rid of the list of exported names that's currently stored in the ModuleBox as we will have this information in the ModuleBuilder.

Instantiate ModuleBuilder before parsing and pass it in to standaloneModule().

Rather than having ModuleBuilder traverse the AST after parsing, call into it every time an import/export parse node is created.

Check for duplicate exported names using the data in the ModuleBuilder and remove ModuleBox::exportNames.

Make some accessors const.

Finally we get to point of all this: don't automatically alias all declarations at module top level but use the builder's export information to alias exported top level bindings. Currently we still need to alias all top level function definitions because of the way we instantiate function declarations ahead of executing the top level code. It may be possible to do better by instantiating only some functions early. I'll look into that when I get back next year. This change means that Reflect.parse now throws syntax errors for module code that exports non-existent bindings, as it should. Test code was updated for that and to reflect the different contents of the module environment.

Comment on attachment 8699108 [details] [diff] [review] bindings-5-alias-fewer-bindings Review of attachment 8699108 [details] [diff] [review]: ----------------------------------------------------------------- This is simpler than I imagined, very nice. Could you please write a Debugger test or two (crib off of the Environment-* tests inside jit-test/tests/debug) to make sure that these unaliased bindings are able to be reflected and fetched by the Debugger API? ::: js/src/frontend/Parser.cpp @@ -305,5 @@ > if (!checkLocalsOverflow(ts)) > return false; > } > - if (atModuleScope()) > - dn->pn_dflags |= PND_CLOSED; \o/ @@ +988,5 @@ > + Definition* def = modulepc.decls().lookupFirst(name); > + if (!def) { > + JSAutoByteString str; > + if (!str.encodeLatin1(context, name)) > + return null(); Why encodeLatin1 here instead of AtomToPrintableString? ::: js/src/jit-test/tests/modules/export-declaration.js @@ +134,5 @@ > ], > null, > false > ) > +]).assert(parseAsModule("let as = 1; export { as as as }")); lol

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a605806d9f0 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7013426d292 https://hg.mozilla.org/integration/mozilla-inbound/rev/95ad4f2242ef https://hg.mozilla.org/integration/mozilla-inbound/rev/53e00d75c914 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb17cb935f2

https://hg.mozilla.org/mozilla-central/rev/7a605806d9f0 https://hg.mozilla.org/mozilla-central/rev/c7013426d292 https://hg.mozilla.org/mozilla-central/rev/95ad4f2242ef https://hg.mozilla.org/mozilla-central/rev/53e00d75c914 https://hg.mozilla.org/mozilla-central/rev/3bb17cb935f2 https://hg.mozilla.org/mozilla-central/rev/a110885c2b5b