Closed Bug 1339963 Opened 7 years ago Closed 7 years ago

export {...} without "from" should check names against Reserved Words

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

Details

Attachments

(3 files)

https://tc39.github.io/ecma262/#sec-exports-static-semantics-early-errors

code example for error message:
  parseModule("export { static };");
  parseModule("export { static as a };");

actual result:
  throws "SyntaxError: local binding for export 'static' not found"

expected result:
  throws "SyntaxError: static is a reserved identifier:"


code example for timing:
  parseModule("export { static }; @");
  parseModule("export { static as a }; @");

actual result:
  throws SyntaxError at "@"

expected result:
  throws SyntaxError at "static"
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Almost just a copy/paste, to split each part into own method,
with additional assertion for current token.

one thing changed is that "bool isConst" -> "DeclarationKind kind".
Attachment #8837852 - Flags: review?(jwalden+bmo)
forgot to use MUST_MATCH_TOKEN for contextual keyword tokens.
Attachment #8837853 - Flags: review?(jwalden+bmo)
this is the actual fix for the export part touched in bug 1336783.
it should check name validity only when there's no "from".
Attachment #8837854 - Flags: review?(jwalden+bmo)
Comment on attachment 8837852 [details] [diff] [review]
Part 1: Split Parser::exportDeclaration.

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

::: js/src/frontend/Parser.cpp
@@ +4976,5 @@
> +template<>
> +SyntaxParseHandler::Node
> +Parser<SyntaxParseHandler>::exportFrom(uint32_t begin, Node specList)
> +{
> +    JS_ALWAYS_FALSE(abortIfSyntaxParser());

Use MOZ_ALWAYS_FALSE throughout.  (We need to remove the JS_* version, but it's never been high priority.)

I'd prefer if all these just started with

  if (!abortIfSyntaxParser())
     return null();

rather than have function specialization that's hairier to work with.  (Or have a ParseHandler function do this internally -- then no need to even check in Parser.cpp.)  Compiler should optimize out the rest of the function body in the syntax case, and the clarity of having only one definition (that the compiler expands semi-invisibly) is a win.
Attachment #8837852 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8837853 [details] [diff] [review]
Part 2: Use MUST_MATCH_TOKEN for contextual keyword where it can be used.

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

Oh -- splitting export parsing into multiple functions is *so* overdue.  (And we probably should do likewise to import stuff eventually, if memory serves.)
Attachment #8837853 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8837854 [details] [diff] [review]
Part 3: Check IdentifierName in ExportClause without from.

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

::: js/src/frontend/Parser.cpp
@@ +5095,5 @@
> +        ParseNode* name = next->pn_left;
> +        MOZ_ASSERT(name->isKind(PNK_NAME));
> +
> +        RootedPropertyName ident(context, name->pn_atom->asPropertyName());
> +        if (!checkLocalExportName(ident, name->pn_pos.begin))

This one, I guess, will need the partial specialization, if it's necessarily getting node offsets.  (i.e. don't add Handler::getPositions.  :-) )

::: js/src/jit-test/tests/modules/export-declaration.js
@@ +428,5 @@
> +        ],
> +        lit("b"),
> +        false
> +    )
> +]).assert(parseAsModule("export { true as a } from 'b'"));

s/a/name/g for these identifiers, to read (IMO) slightly more clearly.
Attachment #8837854 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/58d90297f0bf6ae03a5e7ffa359b626c35099c1f
Bug 1339963 - Part 1: Split Parser::exportDeclaration. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/b27d839625c20ad1cb632eb450036ca5412a0efe
Bug 1339963 - Part 2: Use MUST_MATCH_TOKEN for contextual keyword where it can be used. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9cf4a61aec21800f6f59a3e1a32d685bb3871a
Bug 1339963 - Part 3: Check IdentifierName in ExportClause without from. r=jwalden
You need to log in before you can comment on or make changes to this bug.