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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
Details
Attachments
(3 files)
26.35 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
3.99 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
forgot to use MUST_MATCH_TOKEN for contextual keyword tokens.
Attachment #8837853 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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 5•7 years ago
|
||
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 6•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/58d90297f0bf https://hg.mozilla.org/mozilla-central/rev/b27d839625c2 https://hg.mozilla.org/mozilla-central/rev/7e9cf4a61aec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•