Summary: | JSC's parser should follow the ES6 spec with respect to parsing Declarations | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | basile_clement, commit-queue, fpizlo, ggaren, mark.lam, msaboff, oliver, rniwa, ysuzuki | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 142944 | ||||||||||||||||
Attachments: |
|
Description
Saam Barati
2015-07-05 10:31:22 PDT
Created attachment 256190 [details]
patch
Created attachment 256192 [details]
perf results
Seems okay. I ran them a few times.
Created attachment 256193 [details]
patch
fix a test result.
Created attachment 256224 [details]
patch
Check build.
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 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 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. Created attachment 256239 [details]
patch for landing
Created attachment 256241 [details]
patch for landing
Comment on attachment 256241 [details] patch for landing Clearing flags on attachment: 256241 Committed r186379: <http://trac.webkit.org/changeset/186379> All reviewed patches have been landed. Closing bug. |