930411 - Implement export declarations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Implement parser support for export declarations

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Implement reflect support for export declarations

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Test reflect support for export declarations

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 821525 [details] [diff] [review]
Implement parser support for export declarations

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

r+ with comments.

As before, don't land it without the other two; I'll review them over the next few hours.

::: js/src/frontend/Parser.cpp
@@ +3639,5 @@
> +                     pn->isOp(JSOP_NOP));
> +    } else {
> +        pn = letDeclaration();
> +        if (!MatchOrInsertSemicolon(tokenStream))
> +            return null();

Nit: Please put these two lines (matching a semicolon) back in letDeclaration since it's part of the Declaration production.

I don't think the semicolon stuff in exportDeclaration() can be commoned up anyway (see below).

@@ +3842,5 @@
> +            MUST_MATCH_TOKEN(TOK_RC, JSMSG_RC_AFTER_EXPORT_SPEC_LIST);
> +        } else {
> +            // Handle the form |export *| by adding a special export batch
> +            // specifier to the list.
> +            Node exportSpec = handler.newNullary(PNK_EXPORT_BATCH_SPEC, JSOP_NOP, pos());

BATCH is kind of a weird word to use here, consider ALL or STAR.

@@ +3846,5 @@
> +            Node exportSpec = handler.newNullary(PNK_EXPORT_BATCH_SPEC, JSOP_NOP, pos());
> +            if (!kid)
> +                return null();
> +
> +            handler.addList(kid, exportSpec);

I would expect the special * node to replace exportSpec, not be a child of it.

@@ +3860,5 @@
> +
> +            if (!MatchOrInsertSemicolon(tokenStream))
> +                return null();
> +
> +            return handler.newExportFromDeclaration(begin, kid, moduleSpec);

This early return skips the MatchSemicolon stuff below.

    export {x} from "B" f();
    export * from "A" f();

should both be SyntaxErrors.

@@ +3881,5 @@
> +        kid->pn_xflags = PNX_POPVAR;
> +        break;
> +
> +      case TOK_NAME:
> +        // Handle the from |export a} in the same way as |export let a|, by

Spelling: "form", not "from"; and change the } to a |.

@@ +3882,5 @@
> +        break;
> +
> +      case TOK_NAME:
> +        // Handle the from |export a} in the same way as |export let a|, by
> +        // acting as if we've just seen the ley keyword. Simply unget the token

Spelling: "let" not "ley"

@@ +3897,5 @@
> +        return null();
> +    }
> +
> +    if (!MatchOrInsertSemicolon(tokenStream))
> +        return null();

No semicolon should be matched after a function, so that
   export function f(){} var x = 3;
should be allowed.

Comment 5

[Jason Orendorff [:jorendorff]]

Please add a test, if there isn't one, that the following

    export {x}
    from();

is a SyntaxError, that is, automatic semicolon insertion does NOT happen at the end of the first line.

(My reading of the ASI rules <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-rules-of-automatic-semicolon-insertion> is that they don't apply here; in particular, rule 1 does not apply because the "offending token" is the '('.)

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 821526 [details] [diff] [review]
Implement reflect support for export declarations

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

::: js/src/jsast.tbl
@@ +60,5 @@
>  ASTDEF(AST_IMPORT_DECL,           "ImportDeclaration",              "importDeclaration")
>  ASTDEF(AST_IMPORT_SPEC,           "ImportSpecifier",                "importSpecifier")
> +ASTDEF(AST_EXPORT_DECL,           "ExportDeclaration",              "exportDeclaration")
> +ASTDEF(AST_EXPORT_SPEC,           "ExportSpecifier",                "exportSpecifier")
> +ASTDEF(AST_EXPORT_BATCH_SPEC,     "ExportBatchSpecifier",           "exportBatchSpecifier")

I see this in the esprima source code, and I'm fine with keeping it for API compatibility, but please let's use ALL or STAR elsewhere.

::: js/src/jsreflect.cpp
@@ +1402,5 @@
> +    RootedValue array(cx, NullValue());
> +    if (decl.isNull() && !newArray(elts, &array))
> +        return false;
> +
> +    RootedValue cb(cx, callbacks[AST_IMPORT_DECL]);

AST_EXPORT_DECL

@@ +2029,5 @@
> +                if (!exportSpecifier(next, &elt))
> +                    return false;
> +            } else {
> +                if (!builder.exportBatchSpecifier(&pn->pn_pos, &elt))
> +                    return false;

Oh, I see why you had the AST nodes arranged that way. But let's do it the sane way, at the expense of a little code here (not much). I wonder why esprima does it this way...

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 821528 [details] [diff] [review]
Test reflect support for export declarations

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

Great.

Comment 8

[Eddy Bruel [:ejpbruel]]

Tried to land this today, but still seeing failing reftests:

https://tbpl.mozilla.org/php/getParsedLog.php?id=30329203&tree=Try#error3

Probably something simple though, so this should land soon.

Comment 9

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01555404ca91

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/01555404ca91

Comment 11

[Chris Peterson [:cpeterson]]

Eddy: does this change for module export declarations warrant a Firefox 28 release note or MDN dev docs?

Comment 12

[Jason Orendorff [:jorendorff]]

No. The syntax is recognized by the parser, but still throws a SyntaxError anyway since we don't support it the rest of the way through (there are several more layers before modules work). So this work is not really "on" yet.

Comment 13

[Eddy Bruel [:ejpbruel]]

(In reply to Chris Peterson (:cpeterson) from comment #11)
> Eddy: does this change for module export declarations warrant a Firefox 28
> release note or MDN dev docs?

What Jason said :-)