568953 - (harmony:modules) [meta] ES6 modules

Status: NEW

(Core :: JavaScript Engine, enhancement)

Description

[Dave Herman [:dherman]]

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

Comment 1

[Dave Herman [:dherman]]

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

Comment 2

[Dave Herman [:dherman]]

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

Comment 3

[Dave Herman [:dherman]]

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

Comment 4

[Dave Herman [:dherman]]

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

Comment 5

[Dave Herman [:dherman]]

(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

Comment 6

[Eddy Bruel [:ejpbruel]]

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.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Module objects

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.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 9

[:Benjamin Peterson]

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

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Module objects

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.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: Shared contexts

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.

Comment 12

[Eddy Bruel [:ejpbruel]]

Attachment: Module boxes

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

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support

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.

Comment 14

[Eddy Bruel [:ejpbruel]]

Attachment: Bytecode emitter support

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!

Comment 15

[Eddy Bruel [:ejpbruel]]

Attachment: Interpreter support

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.

Comment 16

[Sam Tobin-Hochstadt [:samth]]

(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.

Comment 17

[Eddy Bruel [:ejpbruel]]

(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 18

[Jason Orendorff [:jorendorff]]

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 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 702254 [details] [diff] [review]
Shared contexts

Review of attachment 702254 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

Comment 20

[:Benjamin Peterson]

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 21

[Jason Orendorff [:jorendorff]]

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.

Comment 22

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0141f43d8c5b

Comment 23

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bee0517d440

Messed up the bug number in this one. Sorry!

Comment 24

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9676f4f869b

Comment 25

[:Benjamin Peterson]

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!

Comment 26

[Eddy Bruel [:ejpbruel]]

(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!

Comment 27

[Eddy Bruel [:ejpbruel]]

Attachment: Fix for Module objects

This patch should fix the issue.

Comment 28

[Daniel Holbert [:dholbert]]

(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]
}

Comment 29

[Daniel Holbert [:dholbert]]

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 30

[:Benjamin Peterson]

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)|.

Comment 31

[Eddy Bruel [:ejpbruel]]

(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?

Comment 32

[Daniel Holbert [:dholbert]]

(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.

Comment 33

[Daniel Holbert [:dholbert]]

(I filed bug 832453 for that asModule build warning)

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 36

[Eddy Bruel [:ejpbruel]]

Attachment: Fix for build warning

Here's a patch that should fix that build warning

Comment 37

[Eddy Bruel [:ejpbruel]]

Attachment: Bindings

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.

Comment 38

[Eddy Bruel [:ejpbruel]]

Attachment: Module scopes

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 39

[Jason Orendorff [:jorendorff]]

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 40

[Jason Orendorff [:jorendorff]]

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 41

[Jason Orendorff [:jorendorff]]

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!

Comment 42

[Luke Wagner [:luke]]

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]

Comment 43

[Daniel Holbert [:dholbert]]

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

Comment 44

[Eddy Bruel [:ejpbruel]]

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

Comment 45

[Eddy Bruel [:ejpbruel]]

(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.

Comment 46

[Daniel Holbert [:dholbert]]

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.)

Comment 47

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a48352004a2

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71811483976f

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a48352004a2

Comment 51

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor ModuleBox

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

Comment 52

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for module declarations

Comment 53

[Eddy Bruel [:ejpbruel]]

Attachment: 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 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 55

[Eddy Bruel [:ejpbruel]]

(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.

Comment 56

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for module declarations

Fixed some bugs I found in this patch

Comment 57

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for module declarations + tests

Fixed a few bugs I found in this patch

Comment 58

[Eddy Bruel [:ejpbruel]]

Attachment: Parser suppor for export declarations

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.

Comment 59

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for export declarations + tests

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

Comment 60

[Eddy Bruel [:ejpbruel]]

Attachment: Add ModuleNode

Comment 61

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for module declarations

Comment 62

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for module declarations + tests

Comment 63

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for export declarations

Comment 64

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for export declarations + tests

Comment 65

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for import declarations

Comment 66

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for import declarations + tests

Comment 67

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 68

[Jason Orendorff [:jorendorff]]

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 69

[Jason Orendorff [:jorendorff]]

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 70

[Jason Orendorff [:jorendorff]]

Comment on attachment 709115 [details] [diff]
Reflect support for module declarations + tests

Review of attachment 709115 [details] [diff]:
-----------------------------------------------------------------

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.

Comment 71

[Eddy Bruel [:ejpbruel]]

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

Comment 72

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0aa698192b7

Comment 73

[Eddy Bruel [:ejpbruel]]

(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.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

(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 75

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c18e6431905

Comment 76

[Eddy Bruel [:ejpbruel]]

Comment on attachment 709114 [details] [diff] [review]
Parser support for module declarations

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8547a266b7

Comment 77

[Ryan VanderMeulen [:RyanVM]]

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

Comment 78

[Anthony Catel [:paraboul]]

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

Comment 79

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 80

[Eddy Bruel [:ejpbruel]]

(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.

Comment 81

[David Teller [:Yoric] - still alive but not very active]

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

Comment 82

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 83

[Sam Tobin-Hochstadt [:samth]]

(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.

Comment 84

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 85

[Jason Orendorff [:jorendorff]]

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.

Comment 86

[David Teller [:Yoric] - still alive but not very active]

(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 87

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/172651edb28e

Comment 88

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for export declarations

Comment 89

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for export declarations + test

Comment 90

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for import declarations

Comment 91

[Eddy Bruel [:ejpbruel]]

Attachment: Reflect support for import declarations + test

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/172651edb28e

Comment 93

[Till Schneidereit [:till]]

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 94

[Till Schneidereit [:till]]

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 95

[Till Schneidereit [:till]]

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 96

[Till Schneidereit [:till]]

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 97

[Jason Orendorff [:jorendorff]]

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 98

[Jason Orendorff [:jorendorff]]

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 99

[Jason Orendorff [:jorendorff]]

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 100

[Jason Orendorff [:jorendorff]]

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.

Comment 101

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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)

Comment 102

[David Teller [:Yoric] - still alive but not very active]

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

Comment 103

[Sam Tobin-Hochstadt [:samth]]

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

Comment 106

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 107

[James Burke [:jrburke]]

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.

Comment 108

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for export statements

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.

Comment 109

[Eddy Bruel [:ejpbruel]]

Attachment: Parser support for import statements

Idem for import statements

Comment 110

[A. Sel.]

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?

Comment 111

[Eddy Bruel [:ejpbruel]]

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 112

[Jason Orendorff [:jorendorff]]

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"}
    ...

Comment 113

[Eddy Bruel [:ejpbruel]]

> @@ +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 114

[Jason Orendorff [:jorendorff]]

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.

Comment 115

[Nicholas Nethercote [inactive]]

> 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 :)

Comment 116

[Eddy Bruel [:ejpbruel]]

Attachment: Back out obsolete module code

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.

Comment 117

[Jason Orendorff [:jorendorff]]

(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 118

[Jason Orendorff [:jorendorff]]

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.

Comment 119

[Eddy Bruel [:ejpbruel]]

(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.

Comment 120

[Jason Orendorff [:jorendorff]]

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.

Comment 121

[Jason Orendorff [:jorendorff]]

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

Comment 122

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c3097bb8b3

Comment 123

[Eddy Bruel [:ejpbruel]]

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

Comment 124

[Ryan VanderMeulen [:RyanVM]]

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

Comment 125

[Chris Peterson [:cpeterson]]

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

Comment 126

[Sebastian Zartner [:sebo]]

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

Sebastian

Comment 127

[Jon Coppeard (:jonco)]

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?

Comment 128

[Jason Orendorff [:jorendorff]]

(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.

Comment 129

[Alan]

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

Comment 130

[Jon Coppeard (:jonco)]

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

Comment 131

[Jason Orendorff [:jorendorff]]

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

Comment 132

[Potch [:potch]]

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

Comment 133

[capocyril]

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

Comment 134

[capocyril]

Attachment: test bug subfolder import

should at least give warning

Comment 135

[capocyril]

firefox 54.0b12 (32-bit)

Comment 136

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 137

[capocyril]

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;

Comment 138

[Rustam]

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

Comment 140

[Stefan Haack]

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