As Saam pointed in https://bugs.webkit.org/show_bug.cgi?id=167962, it seems we aren't parsing Object Rest Destructuring properly. We need to consider other DestructuringKind as well. I'm coming with a test case for that bug soon.
After code analysis I realized that when destructuring is being parsed with rest element, i.e, we find a "...", then we call parseBindingOrAssignmentElement (https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L985) and then check if innerPattern is null. Looking into parseBindingOrAssignmentElement (https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L903) we have 2 cases: 1) kind == DestructuringKind::DestructureToExpressions In that case, we goes to parseAssignmentElement with can return a null inner pattern. 2) kind != DestructuringKind::DestructureToExpressions In that case, the parser falls to parseDestructuringPattern and it only return 0 if kind == DestructuringKind::DestructureToExpressions with will never be the case in that. It means that this check in https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L986 has redundant kind check, since innerPattern only can be true if and only if kind == DestructuringKind::DestructureToExpressions. I'm going to send a Patch ro remove that, because the code right now looks buggy with kind check.
Created attachment 310139 [details] Patch
Comment on attachment 310139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310139&action=review > Source/JavaScriptCore/ChangeLog:17 > + This patch is removing parseDestructuringPattern "kind" check when > + parsing rest destructuring (i.e when it proccess "..." token) > + and parseBindingOrAssignmentElement returns 0. This checks is > + redundant, because parseBindingOrAssignmentElement only can return > + null when "kind" is DestructuringKind::DestructureToExpressions. We > + can verify that looking at parseBindingOrAssignmentElement that falls > + to parseAssignmentElement when "kind == DestructuringKind::DestructureToExpressions" > + and to parseDestructuringPattern otherwise. In > + parseDestructuringPattern we can check that it onlys return 0 if > + kind == DestructuringKind::DestructureToExpressions. I'm not sure that this is true. 1. If kind != DestructuringKind::DestructureToExpressions, parseBindingOrAssignmentElement() returns the result of parseDestructuringPattern(). 2. In parseDestructuringPattern(), a. the OPENBRACKET case has a consumeOrFail that can return 0. b. the OPENBRACE case has failIfTrue, failIfTrueIfStrict, semanticFailIfTrue, etc. that can return 0. These are just a few examples. How did you prove that all of these (and other similar failout macros) are not also returning if kind != DestructuringKind::DestructureToExpressions
> I'm not sure that this is true. > 1. If kind != DestructuringKind::DestructureToExpressions, > parseBindingOrAssignmentElement() returns the result of > parseDestructuringPattern(). > 2. In parseDestructuringPattern(), > a. the OPENBRACKET case has a consumeOrFail that can return 0. > b. the OPENBRACE case has failIfTrue, failIfTrueIfStrict, > semanticFailIfTrue, etc. that can return 0. > > These are just a few examples. How did you prove that all of these (and > other similar failout macros) are not also returning if kind != > DestructuringKind::DestructureToExpressions Oops, you are right about 2. I'm not considering the failout macros, my bad here. I was just considering the plain returns 0 in parseDestructuringPattern. But when they fail, it's going to cascade the syntax error to its client, right? So the check I've removed is still valid in that case, or Am I missing something? The problem I'm trying to solve is to return 0 if interPattern is 0, regardless the destructuring kind. Making it strictly to DestructuringToExpressions doesn't see right here.
Comment on attachment 310139 [details] Patch r- since we're in agreement that this patch is incorrect.
Also resolving as invalid since the premise that the bug is based on is incorrect.