1099956 - Regular expression after yield is not parsed correctly.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

Sorry, I was wrong, nextTokenEndsExpr in line 6149 is not called,
maybe caused by another reason.

Comment 2

[Tooru Fujisawa [:arai]]

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

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Parse regular expression after yield correctly.

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.

Comment 4

[Tooru Fujisawa [:arai]]

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.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Parse regular expression after yield correctly.

Support yield expression and yield as name.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=740f1737f725

Comment 6

[Jeff Walden [:Waldo]]

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);

Comment 7

[Tooru Fujisawa [:arai]]

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.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c910d562c7c3

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c910d562c7c3

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Tooru Fujisawa [:arai]]

Thank you for the fix and letting me know that :)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/919f14472465