Bug 146621 – JSC's parser should follow the ES6 spec with respect to parsing Declarations

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

There are probably a few places where JSC messes up. Notably:

if (b)
    class C { . }

Should not be allowed. 
Parsing a class is not part of a Statement, rather, it is part of a StatementListItem.
StatementListItem = Statement || Declaration
Here is the relevant grammar:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-statements

There are probably other places where the parser messes up.

[Saam Barati]

Created attachment 256190 [details]
patch

[Saam Barati]

Created attachment 256192 [details]
perf results

Seems okay. I ran them a few times.

[Saam Barati]

Created attachment 256193 [details]
patch

fix a test result.

[Saam Barati]

Created attachment 256224 [details]
patch

Check build.

[Mark Lam]

Comment on attachment 256224 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:20
> +        There were a few locations where JSC would allow declaration statements
> +        in incorrect ways. JSC didn't distinguish between 'Statement' and
> +        'StatementListItem' grammar productions. The relevant grammar is here:
> +        https://people.mozilla.org/~jorendorff/es6-draft.html#sec-statements
> +
> +        The following style of declarations are no longer allowed:
> +        'if (condition) const x = 40;'
> +        'if (condition) class C { }'
> +        Instead, we mandate such declaration constructs are within a StatementList 
> +        (which is the production that JSC's Parser::parseSourceElements function
> +        parses):
> +        'if (condition) { const x = 40; }'
> +        'if (condition) { class C { } }'

I think you can make this a bit more explicit like this:

From the ECMA Script 6.0 spec:
1. Section 13.6 The if Statement (http://www.ecma-international.org/ecma-262/6.0/index.html#sec-if-statement) says that IfStatements only takes Statements for the "then-else" clauses, not StatementListItems.
2. Section 13 ECMAScript Language: Statements and Declarations (http://www.ecma-international.org/ecma-262/6.0/index.html#sec-ecmascript-language-statements-and-declarations) defines the syntax of Statements, and they do not include ClassDeclarations and LexicalDeclarations (const, let, see 13.3.1 Let and Const Declarations).  Declarations can only be in the “then-else” clauses when embedded in a StatementListItem in a BlockStatement (see 13.2).

Hence, the following style of declarations are no longer allowed:
    'if (condition) const x = 40;'
    'if (condition) class C { }'

Instead, we mandate such declaration constructs are within a StatementList (which is the production that JSC's Parser::parseSourceElements function parses):
    'if (condition) { const x = 40; }'
    'if (condition) { class C { } }'

What do you think?

> Source/JavaScriptCore/parser/Parser.cpp:409
> +    // https://people.mozilla.org/~jorendorff/es6-draft.html#sec-statements

Please use a url from the official spec.

> Source/JavaScriptCore/parser/Parser.cpp:419
> +    case CONSTTOKEN:
> +        result = parseConstDeclaration(context);
> +        break;
> +#if ENABLE(ES6_CLASS_SYNTAX)
> +    case CLASSTOKEN:
> +        result = parseClassDeclaration(context);
> +        break;
> +#endif

What about other declarations e.g. let?

> LayoutTests/js/parser-syntax-check-expected.txt:-331
> -PASS Valid:   "if (a) var a,b; else const b, c"
> -PASS Valid:   "function f() { if (a) var a,b; else const b, c }"

Should we retain these cases as well with the expectation that they produce an “Invalid” result?

[Saam Barati]

Comment on attachment 256224 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:20
>> +        'if (condition) { class C { } }'
> 
> I think you can make this a bit more explicit like this:
> 
> From the ECMA Script 6.0 spec:
> 1. Section 13.6 The if Statement (http://www.ecma-international.org/ecma-262/6.0/index.html#sec-if-statement) says that IfStatements only takes Statements for the "then-else" clauses, not StatementListItems.
> 2. Section 13 ECMAScript Language: Statements and Declarations (http://www.ecma-international.org/ecma-262/6.0/index.html#sec-ecmascript-language-statements-and-declarations) defines the syntax of Statements, and they do not include ClassDeclarations and LexicalDeclarations (const, let, see 13.3.1 Let and Const Declarations).  Declarations can only be in the “then-else” clauses when embedded in a StatementListItem in a BlockStatement (see 13.2).
> 
> Hence, the following style of declarations are no longer allowed:
>     'if (condition) const x = 40;'
>     'if (condition) class C { }'
> 
> Instead, we mandate such declaration constructs are within a StatementList (which is the production that JSC's Parser::parseSourceElements function parses):
>     'if (condition) { const x = 40; }'
>     'if (condition) { class C { } }'
> 
> What do you think?

Yes, this is nicer. Thanks.

>> Source/JavaScriptCore/parser/Parser.cpp:409
>> +    // https://people.mozilla.org/~jorendorff/es6-draft.html#sec-statements
> 
> Please use a url from the official spec.

Will do.

>> Source/JavaScriptCore/parser/Parser.cpp:419
>> +#endif
> 
> What about other declarations e.g. let?

This will be handled in:
https://bugs.webkit.org/show_bug.cgi?id=142944

>> LayoutTests/js/parser-syntax-check-expected.txt:-331
>> -PASS Valid:   "function f() { if (a) var a,b; else const b, c }"
> 
> Should we retain these cases as well with the expectation that they produce an “Invalid” result?

No, I don't think so. The new test I wrote will make sure that this produces parsing errors.

[Mark Lam]

Comment on attachment 256224 [details]
patch

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

r=me

>>> Source/JavaScriptCore/parser/Parser.cpp:419
>>> +#endif
>> 
>> What about other declarations e.g. let?
> 
> This will be handled in:
> https://bugs.webkit.org/show_bug.cgi?id=142944

OK, can you add a FIXME here just so we don’t forget.  The FIXME can be removed in 142944.

>>> LayoutTests/js/parser-syntax-check-expected.txt:-331
>>> -PASS Valid:   "function f() { if (a) var a,b; else const b, c }"
>> 
>> Should we retain these cases as well with the expectation that they produce an “Invalid” result?
> 
> No, I don't think so. The new test I wrote will make sure that this produces parsing errors.

ok.

[Saam Barati]

Created attachment 256239 [details]
patch for landing

[Saam Barati]

Created attachment 256241 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 256241 [details]
patch for landing

Clearing flags on attachment: 256241

Committed r186379: <http://trac.webkit.org/changeset/186379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.