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.

Sign up for GitHub

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

Jump to bottom

Destructuring Assignment Parse Error - works in Chrome & Safari but not Node #11480

Closed
shanebdavis opened this issue Feb 21, 2017 · 8 comments
Closed

Destructuring Assignment Parse Error - works in Chrome & Safari but not Node #11480

shanebdavis opened this issue Feb 21, 2017 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@shanebdavis
Copy link

  • Version: v6.9.5
  • Platform: Darwin Honor.local 16.4.0 Darwin Kernel Version 16.4.0: Thu Dec 22 22:53:21 PST 2016; root:xnu-3789.41.3~3/RELEASE_X86_64 x86_64
  • Subsystem: parser

This source: a=b=1;((a + 1), {c} = b)

  • Works in Chrome 56.0.2924.87
  • Works in Safari 10.0.3 (12602.4.8)
  • Fails to parse in Node

Quick command-line test:

  • echo "a=b=1;((a + 1), {c} = b)" | node

These slight variations DO work in node:

  • a=b=1;((a), {c} = b)
  • a=b=1;((a + 1), ({c} = b))
@addaleax addaleax added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Feb 21, 2017
@shanebdavis
Copy link
Author

It also works in: Babel 6.23.1

@addaleax
Copy link
Member

@shanebdavis Fwiw, it also works in Node 7.x, which means it was likely fixed during a V8 upgrade.

/cc @nodejs/v8

@TimothyGu TimothyGu added the confirmed-bug Issues with confirmed bugs. label Feb 21, 2017
@shanebdavis
Copy link
Author

@addaleax Thanks! That makes sense. I was wondering why it worked in Chrome and not Node.

Is 6.x scheduled to get V8 updates?

@addaleax
Copy link
Member

Is 6.x scheduled to get V8 updates?

Not regularly. I think the process for these cases is to look for which commit(s) in V8 fixed the problem, see if is feasible to apply them to the old V8 branch and then cherry-pick them back.

@Fishrock123 Fishrock123 added confirmed-bug Issues with confirmed bugs. and removed confirmed-bug Issues with confirmed bugs. labels Feb 21, 2017
@Fishrock123
Copy link
Contributor

Eh I suppose we can keep the label on it if backporting is at all viable.

@targos targos self-assigned this Feb 21, 2017
@targos
Copy link
Member

targos commented Feb 21, 2017

I Identified v8/v8@dfb8d33 as the commit that fixes it using git bisect.

@targos targos mentioned this issue Feb 21, 2017
Closed
3 tasks
@targos
Copy link
Member

targos commented Feb 21, 2017

#11483

targos added a commit to targos/node that referenced this issue Mar 3, 2017
@targos
deps: backport dfb8d33 from V8 upstream
b858d69 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    [email protected]
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{nodejs#36897}

Fixes: nodejs#11480
targos added a commit to targos/node that referenced this issue Mar 3, 2017
@targos
test: add regression test for V8 parse error
6b34806 
Refs: nodejs#11480
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
@targos @MylesBorins
deps: backport dfb8d33 from V8 upstream
b198918 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    [email protected]
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{#36897}

Fixes: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
@targos @MylesBorins
test: add regression test for V8 parse error
b3f32f9 
Refs: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
@targos @MylesBorins
deps: backport dfb8d33 from V8 upstream
d0e934f 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    [email protected]
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{#36897}

Fixes: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
@targos @MylesBorins
test: add regression test for V8 parse error
e71fc70 
Refs: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@targos
Copy link
Member

targos commented Mar 8, 2017

Fixed by d0e934f

@targos targos closed this as completed Mar 8, 2017
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
@targos @MylesBorins
deps: backport dfb8d33 from V8 upstream
3ab070d 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    [email protected]
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{#36897}

Fixes: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
@targos @MylesBorins
test: add regression test for V8 parse error
5365501 
Refs: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@targos targos removed their assignment Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@shanebdavis @addaleax @Fishrock123 @TimothyGu @targos