Closed
Bug 930411
Opened 11 years ago
Closed 11 years ago
Implement export declarations
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
(Blocks 1 open bug)
Details
(Keywords: feature, Whiteboard: [qa-])
Attachments
(3 files)
15.28 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
8.70 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
6.17 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #821525 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #821526 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #821528 -
Flags: review?(jorendorff)
Assignee | ||
Updated•11 years ago
|
Attachment #821526 -
Attachment is patch: true
Attachment #821526 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Updated•11 years ago
|
Summary: Implement export declaration → Implement export declarations
Comment 4•11 years ago
|
||
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+
Comment 5•11 years ago
|
||
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•11 years ago
|
||
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 7•11 years ago
|
||
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•11 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•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Flags: in-testsuite+
Whiteboard: [qa-]
Comment 11•11 years ago
|
||
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
Comment 12•11 years ago
|
||
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•11 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 :-)
You need to log in
before you can comment on or make changes to this bug.
Description
•