The initial design of Harmony simple modules is stabilizing; we need to implement, experiment, and dogfood. Current draft spec at the attached URL. All three relevant documents: http://wiki.ecmascript.org/doku.php?id=strawman:simple_modules http://wiki.ecmascript.org/doku.php?id=strawman:simple_modules_examples http://wiki.ecmascript.org/doku.php?id=strawman:module_loaders Dave

This has been promoted to official Harmony status now: http://wiki.ecmascript.org/doku.php?id=harmony:modules http://wiki.ecmascript.org/doku.php?id=harmony:module_loaders http://wiki.ecmascript.org/doku.php?id=harmony:modules_examples Dave

Jason, Brendan, Andreas: can you guys look over the following game plan and see if it looks reasonable? If so I'll file individual bugs and make this a tracking bug for all of them. These are listed in topologically sorted dependency order. I.e., some of them could land in parallel, but some later ones depend on some earlier ones. 1) add web content support for new provisional mime type for the Harmony mode-in-progress (maybe "application/x-harmony") 2) add parser support for module, import, and export declarations in Harmony mode, *without* support for loading external modules 3) add a static variable resolution pass to Harmony mode 4) implement module execution (runtime data structures, new bytecodes, interpreter cases, probably no JITting at first) 5) module loader core infrastructure *without* user hooks (runtime data structures, fixed pointer from JSScript to its loader, dynamic evaluation through loaders) 6) add parser support for static external module loading (e.g., module m from "foo.js") 7) implement static external module loading (including dynamic restrictions that prevent concurrent modifications of loader while it's compiling) 8) implement full set of loader API convenience methods (I have a lot of headway on all of this in Narcissus, so in the individual bugs I can link to source files that might be helpful guides for doing the real implementation.) Dave

Update: Jason has started working on this, primarily steps 2 - 5. (Step 1 isn't really necessary for now; we can just test in the shell.) Dave

Sorry it took a while to update this. Here's a basic implementation plan: 1) parser support for module, import, and export declarations in Harmony mode - syntax at attached URL - choose one of the variants for module-loading syntax 2) module linking - Sam Tobin-Hochstadt, Andreas Rossberg, and I are still working through the details of the algorithm -- should have this ready for you soon (I'll report back) 3) module execution (runtime data structures, new bytecodes, interpreter cases) 4) basic loader support (sandboxing, fresh globals, synchronous eval) 5) external loading 6) user-defineable loader hooks for resolving, fetching, compiling Dave

(Looks like when FF restored my open tab's state from a previous session, new components had been added so the saved index moved from SpiderMonkey to IPC. Fixing.) Dave

Over the past several weeks, I've been working on a prototype that implements the ES6 module spec sans loading (that is: module blocks and export, but no import yet). It's not yet complete and still needs extensive testing, but the first few patches should start rolling in during the upcoming weeks.

As a first step, this patch adds Module objects. Each module object contains the following fields: - An atom containing its name - A script used to initialise its exports - A scope object in which the script is run (*) - A set of exported names (*) (*) = Will be added in a later patch Note that the accessor functions will eventually have to be updated to use Return<Foo *>. This can be done by using Handle::fromMarkedLocation, which takes a pointer to the private value. Taking the address of a temporary is not allowed however, so we need some workaround for this. As soon as this patch gets r+'d I will file a followup bug for this.

I don't think you want Handle::fromMarkedLocation. But you can cast from T* directly to Return<T*>. So I don't think you would need it anyway.

Comment on attachment 687721 [details] [diff] [review] Module objects Review of attachment 687721 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Module.cpp @@ +32,5 @@ > + return module; > +} > + > +Module & > +JSObject::asModule() You claim this is inline in jsobj.h, so it should not be here. ::: js/src/builtin/Module.h @@ +11,5 @@ > + return (JSAtom *) GetReservedSlot(this, ATOM_SLOT).toPrivate(); > + }; > + > + void setAtom(JSAtom *atom) { > + SetReservedSlot(this, ATOM_SLOT, PrivateValue(atom)); Use (get/set)ReservedSlot rather than these friend apis. Also, rather than PrivateValue(), just store it as a string StringValue(). @@ +19,5 @@ > + return (JSScript *) GetReservedSlot(this, SCRIPT_SLOT).toPrivate(); > + } > + > + void setScript(JSScript *script) { > + SetReservedSlot(this, SCRIPT_SLOT, PrivateValue(script)); Similarily here, use ObjectValue(script). Incidently, why does Module need to hang onto its script? @@ +33,5 @@ > +JSObject * > +js_InitModuleClass(JSContext *cx, js::HandleObject obj); > + > +js::Module * > +js_NewModule(JSContext *cx, JSAtom *atom); Probably should be Module::Create. ::: js/src/gc/Root.h @@ +179,5 @@ > void operator =(S v) MOZ_DELETE; > }; > > typedef Handle<JSObject*> HandleObject; > +typedef Handle<js::Module*> HandleModule; namespace not needed. @@ +706,5 @@ > return ptr_ == other.get(); > } > > typedef Rooted<JSObject*> RootedObject; > +typedef Rooted<js::Module*> RootedModule; ditto

The previous patch was bitrotted, so here's a new attempt, with some of benjamins comments addressed. This patch implements Module objects. Each module has an atom to store its name, and an initialization script. Modules need to hold on to their initialization script because modules may be referred to before they are initialized, and may be initialized multiple times.

We need to be able to detect whether we are currently parsing a global script, a module, or a function. This is usually done by checking whether the current shared context is a global context or a function box. To also support modules, we need to extend this by with module boxes. Incidentally, this also allows us to give module their own set of context variables, which is useful. This patch lays the groundwork for this change by removing the isFunction_ boolean from SharedContext. Instead, for module and function boxes we now use the boxed object's type to determine what kind of context we are dealing with. Since module and function boxes inherit from both SharedContext and ObjectBox, making this work involves some trickery with a virtual function to cast instances of the former to the latter. This is messy, but we agreed over irc that until we come up with a better solution, this is acceptable.

This patch adds module boxes. See previous comment for details.

For the next few patches I'm less confident that they are ready to land as is, so I'm requesting feedback rather than review. The overall approach is sound though, imho. This patch implements partial parser support for modules, particularly module blocks and the export keyword. It only recognizes modules after passing a version check, and only allows the export keyword when already inside a module block. My understanding is that with this version check it should be safe to land this patch without making it easy to crash the browser. Is that correct? I would like to get this landed asap to prevent further bitrotting.

Ugh. This patch was already bitrotted again by the time is finished rebasing it! ): Anyway, the tricky part in this patch is that each script has the notion of an owner object. Up until this point, this object was always a function. A script with no associated owner is assumed to be a global script. The problem is that now the owner can also be a module. To fix this, I used a similar trick as I did for SharedContext. The type of the owner is used to determine whether a script is a function script or a module script. Function scripts have an associated function and module scripts an associated module. Global scripts have neither. Problematic is that many places in the code use script->function() to determine whether a script is a function script. These calls need to be changed to script->isFunctionScript(). Even more problematic is that many function expect the owner object to be passed as an argument, and this argument is a function object. Since the function argument is supposed to be NULL if the script is a global script, I've decided to do the same if the script is actually module script, at least until I come up with a better solution here. Suggestions are welcome!

This patch adds partial module support to the interpreter. I should probably point out why we have three different bytecodes: The JSOP_MODULE bytecode is supposed to be used when a property bar is accessed on a module foo. In that case, the module behaves like a normal object, and needs to be pushed on the stack as such. The JSOP_DEFMODULE bytecode is supposed to be used when a module is defined at the body-level of another function. Like functions, modules need to be hoisted to the top of the module body. This is similar to how JSOP_DEFFUNCTION works. The JSOP_INITMODULE bytecode is used to call the initialization script for the module. Contrary to the actual module definition, this step is *not* hoisted (a module can be referred to before it is defined, but its exported variables are not initialized until it is initialized). Note that modules also get their own frame type. This in preparation for the next patch, which gives module blocks their own scope. The patch after that will forward all property accesses on a module object to this scope object, and the patch after that will add a whitelist to this in order to support the export keyword.

(In reply to Eddy Bruel [:ejpbruel] from comment #10) > This patch implements Module objects. Each module has an atom to store its > name, and an initialization script. It might be better to store module names as a list/array of atoms; since the modules proposal is moving to having structure in module names. > Modules need to hold on to their > initialization script because modules may be referred to before they are > initialized, and may be initialized multiple times. Are these module objects the "module instance objects" described on the ES wiki, or objects that hold the source of a module? If the former, I don't see how they can be initialized multiple times.

(In reply to Sam Tobin-Hochstadt [:samth] from comment #16) > (In reply to Eddy Bruel [:ejpbruel] from comment #10) > > > This patch implements Module objects. Each module has an atom to store its > > name, and an initialization script. > > It might be better to store module names as a list/array of atoms; since the > modules proposal is moving to having structure in module names. > The problem with that is that the emitter needs the module's name in order to emit JSOP_DEFMODULE. > > Modules need to hold on to their > > initialization script because modules may be referred to before they are > > initialized, and may be initialized multiple times. > > Are these module objects the "module instance objects" described on the ES > wiki, or objects that hold the source of a module? If the former, I don't > see how they can be initialized multiple times. I have to admit, I did not think this part through very well yet. I think you're right that the multiple initialization argument doesn't make sense. Even so, modules still need to keep a reference to their initialization script because JSOP_DEFMODULE is hoisted but JSOP_INITMODULE is not, so that definition and initialization are decoupled from each other.

Comment on attachment 702250 [details] [diff] [review] Module objects Review of attachment 702250 [details] [diff] [review]: ----------------------------------------------------------------- Sorry -- r=me on part 1 with these comments addressed. ::: js/src/builtin/Module.cpp @@ +19,5 @@ > +JSObject * > +js_InitModuleClass(JSContext *cx, HandleObject obj) > +{ > + Rooted<GlobalObject *> global(cx, &obj->asGlobal()); > + return global->createBlankPrototype(cx, &ModuleClass); This isn't enough; the new prototype has to be stored in the global object somewhere, since we will want to use it (the same proto object) over and over. global->setReservedSlot(JSProto_Module, ObjectValue(*proto)); In fact, I'm pretty sure the return value of this function (and others like it) is treated as a success/failure boolean everywhere. Please do change it to bool in a separate patch. Another thing to double-check is all the places where JS_FOR_EACH_PROTOTYPE is used; if you don't want all of those things to happen for Modules, you'll need to instead be really specific about what you want. There are a few places where we do that. For example, GlobalObject::{ELEMENT_ITERATOR_PROTO,GENERATOR_PROTO} are dedicated slots in the global object, but that's the only special thing we want for those particular prototype objects. We do not want a global js_ElementIterator_str string, a corresponding JSAtom in every JSRuntime, etc. etc. ::: js/src/builtin/Module.h @@ +24,5 @@ > + } > + > + private: > + static const uint32_t ATOM_SLOT = 0; > + static const uint32_t SCRIPT_SLOT = 1; I think the usual thing would be to declare NUM_SLOTS = 2; here and use that in the definition of js::ModuleClass instead of a bare `2`.

Comment on attachment 702254 [details] [diff] [review] Shared contexts Review of attachment 702254 [details] [diff] [review]: ----------------------------------------------------------------- Looks great.

Comment on attachment 702250 [details] [diff] [review] Module objects Review of attachment 702250 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Module.cpp @@ +25,5 @@ > + > +Module * > +js_NewModule(JSContext *cx, JSAtom *atom) > +{ > + RootedModule module(cx, &NewBuiltinClassInstance(cx, &ModuleClass)->asModule()); NewBuiltinClassInstance could fail.

Comment on attachment 702255 [details] [diff] [review] Module boxes Review of attachment 702255 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments, but I've basically asked for a rewrite of the code mentioning inWith which makes up the bulk of this patch. ::: js/src/frontend/BytecodeEmitter.cpp @@ +1674,5 @@ > return true; > > + if (sc->isModuleBox()) { > + if (sc->asModuleBox()->inWith) > + return true; Modules can't occur inside with-statements. The amount of code dedicated to figuring out if we're in a with-block here seems silly to me-- even before this patch, but the patch makes it worse. Please eliminate all the downcasting and wandering around here using whatever technique seems best. One possibility is to make a base-class bool field or method SharedContext::inWith. If it's a field, it could be automatically populated from the parent in the constructor, I should think. ::: js/src/frontend/Parser.cpp @@ +413,5 @@ > } > + } else if (outerpc->sc->isModuleBox()) { > + ModuleBox *parent = outerpc->sc->asModuleBox(); > + if (parent && parent->inWith) > + inWith = true; inWith = false in this particular case; but again, the amount of code to compute inWith here is just nuts. @@ +466,5 @@ > + while (scope) { > + if (scope->isWith()) { > + inWith = true; > + break; > + } This needs to be a SyntaxError. Consider doing this stuff in a method (which can report an error and return false) rather than a constructor. But more generally: ARGH, more code to compute inWith! Base-class flag (or method) looking better all the time. @@ +471,5 @@ > + scope = scope->enclosingScope(); > + } > + } else if (pc->sc->isModuleBox()) { > + ModuleBox *parent = pc->sc->asModuleBox(); > + if (parent && parent->inWith) If we get here, parent is definitely non-null, right? So drop the "parent &&" part of this if-condition. @@ +475,5 @@ > + if (parent && parent->inWith) > + inWith = true; > + } else { > + FunctionBox *parent = pc->sc->asFunctionBox(); > + if (parent && parent->inWith) Same here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bee0517d440 Messed up the bug number in this one. Sorry!

This segment is still not correct: RootedModule module(cx, &NewBuiltinClassInstance(cx, &ModuleClass)->asModule()); if (module == NULL) return NULL; asModule() derefences the pointer from NewBuiltinClassInstance. This means the compiler would be justified in removing the NULL check!

(In reply to :Benjamin Peterson from comment #25) > This segment is still not correct: > > RootedModule module(cx, &NewBuiltinClassInstance(cx, > &ModuleClass)->asModule()); > if (module == NULL) > return NULL; > > asModule() derefences the pointer from NewBuiltinClassInstance. This means > the compiler would be justified in removing the NULL check! Ugh, how sloppy. Well spotted!

This patch should fix the issue.

(In reply to :Benjamin Peterson from comment #9) > ::: js/src/builtin/Module.cpp > @@ +32,5 @@ > > + return module; > > +} > > + > > +Module & > > +JSObject::asModule() > > You claim this is inline in jsobj.h, so it should not be here. This review-comment was left un-fixed in the patch that landed, and it's causing a lot of build-warning-spam -- every .cpp file that #includes jsobj.h (directly or indirectly) prints a chunk of warning-spam like: { 0:43.48 In file included from /scratch/work/builds/mozilla-inbound/mozilla/js/src/jsscope.h:14:0, 0:43.48 from /scratch/work/builds/mozilla-inbound/mozilla/js/src/jsscript.h:16, 0:43.48 from /scratch/work/builds/mozilla-inbound/mozilla/js/src/vm/SPSProfiler.h:18, 0:43.48 from /scratch/work/builds/mozilla-inbound/mozilla/js/src/jscntxt.h:36, 0:43.48 from /scratch/work/builds/mozilla-inbound/mozilla/js/src/jspropertycache.cpp:9: 0:43.48 Warning: enabled by default in /scratch/work/builds/mozilla-inbound/mozilla/js/src/jsobj.h: inline function 'js::Module& JSObject::asModule()' used but never defined 0:43.48 /scratch/work/builds/mozilla-inbound/mozilla/js/src/jsobj.h:1037:24: warning: inline function 'js::Module& JSObject::asModule()' used but never defined [enabled by default] }

Oh, sorry -- I guess it was addressed (looks like the function definition moved to Module.h instead of Module.cpp) -- but there are apparently many .cpp files that #include jsobj.h but don't #include Module.h, so they warn about the missing function-definition. Perhaps that function-definition belongs in jsobjinlines.h?

Comment on attachment 703910 [details] [diff] [review] Fix for Module objects Review of attachment 703910 [details] [diff] [review]: ----------------------------------------------------------------- That should do the trick. ::: js/src/builtin/Module.cpp @@ +18,5 @@ > Module * > js_NewModule(JSContext *cx, JSAtom *atom) > { > + RootedObject object(cx, NewBuiltinClassInstance(cx, &ModuleClass)); > + if (object == NULL) SM style would be just |if (!object)|.

(In reply to Daniel Holbert [:dholbert] from comment #29) > Oh, sorry -- I guess it was addressed (looks like the function definition > moved to Module.h instead of Module.cpp) -- but there are apparently many > .cpp files that #include jsobj.h but don't #include Module.h, so they warn > about the missing function-definition. > > Perhaps that function-definition belongs in jsobjinlines.h? That's strange. I never saw any warning of this. Otherwise, it would have caught my attention. It's getting late here at this side of the ocean. Can I look into this on monday?

(In reply to Eddy Bruel [:ejpbruel] from comment #31) > That's strange. I never saw any warning of this. Otherwise, it would have > caught my attention. I'm seeing it w/ GCC 4.7 on Ubuntu Linux 12.10, FWIW. > It's getting late here at this side of the ocean. Can I look into this on > monday? Sure -- it's not causing problems, just noise. :) So, doesn't demand immediate attention. I'll file a followup so as not to cause confusion in this bug, too.

(I filed bug 832453 for that asModule build warning)

https://hg.mozilla.org/mozilla-central/rev/0141f43d8c5b https://hg.mozilla.org/mozilla-central/rev/4bee0517d440 https://hg.mozilla.org/mozilla-central/rev/a9676f4f869b

Oh, and please add MPL headers in the new files.

Here's a patch that should fix that build warning

I want to reuse the Bindings class so that it can generate bindings for Modules as well. This patch refactors initWithTemporaryStorage so that it can create shape lineages for more than one class. The idea is to specify either CallClass (for function scopes) or ModuleScopeClass (for module scopes), based on the type of the current shared context when generateBindings is called.

This patch adds the actual module scope objects. Note that modules need access to their underlying scope because when a module foo { ... } is accessed like foo.x, it needs to forward that property access to its scope object.

Comment on attachment 702262 [details] [diff] [review] Parser support Review of attachment 702262 [details] [diff] [review]: ----------------------------------------------------------------- I didn't read closely. It seems like it *must* be pretty sloppy. But going in the right direction. ::: js/src/frontend/Parser.cpp @@ +1850,5 @@ > + return NULL; > + if (Definition *dn = pc->lexdeps.lookupDefn(name)) { > + JS_ASSERT(dn->isDefn()); > + dn->setKind(PNK_MODULE); > + dn->setArity(PN_NAME); What if the previous definition is a function, var, or let? (For that matter, what if it's a module? Is this the desired semantics?) @@ +1855,5 @@ > + dn->pn_pos.begin = pn->pn_pos.begin; > + dn->pn_pos.end = pn->pn_pos.end; > + > + dn->pn_body = NULL; > + dn->pn_cookie.makeFree(); That seems wrong to me. @@ +1882,5 @@ > + MUST_MATCH_TOKEN(TOK_LC, JSMSG_CURLY_BEFORE_MODULE); > + modulebox->bufStart = tokenStream.offsetOfToken(tokenStream.currentToken()); > + pn->pn_body = statements(); > + MUST_MATCH_TOKEN(TOK_RC, JSMSG_CURLY_AFTER_MODULE); > + pn->pn_modulebox->bufEnd = tokenStream.offsetOfToken(tokenStream.currentToken()) + 1; Sorry I missed this bufStart/bufEnd stuff before. We need that for functions in order to implement function .toString(), right? I bet we don't need it for modules. @@ +3777,5 @@ > + pn->pn_kid = statement(); > + PopStatementPC(context, pc); > + if (!pn->pn_kid) > + return NULL; > + return pn; This is too permissive, right? It seems to allow code like module X { export if (x > 3) f(x); } I don't see where pn->pos.end is being set. This would have to be after the call to statement(). Regarding UnaryNode::create, this is how most existing code is written, but long-term what we want to do is: TokenPos pos = tokenStream.currentToken().pos; ... ParseNode *kid = statement(); ... if (!kid) return NULL; pos.end = tokenStream.currentToken().pos.end; return new_<UnaryNode>(PNK_EXPORT, JSOP_NOP, pos, kid); That is, use the constructor since that is more straightforward. Allocate the parent node last, for no reason except to do less mutation as we go. (Ideally the 'pos' part would be easier to get right; a helper class would be nice to have.) @@ +4178,5 @@ > + tokenStream.currentToken().name() == context->names().module && > + tokenStream.peekTokenSameLine(TSF_OPERAND) == TOK_NAME) > + { > + return moduleStmt(); > + } This would want a fall-through comment.

Comment on attachment 702295 [details] [diff] [review] Bytecode emitter support Review of attachment 702295 [details] [diff] [review]: ----------------------------------------------------------------- Again, surely this is way too fast-and-loose. I wouldn't want to r+ this stuff before we had a bunch of tests passing, including a bunch of SyntaxError tests. ::: js/src/frontend/BytecodeEmitter.cpp @@ +6470,5 @@ > #endif /* JS_HAS_BLOCK_SCOPE */ > + > + case PNK_EXPORT: > + ok = EmitTree(cx, bce, pn->pn_kid); > + break; Does the PNK_EXPORT node prevent functions from being hoisted to the top of the module as desired? ::: js/src/methodjit/FrameState-inl.h @@ +1025,5 @@ > if (fe >= a->args) > return StackFrame::offsetOfFormalArg(a->script->function(), uint32_t(fe - a->args)); > if (fe == a->this_) > + return StackFrame::offsetOfThis(a->script->isFunctionScript() > + ? a->script->function() : NULL); When this sort of pattern pops up often enough, the class grows a maybeFunction() method.

Comment on attachment 702262 [details] [diff] [review] Parser support Btw - this also needs to touch jsreflect.cpp ...which means it's testable all by itself! Yay!

I'm getting a ton of warnings on gcc 4.6.3: ../../jsobj.h:1037:24: warning: inline function ‘js::Module& JSObject::asModule()’ used but never defined [enabled by default]

Yup, Eddy has a patch for that (Comment 36), which has r+ -- Eddy, can you land that? :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/71811483976f

(In reply to Eddy Bruel [:ejpbruel] from comment #44) > https://hg.mozilla.org/integration/mozilla-inbound/rev/71811483976f Let me know if this actually fixed the problem. Ms2ger was confident it would, but I couldn't test it locally.

Yup, looks like that fixed it -- thanks! (I just started a clobber m-i build, and it got through all the js*.cpp files without spamming any instances of that warning.)

Based on your feedback, I've decided to break the task of adding parser support up into three steps. Each step adds parser support for module, export, and import declarations, respectively. The first step adds parser support for module declarations. The attack plan is as follows: - Refactor ModuleBox so its more consistent with ObjectBox and FunctionBox - Add a new type of ParseNode, ModuleNode, for module declarations - Add Parser support for module declarations - Add Reflect support for module declarations + tests

Not that much test coverage yet, but will add more tests when I add support for export and import declarations.

Comment on attachment 706903 [details] [diff] [review] Parser support for module declarations Review of attachment 706903 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +1828,5 @@ > +Parser::moduleStmt() > +{ > + JS_ASSERT(versionNumber() == JSVERSION_ECMA_5); > + JS_ASSERT(tokenStream.currentToken().name() == context->runtime->atomState.module); > + if (!((pc->sc->isGlobalSharedContext() || pc->sc->isModuleBox()) && pc->atBodyLevel())) Too many parens

(In reply to :Ms2ger from comment #54) > Comment on attachment 706903 [details] [diff] [review] > Parser support for module declarations > > Review of attachment 706903 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/frontend/Parser.cpp > @@ +1828,5 @@ > > +Parser::moduleStmt() > > +{ > > + JS_ASSERT(versionNumber() == JSVERSION_ECMA_5); > > + JS_ASSERT(tokenStream.currentToken().name() == context->runtime->atomState.module); > > + if (!((pc->sc->isGlobalSharedContext() || pc->sc->isModuleBox()) && pc->atBodyLevel())) > > Too many parens Seriously? Personally, I feel that this most accurately conveys the intended semantics of the condition, namely that a module declaration should appear at the body level of either a program or another module. The alternative takes less parens, true, but its less clear what this condition is supposed to check. I've run into this multiple times when studying the parser code, so in my opinion clarity of semantics should trump clarity of syntax.

Fixed some bugs I found in this patch

Fixed a few bugs I found in this patch

This patch implements parser support for export declarations. Note that the getter/setter and expression forms, as well as the * form probably won't make it to the spec, so I haven't implemented them for now.

This patch implements reflect support for export declarations, and add tests.

Comment on attachment 709118 [details] [diff] [review] Parser support for export declarations Review of attachment 709118 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +2889,5 @@ > + > + /* Tell js_EmitTree to generate a final POP. */ > + pn->pn_kid->pn_xflags |= PNX_POPVAR; > + > + pn->pn_kid = MatchOrInsertSemicolon(context, &tokenStream) ? pn->pn_kid : NULL; This looks somewhat strange to me. Would if (!MatchOrInsertSemicolon(context, &tokenStream)) pn->pn_kid = NULL; work? @@ +2900,5 @@ > + > + /* Tell js_EmitTree to generate a final POP. */ > + pn->pn_kid->pn_xflags |= PNX_POPVAR; > + > + pn->pn_kid = MatchOrInsertSemicolon(context, &tokenStream) ? pn->pn_kid : NULL; (Same here.) @@ +3708,5 @@ > +Parser::letDeclaration() > +{ > + /* > + * This is a let declaration. We must be directly under a block per the > + * proposed ES4 specs, but not an implicit block created due to ES what?

Comment on attachment 709113 [details] [diff] [review] Add ModuleNode Review of attachment 709113 [details] [diff] [review]: ----------------------------------------------------------------- Generally speaking, I think it's bad to increase the number of ParseNodeArity constants. The reason is has to do with "what arity is for". It's only useful in places where we want to walk the parse tree and we *don't care* about most of the parse node kinds or what they mean, we only care about the structure (we want to walk the whole thing, and not miss a subtree). As evidence of this: most of the places where you had to add PN_MODULE code, it's identical or very similar to the existing PN_FUNC case. Therefore, arity should reflect AST structure (only), to make the arity-using code simpler. I suggest renaming PN_FUNC to something like PN_CODE, to cover both functions and modules, and in the two or three existing 'case PN_FUNC:' blocks where we need to treat functions and modules differently, just check the kind for PNK_FUNCTION vs. PNK_MODULE. Whichever approach is less code is what we should do; if you don't think it's worth rewriting, don't do it! r=me either way. ::: js/src/frontend/BytecodeEmitter.cpp @@ +1490,5 @@ > * A named function, contrary to ES3, is no longer useful, because we > * bind its name lexically (using JSOP_CALLEE) instead of creating an > * Object instance and binding a readonly, permanent property in it > * (the object and binding can be detected and hijacked or captured). > * This is a bug fix to ES3; it is fixed in ES3.1 drafts. No action required, but I really don't understand this pre-existing comment. I wonder why we don't optimize away function-statements. ::: js/src/frontend/ParseNode.cpp @@ +390,5 @@ > > + case PN_MODULE: > + NULLCHECK(pn->pn_modulebox = parser->newModuleBox(opn->pn_modulebox->module(), pc)); > + NULLCHECK(pn->pn_body = CloneParseTree(opn->pn_body, parser)); > + break; What does it mean to delete this code? Modules can't be cloned? If so, we need case PN_MODULE: JS_NOT_REACHED("module ast nodes can't be cloned"); return NULL; ::: js/src/frontend/ParseNode.h @@ +483,5 @@ > PN_NULLARY, /* 0 kids, only pn_atom/pn_dval/etc. */ > PN_UNARY, /* one kid, plus a couple of scalars */ > PN_BINARY, /* two kids, plus a couple of scalars */ > PN_TERNARY, /* three kids */ > + PN_MODULE, /* module definition node */ Seems like the previous patch wouldn't compile without this. (For reviews it totally doesn't matter, but when you push, it's nice to either make sure each changeset builds or qfold them; changesets that don't build trip up hg bisect.) ::: js/src/frontend/Parser.cpp @@ +4948,5 @@ > if (!transplant(pn->pn_kid)) > return false; > break; > > + case PN_MODULE: Modules can't appear in expressions, so this can't happen. Better to assert. Gosh, the sooner we can kill this code the better...

Comment on attachment 709114 [details] [diff] [review] Parser support for module declarations Review of attachment 709114 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful. It's great to see some code going into the front end that just says what it means. ::: js/src/frontend/Parser.cpp @@ +1824,5 @@ > return true; > } > > ParseNode * > +Parser::moduleStmt() moduleDecl would be fine. @@ +1829,5 @@ > +{ > + JS_ASSERT(versionNumber() == JSVERSION_ECMA_5); > + JS_ASSERT(tokenStream.currentToken().name() == context->runtime->atomState.module); > + if (!((pc->sc->isGlobalSharedContext() || pc->sc->isModuleBox()) && pc->atBodyLevel())) > + { Style nit: { goes at the end of the previous line. @@ +4110,5 @@ > case TOK_ERROR: > return NULL; > > + case TOK_NAME: > + if (versionNumber() == JSVERSION_ECMA_5 && Unless there is some really good reason not to support the new syntax everywhere, drop the version check.

Comment on attachment 709115 Review of attachment 709115: ----------------------------------------------------------------- Very nice again. Loving this stack so far. :) ::: js/src/jit-test/tests/module/testModuleSyntax1.js @@ +17,5 @@ > +}, SyntaxError); > + > +var node = Reflect.parse("module 'foo' {}"); > +assertEq(typeof node == "object" && node != null, true); > +assertEq(node.type, "Program"); Check out the existing tests/js1_8_5/extensions/reflect-parse.js which uses stuff from tests/js1_8_5/extensions/shell.js for this. I think it'll help. You'll have to move this test into that directory, or move the old test and its support code into the new directory, or better yet, move both to jit-test and put the support code in jit-test/lib. People love to just pile stuff into that existing test, but please, by all means, buck the trend. ::: js/src/jsreflect.cpp @@ +2273,4 @@ > case PNK_FUNCTION: > case PNK_VAR: > case PNK_CONST: > return declaration(pn, dst); It doesn't make any sense for us to have this ::declaration() method, since all it does is switch on parse node kind, which the sole caller has already done. Please fix this en passant if you agree.

https://hg.mozilla.org/integration/mozilla-inbound/rev/126cfa64a877

(In reply to Eddy Bruel [:ejpbruel] from comment #72) > https://hg.mozilla.org/integration/mozilla-inbound/rev/e0aa698192b7 Backed out this patch because it caused bustage on inbound. Will try again tomorrow.

(In reply to Eddy Bruel [:ejpbruel] from comment #71) > https://hg.mozilla.org/integration/mozilla-inbound/rev/126cfa64a877 https://hg.mozilla.org/mozilla-central/rev/126cfa64a877 Note that this also landed with the wrong bug number in the commit message (though it ended up linking to a pretty funny bug all things considered).

Comment on attachment 709114 [details] [diff] [review] Parser support for module declarations https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8547a266b7

https://hg.mozilla.org/mozilla-central/rev/8c18e6431905 https://hg.mozilla.org/mozilla-central/rev/ec8547a266b7

Awesome! Is there a plan to expose this to the jsapi? (i.e. : write binary module and expose to the userland, hook into "import" in order to lazy load files, and so on). Thanks

I have some difficulties extracting the status of this bug from the comments and patches. Do I understand correctly that we will have this feature soon? Lack of modules is perhaps the most fundamental blocker for bug 853460.

(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until March 26th> from comment #79) > I have some difficulties extracting the status of this bug from the comments > and patches. Do I understand correctly that we will have this feature soon? > Lack of modules is perhaps the most fundamental blocker for bug 853460. I got distracted from this bug after I filed some patches to add parser support and they got bitrotted before they could be reviewed. I want to refile those patches this week. Jason and I will also start talking about how to implement the loader part of the module spec, but it's hard to give an ETA on that at this stage.

Well, the lack of modules in workers is rather annoying for us, so whenever it lands, this will be cause for celebration :)

Also, if for some reason you are planning to land subsets that would already be usable by Chrome Workers, we would be interested in/available for test-driving them.

(In reply to David Rajchenbach Teller [:Yoric] from comment #81) > Well, the lack of modules in workers is rather annoying for us, so whenever > it lands, this will be cause for celebration :) Can you (or someone else) expand on what in particular is interesting about modules in workers, as opposed to other code. The way you phrase your comment is odd given that modules (in the sense of this bug) are available nowhere at all.

Well, we are attempting to move as much work as possible from the main thread to workers. Since XPConnect disappeared from workers, this means that we need to recode many features specifically for workers. The synchronous version of OS.File is an example of this, but we also have plans for gzip bindings for workers, sqlite bindings for workers, etc. Doing this with |importScripts| and using closures for scope is possible but quickly leads to ugly and error-prone code. I have the rather strong impression that we would be much better off using modules.

Yoric, I'm not sure how ES6 modules will help you write new bindings for all the things. We are working on it, but it's still early. We'll update this bug as things progress.

(In reply to Jason Orendorff [:jorendorff] from comment #85) > Yoric, I'm not sure how ES6 modules will help you write new bindings for all > the things. Bindings, I can do. The difficult part is doing stuff in a modular way. > We are working on it, but it's still early. We'll update this > bug as things progress. Sounds good, thanks.

Comment on attachment 732393 [details] [diff] [review] Parser support for export declarations Review of attachment 732393 [details] [diff] [review]: ----------------------------------------------------------------- I went over all the parser patches here during the flight. No internet, so no checking of some facts, but I can't remember being stuck because of that, right now. Still, salt to taste. ---- I think it would make sense to move the changes to let parsing into their own patch. They're required for export parsing, but not really otherwise related to it. That'd also give you the chance (or burden?) to chnage the let semantics to better match what ES6 specifies. Additionally, there seem to be a few problems with how you're using the extracted letDeclaration handler. Specifically, you're not consistently calling MatchOrInsertSemicolon. ::: js/src/frontend/Parser.cpp @@ +4028,5 @@ > +Parser<FullParseHandler>::letDeclaration() > +{ > + /* > + * This is a let declaration. We must be directly under a block per the > + * proposed ES4 specs, but not an implicit block created due to ES4 ... @@ +4050,5 @@ > + JS_ASSERT(pc->blockChain == stmt->blockObj); > + } else { > + if (pc->atBodyLevel()) { > + /* > + * ES4 specifies that let at top level and at body-block scope ES4 ... @@ +4120,5 @@ > + return null(); > + pn->pn_xflags = PNX_POPVAR; > + return pn; > +} > +#endif // JS_HAS_BLOCK_SCOPE The following three definitions must be within the #ifdef. (Which we should probably just get rid of at some point ...) @@ +4148,5 @@ > + > + /* Let expressions require automatic semicolon insertion. */ > + JS_ASSERT(pn->isKind(PNK_SEMI) || pn->isOp(JSOP_NOP)); > + } else { > + return letDeclaration(); seems to me like this should be pn = letDeclaration(); if (!pn) return null(); The way it is now, you don't call MatchOrInsertSemicolon, below. @@ +4223,5 @@ > + if (!pn) > + return null(); > + > + switch (tokenStream.getToken(TSF_OPERAND)) { > + case TOK_LC: braces here and for all other multi-line cases @@ +4238,5 @@ > + return null(); > + > + /* Tell js_EmitTree to generate a final POP. */ > + pn->pn_kid->pn_xflags |= PNX_POPVAR; > + Fix whitespace @@ +4241,5 @@ > + pn->pn_kid->pn_xflags |= PNX_POPVAR; > + > + pn->pn_kid = MatchOrInsertSemicolon(context, &tokenStream) ? pn->pn_kid : null(); > + break; > + Fix whitespace @@ +4242,5 @@ > + > + pn->pn_kid = MatchOrInsertSemicolon(context, &tokenStream) ? pn->pn_kid : null(); > + break; > + > + case TOK_CONST: Can you combine this with TOK_VAR? Something along the lines of case TOK_VAR: isVar = true; case TOK_CONST: { pn->pn_kid = variables(isVar ? PNK_VAR : PNK_CONST); should work. With isVar declared above the switch, of course; @@ +4249,5 @@ > + return null(); > + > + /* Tell js_EmitTree to generate a final POP. */ > + pn->pn_kid->pn_xflags |= PNX_POPVAR; > + Fix whitespace @@ +4253,5 @@ > + > + pn->pn_kid = MatchOrInsertSemicolon(context, &tokenStream) ? pn->pn_kid : null(); > + break; > + > +#ifdef JS_HAS_BLOCK_SCOPE Fix whitespace @@ +4255,5 @@ > + break; > + > +#ifdef JS_HAS_BLOCK_SCOPE > + case TOK_LET: > + pn->pn_kid = letDeclaration(); MatchOrInsertSemicolon here, too? @@ +4258,5 @@ > + case TOK_LET: > + pn->pn_kid = letDeclaration(); > + break; > +#endif > + Fix whitespace

Comment on attachment 732401 [details] [diff] [review] Reflect support for export declarations + test Review of attachment 732401 [details] [diff] [review]: ----------------------------------------------------------------- Missing serialization support for lets, plus tests for those. Looking good otherwise. ::: js/src/jsreflect.cpp @@ +1763,5 @@ > +{ > + RootedValue expr(cx); > + ParseNode *pnkid = pn->pn_kid; > + switch (pnkid->getKind()) { > + case PNK_EXPORTLIST: braces @@ +1770,5 @@ > + break; > + > + case PNK_FUNCTION: > + case PNK_VAR: > + case PNK_CONST: braces @@ +1775,5 @@ > + if (!declaration(pnkid, &expr)) > + return false; > + break; > + > + default: case PNK_LET seems to be missing @@ +1790,5 @@ > + return false; > + for (ParseNode *next = pn->pn_head; next; next = next->pn_next) { > + RootedValue elt(cx); > + switch (next->getKind()) { > + case PNK_NAME: braces @@ +1795,5 @@ > + if (!identifier(next, &elt)) > + return false; > + break; > + > + case PNK_COLON: braces @@ +1804,5 @@ > + default: > + LOCAL_NOT_REACHED("unexpected export specifier type"); > + }; > + elts.infallibleAppend(elt); > + } fix whitespace

Comment on attachment 732432 [details] [diff] [review] Parser support for import declarations Review of attachment 732432 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I can't comment on spec-conformance, however: The current draft doesn't contain ImportDeclaration and checking the wiki isn't possible from the plane I'm on. Update: a very quick perusal of the wiki indicates that the implementation should be fine. Do note the *very* and the *quick* parts in that sentence, though.

Comment on attachment 732433 [details] [diff] [review] Reflect support for import declarations + test Review of attachment 732433 [details] [diff] [review]: ----------------------------------------------------------------- Looks good ::: js/src/jsreflect.cpp @@ +1891,5 @@ > +{ > + RootedValue left(cx); > + RootedValue right(cx); > + switch (pn->getKind()) { > + case PNK_AS: braces @@ +1898,5 @@ > + if (!identifier(pn->pn_right, &right)) > + return false; > + break; > + > + case PNK_FROM: braces @@ +1920,5 @@ > + return false; > + for (ParseNode *next = pn->pn_head; next; next = next->pn_next) { > + RootedValue elt(cx); > + switch (next->getKind()) { > + case PNK_NAME: braces @@ +1925,5 @@ > + if (!identifier(next, &elt)) > + return false; > + break; > + > + case PNK_COLON: braces @@ +1934,5 @@ > + default: > + LOCAL_NOT_REACHED("unexpected import specifier type"); > + }; > + elts.infallibleAppend(elt); > + } fix whitespace

Comment on attachment 732393 [details] [diff] [review] Parser support for export declarations Review of attachment 732393 [details] [diff] [review]: ----------------------------------------------------------------- I think a case or two must be added in js::frontend::EmitTree, so that actually using these new bits of syntax causes a SyntaxError rather than an assertion failure. (Also in jsreflect.cpp. The very next patch adds them---just make sure not to this one without that one; and honestly I'd prefer not to push changesets with known assertion failures at all. Just merging the two changesets before pushing would be an improvement, I think.) This doesn't support all the export syntax that's in the latest document https://docs.google.com/document/d/1FL3VF_OEwMPQ1mZKjxgR-R-hkieyyZO-8Q_eNTiyNFs/edit# In particular it does not support: export a; // identifiers as ExportSpecifierSets export a, b; // multiple ExportSpecifierSets in a single ExportDeclaration export {a}, {b}; export *; // export * export * from "m"; // "from" clauses in ExportSpecifierSet export {a: b from "m"}; // "from" clause in ExportSpecifier I'm fine with this patch even without all that. Some of that syntax is baffling to me anyway. ::: js/src/frontend/Parser.cpp @@ +4149,5 @@ > + /* Let expressions require automatic semicolon insertion. */ > + JS_ASSERT(pn->isKind(PNK_SEMI) || pn->isOp(JSOP_NOP)); > + } else { > + return letDeclaration(); > + } If there isn't a test for a let-declaration with missing semicolon, please add one! @@ +4242,5 @@ > + > + pn->pn_kid = MatchOrInsertSemicolon(context, &tokenStream) ? pn->pn_kid : null(); > + break; > + > + case TOK_CONST: I agree with Till's remark; here's another way to write it: switch (TokenKind token = tokenStream.getToken(TSF_OPERAND)) { ... case TOK_VAR: case TOK_CONST: pn->pn_kid = variables(token == TOK_VAR ? PNK_VAR : PNK_CONST); ... break; ... }

Comment on attachment 732401 [details] [diff] [review] Reflect support for export declarations + test Review of attachment 732401 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/modules/exportDeclaration.js @@ +24,5 @@ > + node = node.declaration; > + assertEq(typeof node == "object" && node != null, true); > + assertEq(node.type, type); > + return node; > +}; Extra semicolon. This could be a lot shorter using the pattern-matching stuff the existing reflect-parse.js test uses; I vote factor out that matching code into a file so it can be reused in other tests. @@ +45,5 @@ > +assertEq(typeof right == "object" && right != null, true); > +assertEq(right.type, "Identifier"); > +assertEq(right.name, "c"); > + > +test("module 'foo' { export function f() {}; }", "FunctionDeclaration"); The extra semicolon in there is an EmptyStatement. ::: js/src/jsreflect.cpp @@ +1010,5 @@ > + > + return newNode(AST_EXPORT_SPEC, pos, > + "left", left, > + "right", right, > + dst); Use "key" and "value", just like in an ObjectPattern or ObjectLiteral, rather than "left" and "right".

Comment on attachment 732432 [details] [diff] [review] Parser support for import declarations Review of attachment 732432 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +4279,5 @@ > } > > +template <> > +FullParseHandler::Node > +Parser<FullParseHandler>::importStatement() Please try implementing the method generically: template <class ParseHandler> ParseHandler::Node Parser<ParseHandler>::importStatement() { ... } There's nothing much behind this except "generic is good", so do what seems best. But it's not really harder to write generic code; the ParseHandler protocol ought to support everything you're trying to do here. @@ +4310,5 @@ > + MUST_MATCH_TOKEN(TOK_NAME, JSMSG_SYNTAX_ERROR); > + RootedPropertyName name(context, tokenStream.currentToken().name()); > + pnelem->pn_right = handler.newName(name, pc); > + if (!pnelem->pn_right) > + return null(); To make this generic, you'll have to create the binary node last: Node moduleName = atomNode(PNK_STRING, JSOP_NOP); if (!moduleName) return null(); ... Node bindingName = handler.newName(name, pc); if (!bindingName) return null(); ... pnelem = handler.newBinary(PNK_AS, moduleName, bindingName); if (!pnelem) return null(); break; I actually think this reads nicer anyway. @@ +4342,5 @@ > + report(ParseError, true, null(), JSMSG_SYNTAX_ERROR); > + return NULL; > + } > + > + pn->append(pnelem); The generic way is: handler.addList(pn, pnelem);

Comment on attachment 732433 [details] [diff] [review] Reflect support for import declarations + test Review of attachment 732433 [details] [diff] [review]: ----------------------------------------------------------------- Same comments as above about testing infrastructure and .{key,value} vs .{left,right}. ::: js/src/jsreflect.cpp @@ +1038,5 @@ > + > + return newNode(AST_IMPORT_CLAUSE, pos, > + "left", left, > + "right", right, > + dst); It's a little strange for the meaning of "left" and "right" to be swapped depending on whether the preposition is "as" or "from", especially since the preposition isn't exposed in the Reflect API. Instead of "left" and "right", how about "bindings" and "module", and we'll internally swap them so that: import "xyzzy" as plugh; { type: "ImportClause" module: "xyzzy", bindings: {type: "Identifier", name: "plugh"} } import {foo, bar} from "xyzzy"; { type: "ImportClause" bindings: { type: "ImportSpecifierSet", specifiers: [ {type: "Identifier", name: "foo"}, {type: "Identifier", name: "bar"} ] }, module: "xyzzy" } That makes a lot more sense to me; YMMV. Or, it might help to add a {"kind": "as"/"from"} property to ImportClause nodes. It's up to you. @@ +1057,5 @@ > + return callback(cb, left, right, pos, dst); > + > + return newNode(AST_IMPORT_SPEC, pos, > + "left", left, > + "right", right, "key" and "value" here.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #92) > https://hg.mozilla.org/mozilla-central/rev/172651edb28e This needs backporting to aurora to fix crashes that decoder found. (Having multiple landings in the same bug straddle merge windows seems to make things a little confusing)

Removing blocker to bug 853460, as bug 872421 can provide a lightweight alternative that should do the trick as long as necessary.

Yoric, just for future reference, the term "module" in bug 872421 and in this bug mean totally unrelated things.

I have been told that potentially part of the implementation of Harmony Modules can be done in JavaScript. If there is anything that can be done on the Gaia side to help implementing this part that would be fantastic. Like finding resources to help (James would you be interested in helping there if some part can be done in JS?), or dogfooding Harmony Modules.

I have given feedback into the spec process, and I have an in-process ES6 Module Loader, but written as an adapter for today's code, not something for use as a browser implementation. I hit a few bumps and had some feedback privately to the spec authors on TC39. Jason Orendorff has been writing psuedo code/actual code for a Module Loader implementation that may be more suitable as a browser implementations long term but mostly to help work out the API design issues: https://github.com/jorendorff/js-loaders I have offered other help for during the spec process, but as I understand it, the spec authors are still finalizing design and gathering other feedback. While the syntax for module declaration and import seem fairly stable, the Module Loader API seems less so. Some folks on the internet have also done a prototype for a Module Loader that runs in ES5 here: https://github.com/ModuleLoader/es6-module-loader but I believe that one is premature. It works off the existing public spec for the Module Loader API, which likely will have some changes. So in general, happy to help, but it seemed the help that may be useful at this point is feedback on the design and APIs.

Rebased and merged the two patches, and addressed comments by Till and Jorendorff where appropriate. Putting up for review again because changes were significant. Lets try not to let the patch bitrot this time.

Idem for import statements

Out of curiosity, what is the status of the harmony modules implementation? It doesn't seem to depend on any other bug anymore. Which parts do still need to be implemented? Another thing: Is the plan to refactor all JS modules (Components.import) into harmony modules once these are supported?

jorendorff, you explicitly asked me to rebase these patches so you could review them. I'd appreciate it if you didn't let them bitrot again.

Comment on attachment 779831 [details] [diff] [review] Parser support for export statements Review of attachment 779831 [details] [diff] [review]: ----------------------------------------------------------------- Lovely! ::: js/src/frontend/ParseNode.h @@ +133,5 @@ > F(ARGSBODY) \ > F(SPREAD) \ > F(MODULE) \ > + F(EXPORT) \ > + F(EXPORTLIST) \ Cool. Please also update the big comment below, under "<Definitions>", describing the new node types. ::: js/src/frontend/Parser.cpp @@ +3439,5 @@ > +Parser<FullParseHandler>::letDeclaration() > +{ > + /* > + * This is a let declaration. We must be directly under a block per the > + * proposed ES5 specs, but not an implicit block created due to ES6, not ES5. @@ +3461,5 @@ > + JS_ASSERT(pc->blockChain == stmt->blockObj); > + } else { > + if (pc->atBodyLevel()) { > + /* > + * ES5 specifies that let at top level and at body-block scope I think ES6 consensus says something else, at least for the moment -- the controversial "second tier" of "global" variables that aren't also properties of the global object. So this comment should probably just drop the "ES5 specifies" and begin "At top level..." @@ +3587,5 @@ > + Node pnelem = newName(name); > + if (!pnelem) > + return null(); > + > + if (tokenStream.matchToken(TOK_COLON)) { The syntax has changed and now the identifier `as` is used here. ExportSpecifier ::= Identifier ("as" IdentifierName)? @@ +3590,5 @@ > + > + if (tokenStream.matchToken(TOK_COLON)) { > + Node key = pnelem; > + > + MUST_MATCH_TOKEN(TOK_NAME, JSMSG_SYNTAX_ERROR); This one may be a keyword (that's the difference between IdentifierName and Identifier). MUST_MATCH_TOKEN() won't do this properly. We do have code that does this, but it's not factored out nicely. You can add a new method Parser::identifierName, factoring out the implementation from Parser::memberExpr, under `if (tt == TOK_DOT) { … }`. @@ +3596,5 @@ > + Node value = newName(name); > + if (!value) > + return null(); > + > + pnelem = handler.newBinary(PNK_COLON, key, value); I'd slightly prefer adding a new PNK_AS node kind, given the syntax change. Your call. @@ +3625,5 @@ > + case TOK_LC: > + pnkid = specifierSet(PNK_EXPORTLIST); > + if (!pnkid) > + return null(); > + break; According to the wiki, this kind of export has a semicolon at the end: ExportDeclaration ::= "export" ExportSpecifierSet ("from" ModuleSpecifier)? ";" so throw in a call to MatchOrInsertSemicolon. Please either implement the optional `"from" ModuleSpecifier` bit in a second patch in this bug, or file a followup. I'm looking at: http://wiki.ecmascript.org/doku.php?id=harmony:modules @@ +3643,5 @@ > + /* Tell js_EmitTree to generate a final POP. */ > + pnkid->pn_xflags |= PNX_POPVAR; > + > + if (!MatchOrInsertSemicolon(tokenStream)) > + return null(); stray trailing whitespace on this line ::: js/src/jit-test/lib/match.js @@ +4,5 @@ > + * http://creativecommons.org/licenses/publicdomain/ > + */ > + > +// A little pattern-matching library. > +var Match = If you'd like to migrate the existing Reflect.parse tests :-D to jit-tests, and remove this code from js1_8_5/extensions/shell.js, that would be fine with me. ::: js/src/jit-test/tests/modules/exportStatement.js @@ +1,4 @@ > +load(libdir + "asserts.js"); > +load(libdir + "match.js"); > + > +var { Pattern, MatchError } = Match; MatchError isn't used here, fwiw. @@ +32,5 @@ > +}, SyntaxError); > + > +assertThrowsInstanceOf(function () { > + Reflect.parse("export var x;"); > +}, SyntaxError); This assertion looks identical to the previous one to me. Did you intend to test `export let`? Please add a test that module 'foo' { export let (x = 1) {} } is a SyntaxError. ::: js/src/jsreflect.cpp @@ +988,5 @@ > + dst); > +} > + > + > + Delete the extra blank lines. There are a few spaces on one of these lines too. @@ +1005,5 @@ > + return callback(cb, key, value, pos, dst); > + > + return newNode(AST_EXPORT_SPEC, pos, > + "key", key, > + "value", value, "key" and "value" aren't very clear here. Unfortunately it's the kind of distinction that doesn't have any obvious two-word summary... The only way I can think of to make it crystal clear which is which is to use "as" for the one that comes after "as" in the syntax. So I propose "name" and "as" instead of "key" and "value": export {foo as bar}; ... type: "ExportSpecifier", name: {type: "Identifier", name: "foo"}, as: {type" Identifier", name: "bar"} ...

> @@ +1005,5 @@ > > + return callback(cb, key, value, pos, dst); > > + > > + return newNode(AST_EXPORT_SPEC, pos, > > + "key", key, > > + "value", value, > > "key" and "value" aren't very clear here. > > Unfortunately it's the kind of distinction that doesn't have any obvious Weren't you the one that suggested "key" and "value" in comment 100 after I used "left" and "right" (which was, admittedly, even less clear)? > two-word summary... > > The only way I can think of to make it crystal clear which is which > is to use "as" for the one that comes after "as" in the syntax. > So I propose "name" and "as" instead of "key" and "value": > > export {foo as bar}; > > ... > type: "ExportSpecifier", > name: {type: "Identifier", name: "foo"}, > as: {type" Identifier", name: "bar"} > ...

Comment on attachment 779834 [details] [diff] [review] Parser support for import statements Review of attachment 779834 [details] [diff] [review]: ----------------------------------------------------------------- Eddy, can you revise this patch once again? It has not bitrotted: but the *proposal* has changed out from under it. :-\ ::: js/src/frontend/ParseNode.h @@ +135,5 @@ > F(MODULE) \ > F(EXPORT) \ > F(EXPORTLIST) \ > + F(IMPORT) \ > + F(AS) \ Yay PNK_AS! ::: js/src/frontend/Parser.cpp @@ +3694,5 @@ > + if (!moduleName) > + return null(); > + > + MUST_MATCH_TOKEN(TOK_NAME, JSMSG_SYNTAX_ERROR); > + if (tokenStream.currentToken().name() != context->names().as) { The proposal has changed again, I'm afraid. This: import "specifier" as name; is now written like this: module name from "specifier"; which dherman assures me is the very last final syntax for keeps, no backsies. (Actually there is just one last change he plans to make, which involves default exports: https://mail.mozilla.org/pipermail/es-discuss/2013-June/031048.html We can implement that in a later bug, though. We need to ban keywords as function names first.) In addition there's a new syntax: import "specifier"; which means... well, I'm not going to worry too much about what it means, let's just get it in. @@ +3712,5 @@ > + } > + > + case TOK_LC: > + { > + Node importList = specifierSet(PNK_IMPORTLIST); The syntax for ImportSpecifier is now just slightly different from ExportSpecifier (regarding the treatment of names that are keywords). @@ +3739,5 @@ > + return null(); > + } > + > + handler.addList(pn, pnelem); > + } while (tokenStream.matchToken(TOK_COMMA)); The syntax on the wiki page doesn't support multiple imports separated by commas.

> Eddy, can you revise this patch once again? And if you could make a module name in one of your tests a multi-line string, like the example in bug 900346 comment 2, that would be great, as it would improve the testing of that bug's fix :)

It's been a while since we've seen any progress on this bug. Getting reviews turned out to be hard, and meanwhile the spec kept changing out from under us. Those issues should now be resolved, so we're going to make another push to get towards an implementation. Unfortunately, the latest spec changed obsoleted most of the work that already landed so far. As a first step, here's a patch to back all that out so we can make a fresh start. Some of this stuff might eventually be re-added again, but let's cross that bridge when we get to it. I have more patches coming up after this one to implement parser and reflect support for import statements.

(In reply to Eddy Bruel [:ejpbruel] from comment #116) > It's been a while since we've seen any progress on this bug. Getting reviews > turned out to be hard, and meanwhile the spec kept changing out from under > us. Those issues should now be resolved, so we're going to make another push > to get towards an implementation. A ton of progress has been made in https://github.com/jorendorff/js-loaders toward producing both a finished spec and a self-hosted implementation.

Comment on attachment 817414 [details] [diff] [review] Back out obsolete module code Review of attachment 817414 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/modules/nonkeyword.js @@ -1,1 @@ > -// 'module' is not a keyword in these contexts. You could keep this file, right? All these should still pass with the module stuff ripped out.

(In reply to Jason Orendorff [:jorendorff] from comment #118) > Comment on attachment 817414 [details] [diff] [review] > Back out obsolete module code > > Review of attachment 817414 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit-test/tests/modules/nonkeyword.js > @@ -1,1 @@ > > -// 'module' is not a keyword in these contexts. > > You could keep this file, right? All these should still pass with the module > stuff ripped out. Well yeah, but module isn't a keyword anymore, period, isn't it? It doesn't make sense to check that every identifier that's not supposed to be a keyword isn't treated as one, so I feel like this test is pointless now.

There is this: ModuleDeclaration: module [no LineTerminator here] Identifier from ModuleSpecifier ; It's not implemented yet, but it will be. Please just keep the test.

Eddy, please land the patch backing out the obsolete module syntax!

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b77f7733578

https://hg.mozilla.org/mozilla-central/rev/82c3097bb8b3 https://hg.mozilla.org/mozilla-central/rev/2b77f7733578

jorendorff says modules are blocked on bug 589199 (top-level let) and bug 1001090 (let temporal dead zone).

The 'Target Milestone' is pretty outdated by now and should be changed. Sebastian

I'd like to work on this. Do you know if the spec in a stable place now where it makes sense to implement it? My initial plan is to un-bitrot the previous set of patches and figure out how they work, and then adapt/rewrite them to the current spec. Does that sound reasonable?

(In reply to Jon Coppeard (:jonco) from comment #127) > My initial plan is to un-bitrot the previous set of patches and figure out > how they work, and then adapt/rewrite them to the current spec. > > Does that sound reasonable? Yes, but let's also ask Waldo if the scoping situation in blocking bug 589199 is going to get resolved soon. I think Eddy's previous approach was to hack around that---which is fine with me, for the short term. I think the syntax changed some more since this last attempt, so also consult ES6.

Is there any way to play with this outside of the JS shell?

(In reply to Alan from comment #129) Currently this is only implemented in the JS shell, but see bug 1240072.

Removing from the es6 dependency tree - remaining work is post-ES6.

Looks like <script type="module"> is only supported for chrome documents[1]. What are the plans to roll it out more widely? [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp?q=nsModuleLoadRequest&redirect_type=direct#1445

I don't know if it is the right place. Here is a bug report: When using modules, with an html file, served directly from disk with file:/// import statement that use relative path with a sub-folder like import {helperMethod} from '../1.js'; do not work Note it works perfectly fine when the file is in the same folder: e.g import {helperMethod} from './1.js'; What is strange, is that there is no Error displayed aat all in the console. Going to attach file subfolderbugrepro.zip to reproduce

should at least give warning

firefox 54.0b12 (32-bit)

(In reply to capocyril from comment #133) > I don't know if it is the right place. Please open a new bug for this issue. However, I suspect the bug will end up being WONTFIX - file:// URLs get special treatment all over the place, and this may be another.

Export syntax with as in not understood, syntax error Folling the second example from MDN // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import // in another file import { registerWorker, SYMBOLS, work } from "../source/worka.js"; // this does not work // SyntaxError: missing '}' after export specifier list // export { // worka.registerWorker as registerWorker, // worka.work as work, // worka.workerSupport as workerSupport, // worka.SYMBOLS as SYMBOLS, // }; // this does export const registerWorker = worka.registerWorker; export const work = worka.work; export const workerSupport = worka.workerSupport; export const SYMBOLS = worka.SYMBOLS;

When it will be without dom.moduleScripts.enabled flag?

Is there a plan, when ES6 modules will be available without dom.moduleScripts.enabled?