Bug 146621 - JSC's parser should follow the ES6 spec with respect to parsing Declarations
Summary: JSC's parser should follow the ES6 spec with respect to parsing Declarations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks: 142944
  Show dependency treegraph
 
Reported: 2015-07-05 10:31 PDT by Saam Barati
Modified: 2015-07-06 15:20 PDT (History)
9 users (show)

See Also:


Attachments
patch (18.72 KB, patch)
2015-07-05 21:28 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
perf results (54.87 KB, text/plain)
2015-07-05 21:31 PDT, Saam Barati
no flags Details
patch (19.44 KB, patch)
2015-07-05 21:39 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (19.61 KB, patch)
2015-07-06 11:33 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (20.68 KB, patch)
2015-07-06 14:27 PDT, Saam Barati
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch for landing (20.69 KB, patch)
2015-07-06 14:29 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-07-05 10:31:22 PDT
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.
Comment 1 Saam Barati 2015-07-05 21:28:57 PDT
Created attachment 256190 [details]
patch
Comment 2 Saam Barati 2015-07-05 21:31:05 PDT
Created attachment 256192 [details]
perf results

Seems okay. I ran them a few times.
Comment 3 Saam Barati 2015-07-05 21:39:11 PDT
Created attachment 256193 [details]
patch

fix a test result.
Comment 4 Saam Barati 2015-07-06 11:33:18 PDT
Created attachment 256224 [details]
patch

Check build.
Comment 5 Mark Lam 2015-07-06 12:53:31 PDT
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?
Comment 6 Saam Barati 2015-07-06 12:58:18 PDT
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.
Comment 7 Mark Lam 2015-07-06 13:01:00 PDT
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.
Comment 8 Saam Barati 2015-07-06 14:27:44 PDT
Created attachment 256239 [details]
patch for landing
Comment 9 Saam Barati 2015-07-06 14:29:48 PDT
Created attachment 256241 [details]
patch for landing
Comment 10 WebKit Commit Bot 2015-07-06 15:20:02 PDT
Comment on attachment 256241 [details]
patch for landing

Clearing flags on attachment: 256241

Committed r186379: <http://trac.webkit.org/changeset/186379>
Comment 11 WebKit Commit Bot 2015-07-06 15:20:06 PDT
All reviewed patches have been landed.  Closing bug.