Summary: | [ES6] Arrow function syntax. Arrow function should support the destructuring parameters. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | GSkachkov <gskachkov> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, gskachkov, keith_miller, mark.lam, msaboff, saam, ysuzuki | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 140855 | ||||||||||||
Attachments: |
|
Description
GSkachkov
2015-07-14 12:21:36 PDT
Created attachment 268657 [details]
Patch
Patch
Comment on attachment 268657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268657&action=review > Source/JavaScriptCore/parser/Parser.h:1080 > + else if (match(OPENPAREN)) { > + // Let's check if this destructuring paramters > + SavePoint saveArrowFunctionPoint = createSavePoint(); > + next(); // Consume OPENPAREN; > + auto pattern = tryParseDestructuringPatternExpression(context, AssignmentContext::DeclarationStatement); > + isArrowFunction = pattern && consume(CLOSEPAREN) && match(ARROWFUNCTION); > + > + restoreSavePoint(saveArrowFunctionPoint); > + } > + This code isn't correct. It won't parse: ``` (a, b, {c}, [d], {e, f}) => ...; ``` I suggest an alternate approach here: have this code check if we have the single parameter name with no parens case: ``` let x = a=>a; ``` and otherwise, we already know how to parse "(p1, p2, ...)" style parameter lists in another function in the parser. We should use that same code here. Either we use it directly as is (probably not possible), or we should abstract that code in such a way that this code can use it. That way, we don't have to places in the parser where we parse parameters. The reason we have this problem in the first place is that we have two places where we parse parameters, lets not make that mistake again. Created attachment 268861 [details]
Patch
Patch with fixed comments
Comment on attachment 268657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268657&action=review >> Source/JavaScriptCore/parser/Parser.h:1080 >> + > > This code isn't correct. > It won't parse: > ``` > (a, b, {c}, [d], {e, f}) => ...; > ``` > > I suggest an alternate approach here: > have this code check if we have the single parameter name with no parens case: > ``` > let x = a=>a; > ``` > and otherwise, we already know how to parse "(p1, p2, ...)" style parameter lists > in another function in the parser. We should use that same code here. Either we use > it directly as is (probably not possible), or we should abstract that code in such a way > that this code can use it. That way, we don't have to places in the parser where we parse parameters. > > The reason we have this problem in the first place is that we have two places where we parse > parameters, lets not make that mistake again. I've tried to use function that parse parameters - parseFormalParameters, however parseFormalParameters adds parsed parameters to current scope, so I had to make face scope to avoid errors. Could you please check if it acceptable? (In reply to comment #4) > Comment on attachment 268657 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268657&action=review > > >> Source/JavaScriptCore/parser/Parser.h:1080 > >> + > > > > This code isn't correct. > > It won't parse: > > ``` > > (a, b, {c}, [d], {e, f}) => ...; > > ``` > > > > I suggest an alternate approach here: > > have this code check if we have the single parameter name with no parens case: > > ``` > > let x = a=>a; > > ``` > > and otherwise, we already know how to parse "(p1, p2, ...)" style parameter lists > > in another function in the parser. We should use that same code here. Either we use > > it directly as is (probably not possible), or we should abstract that code in such a way > > that this code can use it. That way, we don't have to places in the parser where we parse parameters. > > > > The reason we have this problem in the first place is that we have two places where we parse > > parameters, lets not make that mistake again. > > I've tried to use function that parse parameters - parseFormalParameters, > however parseFormalParameters adds parsed parameters to current scope, so I > had to make face scope to avoid errors. > Could you please check if it acceptable? I think this approach is okay. It's unfortunate we have to create a scope here, but this is probably the most reliable way to future-proof this code. Comment on attachment 268861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268861&action=review this seems like the right approach, just a few comments to clean up the code. > Source/JavaScriptCore/parser/Parser.h:1047 > + ALWAYS_INLINE bool isArrowFunctionNoOrOneParamter() > { > bool isArrowFunction = false; > > - if (match(EOFTOK)) > + if (match(EOFTOK) || (!match(IDENT) && !match(OPENPAREN))) > return isArrowFunction; > > SavePoint saveArrowFunctionPoint = createSavePoint(); > + if (match(IDENT)) > + isArrowFunction = consume(IDENT) && match(ARROWFUNCTION); > + else if (consume(OPENPAREN) && consume(CLOSEPAREN)) > + isArrowFunction = match(ARROWFUNCTION); > > - if (consume(OPENPAREN)) { > - bool isArrowFunctionParamters = true; > + restoreSavePoint(saveArrowFunctionPoint); > + > + return isArrowFunction; > + } This function has some redundancy with the below function, lets just combine the necessary bits into the below function. > Source/JavaScriptCore/parser/Parser.h:1049 > + template <class TreeBuilder> bool isArrowFunctionParamters(TreeBuilder& context) We don't need to templatize this once we move it to SyntaxChecker (see below). Also, this function has been misspelled, lets fix the spelling in this patch: "isArrowFunctionParamters" => "isArrowFunctionParameters" > Source/JavaScriptCore/parser/Parser.h:1057 > + if (isArrowFunctionNoOrOneParamter()) > + isArrowFunction = true; Instead of calling the above function, you can just check: ``` if (match(IDENT)) { save point create; next(); check if arrow function save point restore; } else if .... ``` > Source/JavaScriptCore/parser/Parser.h:1066 > + unsigned parametersCount = 1; start this at zero. Even though we ignore this number, it should start at zero. > Source/JavaScriptCore/parser/Parser.h:1067 > + isArrowFunction = parseFormalParameters(context, context.createFormalParameterList(), parametersCount) && consume(CLOSEPAREN) && match(ARROWFUNCTION); Instead of passing in "context" here, we should create a SyntaxChecker and pass that in. That way we're not actually allocating AST nodes that we throw away. > LayoutTests/js/script-tests/arrowfunction-syntax.js:79 > add a test or a few tests for rest parameters: ``` (...rest) => rest; ``` etc. Created attachment 268951 [details]
Patch
Patch with fixed comments
Comment on attachment 268861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268861&action=review >> Source/JavaScriptCore/parser/Parser.h:1047 >> + } > > This function has some redundancy with the below function, lets just combine > the necessary bits into the below function. Merged into one function >> Source/JavaScriptCore/parser/Parser.h:1049 >> + template <class TreeBuilder> bool isArrowFunctionParamters(TreeBuilder& context) > > We don't need to templatize this once we move it to SyntaxChecker (see below). > Also, this function has been misspelled, lets fix the spelling in this patch: > "isArrowFunctionParamters" => "isArrowFunctionParameters" Fixed >> Source/JavaScriptCore/parser/Parser.h:1057 >> + isArrowFunction = true; > > Instead of calling the above function, you > can just check: > ``` > if (match(IDENT)) { > save point create; > next(); > check if arrow function > save point restore; > } else if .... > ``` Refactored >> Source/JavaScriptCore/parser/Parser.h:1067 >> + isArrowFunction = parseFormalParameters(context, context.createFormalParameterList(), parametersCount) && consume(CLOSEPAREN) && match(ARROWFUNCTION); > > Instead of passing in "context" here, we should create a SyntaxChecker and pass that in. > That way we're not actually allocating AST nodes that we throw away. Done Comment on attachment 268951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268951&action=review > Source/JavaScriptCore/parser/Parser.cpp:377 > +bool Parser<LexerType>::isArrowFunctionParameters() This looks good to me, just some really small nits on how to write the function: > Source/JavaScriptCore/parser/Parser.cpp:382 > + > + if (match(EOFTOK) || (!match(OPENPAREN) && !match(IDENT))) > + return isArrowFunction; I would write this like: ``` if (match(EOFTOK)) return false; bool isOpenParen = match(OPENPAREN); bool isIdent = match(IDENT); if (!isOpenParen && !isIdent) return false; ``` Then, below, you can use these booleans instead of performing the match(...) twice. I say this because I think that LLVM will CSE this, but lets take matters into our own hand. > Source/JavaScriptCore/parser/Parser.cpp:386 > + if (match(IDENT)) { Then this could be: "if (isIdent) { next(); isArrowFunction = match(ARROWFUNCTION) } > Source/JavaScriptCore/parser/Parser.cpp:388 > + } else if (consume(OPENPAREN)) { Then I would change this to a pure else clause. Then, I would have the first two lines be: RELEASE_ASSERT(isOpenParen); // your check at the top of the function guarantees it, so lets not pretend it isn't true. next(); > Source/JavaScriptCore/parser/Parser.cpp:390 > + isArrowFunction = consume(CLOSEPAREN) && match(ARROWFUNCTION); I would write this as: if { next(); isArrowFunction = match(ARROWFUNCTION); } > Source/JavaScriptCore/parser/Parser.cpp:393 > + // We make fake scope, otherwise parseFormalParameters will add variable to current scope that lead to errors style: indentation. Created attachment 269172 [details]
Patch
Patch
Comment on attachment 268951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268951&action=review >> Source/JavaScriptCore/parser/Parser.cpp:382 >> + return isArrowFunction; > > I would write this like: > ``` > if (match(EOFTOK)) > return false; > bool isOpenParen = match(OPENPAREN); > bool isIdent = match(IDENT); > if (!isOpenParen && !isIdent) > return false; > ``` > Then, below, you can use these booleans instead of performing the match(...) twice. > I say this because I think that LLVM will CSE this, but lets take matters into our own hand. This condition is refactored Comment on attachment 269172 [details] Patch Clearing flags on attachment: 269172 Committed r195178: <http://trac.webkit.org/changeset/195178> All reviewed patches have been landed. Closing bug. |