I've written a first prototype that basically compiles modules as if they were scripts, but using a ModuleBox rather than a GlobalSharedContext (so the parser can tell we're parsing a module), and storing the resulting script in a Module object and returning that, rather than returning the script directly. This prototype is still far from complete, and I've added a few comments with questions about parts that are unclear. Could you take a look at those when you have time, so we can talk about them on irc later? Most importantly, I still need to figure out how to deal with module scope objects. I'd like to talk to you about that too.

I've made some significant progress with respect to the previous prototype. I've cleaned up the code for CompileModule, and added support for directive parsing and lazy parsing. CompileModule now almost does what CompileScript does, with a few significant differences: 1. The compiled script is stored in a module object. Instead of returning the script directly, the module object is returned instead. 2. I've removed all code that deals with things like eval, since modules can never be evalled. In this version, I've also added some code to CompileModule that creates a StaticBlockScope, and then push a STMT_BLOCK statement, to fool the parser into thinking it is parsing inside a block scope. Once parsing and emitting is complete, I set the scope chain of the block scope to be the global object, and store it inside the module. When the module is executed, the block scope is passed as its scope chain. Since the scope chain of the block scope is the global object we used during parsing, this should give us the correct semantics. For testing purposes, I've disabled converting top-level lets back to vars, and tried to compile: executeModule(compileModule("let x = 42; print(x); { let x = 42; print(x); }")); The resulting bytecode looks promising: http://pastebin.mozilla.org/3362976 As you can see, the let statement outside the block statement generates the same bytecodes as the one within the block statement. This is what I expect, since my goal was to fool the parser into thinking its parsing inside a block statement. However, the results are not what I expect. In particular, the first print results in: function print() { [native code] } Whereas the second one results in: 42 Somehow, the interpreter is looking for the value to be printed in the wrong place, causing print to print itself instead. Apparently, I've messed up the stack model of the emitter/interpreter somewhere. Do you have any ideas what I could be doing wrong?

I should probably add that I've had to disable several assertions in order for the interpreter to accept StaticBlockScope as scopeChain. It's likely that there are places where the code doesn't expect this. Pending your feedback, I'm going to try my original approach, which is to push our StaticBlockScope using pushBlock right after the execute frame has been pushed.

Great work! I see what's going on here. The JSOP_ENTERBLOCK opcode that we emit for blocks does three things: 1. it clones a block object and adjusts the scope chain, which we definitely don't want 2. it also adjusts sp to allocate stack slots for the non-aliased block-local variables 3. and initializes all those slots to undefined Things 2 and 3 are things we *do* need. Since nothing is allocating stack slots for the variable x in your example, x is sharing its stack slot with the operand stack, which happens to contain the print function. Try having the bytecode emitter set JSScript::nfixed (for a module script) to the number of non-aliased variables (like x) in that implicit top-level block. I think this would fix #2 because -> Interpret() -> FrameRegs::prepareToRun -> sp = fp.slots() + script->nfixed; and fix #3 because -> Interpret() -> ExecuteState::pushInterpreterFrame -> InterpreterStack::pushExecuteFrame -> StackFrame::initVarsToUndefined -> SetValueRangeToUndefined(slots(), script()->nfixed); Currently I think nfixed is always for 0 for non-function scripts.

I've made significant process towards a usable prototype. I've fixed the earlier bug where marking outer let bindings as aliased would also cause inner let bindings of the same name to be marked as aliased (the solution is to simply not copy this flag from the previous definition if the definition is a let). Another problem I ran into is that we want the value for top level bindings to be stored on the module's block object. That means we want to push the module's block object itself on the scope chain, rather than a clone. However, aliased opcodes can only refer to a cloned block object, not a static one. I solved this by adding some special casing to the code for pushing blocks so that for module blocks, instead of creating a clone, the block itself is used as the cloned block. In other words, module blocks are both static blocks and cloned blocks. This is reflected in the specializations for is<>, which helps to avoid some nasty assertions in the vm. I still need to fix the following things: 1. Treat top level vars, consts, etc in a module as lets 2. Define functions on the module block early 3. Only alias exported bindings, rather than all top level lets (as we do now) 4. Code cleanup, commenting, and testing I plan to work on step 1 and 4 tomorrow and then put something up for review. The other steps will have to wait for a while. Since compile/runModule is only exposed to the shell for now, its not a big deal if we don't have that many tests. In fact, I think the sensible thing to do is to land this code early, start playing with it, and write tests for any bugs we encounter as we go along. Jorendorff, do you agree?

Oh man, I just hit another problem. AssertDynamicScopeMatchesStaticScope *really* doesnt like it if we use the same object for the static and the dynamic scope chain (presumably because their value of enclosingScope is different?) This is starting to get stupid. We're basically struggling to fool the interpreter to eat a static block object as if it were a cloned block object. At the same time, that cloned block object needs a pointer to the static block object from which it was cloned, and because their scope chains aren't identical, that can't be the same object. So, here's a new thought. static block objects are templates for cloned block objects in the sense that they completely define the layout of their clones. Furthermore, if we allow a module's block object to be cloned it will only ever be cloned once (assuming module scripts are only run once). Consequently, there is a 1-on-1 relation between the static block for a module and the cloned block for the module. What we could do is this: when the module script is run, clone the module's static block, but add some special casing to pushBlock to store the cloned block in a reserved slot for the static block. Since modules store their static block, this gives the access to the values stored in the cloned block via the static block from which this block was created. We can still do the linking early because the slot layout of the static and cloned block are identical. The problem with this approach is that we need to define functions early. The static blocks slots are already used to store which slots contain aliased values, so we can't store them there. This is where I'm stuck currently. Suggestions would be welcome.

Ok, I think I figured it out. How does this sound? Inherit ModuleBlockObject from StaticBlockObject. Assuming a module's init script is only ever run once, a ModuleBlockObject is only ever cloned once. This means we can clone the ModuleBlockObject early, during compilation. The resulting ClonedBlockObject is stored in the ModuleBlockObject itself. When a ModuleBlockObject is pushed, the ClonedBlockObject associated with the ModuleBlockObject is pushed on the scope chain, and the ModuleBlockObject itself is pushed on the block chain. This should keep the interpreter's model of both the static and the dynamic scope intact. The ClonedBlockObject is what will end up storing the exported values, so we store this block in the Module object.

Eddy and I agreed on IRC today that this is the right plan.

A Module object is simply a container that stores the modules script and its scope object (on which the exported variables will live). Eventually, we also want to store a list of exported names and their associated slots here.

This initial implementation of CompileModule simply mimics what CompileScript does (with some code for handling things such as eval removed), but returns a Module object instead of a JSScript as result. Subsequent patches will further specialise this function for modules.

ExecuteModule does exactly what you'd expect. No magic involved here.

This patch refactors CompileModule so that instead of compiling a module like a top-level script (i.e., one statement at a time), we compile it in a similar way as a standalone function. To this end, I've added a method, Parser::standaloneModuleBody, that pushes a lexical scope and a statement of type STMT_MODULE on the scope/statement stack, before parsing the module body. When the top level statement is STMT_MODULE, ParseContext::define knows to automatically mark any let bindings it encounters as closed over, ensuring that the values of these lets will end up in the block object for the pushed scope. Note that this solution is not complete yet. We still need to make sure that top level var/consts/functions/etc in a module are treated as lets as well, and we need to optimize it so that only lets that are actually being exported are aliased. I intend to fix these parts in a followup bug.

Our stack model requires us to have both a static and a cloned block for a module's lexical scope. The cloned block is the one we're interested in, since this is where the exported values will end up being stored. However, the cloned block is normally only created when the module script is run. For linking, we need it sooner than that. This patch fetches the static block created for the lexical scope pushed by standaloneModule from the emitter, creates the cloned block for this static block early, and associates the cloned block with both the static block and the module, where the linker can access it. If the interpreter needs to clone a static block and finds that it already has a cloned block associated with it, it grabs that cloned block and fully initializes it, rather than creating a new one. Note that this is based on the assumption that static blocks for modules are only ever cloned once, since module scripts are only ever run once.

Friendly review ping. Please don't let these patches bitrot! :-)

Comment on attachment 829286 [details] [diff] [review] Implement Module objects Review of attachment 829286 [details] [diff] [review]: ----------------------------------------------------------------- Adopting. Updated patch coming soonish.

(In reply to Eddy Bruel [:ejpbruel] from comment #14) > Friendly review ping. Please don't let these patches bitrot! :-) ehm

Add frontend::ParseModule() to parse source text and create a ModuleObject.

Add module support to the bytecode emitter when called via ParseModule().

The top level lexical scope of a module needs to be available before the module is executed so that bindings can be made available for import. Allow static block objects to be cloned and the cloned block object attached. Adding a slot to StaticBlockObject broke the assertion in ScopeObject::setAliasedVar(). I've changed the assert to check the slot number is valid depending on the type of object (and the tests pass) but I'm not 100% sure this is right.

Calculate the RequestedModules field of a module. For testing purposes this also makes it a visible property of the module object.

Calculate module ImportEntries field, adding ImportEntryObject and populating a list of these on the module.

Populate module export entry fields.

I will try to get these done over the next week, but FYI, I will be at a conference (ECOOP) next week, and taking PTO the week after.

Comment on attachment 8628909 [details] [diff] [review] modules-1-parse-module v2 Review of attachment 8628909 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me architecturally. I would like to see ParseModule refactored before r+ though. The code in there is borderline incantation: things that must be done to set up the frontend, and are mostly shared with CompileScript. ::: js/src/builtin/ModuleObject.cpp @@ +35,5 @@ > + > +ModuleObject* > +ModuleObject::create(JSContext* cx) > +{ > + RootedObject object(cx, NewBuiltinClassInstance(cx, &class_)); You should use the templated NewBuiltinClassInstance<ModuleObject>, which will do the casting automatically. @@ +38,5 @@ > +{ > + RootedObject object(cx, NewBuiltinClassInstance(cx, &class_)); > + if (!object) > + return nullptr; > + ModuleObject &module = object->as<ModuleObject>(); Style nit: ModuleObject& module ::: js/src/builtin/ModuleObject.h @@ +10,5 @@ > +#include "vm/NativeObject.h" > + > +namespace js { > + > +class ClonedBlockObject; Accidentally left in? @@ +17,5 @@ > +{ > + public: > + enum > + { > + ScriptSlot, ScriptSlot = 0 for clarity? Is it specced that enums start at 0? I don't actually know. @@ +24,5 @@ > + > + static const Class class_; > + > + static ModuleObject* create(JSContext* cx); > + void init(HandleScript script); I see no reason to make this take a HandleScript over JSScript*. @@ +29,5 @@ > + > + JSScript* script() const; > + > + private: > + void initScript(HandleScript script); JSScript* here too. ::: js/src/frontend/BytecodeCompiler.cpp @@ +463,5 @@ > > +ModuleObject * > +frontend::ParseModule(JSContext* cx, HandleObject obj, > + const ReadOnlyCompileOptions& optionsInput, > + SourceBufferHolder& srcBuf) Style nit: arguments not lined up. @@ +497,5 @@ > + > + Rooted<ScopeObject*> enclosingStaticScope(cx); > + enclosingStaticScope = StaticNonSyntacticScopeObjects::create(cx, nullptr); > + if (!enclosingStaticScope) > + return nullptr; Please change this per our Whistler discussion. I think we decided to make a new kind of static scope object? ::: js/src/frontend/BytecodeCompiler.h @@ +30,5 @@ > SourceBufferHolder& srcBuf, JSString* source_ = nullptr, > unsigned staticLevel = 0, SourceCompressionTask* extraSct = nullptr); > > +ModuleObject * > +ParseModule(JSContext *cx, HandleObject obj, const ReadOnlyCompileOptions &options, I am not happy with this function. Too much copy/paste with CompileScript. Is it possible to refactor? Also, for consistency, this should be named CompileModule, since we're emitting bytecode for it. ::: js/src/frontend/Parser.cpp @@ +780,5 @@ > +Parser<ParseHandler>::standaloneModuleBody() > +{ > + StmtInfoPC stmtInfo(context); > + > + Node pn = pushLexicalScope(&stmtInfo); Just to make sure: per spec, module lexical scopes are exactly like regular block lexical scopes in every way?

Comment on attachment 8628910 [details] [diff] [review] modules-2-emitter-support Review of attachment 8628910 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeCompiler.cpp @@ +573,5 @@ > if (!NameFunctions(cx, pn)) > return nullptr; > if (!bce.updateLocalsToFrameSlots()) > return nullptr; > + if (!bce.emitModule(pn)) Why not initialize a BCE that's isModule to begin with?

Comment on attachment 8628911 [details] [diff] [review] modules-3-requested-modules-field v2 Review of attachment 8628911 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: js/src/builtin/ModuleObject.cpp @@ +173,5 @@ > + > + for (ParseNode* pn = stmtsNode->pn_head; pn; pn = pn->pn_next) { > + switch (pn->getKind()) { > + case PNK_IMPORT: > + if (!processImport(pn)) Since you already do the switch on the ParseNode kind, I think it'd be cleaner to change processImport and processExportFrom to take atoms directly. ::: js/src/builtin/ModuleObject.h @@ +32,5 @@ > }; > > static const Class class_; > > + static bool is(HandleValue value); What's this used for? I wonder if it'll be confusing with the non-static is<T>. ::: js/src/jsprototypes.h @@ +111,5 @@ > IF_SAB(real,imaginary)(SharedUint8ClampedArray, 51, InitViaClassSpec, SHARED_TYPED_ARRAY_CLASP(Uint8Clamped)) \ > real(TypedArray, 52, InitViaClassSpec, &js::TypedArrayObject::sharedTypedArrayPrototypeClass) \ > IF_SAB(real,imaginary)(Atomics, 53, InitAtomicsClass, OCLASP(Atomics)) \ > real(SavedFrame, 54, InitViaClassSpec, &js::SavedFrame::class_) \ > + real(Module, 55, InitModuleClass, OCLASP(Module)) \ Is this for testing purposes only, or are modules actually reflectable? If the former, could you move this to the testing code only?

Thanks for all the comments. I refactored BytecodeCompiler.cpp in bug 1182428 (which should land soon) so there is much less duplication here now. Re passing JSScript*/HandleScript, I prefer to pass pointers to GC things as handles all the time except where it's a performance issue because it makes the code more consistent and helps to ensure that everything remains rooted. Let me know if you feel strongly otherwise and I'll change it. From my reading of the spec, module lexical scopes are just like regular block scopes except that 1) they can contain import bindings, 2) bindings are not deleteable (and modules are strict so this can't happen anyway) and 3) there is a binding for 'this' that has the value undefined.

Updated with review comments.

Updated with review comments. > Since you already do the switch on the ParseNode kind, I think it'd be > cleaner to change processImport and processExportFrom to take atoms directly. The ParseNode itself gets used by later patches. > > + static bool is(HandleValue value); > > What's this used for? I wonder if it'll be confusing with the non-static > is<T>. It's for CallNonGenericMethod() and the machinery for adding methods to JS objects. I just copied if from somewhere else, but actually it is confusing so I renamed it to isInstance() instead. > > + real(Module, 55, InitModuleClass, OCLASP(Module)) \ > > Is this for testing purposes only, or are modules actually reflectable? If > the former, could you move this to the testing code only? It's necessary for modules to be visible to JS code so they can be used by the module loader. In subsequent patches I end up exposing a lot more than necessary though as it makes testing easier. The eventual aim was to remove these before exposing modules beyond the shell, although I could do that now and add another way to expose module data for testing. The current loader spec does make use of requestedModules to calculate dependencies so this needs to be exposed somehow, although as far as I understand nothing says that it has to be a property.

Create array of import entries from the parse tree.

Collect information about export entries from the parse tree.

Comment on attachment 8637948 [details] [diff] [review] modules-1-parse-module v3 Review of attachment 8637948 [details] [diff] [review]: ----------------------------------------------------------------- The refactor made this much nicer, thanks. ::: js/src/builtin/ModuleObject.cpp @@ +32,5 @@ > + nullptr, /* construct */ > + ModuleObject::trace > +}; > + > +ModuleObject* /* static */ ::: js/src/builtin/ModuleObject.h @@ +30,5 @@ > + private: > + static void trace(JSTracer* trc, JSObject* obj); > +}; > + > +} // namespace JS namespace js ::: js/src/frontend/BytecodeCompiler.cpp @@ +680,5 @@ > + do { > + pn = parser->standaloneModule(); > + if (!pn && !handleParseFailure()) > + return nullptr; > + } while (!pn); I don't think this needs to be a loop, unlike the function case. From my reading I don't see how handleParseFailure would ever return true here, since modules aren't syntax parsed.

Comment on attachment 8638620 [details] [diff] [review] modules-4-import-entries-field Review of attachment 8638620 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: js/src/builtin/ModuleObject.cpp @@ +106,5 @@ > + JSAtom* importName, > + JSAtom* localName, > + AutoKeepAtoms& keepAtoms) > +{ > + RootedImportEntry self(cx, &NewBuiltinClassInstance(cx, &class_)->as<ImportEntryObject>()); NewBuiltinClassInstance<ImportEntryObject> ::: js/src/builtin/ModuleObject.h @@ +85,3 @@ > AutoAtomVector requestedModules_; > + AutoAtomVector importedBoundNames_; > + AutoImportEntryVector importEntries_; Now that we have the nicer TraceableVector<T>, do you think it's worth to switch these over to Rooted<TraceableVector<T>>? Or should we wait for the 'using AutoFooVector = Rooted<TraceableVector<Foo>>' patch?

Comment on attachment 8638622 [details] [diff] [review] modules-5-export-entry-fields Review of attachment 8638622 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/ModuleObject.cpp @@ +200,5 @@ > + RootedObject object(cx, NewBuiltinClassInstance(cx, &class_)); > + if (!object) > + return nullptr; > + ExportEntryObject &self = object->as<ExportEntryObject>(); > + self.initReservedSlot(ExportNameSlot, StringOrNullValue(maybeExportName)); Why are these null instead of undefined on non-existence? I thought usually null means "no object". @@ +386,5 @@ > + if (ie->importName() == cx_->names().star) { > + if (!append(localExportEntries_, exp)) > + return false; > + } else { > + RootedExportEntry iee(cx_); Let's do importEntry and exportEntry instead of ie and iee. :) ::: js/src/builtin/ModuleObject.h @@ +41,5 @@ > > +class ExportEntryObject : public NativeObject > +{ > + public: > + enum { ExportNameSlot, ModuleRequestSlot, ImportNameSlot, LocalNameSlot, SlotCount }; ExportNameSlot = 0 for clarity @@ +58,5 @@ > + JSAtom* localName(); > +}; > + > +typedef Rooted<ExportEntryObject*> RootedExportEntry; > +typedef AutoVectorRooter<ExportEntryObject*> AutoExportEntryVector; Rooted<TraceableVector<...>>

(In reply to Shu-yu Guo [:shu] from comment #37) Thanks for all the reviews! > Why are these null instead of undefined on non-existence? I thought usually > null means "no object". It's defined that way in the spec, see table 41.

Updated to use Rooted<TraceableVector<>>.

Updated to use Rooted<TraceableVector<>> and fix spurious rooting hazards by rooting atoms as the analysis doesn't understand AutoKeepAtoms.

Updated to use Rooted<TraceableVector<>> and fix spurious rooting hazards.

Add static and dynamic scope objects for modules. This adds a method on the static block object to get the dynamic scope object which returns the pre-created object in the case of modules.

Hook up the module environment. This patch adds an 'initialEnvironment' property to a module that is the pre-created dynamic scope object that functions as the top level module environment (the spec calls for an 'environment' property but that has to be undefined before module declarations have been instantiated). This also creates bindings for modules unlike the empty bindings we had previously. ParseContext<ParseHandler>::initModuleEnvironment() initialises the environment so that cyclic import bindings can be established before we execute anything. Top level lexicals as marked as aliased so they get put in our new environment, and top level vars are marked as deoptimised to prevent the emitter using GNAME instructions for them.

Comment on attachment 8642514 [details] [diff] [review] modules-6-scope-objects Review of attachment 8642514 [details] [diff] [review]: ----------------------------------------------------------------- Not done reviewing yet, but I would like the answer to the following question. ::: js/src/vm/ScopeObject.h @@ +610,5 @@ > * static scope is a JSFunction. > */ > inline StaticBlockObject* enclosingBlock() const; > > + ClonedBlockObject* getOrCreateClonedBlock(JSContext* cx, AbstractFramePtr frame); Why is pre-creating the dynamic module scope needed? Aren't module scripts run-once? Is it for getting bindings out later? In any case, why is this necessary on StaticBlockObject? Couldn't this logic and the extra slot be moved to StaticModuleScopeObject?

Comment on attachment 8642526 [details] [diff] [review] modules-7-environment Review of attachment 8642526 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand the implementation strategy here. What will a module script's Bindings object be used for? Why is it necessary that all of a module script's body-level bindings be aliased? Let's discuss an implementation strategy next week. I'm thinking along the lines of letting JSScript distinguish themselves between function scripts (by having a JSScript::function_), module scripts (by having a new module_ field, maybe unioned with function_ or something), and global scripts (by having neither). Heavyweight function scripts would push a CallObject onto the scope chain when executing, and module scripts would push their module scope object. Module scripts, like global scripts, probably should never generate a Bindings. ::: js/src/frontend/BytecodeCompiler.cpp @@ +699,5 @@ > emitter->tellDebuggerAboutCompiledScript(cx); > > + RootedDynamicModuleScopeObject dynamicScope(cx, staticScope->createDynamicScope(cx)); > + if (!dynamicScope || > + !parseContext.ref().initModuleEnvironment(cx, dynamicScope)) Nit: Could use operator-> instead of ref()?

Comment on attachment 8642514 [details] [diff] [review] modules-6-scope-objects Review of attachment 8642514 [details] [diff] [review]: ----------------------------------------------------------------- Canceling pending discussion.

(In reply to Shu-yu Guo [:shu] from comment #45) > What will a module script's Bindings object be used for? I got assertions in the emitter when emitting variable accesses before I did this. I think it was failing when accessing localsToFrameSlots_, which is be calculated from information in the bindings. I don't know if the are necessary for other reasons too. > Why is it necessary that all of a > module script's body-level bindings be aliased? It's only necessary to mark exported bindings as aliased so they end up stored in the scope object. For now I was aliasing everything, but this can be fixed.

(In reply to Shu-yu Guo [:shu] from comment #44) > Why is pre-creating the dynamic module scope needed? Aren't module scripts > run-once? Is it for getting bindings out later? The bindings need to be available before the module is executed, so we can support cyclic dependencies between modules. > In any case, why is this necessary on StaticBlockObject? Couldn't this logic > and the extra slot be moved to StaticModuleScopeObject? It could, but this would require that methods inherited from BlockObject and StaticBlockObject check the object class or otherwise calculate which slot contains the first variable. Doing it that way seem to make the non-module case more complicated.

Since our conversation last week, I've reworked these patches to parse modules much more like how we parse functions. This was slightly more work but turned out to make things clearer and more explicit, and I'm much happier with the result. So now we use ModuleObject itself as the static scope object and will have a new ModuleEnvironmentObject for the dynamic scope object which is derived from CallObject (see later patches). The environment object will be created ahead of time as before, but will now be pushed on the scope chain in the same was as a function's call object. When we parse a module we have a ModuleBox which holds a pointer to the ModuleObject and and we do the same trick with having a StaticModuleBoxScopeObject on the static scope chain initially which is swapped for the ModuleObject later. This part is probably unnecessary but I did it to mirror the way function scopes work initially, and I'll try and remove this later.

This is pretty much the same as before, but now we can check the shared context to see if we're emitting a module and don't need the isModule flag in the emitter.

No significant changes, carrying review.

No significant changes, carrying review.

No significant changes, carrying review.

Add ModuleEnvironmentObject dynamic scope object and make scope iterators work with the new scope objects.

Hook up the new dynamic scope objects, creating them when the module is parsed and pushing them on the scope chain when the module's script is executed. Add a new interpreter frame type for modules. Alias everything at module top level.

Add check for duplicate exported names.

Implement default exports

Make imports into definitions so we get an error if there is a name clash.

Add a hook for HostResolveImportedModule.

(In reply to Jon Coppeard (:jonco) from comment #49) > Created attachment 8648798 [details] [diff] [review] > modules-1-parse-module v4 > > When we parse a module we have a ModuleBox which holds a pointer to the > ModuleObject and and we do the same trick with having a > StaticModuleBoxScopeObject on the static scope chain initially which is > swapped for the ModuleObject later. This part is probably unnecessary but I > did it to mirror the way function scopes work initially, and I'll try and > remove this later. I liked that approach, but it turned out to regress codeload due to the extra static scopes, and I had to change it to use JSFunction instead. I think you have to change it sooner than later. See bug 1179063. Sorry about the churn. :(

Comment on attachment 8648798 [details] [diff] [review] modules-1-parse-module v4 Review of attachment 8648798 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty nice and was much easier for me to understand. Thanks for redoing it! ::: js/src/frontend/BytecodeCompiler.cpp @@ +662,5 @@ > + module->init(script); > + > + ParseNode* pn = parser->standaloneModule(module); > + if (!pn) > + return nullptr; Do you need call |handleParseFailure| here and try again in case of syntax parse failure? ::: js/src/frontend/BytecodeEmitter.cpp @@ +124,5 @@ > + MOZ_ASSERT(staticScope_->as<StaticModuleBoxScopeObject>().moduleBox() == this); > + staticScope_ = module(); > + } > + MOZ_ASSERT(staticScope_ == module()); > +} switchStaticScopeToFunction was removed due to codeload regression. The trick we did for functions was to store the FunctionBox* temporarily in the JSFunction in a field unioned with the environment during parsing. But that's only because JSFunctions are memory constrained and they're generally a mess. Modules can probably do something more sane without using unions: have a pointer to their ModuleBox while being parsed so StaticScopeIter can find the enclosing scope link, otherwise use their JSScript. See AutoParseUsingFunctionBox. ::: js/src/frontend/Parser.cpp @@ +762,5 @@ > +template <> > +ModuleBox* > +Parser<SyntaxParseHandler>::newModuleBoxWithScope(Node pn, ModuleObject* module, JSObject* staticScope) > +{ > + JS_ALWAYS_FALSE(abortIfSyntaxParser()); nit: MOZ_ALWAYS_FALSE ::: js/src/frontend/SharedContext.h @@ +378,5 @@ > +class ModuleBox : public ObjectBox, public SharedContext > +{ > + public: > + Bindings bindings; > + JSObject* staticScope_; Could you store staticScope_ using the most specific type? ::: js/src/jsscript.h @@ +1151,5 @@ > // Add padding so JSScript is gc::Cell aligned. Make padding protected > // instead of private to suppress -Wunused-private-field compiler warnings. > protected: > #if JS_BITS_PER_WORD == 32 > + // No padding currently required. \o/ free words don't come 'round every day. ::: js/src/shell/js.cpp @@ +2073,5 @@ > + if (value.isObject() && value.toObject().is<ModuleObject>()) { > + script = value.toObject().as<ModuleObject>().script(); > + } else { > + script = ValueToScript(cx, value, fun.address()); > + } nit: no need to brace around single-line conditionals

Comment on attachment 8648810 [details] [diff] [review] modules-6-scope-objects Review of attachment 8648810 [details] [diff] [review]: ----------------------------------------------------------------- This is nice and clean. ::: js/src/frontend/Parser.cpp @@ +431,5 @@ > > return Bindings::initWithTemporaryStorage(cx, bindings, args_.length(), vars_.length(), > bodyLevelLexicals_.length(), blockScopeDepth, > numUnaliasedVars, numUnaliasedBodyLevelLexicals, > + packedBindings, isModule); Could you avoid threading a bool through generateBindings by doing sc->isModuleBox() here? ::: js/src/vm/ScopeObject-inl.h @@ +151,5 @@ > + if (obj->template is<StaticNonSyntacticScopeObjects>()) > + return NonSyntactic; > + if (obj->template is<ModuleObject>()) > + return Module; > + return Function; Oh man, thanks for converting this to something readable. ::: js/src/vm/ScopeObject.cpp @@ +340,5 @@ > + return nullptr; > + > + Rooted<ModuleEnvironmentObject*> scope(cx, &obj->as<ModuleEnvironmentObject>()); > + > + // Set uninitialized lexicals even on template objects, as Ion will use nit: could you fix the comment while you're here? Extra 'use' ::: js/src/vm/ScopeObject.h @@ +397,5 @@ > + static const Class class_; > + > + static ModuleEnvironmentObject* create(ExclusiveContext* cx, HandleModuleObject module); > + ModuleObject& module() const; > +}; Nice!

Comment on attachment 8648813 [details] [diff] [review] modules-7-environment Review of attachment 8648813 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I'd like to be reminded of the aliased decision -- you don't need to change the code. ::: js/src/builtin/ModuleObject.h @@ +97,5 @@ > HandleArrayObject localExportEntries, > HandleArrayObject indiretExportEntries, > HandleArrayObject starExportEntries); > > + ModuleEnvironmentObject* createDynamicScope(ExclusiveContext* cx); Dead declaration. ::: js/src/frontend/BytecodeEmitter.h @@ +225,5 @@ > JSObject* blockScopeOfDef(ParseNode* pn) const { > MOZ_ASSERT(pn->resolve()); > + unsigned blockid = pn->resolve()->pn_blockid; > + JSObject* scope = parser->blockScopes[blockid]; > + MOZ_ASSERT(scope); Until global lexical scope, it's still okay for this to return nullptr in the case of global bindings. ::: js/src/frontend/Parser-inl.h @@ +17,5 @@ > template <typename ParseHandler> > bool > ParseContext<ParseHandler>::init(Parser<ParseHandler>& parser) > { > + JSObject* scope = sc->isGlobalContext() ? sc->staticScope() : sc->toObjectBox()->object; I forgot to change this with the removal of StaticFunctionBoxScope. If you remove StaticModuleBoxScope accordingly, this can become just sc->staticScope(). ::: js/src/frontend/Parser.cpp @@ +237,5 @@ > if (!checkLocalsOverflow(ts)) > return false; > } > + if (atModuleScope()) > + dn->pn_dflags |= PND_CLOSED; Why is everything aliased again? To make the first implementation simpler instead of marking exports as aliased?

(In reply to Shu-yu Guo [:shu] from comment #61) Thanks for all the reviews. > Do you need call |handleParseFailure| here and try again in case of syntax > parse failure? We don't syntax parse modules, so I don't think this is needed here.

(In reply to Shu-yu Guo [:shu] from comment #63) > Why is everything aliased again? To make the first implementation simpler > instead of marking exports as aliased? We need to alias exports and anything referenced by exported functions. So yes it's a simplification for the first implementation, but in practice for a lot of modules we will end up aliasing everything anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f78c80504443f2f66b2550837dbd6de3724e54ff Bug 930414 - Add ModuleObject and CompileModule() function r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/52f4c8ddfe07f83c3a36ff96118cc3db1ce6af94 Bug 930414 - Add initial bytecode emitter support for modules r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc1a5cd4b301a3f9507697090e71d9f7e975c9b Bug 930414 - Add module requestedModules field r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/7861ef6582a9a013436d2c0f1c70150222775754 Bug 930414 - Add module importEntries field r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/178d594d4934fa20df30f2883460ede1d15eb3ec Bug 930414 - Add module fields relating to exports r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac1f5052b91cdc341570cb2f8c03efc561faa54 Bug 930414 - Add module scopes, using ModuleObject as the static scope and ModuleEnvironementObject as the dynamic scope r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/0773712473c9cea41fa3a063f97cbd2dc55d86a4 Bug 930414 - Hook up module environements, alising everything at top level for now r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5126631df0af76f91e35af894f0ea823ecb875c Bug 930414 - Fix base class initialization in ModuleBox constructor r=me on a CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/f78c80504443 https://hg.mozilla.org/mozilla-central/rev/52f4c8ddfe07 https://hg.mozilla.org/mozilla-central/rev/7fc1a5cd4b30 https://hg.mozilla.org/mozilla-central/rev/7861ef6582a9 https://hg.mozilla.org/mozilla-central/rev/178d594d4934 https://hg.mozilla.org/mozilla-central/rev/9ac1f5052b91 https://hg.mozilla.org/mozilla-central/rev/0773712473c9 https://hg.mozilla.org/mozilla-central/rev/b5126631df0a

Currently Reflect.parse() is used in tests and allows you to parse module source inlcuding imports and exports as a script (it's not possible to emit bytecode for it though). This is wrong and also means we can't depend on there being a modulebox when parsing such things. This patch adds an option to Reflect.parse() to parse the text as a module rather than a script. This takes the form of a 'target' option which can either be the string 'script' or the string 'module'. Also it allows modules to be parsed without a script, as happens in Reflect.parse(). I added ModuleObject::enclosingStaticScope() which always returns nullptr as modules are currently always the last thing on the scope chain before the global. I guess this will probably need to change when we get the top level lexcial scope.

Add a list of exported names to the ModuleBox and use it to generate an error if we try to export two things with the same name, as required by the spec.

Default exports. Add a second child to the export default parse node for a definition that is created when we parse |export default expression| and emit it if present.

Patch to turn imports into defintions and hence generate errors on duplicates in the normal way (the bound names of module imports are lexically declared names of the module toplevel according to the spec). This adds a new kind of definition, IMPORT. I wanted to do this by adding a PND_IMPORT flag, but there was no space to do this in the bitfield without reducing the allowed number of block ids, something we've had to increse in the past. Instead I added a flag to the ParseNode itself, which is not ideal but works. There is probably a better way of doing this. This also adds a flag to the emitter to indicate whether were emitting something inside a module, in which case we don't attempt to optimise unbound name accesses to use GNAME ops. This is necessary for functions inside a module which are parsed lazily.

Comment on attachment 8652445 [details] [diff] [review] modules-7.5-reflect-api Review of attachment 8652445 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/src/builtin/ModuleObject.cpp @@ +289,5 @@ > > +bool > +ModuleObject::hasScript() const > +{ > + return !getReservedSlot(ScriptSlot).isUndefined(); When would a module not have a script? Those that are still being parsed or errored out? @@ +309,5 @@ > +ModuleObject::enclosingStaticScope() const > +{ > + // A ModuleObject is always the last thing on the scope chain before the global. > + // TODO: This may no longer be true when we get top-level lexical scopes. > + MOZ_ASSERT_IF(!getReservedSlot(ScriptSlot).isUndefined(), Use hasScript() here for readability. ::: js/src/builtin/ReflectParse.cpp @@ +2330,5 @@ > case PNK_VAR: > case PNK_CONST: > case PNK_GLOBALCONST: > case PNK_LET: > + if (!variableDeclaration(kid, (kind == PNK_LET || kind == PNK_CONST), &decl)) Why this change to |kind == PNK_LET || kind == PNK_CONST|? @@ +3772,5 @@ > + return false; > + > + if (!prop.isString()) { > + ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_UNEXPECTED_TYPE, JSDVG_SEARCH_STACK, > + prop, nullptr, "not a string", nullptr); Might as well just give the more helpful error message of the property needing to be 'script' or 'module' here. @@ +3824,5 @@ > + if (target == ParseTarget::Script) { > + pn = parser.parse(); > + if (!pn) > + return false; > + nit: superfluous newline

Comment on attachment 8652447 [details] [diff] [review] modules-8-check-duplicate-exports v2 Review of attachment 8652447 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +4633,5 @@ > +bool > +Parser<FullParseHandler>::addExportName(JSAtom* exportName) > +{ > + if (!pc->sc->isModuleBox()) > + return false; Should this be asserted? Shouldn't we have already errored out if we're not parsing a module?

Comment on attachment 8653979 [details] [diff] [review] modules-10-check-duplicate-imports Review of attachment 8653979 [details] [diff] [review]: ----------------------------------------------------------------- Looks okay to me -- stylistically it's a bit off as you say yourself. The binding stuff feels unprincipled and messy. :( r=me with questions answered ::: js/src/frontend/BytecodeEmitter.cpp @@ +1577,5 @@ > > + // If we are inside a module then unbound names in a function may refer to > + // imports. Don't try to optimize these. > + if (insideModule) > + return false; Well, we will later though, I hope? Maybe add a TODO? @@ +1835,4 @@ > return true; > > case Definition::MISSING: > + MOZ_CRASH("unexpected definiton kind"); nit: typo definition @@ +3389,5 @@ > > +static bool > +IsModuleOnScopeChain(ExclusiveContext* cx, JSObject* obj) > +{ > + for (StaticScopeIter<CanGC> ssi(cx, obj); !ssi.done(); ssi++) { nit: this can be NoGC ::: js/src/frontend/ParseNode.h @@ +524,4 @@ > pn_parens : 1, /* this expr was enclosed in parens */ > pn_used : 1, /* name node is on a use-chain */ > + pn_defn : 1, /* this node is a Definition */ > + pn_import : 1; /* this node is a module import definition */ Well this is unfortunate... Looking at the list of PND_* flags, EMITTEDFUNCTION seems to be the most useless. If you're up for it, you could try to move that flag onto the FunctionBox instead and free up that bit for PND_IMPORT. ::: js/src/frontend/Parser-inl.h @@ +18,5 @@ > bool > ParseContext<ParseHandler>::init(Parser<ParseHandler>& parser) > { > + JSObject* scope = sc->staticScope(); > + MOZ_ASSERT_IF(!sc->isGlobalContext(), scope); I'd rather you didn't add this assert. It'll be changed soon for global lexicals anyways. ::: js/src/frontend/Parser.cpp @@ +4346,5 @@ > if (!dn) > return null(); > handler.setPosition(dn, pos); > + if (isConst) > + dn->pn_dflags |= PND_CONST; Why is this needed? Shouldn't bindInitialized below set PND_CONST? @@ +4449,5 @@ > + > + importNode->setImport(); > + importNode->pn_dflags |= PND_CONST; > + BindData<FullParseHandler> data(context); > + data.initLexical(HoistVars, nullptr, JSMSG_TOO_MANY_LOCALS); Double checking: imports *do* hoist? ::: js/src/vm/ScopeObject-inl.h @@ +87,5 @@ > JSFunction& fun = obj->template as<JSFunction>(); > if (fun.isBeingParsed()) > obj = fun.functionBox()->enclosingStaticScope(); > + else if (fun.isInterpretedLazy()) > + obj = fun.lazyScript()->enclosingScope(); When do you need to iterate through a lazy script that currently isn't being parsed? That is, when are you starting scope iteration at a scope that is *under* a lazy function?

(In reply to Shu-yu Guo [:shu] from comment #74) Thanks for the reviews. > When would a module not have a script? Those that are still being parsed or > errored out? I think I may have been experimenting with creating the script later on. At the moment modules always have a script though so I'll take this out. > > + if (!variableDeclaration(kid, (kind == PNK_LET || kind == PNK_CONST), &decl)) > > Why this change to |kind == PNK_LET || kind == PNK_CONST|? PNK_CONST is lexical binding and variableDeclaration() asserts that its |lexical| argument is true for these nodes, so this was just a bug.

(In reply to Jon Coppeard (:jonco) from comment #77) > I think I may have been experimenting with creating the script later on. At > the moment modules always have a script though so I'll take this out. Oh wait, no the point of this is that when parsing a module with the reflect API there is no script.

(In reply to Shu-yu Guo [:shu] from comment #76) > > + // If we are inside a module then unbound names in a function may refer to > > + // imports. Don't try to optimize these. > > + if (insideModule) > > + return false; > > Well, we will later though, I hope? Maybe add a TODO? This wasn't clear. What I meant was: don't try to optimize these using by using GNAME ops, because that would be wrong. I'll update the comment. > Well this is unfortunate... Looking at the list of PND_* flags, > EMITTEDFUNCTION seems to be the most useless. If you're up for it, you could > try to move that flag onto the FunctionBox instead and free up that bit for > PND_IMPORT. Good idea, I'll do that. This is much better. > Double checking: imports *do* hoist? As I understand it, yes. Checking the spec, CreateImportBinding() is called for imports during ModuleDeclarationInstantiation() before the code is executed and this "creates a new initialized immutable indirect binding". > > + else if (fun.isInterpretedLazy()) > > + obj = fun.lazyScript()->enclosingScope(); > > When do you need to iterate through a lazy script that currently isn't being > parsed? That is, when are you starting scope iteration at a scope that is > *under* a lazy function? Hmm, this doesn't seem to be necessary. I'll take it out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d0dfcabdca6 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6255e15eba3 https://hg.mozilla.org/integration/mozilla-inbound/rev/c31023810840 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d258339178

https://hg.mozilla.org/mozilla-central/rev/6d0dfcabdca6 https://hg.mozilla.org/mozilla-central/rev/e6255e15eba3 https://hg.mozilla.org/mozilla-central/rev/c31023810840 https://hg.mozilla.org/mozilla-central/rev/d3d258339178

Add support for the HostResolveImportedModule operation. This can be set by the embedding and is provided by the module loader to look up a previously loaded module. The patch adds a new slot to the global which is used to hold a function representing the operation.

Implement the ResolveExport() operation on modules. This is done as self-hosted JS and is a pretty straightforward translation of the spec. The operation itself is used by later patches.

Implement import bindings and ModuleDeclarationInstantiation(). This adds a map of indirect bindings to the module object which is populated by the self-hosted ModuleDeclarationInstantiation() operation according to the spec. The ModuleEnvironmentObject get object ops so that scope lookups see these bindings.

Support module evaluation. Add ModuleEvaluation() operation. This mainly involves keeping track of the 'evaluated' state on the module and executing the module script if necessary. Fix Baseline and Ion to use the correct scope chain (i.e. the ModuleObject) when compiling a module.

Implement module namespace objects. These are similar to module environment objects in that they have a map of bindings and property accesses on them look up the name in the map. I implemented these as proxies since the spec's definition of their internal methods maps closely to our proxy hooks.

Comment on attachment 8658675 [details] [diff] [review] modules-11-host-resolve-hook v2 Review of attachment 8658675 [details] [diff] [review]: ----------------------------------------------------------------- This looks okay. I don't really understand the intended usage of this handle though. Some questions: 1) Why is it callable from self-hosted code? 2) Who is the "module loader"? The embedding? 3) Why is it a slot on the global? Is it exposed to script? ::: js/src/shell/js.cpp @@ +3120,5 @@ > + return false; > + } > + > + RootedFunction hook(cx, &args[0].toObject().as<JSFunction>()); > + Rooted<GlobalObject*> global(cx, cx->global()); No need to root, global() already returns a Handle, I think.

Comment on attachment 8658677 [details] [diff] [review] modules-12-resolve-export Review of attachment 8658677 [details] [diff] [review]: ----------------------------------------------------------------- Ah, here's where you call that resolve hook in self-hosted code. ::: js/src/builtin/Module.js @@ +16,5 @@ > + > + // Step 2 > + for (let i = 0; i < resolveSet.length; i++) { > + let r = resolveSet[i]; > + if (r.module == module && r.exportName == exportName) Use ===, the spec uses SameValue. @@ +35,5 @@ > + // Step 5 > + let indirectExportEntries = module.indirectExportEntries; > + for (let i = 0; i < indirectExportEntries.length; i++) { > + let e = indirectExportEntries[i]; > + if (exportName == e.exportName) { === @@ +40,5 @@ > + let importedModule = HostResolveImportedModule(module, e.moduleRequest); > + let indirectResolution = importedModule.resolveExport(e.importName, > + resolveSet, > + exportStarSet); > + if (indirectResolution != null) !==

Comment on attachment 8658733 [details] [diff] [review] modules-13-module-declaration-instantiation Review of attachment 8658733 [details] [diff] [review]: ----------------------------------------------------------------- Oof... the JIT folks will have fun making imports fast. ::: js/src/builtin/ModuleObject.h @@ +77,5 @@ > + RelocatablePtr<ModuleEnvironmentObject*> environment; > + RelocatableId localName; > +}; > + > +typedef HashMap<jsid, IndirectBinding, JsidHasher, SystemAllocPolicy> BindingMap; Name this IndirectBindingMap for clarity. ::: js/src/jit-test/tests/modules/module-declaration-instantiation.js @@ +23,5 @@ > +a.declarationInstantiation(); > +b.declarationInstantiation(); > + > +assertEq(Object.keys(a.environment).length, 1); > +assertEq(Object.keys(b.environment).length, 1); Should this test be more specific, testing the existence of specific keys? ::: js/src/vm/ScopeObject.cpp @@ +398,5 @@ > +} > + > +bool > +ModuleEnvironmentObject::createImportBinding(JSContext*cx, HandleAtom importName, > + HandleModuleObject module, HandleAtom exportName) Nit: here and below, please line up the 2nd line of parameters. ::: js/src/vm/ScopeObject.h @@ +28,5 @@ > class StaticEvalObject; > class StaticNonSyntacticScopeObjects; > > +struct IndirectBinding; > +typedef HashMap<jsid, IndirectBinding, JsidHasher, SystemAllocPolicy> BindingMap; You wrote this typedef twice. Please consolidate.

Comment on attachment 8658748 [details] [diff] [review] modules-14-module-evaluation Review of attachment 8658748 [details] [diff] [review]: ----------------------------------------------------------------- A side observation/question: The environment returned by module.environment contains ALL bindings, right? Both exported and top-level vars, lets, and consts. Is that intended? The environment isn't accessible to script, right? ::: js/src/vm/Interpreter.cpp @@ +1007,5 @@ > if (!thisObj) > return false; > Value thisv = ObjectValue(*thisObj); > > + ExecuteType type = script->module() ? EXECUTE_MODULE : EXECUTE_GLOBAL; Does EXECUTE_MODULE actually do anything different? ::: js/src/vm/SelfHosting.cpp @@ +1318,5 @@ > + RootedModuleObject module(cx, &args[0].toObject().as<ModuleObject>()); > + module->setEvaluated(); > + args.rval().setUndefined(); > + return true; > +} Is the reason that this is an intrinsic and not a setter some kind of encapsulation guarantees?

(In reply to Shu-yu Guo [:shu] from comment #90) Thanks for the reviews. > Who is the "module loader"? The embedding? Yes, the module loader is a chunk of code supplied by the embedding that does the platform specific parts, e.g. actually getting the module source from somewhere. > Why is it a slot on the global? Is it exposed to script? It's not exposed to script. I just put it there because it seemed convenient. Is there a better place to put this? > The environment returned by module.environment contains ALL bindings, right? > Both exported and top-level vars, lets, and consts. Is that intended? The > environment isn't accessible to script, right? Yes, all bindings. At the moment this is exposed for testing purposes. What I need to do is to work out what needs to be made available to the module loader and what needs to be avilable only for testing and how to expose these. None of them actually need to be getters on the module object itself. Is it OK to land this as is and I'll address this in a subsequent patch? > Does EXECUTE_MODULE actually do anything different? Yes it's used to set InterpreterFrame::flags_ which makes isModuleFrame() work and is used in a bunch of places. > > + module->setEvaluated(); > > Is the reason that this is an intrinsic and not a setter some kind of > encapsulation guarantees? Yes, this is something that really doesn't want to be exposed.

(In reply to Jon Coppeard (:jonco) from comment #91) > (In reply to Shu-yu Guo [:shu] from comment #90) > Thanks for the reviews. > > > Who is the "module loader"? The embedding? > > Yes, the module loader is a chunk of code supplied by the embedding that > does the platform specific parts, e.g. actually getting the module source > from somewhere. > > > Why is it a slot on the global? Is it exposed to script? > > It's not exposed to script. I just put it there because it seemed > convenient. Is there a better place to put this? > Hm, if it's a hook that cannot differ from compartment to compartment, maybe just on the JSRuntime? > > The environment returned by module.environment contains ALL bindings, right? > > Both exported and top-level vars, lets, and consts. Is that intended? The > > environment isn't accessible to script, right? > > Yes, all bindings. At the moment this is exposed for testing purposes. > What I need to do is to work out what needs to be made available to the > module loader and what needs to be avilable only for testing and how to > expose these. None of them actually need to be getters on the module object > itself. > > Is it OK to land this as is and I'll address this in a subsequent patch? > Yes, perfectly fine.

Comment on attachment 8658774 [details] [diff] [review] modules-15-module-namespaces Review of attachment 8658774 [details] [diff] [review]: ----------------------------------------------------------------- This seems pretty straightforward. Just to make sure I understand, the idea is that a Namespace is the thing that holds all unambiguous exported names from a module. It is the value of 'foo' in 'import * as foo from bar'. A more spec question: I'm not clear on why Namespaces don't error on ambiguous names, unlike importing a specific binding. Is it because it's trying to keep in line with the esprit de JS and only erroring when absolutely necessary? ::: js/src/builtin/Module.js @@ +153,5 @@ > + if (resolution === null) > + ThrowSyntaxError(JSMSG_MISSING_NAMESPACE_EXPORT); > + if (resolution !== "ambiguous") > + unambiguousNames.push(name); > + } Nit: wrong indentation @@ +154,5 @@ > + ThrowSyntaxError(JSMSG_MISSING_NAMESPACE_EXPORT); > + if (resolution !== "ambiguous") > + unambiguousNames.push(name); > + } > + namespace = ModuleNamespaceCreate(module, unambiguousNames.sort()); Why is unambiguousNames sorted? @@ +164,5 @@ > + > +// 9.4.6.13 ModuleNamespaceCreate(module, exports) > +function ModuleNamespaceCreate(module, exports) > +{ > + exports.sort(); I still don't understand why exports is being sorted, but in any case, it's being sorted twice. @@ +173,5 @@ > + // access. > + for (let i = 0; i < exports.length; i++) { > + let name = exports[i]; > + let binding = module.resolveExport(name); > + assert(binding !== null && binding !== "ambiguous", "Failed to resolve binding"); Hmm, does this assert get compiled out when !DEBUG? ::: js/src/builtin/ModuleObject.cpp @@ +239,5 @@ > + > + // Create a delegate object to hold symbol properties. > + RootedObject delegate(cx, JS_NewPlainObject(cx)); > + if (!delegate) > + return nullptr; I don't know much about our Proxy API. There's no easier way to hold these special symbol properties than an otherwise useless delegate object? @@ +312,5 @@ > + RootedFunction fun(cx, &args.callee().as<JSFunction>()); > + RootedModuleNamespace ns(cx, &fun->getExtendedSlot(0).toObject().as<ModuleNamespace>()); > + > + // TODO: This is supposed to be a list iterator, not an array iterator > + // object. I'm not sure this is observable though. I don't know what the difference is between a list and an array iterator. @@ +384,5 @@ > + Rooted<ModuleNamespace*> ns(cx, &proxy->as<ModuleNamespace>()); > + if (JSID_IS_SYMBOL(id)) { > + RootedObject delegate(cx, &ns->propertiesDelegate()); > + return GetOwnPropertyDescriptor(cx, delegate, id, desc); > + } What is this if block doing, checking for @@iterator?

(In reply to Shu-yu Guo [:shu] from comment #92) > Hm, if it's a hook that cannot differ from compartment to compartment, maybe > just on the JSRuntime? It is per-compartment - each compartment gets a separate module loader (even if they share the same code, the repository of loaded modules is separate).

https://hg.mozilla.org/integration/mozilla-inbound/rev/4219be7a3b21 https://hg.mozilla.org/integration/mozilla-inbound/rev/3eef8a35884c https://hg.mozilla.org/integration/mozilla-inbound/rev/e110a99894b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/db4c17553be9

(In reply to Shu-yu Guo [:shu] from comment #93) > Why is unambiguousNames sorted? The spec says in section 9.4.6 table 29 that the [[Exports]] internal slot of a module namespace object is sorted (although FWIW it doesn't actually say to sort it at any point I could find). Sorting it twice is definitely unneccesary though :) > does this assert get compiled out when !DEBUG? Yes, it turns out we pre-process our self hosted code, and assert is a DEBUG-only #define. > There's no easier way to hold these special symbol properties than an otherwise useless delegate object? There are two 'extra' slots on proxies that can be used. Originally I used these for the exports and the bindings, but I moved these to ModuleObject so I can just use these for the symbol-keyed properties now and get rid of the delegate object althogether. > What is this if block doing, checking for @@iterator? Yes, and @@toStringTag when it's implemented. > I don't know what the difference is between a list and an array iterator. Looking at the spec it seems that the differences are that: 1) They have different prototypes (IteratorPrototype vs. ArrayIteratorPrototype) 2) ListIterator's next method throws if applied to any other kind of object So this is observable after all. I'll look into fixing it in a separate patch. Also I'm going to rename the class to ModuleNamespaceObject as it should be.

https://hg.mozilla.org/mozilla-central/rev/4219be7a3b21 https://hg.mozilla.org/mozilla-central/rev/3eef8a35884c https://hg.mozilla.org/mozilla-central/rev/e110a99894b4 https://hg.mozilla.org/mozilla-central/rev/db4c17553be9

Patch updated as per previous comments.

Add self-hosted iterator for module namespaces. I copied the guts from the ArrayIterator implementation.

The value of |this| in modules is undefined. Make this the case and add tests.

Make modules work with the debugger API. Add tests and fixes issues that came up.

Comment on attachment 8665537 [details] [diff] [review] modules-15.5-namespace-iterator Review of attachment 8665537 [details] [diff] [review]: ----------------------------------------------------------------- What do you think about implementing a generic ListIterator instead of special casing ModuleNamespaceIterator here? ::: js/src/builtin/Module.js @@ +278,5 @@ > + var iterator = NewNamespaceIterator(); > + UnsafeSetReservedSlot(iterator, ITERATOR_SLOT_TARGET, ModuleNamespaceExports(ns)); > + UnsafeSetReservedSlot(iterator, ITERATOR_SLOT_NEXT_INDEX, 0); > + iterator.next = NamespaceIteratorNext; > + iterator[std_iterator] = NamespaceIteratorIdentity; Is this an implementation of the Iterator [[Prototype]]'s @@iterator property? Why is this on the iterator object itself instead of on the proto chain? Looks like our other iterators are similarly incorrect. @@ +284,5 @@ > +} > + > +function NamespaceIteratorIdentity() { > + if (!IsObject(this) || !IsNamespaceIterator(this)) > + return callFunction(CallNamespaceIteratorMethodIfWrapped, this, "NamespaceIteratorIdentity"); As a note, this ability to see through CCWs is something the other iterators do not have. I suppose all iterators should have this. @@ +291,5 @@ > +} > + > +function NamespaceIteratorNext() { > + if (!IsObject(this) || !IsNamespaceIterator(this)) > + return callFunction(CallNamespaceIteratorMethodIfWrapped, this, "NamespaceIteratorNext"); For spec compliance, 7.4.8.1 step 5 needs to be implemented. @@ +297,5 @@ > + var a = UnsafeGetObjectFromReservedSlot(this, ITERATOR_SLOT_TARGET); > + var index = UnsafeGetInt32FromReservedSlot(this, ITERATOR_SLOT_NEXT_INDEX); > + var result = { value: undefined, done: false }; > + > + // FIXME: This should be ToLength, which clamps at 2**53. Bug 924058. I don't understand this comment. The line below does use ToLength. ::: js/src/vm/CommonPropertyNames.h @@ +276,5 @@ > macro(Symbol_species, Symbol_species, "Symbol.species") \ > macro(Symbol_toPrimitive, Symbol_toPrimitive, "Symbol.toPrimitive") \ > macro(Symbol_toStringTag, Symbol_toStringTag, "Symbol.toStringTag") \ > macro(Symbol_unscopables, Symbol_unscopables, "Symbol.unscopables") \ > + /* Functions names for properites named by symbols. */ \ Typo: properties "Functions names"?

Comment on attachment 8665539 [details] [diff] [review] modules-16-this Review of attachment 8665539 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Interpreter.cpp @@ +1019,5 @@ > + JSObject* thisObj = GetThisObject(cx, scopeChain); > + if (!thisObj) > + return false; > + thisv = ObjectValue(*thisObj); > + } scopeChain in this case is the ModuleEnvironmentObject or whatever it's called, right? You could give it a 'this' hook like the With scope and avoid special-casing EXECUTE_MODULE here.

Comment on attachment 8665541 [details] [diff] [review] modules-17-debugger-support Review of attachment 8665541 [details] [diff] [review]: ----------------------------------------------------------------- Please update docs/Debugger/Debugger.Frame.md to mention the 'module' type on Debugger.Frame instances. r=me with that. ::: js/src/jit-test/tests/modules/debugger-frames.js @@ +47,5 @@ > + assertThrowsInstanceOf(() => env.setVariable('z', 9), TypeError); > + assertArrayEq(['a', 'b', 'c'].map(env.getVariable, env), [1, 2, 3]); > + assertArrayEq(['x', 'y', 'z'].map(env.getVariable, env), [7, 8, 6]); > + > + // The global is the only thing after the module on the scope chain. Please put a FIXME here about bug 589199 ::: js/src/vm/ScopeObject.cpp @@ +1480,5 @@ > *accessResult = ACCESS_GENERIC; > LiveScopeVal* maybeLiveScope = DebugScopes::hasLiveScope(*scope); > > + if (scope->is<ModuleEnvironmentObject>()) { > + /* Everything is aliased and stored in the environement object. */ Typo: environment @@ +1915,5 @@ > /* > * Function scopes are optimized to not contain unaliased variables so > * they must be manually appended here. > */ > + if (isFunctionScope(*scope)) { Thanks for this cleanup.

(In reply to Shu-yu Guo [:shu] from comment #102) > What do you think about implementing a generic ListIterator instead of > special casing ModuleNamespaceIterator here? Good plan, done. > Is this an implementation of the Iterator [[Prototype]]'s @@iterator > property? Why is this on the iterator object itself instead of on the proto > chain? Looks like our other iterators are similarly incorrect. Yes it's wrong but I was copying what we did elsewhere. It's complicated by the fact we have a non-standard |Iterator| class which conflicts with what the spec expects of IteratorPrototype. I'll file a separate bug for this. Other issues addressed.

(In reply to Shu-yu Guo [:shu] from comment #102) > Looks like our other iterators are similarly incorrect. Bug 1201089 has appeared for this issue.

Comment on attachment 8667977 [details] [diff] [review] modules-15.5-namespace-iterator v2 Review of attachment 8667977 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good. r=me with the question below clarified. ::: js/src/builtin/Iterator.js @@ +88,5 @@ > +function CreateListIterator(array) { > + let iterator = NewListIterator(); > + UnsafeSetReservedSlot(iterator, ITERATOR_SLOT_TARGET, array); > + UnsafeSetReservedSlot(iterator, ITERATOR_SLOT_NEXT_INDEX, 0); > + iterator.next = ListIteratorNext; This is kind of subtle... this ListIteratorNext lookup ends up being compiled a GETINTRINSIC, right? Which means it clones ListIteratorNext, which means each new ListIterator actually gets distinct copies of ListIteratorNext, which ensures 7.4.8.1 step 5 is satisfied with the check below. Is that right? @@ +89,5 @@ > + let iterator = NewListIterator(); > + UnsafeSetReservedSlot(iterator, ITERATOR_SLOT_TARGET, array); > + UnsafeSetReservedSlot(iterator, ITERATOR_SLOT_NEXT_INDEX, 0); > + iterator.next = ListIteratorNext; > + iterator[std_iterator] = IteratorIdentity; Do you mean ListIteratorIdentity?

(In reply to Shu-yu Guo [:shu] from comment #107) > This is kind of subtle... this ListIteratorNext lookup ends up being > compiled a GETINTRINSIC, right? Which means it clones ListIteratorNext, > which means each new ListIterator actually gets distinct copies of > ListIteratorNext, which ensures 7.4.8.1 step 5 is satisfied with the check > below. Is that right? Ok, so this was more by accident than by design. But yes, when we get an intrinsic value we clone it and this can be used to satisfy the requirement that each iterator gets a new next function. I need to store the function in the iterator object itself though to make 7.4.8.1 step 5 work. And it certainly needs a comment. I'll fix this and flag for re-review.

Now we have the global lexical scope we need to fix a couple of places where the code assumed that the global was the only thing on the scope chain after a module. Also, this patch changes the way the scope chain works for modules. Previously a module's script's enclosing static scope was null, and the script was executed with only the global on its dynamic scope chain. This changes the situation so that the static scope of a module script is the module object. Then when we execute a script we have a method to get the dynamic scope to use which is called if no non-syntactic scopes are supplied. This returns the global lexical for global scripts or the module environment for module scripts. This means we don't need to have a special case to push the extra object on the dynamic scope chain later on.

Updated to fix issues with ListIterator next() method. It turns out we don't clone the function object every time (which is good) so I made this into a function expression in CreateListIterator to get the correct behaviour. I had to add an intrinsic to get the current active function because I couldn't see another way to do this.

This patch is for you suggestion of adding a 'this' hook to the module environment object, which turned out to be a lot lager than expected. The previous hook returned an object, which had to be changed to a one that returned a value so we could return undefined. This then had lots of knock-on changes. Anyway, I think on balance I prefer the value version and it seems to be less code in most places the hook is called. But feel free to r- if you think this is too large a change.

For function declarations at the top level of a module, we're meant to instantiate them before we execute the module. I believe this is so that we can have cyclic dependencies between modules where each imports the other's exported functions and can call them during module evaluation. The relevant part of the spec is 15.2.1.16.4.16.iv.

Comment on attachment 8672683 [details] [diff] [review] modules-14.5-scope-chain-update Review of attachment 8672683 [details] [diff] [review]: ----------------------------------------------------------------- The one comment I have below is asking for a different API so I'd like to review it again. Everything else looks good. ::: js/src/jsscript.h @@ +1651,5 @@ > // global lexical scope to the main thread compartment's. > void fixEnclosingStaticGlobalLexicalScope(); > > + JSObject* maybeSyntacticDynamicScope(JSContext* cx) const; > + bool getSyntacticDynamicScope(JSContext* cx, JS::MutableHandleObject scopeOut) const; I think these methods wouldn't be needed if we reorganize the public JSAPI a bit. Pre-modules, the way I mentally organize "execution units" in the engine is divided among global scripts and functions. - If executing a function, pass a JSFunction, which will pull out the correct environment and script. - If executing a global script, pass a JSScript directly. With modules being more analogous to functions, I would prefer a set of ExecuteModule JSAPI functions that take ModuleObjects, instead of overloading ExecuteScript to take either global or module scripts.

Comment on attachment 8672685 [details] [diff] [review] modules-15.5-namespace-iterator v3 Review of attachment 8672685 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Iterator.js @@ +96,5 @@ > + let next = function() { > + if (!IsObject(this) || !IsListIterator(this)) > + return callFunction(CallListIteratorMethodIfWrapped, this, "ListIteratorNext"); > + > + if (ActiveFunction() !== UnsafeGetReservedSlot(this, ITERATOR_SLOT_NEXT_METHOD)) Is ActiveFunction() needed? Could you use |arguments.callee|? It even has Ion inlines.

Comment on attachment 8672689 [details] [diff] [review] modules-16.5-this-value Review of attachment 8672689 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this! I agree, I like the Value version better. ::: js/src/vm/ScopeObject.cpp @@ +710,2 @@ > { > + vp.setObject(obj->as<DynamicWithObject>().withThis()); Could you change withThis to return the Value directly, while asserting that the value is an Object?

Comment on attachment 8674171 [details] [diff] [review] modules-18-cyclic-function-imports Review of attachment 8674171 [details] [diff] [review]: ----------------------------------------------------------------- I don't really understand this. Can we chat about it?

There's only one place we execute a module script so I can take out these changes and just call js::Execute() instead.

(In reply to Shu-yu Guo [:shu] from comment #114) > Is ActiveFunction() needed? Could you use |arguments.callee|? This doesn't work because self-hosted code is strict and arguments.callee is not supported.

> Could you change withThis to return the Value directly Good idea, done.

Comment on attachment 8674806 [details] [diff] [review] modules-16.5-this-value v2 Requesting review for the non-JS parts.

Comment on attachment 8674806 [details] [diff] [review] modules-16.5-this-value v2 >+bool >+ObjectToOuterObjectValue(JSContext* cx, JS::HandleObject obj, JS::MutableHandleValue vp) JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>

(In reply to Olli Pettay [:smaug] from comment #121) Indeed, fixed. > I assume JS_ObjectToOuterObject can't return null The comments on js::GetOuterObject() say no.

Comment on attachment 8674171 [details] [diff] [review] modules-18-cyclic-function-imports Review of attachment 8674171 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/ModuleObject.cpp @@ +740,5 @@ > +bool > +ModuleObject::noteFunctionDeclaration(ExclusiveContext* cx, HandleAtom name, HandleFunction fun) > +{ > + FunctionDeclarationVector* funDecls = functionDeclarations(); > + return funDecls->emplaceBack(name, fun); Shouldn't this be append? I don't see any resizing of the vector anywhere. Your test only also contains 1 function. ::: js/src/frontend/BytecodeEmitter.cpp @@ +5876,5 @@ > /* > * For a script we emit the code as we parse. Thus the bytecode for > * top-level functions should go in the prologue to predefine their > * names in the variable object before the already-generated main code > + * is executed. This comment block about emitting as we parse is now inaccurate. While you're here, could you update this? @@ +5917,5 @@ > + } else { > + RootedModuleObject module(cx, sc->asModuleBox()->module()); > + RootedAtom name(cx, fun->atom()); > + if (!module->noteFunctionDeclaration(cx, name, fun)) > + return false; I feel like this way of doing it is unfortunate, but I don't have an alternative to suggest. I wonder if we can unify these somehow when someone implements block scoping for functions.

(In reply to Shu-yu Guo [:shu] from comment #123) > > +bool > > +ModuleObject::noteFunctionDeclaration(ExclusiveContext* cx, HandleAtom name, HandleFunction fun) > > +{ > > + FunctionDeclarationVector* funDecls = functionDeclarations(); > > + return funDecls->emplaceBack(name, fun); > > Shouldn't this be append? I don't see any resizing of the vector anywhere. emplaceBack() grows the vector so this is fine unless I've missed something? > Your test only also contains 1 function. Good point, I'll add more.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbeed0536844 https://hg.mozilla.org/integration/mozilla-inbound/rev/de036df63e4d https://hg.mozilla.org/integration/mozilla-inbound/rev/75365568c45b https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fc7f42fddb https://hg.mozilla.org/integration/mozilla-inbound/rev/a8986b136f8d https://hg.mozilla.org/integration/mozilla-inbound/rev/935cdbf4fcf5

https://hg.mozilla.org/mozilla-central/rev/cbeed0536844 https://hg.mozilla.org/mozilla-central/rev/de036df63e4d https://hg.mozilla.org/mozilla-central/rev/75365568c45b https://hg.mozilla.org/mozilla-central/rev/c7fc7f42fddb https://hg.mozilla.org/mozilla-central/rev/a8986b136f8d https://hg.mozilla.org/mozilla-central/rev/935cdbf4fcf5