Closed
Bug 1099956
Opened 10 years ago
Closed 10 years ago
Regular expression after yield is not parsed correctly.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(1 file, 1 obsolete file)
2.47 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
js> function* f() { yield /a/g; } typein:1:22 SyntaxError: expected expression, got '/': typein:1:22 function* f() { yield /a/g; } typein:1:22 ......................^ in Parser<ParseHandler>::assignExpr(). http://hg.mozilla.org/mozilla-central/file/acbd7b68fa8c/js/src/frontend/Parser.cpp#l6149 nextTokenEndsExpr gets the token next to "yield" with TokenStream::None, so leading slash of regular expression is tokenized as TOK_DIV.
Assignee | ||
Comment 1•10 years ago
|
||
Sorry, I was wrong, nextTokenEndsExpr in line 6149 is not called, maybe caused by another reason.
Assignee | ||
Comment 2•10 years ago
|
||
This peekToken uses TokenStream::None for the token next to "yield" http://hg.mozilla.org/mozilla-central/file/acbd7b68fa8c/js/src/frontend/Parser.cpp#l5810
Assignee | ||
Comment 3•10 years ago
|
||
Added TokenStream::Operand modifier to peekToken which checks TOK_COLON after TOK_YIELD. This may still cause the assertion failure in bug 1089045, when consuming TOK_COLON in labeledStatement, but it should be addressed there. (currently consumeKnownToken does not take modifier parameter, and I'm going to add it in bug 1089045) Tested locally, jstests and jit-test are passed. I'll push this to tryserver when it opens.
Attachment #8523443 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8523443 [details] [diff] [review] Parse regular expression after yield correctly. Oops, sorry, I should call checkYieldNameValidity before peekToken, because "yield /a/g;" should be parsed as division if yield is valid name.
Attachment #8523443 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•10 years ago
|
||
Support yield expression and yield as name. Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=740f1737f725
Attachment #8523443 -
Attachment is obsolete: true
Attachment #8523486 -
Flags: review?(jwalden+bmo)
Comment 6•10 years ago
|
||
Comment on attachment 8523486 [details] [diff] [review] Parse regular expression after yield correctly. Review of attachment 8523486 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +5806,5 @@ > return expressionStatement(); > > case TOK_YIELD: { > TokenKind next; > + TokenStream::Modifier modifier = (versionNumber() >= JSVERSION_1_7 || pc->isGenerator()) ? TokenStream::Operand : TokenStream::None; So this condition is correct because it exactly duplicates the one in if (tt == TOK_YIELD && (versionNumber() >= JSVERSION_1_7 || pc->isGenerator())) in assignExpr(). Let's unify those into a single function so it's clearer what makes the modifier consistent with the rest of the parser -- Parser::yieldExpressionsSupported() maybe? The function's simple enough you can just add it to Parser.h, I think. ::: js/src/jit-test/tests/generators/yield-regexp.js @@ +1,5 @@ > +// Bug 1099956 > + > +load(libdir + "asserts.js"); > + > +function f1() { Add this comment above this, separated from the load() and function by blank lines either side. // ES6 treating yield as an identifier except in ES6 generators introduces a // syntax conflict with permissible JS >= 1.7 legacy generator syntax. Is // |yield /a/g| inside a function an attempt to convert the function into a // legacy generator, yielding a RegExp instance? Or does it instead read as // |(yield / a) / g|? Similar ambiguities exist for different textual content // in place of |a| -- |yield /x+17/g| or |(yield / x) + 17 / g|, and so on. // (And, much less importantly, is |yield /a/g| a syntax error in global code // as in JS >= 1.7, or is it |(yield / a) / g|.) // // For now, in JS >= 1.7, we preserve the old behavior. In all other JS we // conform to ES6: |yield /a/g| is a YieldExpression inside an ES6 generator, // and it's an IdentifierReference divided twice when not in an ES6 generator. // This test will need changes if we change our JS >= 1.7 parsing to be // ES6-compatible. ::: js/src/tests/ecma_6/Generators/yield-non-regexp.js @@ +1,1 @@ > +// |reftest| skip-if(xulRuntime.shell) I think you can remove this. It so happens that none of the shell.js that this test will load, modify the version from the default (0) set by js/src/tests/shell.js. And none of the browser.js modify it either. So it's okay to run this everywhere, as far as I can tell without testing this myself. @@ +8,5 @@ > + > +printBugNumber(BUGNUMBER); > +printStatus (summary); > + > +var yield = 12, a = 4, g = 2; Let's make a = 3 so that the divisions produce nice whole numbers, for slight readability. @@ +10,5 @@ > +printStatus (summary); > + > +var yield = 12, a = 4, g = 2; > + > +yield /a/g; Verify the expression does what's expected, too: var yieldParsedAsIdentifier = false; yield /a; yieldParsedAsIdentifier = true; a/g; assertEq(yieldParsedAsIdentifier, true);
Attachment #8523486 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Thank you! (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6) > ::: js/src/tests/ecma_6/Generators/yield-non-regexp.js > @@ +1,1 @@ > > +// |reftest| skip-if(xulRuntime.shell) > > I think you can remove this. It so happens that none of the shell.js that > this test will load, modify the version from the default (0) set by > js/src/tests/shell.js. And none of the browser.js modify it either. So > it's okay to run this everywhere, as far as I can tell without testing this > myself. It works, thanks :) > @@ +10,5 @@ > > +printStatus (summary); > > + > > +var yield = 12, a = 4, g = 2; > > + > > +yield /a/g; > > Verify the expression does what's expected, too: > > var yieldParsedAsIdentifier = false; > yield /a; yieldParsedAsIdentifier = true; a/g; > > assertEq(yieldParsedAsIdentifier, true); Cool, I've never thought that technique. Try run with fixed version is green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3cb3fe8f640c I'll land it shortly.
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c910d562c7c3
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c910d562c7c3
Assignee: nobody → arai_a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 10•10 years ago
|
||
Micro-style nitpick: instead of T v = cond ? truthy : falsy; JS uses T v = cond ? truthy : falsy; Corrected here: https://hg.mozilla.org/integration/mozilla-inbound/rev/919f14472465
Assignee | ||
Comment 11•10 years ago
|
||
Thank you for the fix and letting me know that :)
You need to log in
before you can comment on or make changes to this bug.
Description
•