Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible parsing error with destructuring assignment #23142

Closed
ArnauldChevallier opened this issue Sep 28, 2018 · 9 comments
Closed

Possible parsing error with destructuring assignment #23142

ArnauldChevallier opened this issue Sep 28, 2018 · 9 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@ArnauldChevallier
Copy link

ArnauldChevallier commented Sep 28, 2018

  • Version: 10.9.0
  • Platform 1: Fedora 28 (Cloud Edition) x86_64
  • Platform 2: Windows 10 x86_64
  • Subsystem: n/a

Summary

Passing an arrow function and a destructuring assignment as two consecutive parameters of a function causes a syntax error.

Description

The following piece of code does not make much sense (and putting the initialization of [x, y] in the 2nd parameter of map() is definitely not a good idea), but I'd expect it to work anyway:

[[3, 4]].map(([X, Y]) => [X + x, Y + y], [x, y] = [2, 1])

In fact, this does work on both SpiderMonkey and Chakra which are returning [[5, 5]].

In Node (and Chrome), however, we get the following error:

SyntaxError: Invalid destructuring assignment target

Worst yet, with the following code:

[].map(_ => _, [x, y] = [2, 1])

We now get the following error:

SyntaxError: Unexpected token =>

This message is inconsistent, and that's why I suspect some kind of parsing error here.

Some more examples

This works:

foo = a => a;
foo(0, [x, y] = [2, 1]);

This also works:

foo = a => a;
foo(a => a, x = [2, 1]);

But this fails with "SyntaxError: Unexpected token =>":

foo = a => a;
foo(a => a, [x, y] = [2, 1]);
@mscdex
Copy link
Contributor

mscdex commented Sep 28, 2018

This is a language-level issue and not node-specific.

The problem is that without braces, there is some ambiguity, since if you add them it is parsed just fine. The parser doesn't know whether the a => a is the entirety of the function and [x, y] = ... is a second argument being passed to foo or if it's a continuation of the a => a, ... function argument. By adding braces you are breaking that ambiguity: foo(a => {a, [x, y] = [2, 1]});.

FWIW this is why I personally always use braces (and semicolons to avoid that class of issues) to avoid such tricky behavior with the language.

@mscdex mscdex closed this as completed Sep 28, 2018
@Hakerh400
Copy link
Contributor

Minimal repro: a(a=>a,[]=a)
This seems like a spec-violation to me.
Since the same behavior is seen in Chrome, if it is a bug, it is a v8 bug and has nothing to do with Node.js

@ArnauldChevallier
Can you please report it to https://bugs.chromium.org/p/v8/issues/list

@mscdex mscdex reopened this Sep 28, 2018
@mscdex
Copy link
Contributor

mscdex commented Sep 28, 2018

It does appear that Firefox does not have this issue.

/cc @nodejs/v8 ?

@hashseed
Copy link
Member

@gsathya

@ArnauldChevallier
Copy link
Author

@mscdex The intended behavior is that [x, y] = [2, 1] is parsed as a second argument for foo. We can force a correct parsing by adding parentheses: foo(a => a, ([x, y] = [2, 1])) (or foo(a => { return a }, ([x, y] = [2, 1])) with braces). Doing assignments within passed parameters can be considered a dubious practice, but I don't think there's anything wrong with this syntax with strict regards to the spec.

Now, this does indeed look like a V8 issue rather than a Node.js one.

@gsathya
Copy link
Member

gsathya commented Sep 28, 2018

Filed https://bugs.chromium.org/p/v8/issues/detail?id=8241 to track this

@Hakerh400
Copy link
Contributor

Seems to be fixed in v8/v8@cd21f71

@Trott Trott added the v8 engine Issues and PRs related to the V8 dependency. label Nov 24, 2018
@Trott
Copy link
Member

Trott commented Nov 24, 2018

Should we float v8/v8@cd21f71? @nodejs/v8-update

@targos targos added the v10.x label Jun 13, 2020
targos added a commit to targos/node that referenced this issue Jun 13, 2020
Original commit message:

    [parser] Validate destructuring assignment pattern in correct classifier

    Previously we'd first accumulate errors to the parent and validate the
    destructuring pattern in the parent. In the case of ParseArguments this
    will invalidly propagate binding pattern errors from one argument to the
    next. The reason why ParseArguments keeps track of binding pattern errors
    is because it could also be used to parse async arrow function parameters.
    If we see async(a,b) we don't yet know whether this is the head of an
    async arrow function, or a call to async with arguments a and b.

    Bug: v8:8241
    Change-Id: I670ab9a9c6f2e0bee399808b02a465ae1afa7c3f
    Reviewed-on: https://chromium-review.googlesource.com/c/1296229
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#56887}

Refs: v8/v8@cd21f71
Fixes: nodejs#23142
richardlau pushed a commit that referenced this issue Jul 1, 2020
Original commit message:

    [parser] Validate destructuring assignment pattern in correct classifier

    Previously we'd first accumulate errors to the parent and validate the
    destructuring pattern in the parent. In the case of ParseArguments this
    will invalidly propagate binding pattern errors from one argument to the
    next. The reason why ParseArguments keeps track of binding pattern errors
    is because it could also be used to parse async arrow function parameters.
    If we see async(a,b) we don't yet know whether this is the head of an
    async arrow function, or a call to async with arguments a and b.

    Bug: v8:8241
    Change-Id: I670ab9a9c6f2e0bee399808b02a465ae1afa7c3f
    Reviewed-on: https://chromium-review.googlesource.com/c/1296229
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#56887}

Refs: v8/v8@cd21f71
Fixes: #23142

PR-URL: #33862
Reviewed-By: Richard Lau <[email protected]>
@richardlau
Copy link
Member

Fixed in v10.22.0 by #33862.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

8 participants