Implement export declarations

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 1 bug, {feature})

unspecified
mozilla28
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 821525 [details] [diff] [review]
Implement parser support for export declarations
Attachment #821525 - Flags: review?(jorendorff)
(Assignee)

Comment 2

4 years ago
Created attachment 821526 [details] [diff] [review]
Implement reflect support for export declarations
Attachment #821526 - Flags: review?(jorendorff)
(Assignee)

Comment 3

4 years ago
Created attachment 821528 [details] [diff] [review]
Test reflect support for export declarations
Attachment #821528 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Attachment #821526 - Attachment is patch: true
Attachment #821526 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Updated

4 years ago
Summary: Implement export declaration → Implement export declarations
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.
Attachment #821525 - Flags: review?(jorendorff) → review+
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 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...
Attachment #821526 - Flags: review?(jorendorff) → review+
Comment on attachment 821528 [details] [diff] [review]
Test reflect support for export declarations

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

Great.
Attachment #821528 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/01555404ca91
https://hg.mozilla.org/mozilla-central/rev/01555404ca91
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Flags: in-testsuite+
Whiteboard: [qa-]
Eddy: does this change for module export declarations warrant a Firefox 28 release note or MDN dev docs?
Flags: needinfo?(ejpbruel)
OS: Mac OS X → All
Hardware: x86 → All
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.
(Assignee)

Comment 13

4 years ago
(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 :-)
Flags: needinfo?(ejpbruel)
Keywords: feature

Updated

3 years ago
Depends on: 1105608
You need to log in before you can comment on or make changes to this bug.