Destructuring of arrow function arguments via computed property throws in v6.x #10347

oaleynik · 2016-12-19T22:50:03Z

node -v: v6.x
uname -a: Darwin Olegs-MacBook-Pro.local 16.4.0 Darwin Kernel Version 16.4.0: Wed Dec 7 12:06:26 PST 2016; root:xnu-3789.41.1~5/RELEASE_X86_64 x86_64

// Throws in Nodejs 6.x with -> ReferenceError: y is not defined
var y = 'a';
var g20 = ({[y]: x}) => { var y = 'b'; return x; };
require('assert').equal(1, g20({a: 1, b: 2}));

The test case is taken from: https://github.com/nodejs/node/blob/v6.x/deps/v8/test/mjsunit/es6/destructuring.js#L893

vsemozhetbyt · 2016-12-19T23:13:46Z

According to node.green it should work in 6.9.0–6.9.2. This example from "computed properties" tooltip there works fine in the 6.9.2

function test(){
  var qux = "corge";
  return function({ [qux]: grault }) {
    return grault === "garply";
  }({ corge: "garply" });
}

console.log(test());

oaleynik · 2016-12-19T23:19:20Z

@vsemozhetbyt example from node.green indeed works. But the same example, slightly changed, throws:

const qux = "corge";
const fn = ({ [qux]: grault }) => {
  return grault === "garply";
};

console.log(fn({ corge: "garply" }));

UPDATE: What is interesting - if replace arrow function with the regular function - it works.

vsemozhetbyt · 2016-12-19T23:37:57Z

So this is corrected formulation: destructuring of arrow function arguments via computed property throws in the Node.js 6.9.2:

// does not throw in the Node.js 6.9.2
const obj = { key: 'value' };

const keyComputed = 'key';

function fn({ [keyComputed]: param }) {}

fn(obj);

// throws in the Node.js 6.9.2
const obj = { key: 'value' };

const keyComputed = 'key';

const fn = ({ [keyComputed]: param }) => {};

fn(obj);

oaleynik · 2016-12-19T23:47:01Z

Thanks @vsemozhetbyt, I've updated the issue title and description.

vsemozhetbyt · 2016-12-20T00:14:55Z

It seems the bug is actual only for destructuring. Other evaluations in the parameters scope are OK.

// does not throw in the Node.js 6.9.2
const obj = { key: 'value' };

const keyComputed = 'key';

function fn(param = obj[keyComputed]) { console.log(param); }

fn();

// does not throw in the Node.js 6.9.2
const obj = { key: 'value' };

const keyComputed = 'key';

const fn = (param = obj[keyComputed]) => { console.log(param); };

fn();

oaleynik · 2016-12-20T00:27:38Z

I see the test case for this in V8 on 6.x branch: https://github.com/nodejs/node/blob/v6.x/deps/v8/test/mjsunit/es6/destructuring.js#L893

Should not it fail?

vsemozhetbyt · 2016-12-20T00:46:21Z

Well, this throws:

// throws in the Node.js 6.9.2
var y = 'a';
var g20 = ({[y]: x}) => { var y = 'b'; return x; };
require('assert').equal(1, g20({a: 1, b: 2}));

I don't know why it does not fail there(

vsemozhetbyt · 2016-12-20T00:50:31Z

Maybe these tests are not actually tested during Node.js build (unlike Node tests)?

MylesBorins · 2016-12-20T01:00:27Z

the V8 tests are not run during the normal node CI

…

On Mon, Dec 19, 2016, 7:50 PM Vse Mozhet Byt ***@***.***> wrote: Maybe these tests are not actually tested during Node.js build (unlike Node tests <https://github.com/nodejs/node/tree/master/test>)? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#10347 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV_qmc70Sg0sn2EgYzPqz3CeYCqAhks5rJyZdgaJpZM4LRRZ2> .

hashseed · 2016-12-21T08:24:19Z

This seems to work correctly on the newest V8 (version 5.7).
@fhinkel is there any merit in bisecting what the fix was?

fhinkel · 2016-12-21T10:03:27Z

@hashseed I think this is a fix we want to backport. Do I read it correctly that it was broken because of some interaction with Node (because V8 has a test for it and it's not a regression test)?

hashseed · 2016-12-21T11:35:21Z

@fhinkel I'm under the impression that it's just broken due to V8, and should be reproducible on d8 at an older version. I haven't tested though.

targos · 2016-12-21T12:25:21Z

Just compared node with d8 (V8 5.1.281.89) both built from v6.x-staging and the following test:

(function(){
const qux = "corge";
const fn = ({ [qux]: grault }) => {
  return grault === "garply";
};

var _print = typeof console === 'undefined' ? print : console.log;

_print(fn({ corge: "garply" }));
})();

Test fails with both, only when the code is wrapped in an IIFE (simulating our module wrapper).

targos · 2016-12-21T13:25:02Z

The bug is already fixed in 5.2-lkgr. I'm trying to do a bisect but I get the following error at the end of the build:

make[1]: Entering directory '/home/mzasso/git/chromium/v8/v8/out'
  LINK(target) /home/mzasso/git/chromium/v8/v8/out/x64.release/shell
  LINK(target) /home/mzasso/git/chromium/v8/v8/out/x64.release/hello-world
  LINK(target) /home/mzasso/git/chromium/v8/v8/out/x64.release/process
  LINK(target) /home/mzasso/git/chromium/v8/v8/out/x64.release/d8
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o: unsupported reloc 41 against global symbol __libc_start_main
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o: unsupported reloc 42 against global symbol __gmon_start__
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o: unsupported reloc 41 against global symbol __libc_start_main
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o: unsupported reloc 42 against global symbol __gmon_start__
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o: unsupported reloc 41 against global symbol __libc_start_main
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o: unsupported reloc 42 against global symbol __gmon_start__
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o: unsupported reloc 41 against global symbol __libc_start_main
/home/mzasso/git/chromium/v8/v8/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: /usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o: unsupported reloc 42 against global symbol __gmon_start__
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o(.init+0x7): error: unsupported reloc 42
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o(.init+0x7): error: unsupported reloc 42
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o:function _start: error: unsupported reloc 41
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o:function _start: error: unsupported reloc 41
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o(.init+0x7): error: unsupported reloc 42
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o:function _start: error: unsupported reloc 41
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crti.o(.init+0x7): error: unsupported reloc 42
/usr/lib/gcc/x86_64-redhat-linux/6.2.1/../../../../lib64/crt1.o:function _start: error: unsupported reloc 41
clang: error: linker command failed with exit code 1 (use -v to see invocation)
samples/hello-world.target.x64.release.mk:265: recipe for target '/home/mzasso/git/chromium/v8/v8/out/x64.release/hello-world' failed
make[1]: *** [/home/mzasso/git/chromium/v8/v8/out/x64.release/hello-world] Error 1
make[1]: *** Waiting for unfinished jobs....
clang: error: linker command failed with exit code 1 (use -v to see invocation)
samples/shell.target.x64.release.mk:265: recipe for target '/home/mzasso/git/chromium/v8/v8/out/x64.release/shell' failed
make[1]: *** [/home/mzasso/git/chromium/v8/v8/out/x64.release/shell] Error 1
clang: error: linker command failed with exit code 1 (use -v to see invocation)
samples/process.target.x64.release.mk:265: recipe for target '/home/mzasso/git/chromium/v8/v8/out/x64.release/process' failed
make[1]: *** [/home/mzasso/git/chromium/v8/v8/out/x64.release/process] Error 1
clang: error: linker command failed with exit code 1 (use -v to see invocation)
src/d8.target.x64.release.mk:267: recipe for target '/home/mzasso/git/chromium/v8/v8/out/x64.release/d8' failed
make[1]: *** [/home/mzasso/git/chromium/v8/v8/out/x64.release/d8] Error 1
make[1]: Leaving directory '/home/mzasso/git/chromium/v8/v8/out'
Makefile:312: recipe for target 'x64.release' failed
make: *** [x64.release] Error 2

hashseed · 2016-12-21T13:27:54Z

Looks like a gclient sync issue.

targos · 2016-12-21T13:32:13Z

Still happens after removing binutils and syncing again.
Fixed with ln -fs /usr/bin/ld.gold ./third_party/binutils/Linux_x64/Release/bin/ld.gold

Original commit message: Rewrite scopes in computed properties in destructured parameters While we properly handled scopes of initializers in destructured parameters, we never did the right thing for computed properties. This patch fixes that by factoring out PatternRewriter's scope rewriting logic and calls it for the computed property case. BUG=chromium:620119 Review-Url: https://codereview.chromium.org/2084103002 Cr-Commit-Position: refs/heads/master@{nodejs#37228} Fixes: nodejs#10347

targos · 2016-12-21T15:25:04Z

#10386

Original commit message: Rewrite scopes in computed properties in destructured parameters While we properly handled scopes of initializers in destructured parameters, we never did the right thing for computed properties. This patch fixes that by factoring out PatternRewriter's scope rewriting logic and calls it for the computed property case. BUG=chromium:620119 Review-Url: https://codereview.chromium.org/2084103002 Cr-Commit-Position: refs/heads/master@{#37228} Fixes: #10347 PR-URL: #10386 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

fhinkel · 2017-01-20T14:06:30Z

Looks like @targos fixed it.

targos · 2017-01-20T14:08:22Z

Thanks. I forgot that we can only close issues by pushing on master!

addaleax added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Dec 19, 2016

oaleynik changed the title ~~Destructuring of function arguments via computed property throws~~ Destructuring of arrow function arguments via computed property throws Dec 19, 2016

oaleynik changed the title ~~Destructuring of arrow function arguments via computed property throws~~ Destructuring of arrow function arguments via computed property throws in v6.9.2 Dec 19, 2016

MylesBorins self-assigned this Dec 20, 2016

oaleynik changed the title ~~Destructuring of arrow function arguments via computed property throws in v6.9.2~~ Destructuring of arrow function arguments via computed property throws in v6.x Dec 20, 2016

targos mentioned this issue Dec 21, 2016

deps: backport f795a79 from upstream V8 #10386

Closed

2 tasks

fhinkel closed this as completed Jan 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destructuring of arrow function arguments via computed property throws in v6.x #10347

Destructuring of arrow function arguments via computed property throws in v6.x #10347

oaleynik commented Dec 19, 2016 •

edited

Loading

vsemozhetbyt commented Dec 19, 2016

oaleynik commented Dec 19, 2016 •

edited

Loading

vsemozhetbyt commented Dec 19, 2016

oaleynik commented Dec 19, 2016

vsemozhetbyt commented Dec 20, 2016 •

edited

Loading

oaleynik commented Dec 20, 2016

vsemozhetbyt commented Dec 20, 2016

vsemozhetbyt commented Dec 20, 2016

MylesBorins commented Dec 20, 2016 via email

hashseed commented Dec 21, 2016

fhinkel commented Dec 21, 2016

hashseed commented Dec 21, 2016

targos commented Dec 21, 2016

targos commented Dec 21, 2016

hashseed commented Dec 21, 2016

targos commented Dec 21, 2016

targos commented Dec 21, 2016

fhinkel commented Jan 20, 2017

targos commented Jan 20, 2017

Destructuring of arrow function arguments via computed property throws in v6.x #10347

Destructuring of arrow function arguments via computed property throws in v6.x #10347

Comments

oaleynik commented Dec 19, 2016 • edited Loading

vsemozhetbyt commented Dec 19, 2016

oaleynik commented Dec 19, 2016 • edited Loading

vsemozhetbyt commented Dec 19, 2016

oaleynik commented Dec 19, 2016

vsemozhetbyt commented Dec 20, 2016 • edited Loading

oaleynik commented Dec 20, 2016

vsemozhetbyt commented Dec 20, 2016

vsemozhetbyt commented Dec 20, 2016

MylesBorins commented Dec 20, 2016 via email

hashseed commented Dec 21, 2016

fhinkel commented Dec 21, 2016

hashseed commented Dec 21, 2016

targos commented Dec 21, 2016

targos commented Dec 21, 2016

hashseed commented Dec 21, 2016

targos commented Dec 21, 2016

targos commented Dec 21, 2016

fhinkel commented Jan 20, 2017

targos commented Jan 20, 2017

oaleynik commented Dec 19, 2016 •

edited

Loading

oaleynik commented Dec 19, 2016 •

edited

Loading

vsemozhetbyt commented Dec 20, 2016 •

edited

Loading