Bug 172031 – [ESnext] Parsing Object Rest Destructuring isn't handle errors cases properly

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Caio Lima]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 310139 [details]
Patch

[Mark Lam]

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

[Caio Lima]

> 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.

[Mark Lam]

Comment on attachment 310139 [details]
Patch

r- since we're in agreement that this patch is incorrect.

[Mark Lam]

Also resolving as invalid since the premise that the bug is based on is incorrect.