Bug 158460 – Clarify SyntaxErrors around yield and unskip tests

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

Clarify SyntaxErrors around yield and unskip tests.

  - Source/JavaScriptCore/tests/stress/generator-syntax.js
  - Source/JavaScriptCore/tests/stress/yield-out-of-generator.js

These tests currently fail.

They seem to expect syntax errors for "yield" used outside of generators, but in actuality we are allowing it and in some cases "yield" is treated as a variable name.

It seems the test should be clarified / cleaned up, and maybe there is a legit issue.

[Caitlin Potter (:caitp)]

Incorrect tests:

In Source/JavaScriptCore/tests/stress/generator-syntax.js

```
testSyntaxError(`
var value = () => {
    yield
}
`, `SyntaxError: Unexpected keyword 'yield'. Cannot use yield expression out of generator.`);
```

"yield" is always a BindingIdentifier in a ConciseBody production, not a YieldExpression (https://tc39.github.io/ecma262/#prod-ConciseBody)

```
testSyntaxError(`
var value = (val = yield) => {
}
`, `SyntaxError: Unexpected keyword 'yield'. Cannot use yield expression out of generator.`);
```

"yield" in these arrow formal parameters is a BindingIdentifier, not a YieldExpression, due to the arrow function not being defined within a Generator (thus, with the [~Yield] parameterization)

```
testSyntaxError(`
function *gen() {
    function ng(val = yield) {
    }
}
`, `SyntaxError: Unexpected keyword 'yield'. Cannot use yield expression out of generator.`);
```

Per https://tc39.github.io/ecma262/#prod-FunctionDeclaration, the "yield" in the formal parameters initializer is parsed with [~Yield] production, and is not a YieldExpression, totally legal.

```
testSyntaxError(`
function gen(val = yield) {
}
`, `SyntaxError: Unexpected keyword 'yield'. Cannot use yield expression out of generator.`);
```

Again, [~Yield] production parameterization here

---

In Source/JavaScriptCore/tests/stress/yield-out-of-generator.js

Most of these tests are invalid, but many of  them are only invalid for the error reported. A lot of these should be "Unexpected token `;`" errors (when trying to parse the RHS of a MultiplicativeExpression, or "unexpected whatever" for yields that don't look like delegating yields.

The tests starting at line 97 in the file all appear valid, but the other ones need some fixups.

[Caitlin Potter (:caitp)]

Created attachment 280712 [details]
Patch

[Caitlin Potter (:caitp)]

Starter patch --- I have some other work to do today, but someone is welcome to take it over if they're interested.

I did a quick Octane run to see how bad the changes affected them, results are sort of inconclusive:

```
Benchmark report for Octane on EchoBeach (MacBookPro11,5).

VMs tested:
"Baseline" at /Users/caitp/git/WebKit/WebKitBuild/Baseline/jsc
"Fixed" at /Users/caitp/git/WebKit/WebKitBuild/Release/jsc

Collected 500 samples per benchmark/VM, with 50 VM invocations per benchmark. Emitted a call to
gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used
the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark
execution times with 95% confidence intervals in milliseconds.

                            Baseline                   Fixed                                       

gbemu          x2      20.33802+-0.27565         19.96352+-0.27174         might be 1.0188x faster
closure                 0.50537+-0.00378          0.50449+-0.00395       
jquery                  6.87450+-0.04099          6.85369+-0.07138       
typescript     x2     354.38279+-8.42951    ?   359.40035+-8.18052       ? might be 1.0142x slower

<geometric>            23.61085+-0.26073         23.55593+-0.25368         might be 1.0023x faster
```

I don't know if I really trust any of these results to be particularly meaningful, which means there _probably_ isn't a statistically significant impact on those Octane tests.

But, obviously that's only a small subset of benchmarks

[Caitlin Potter (:caitp)]

Created attachment 280721 [details]
Patch

[Saam Barati]

Comment on attachment 280721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280721&action=review

> Source/JavaScriptCore/ChangeLog:7
> +

Please add information here.

> Source/JavaScriptCore/parser/Parser.cpp:2030
> +
> +        if (upperScopeIsGenerator)
> +            functionScope->setMasqueradeArrowFunctionAsGenerator();
> +
>          parseFunctionParameters(syntaxChecker, mode, functionInfo);
>          propagateError();
>  
> +        if (upperScopeIsGenerator)
> +            functionScope->unsetMasqueradeArrowFunctionAsGenerator();
> +

Is this really needed?
If so, it seems potentially incorrect because it parseFunctionParameters can be called recursively. Maybe it should be an RAII using SetForScope or something.

> Source/JavaScriptCore/parser/Parser.cpp:3171
> +    failIfFalse(currentScope()->isGenerator() && !currentScope()->isArrowFunctionBoundary(), "Cannot use yield expression out of generator");

I'm confused about what this is solving.
Can you elaborate on the type of program?

[Caitlin Potter (:caitp)]

Comment on attachment 280721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280721&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:2030
>> +
> 
> Is this really needed?
> If so, it seems potentially incorrect because it parseFunctionParameters can be called recursively. Maybe it should be an RAII using SetForScope or something.

Could be --- Like I said earlier, I think someone else should take over this patch, since I have a full plate of stuff to do this week. The test changes should be good, the parser changes that are needed for it are up to you guys. Or I can get back to this one later on when I have some time.

>> Source/JavaScriptCore/parser/Parser.cpp:3171
>> +    failIfFalse(currentScope()->isGenerator() && !currentScope()->isArrowFunctionBoundary(), "Cannot use yield expression out of generator");
> 
> I'm confused about what this is solving.
> Can you elaborate on the type of program?

As an example,

```
var yield = 1;
function* upperGenerator()
{
    var f = (index = yield) => index;
    yield f();
}

[...upperGenerator()];
```

In WebKit trunk, this code would print produce `[1]`.
however, per spec, ArrowFormalParameters inside of a generator body, are parsed with the [Yield] parameterization (https://tc39.github.io/ecma262/#prod-ArrowFormalParameters), so some extra logic is needed to report an error.

The FunctionParsePhase can't be relied on here as it's set to `Body` when parsing arrow formals for the last time (of course this could be changed, but this error message seems more "correct", doesn't matter a whole lot to me though) --- so instead you rely on this extra test, where the two of them are only ever set together for arrow function within a generator.

[Saam Barati]

Caitlin, do you think you can rebase this patch?

[Caitlin Potter (:caitp)]

Created attachment 285223 [details]
Patch

[Caitlin Potter (:caitp)]

(In reply to comment #8)
> Created attachment 285223 [details]
> Patch

I've rebased, and am in the process of implementing the suggestion from c5

[Caitlin Potter (:caitp)]

Created attachment 285226 [details]
Patch

[Saam Barati]

Comment on attachment 285226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285226&action=review

LGTM, just a few comments.

> Source/JavaScriptCore/parser/Parser.cpp:2038
> +            Scope::MaybeParseAsGeneratorForScope parseAsGenerator(functionScope, upperScopeIsGenerator);

Nit: I actually think this would be easier to read if you just used SetForScope directly here.

> Source/JavaScriptCore/parser/Parser.cpp:3187
> +    failIfFalse(currentScope()->isGenerator() && !currentScope()->isArrowFunctionBoundary(), "Cannot use yield expression out of generator");

Why does Scope::setIsArrowFunction() not set m_isGenerator to false?
Is there a reason for this I'm not seeing? If so, would that allow us to remove the SetForScope above?

> Source/JavaScriptCore/parser/Parser.h:670
> +    class MaybeParseAsGeneratorForScope;

Style: We tend to predeclare classes at the top of the file.

> JSTests/stress/generator-syntax.js:57
> +// Confusingly, FormalParameters[~Yield] does not product a SyntaxError for "yield", even
> +// inside of a generator (https://tc39.github.io/ecma262/#prod-FunctionDeclaration)

This is quite confusing indeed!
What does this even do? Resolve a variable named "yield"?
Maybe you can add a test that does that?

[Caitlin Potter (:caitp)]

Comment on attachment 285226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285226&action=review

I'll try to get the next patch up in a few minutes.

>> Source/JavaScriptCore/parser/Parser.cpp:2038
>> +            Scope::MaybeParseAsGeneratorForScope parseAsGenerator(functionScope, upperScopeIsGenerator);
> 
> Nit: I actually think this would be easier to read if you just used SetForScope directly here.

The caveat is making Scope fields public, or friending Parser<T>. Fine with me, but not always the preferred approach.

>> Source/JavaScriptCore/parser/Parser.cpp:3187
>> +    failIfFalse(currentScope()->isGenerator() && !currentScope()->isArrowFunctionBoundary(), "Cannot use yield expression out of generator");
> 
> Why does Scope::setIsArrowFunction() not set m_isGenerator to false?
> Is there a reason for this I'm not seeing? If so, would that allow us to remove the SetForScope above?

So here's the thing:

You parse arrow functions in two passes --- first you parse the arrow head, and after seeing =>, add a second pass as a proper arrow function.

In pass #1, YieldExpressions are legal in the spec, until you see the => and discover you're actually parsing formal parameters.

Previously, pass #2 would reparse the "yield" as a BindingIdentifier, which in the context of an arrow closure within a generator function, is invalid and should report a SyntaxError.

So, the change essentially parses the arrow function formal parameters with the [+Yield] parameterization (it is a parameterized production in the spec https://tc39.github.io/ecma262/#prod-ArrowFormalParameters), so that we are able to report the error if needed.

After the formal parameters, you go back to parsing as a non-generator, and "yield" within the ConciseBody refers to the BindingIdentifier "yield", thus we can't just propagate the "m_isGenerator" state inside arrow functions, it would propagate too much.

I hope that explains my reasoning for this approach

>> Source/JavaScriptCore/parser/Parser.h:670
>> +    class MaybeParseAsGeneratorForScope;
> 
> Style: We tend to predeclare classes at the top of the file.

I'm not sure it's possible to do that for nested classes in C++, is it? I admit I don't think I've ever tried.

>> JSTests/stress/generator-syntax.js:57
>> +// inside of a generator (https://tc39.github.io/ecma262/#prod-FunctionDeclaration)
> 
> This is quite confusing indeed!
> What does this even do? Resolve a variable named "yield"?
> Maybe you can add a test that does that?

a non syntax checking test, I presume? sure

[Saam Barati]

Comment on attachment 285226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285226&action=review

:

>>> Source/JavaScriptCore/parser/Parser.cpp:2038
>>> +            Scope::MaybeParseAsGeneratorForScope parseAsGenerator(functionScope, upperScopeIsGenerator);
>> 
>> Nit: I actually think this would be easier to read if you just used SetForScope directly here.
> 
> The caveat is making Scope fields public, or friending Parser<T>. Fine with me, but not always the preferred approach.

Right. I think it's OK to keep it as is.

>>> Source/JavaScriptCore/parser/Parser.cpp:3187
>>> +    failIfFalse(currentScope()->isGenerator() && !currentScope()->isArrowFunctionBoundary(), "Cannot use yield expression out of generator");
>> 
>> Why does Scope::setIsArrowFunction() not set m_isGenerator to false?
>> Is there a reason for this I'm not seeing? If so, would that allow us to remove the SetForScope above?
> 
> So here's the thing:
> 
> You parse arrow functions in two passes --- first you parse the arrow head, and after seeing =>, add a second pass as a proper arrow function.
> 
> In pass #1, YieldExpressions are legal in the spec, until you see the => and discover you're actually parsing formal parameters.
> 
> Previously, pass #2 would reparse the "yield" as a BindingIdentifier, which in the context of an arrow closure within a generator function, is invalid and should report a SyntaxError.
> 
> So, the change essentially parses the arrow function formal parameters with the [+Yield] parameterization (it is a parameterized production in the spec https://tc39.github.io/ecma262/#prod-ArrowFormalParameters), so that we are able to report the error if needed.
> 
> After the formal parameters, you go back to parsing as a non-generator, and "yield" within the ConciseBody refers to the BindingIdentifier "yield", thus we can't just propagate the "m_isGenerator" state inside arrow functions, it would propagate too much.
> 
> I hope that explains my reasoning for this approach

It does. I understand the problem now. When parsing formal parameters of an arrow function inside a generator we're in a weird hybrid state.

>>> Source/JavaScriptCore/parser/Parser.h:670
>>> +    class MaybeParseAsGeneratorForScope;
>> 
>> Style: We tend to predeclare classes at the top of the file.
> 
> I'm not sure it's possible to do that for nested classes in C++, is it? I admit I don't think I've ever tried.

I see. You're defining the class in Parser.cpp. Ignore my suggestion, it's wrong.

> Source/JavaScriptCore/parser/Parser.h:673
> +    friend class ParseAsGeneratorForScope;

Where is this declared? Maybe this should be removed?

[Caitlin Potter (:caitp)]

Created attachment 285273 [details]
Patch

Added a test and clarified comments around the surrounding new tests

[Caitlin Potter (:caitp)]

Created attachment 285276 [details]
Patch

Also delete the extra friend class in Parser.h

[Saam Barati]

Comment on attachment 285276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285276&action=review

r=me
Have you also run test262 tests to check if you've made new things pass?
You can do that by running:
./Tools/Scripts/run-jsc-stress-tests ./JSTests/test262.yaml

> Source/JavaScriptCore/parser/Parser.cpp:2037
> +            // Parse formal parameters with [+Yield] parameterization, if necessary

I think it's worth explaining a bit more for this comment to motivate why we need this.
For example, the example you described to me with how we parse arrow functions, and how
that conflicts with reparsing parameters when we finally realize we're parsing arrow function
parameters is worth writing here, instead of just saying we parse with the [+Yield] parameterization.

[Caitlin Potter (:caitp)]

It's possible that there are new Test262 passing tests, but I'm not sure how to run them, unless the habit of JSC devs is just to plug jsc into Brian Terlson's harness (which I haven't done yet).

Will expand on the comment a bit.

[Saam Barati]

(In reply to comment #17)
> It's possible that there are new Test262 passing tests, but I'm not sure how
> to run them, unless the habit of JSC devs is just to plug jsc into Brian
> Terlson's harness (which I haven't done yet).
> 
> Will expand on the comment a bit.

We have a version of test262 checked into webkit.
You can run it using:
./Tools/Scripts/run-jsc-stress-tests ./JSTests/test262.yaml

[Caitlin Potter (:caitp)]

Created attachment 285280 [details]
Patch

Add to comment in Parser.cpp, and update test262 expectations

[WebKit Commit Bot]

Comment on attachment 285280 [details]
Patch

Clearing flags on attachment: 285280

Committed r204111: <http://trac.webkit.org/changeset/204111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #20)
> Comment on attachment 285280 [details]
> Patch
> 
> Clearing flags on attachment: 285280
> 
> Committed r204111: <http://trac.webkit.org/changeset/204111>

stress/generator-syntax.js fails on all bots, see build.webkit.org for details.

[Caitlin Potter (:caitp)]

Strange that the file passes locally. Can you roll out the patchset? I'll send a fix later today.

[Caitlin Potter (:caitp)]

I was mistaken, they were not passing locally. I guess I changed them after verifying and figured it wouldn't break.

Test fixes are at https://bugs.webkit.org/show_bug.cgi?id=160550