[destructuring] Support computed property names in patterns.

[destructuring] Support computed property names in patterns.

R=arv@chromium.org,rossberg@chromium.org
BUG=v8:811
LOG=N

Committed: https://crrev.com/3199077791ff1c062b92fe92614e8ee21b1f82d3
Cr-Commit-Position: refs/heads/master@{#28487}

[Dmitry Lomov (no reviews), 2015-05-19 06:12:30]

PTAL (based on destructuring-initializers
https://codereview.chromium.org/1146683002)

[rossberg, 2015-05-19 11:50:35]

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:286: (function TestComputedNames() {
Add tests with computed accessor and method names. Do they actually work
already?

[caitp (gmail), 2015-05-19 13:35:42]

Just a general note about this --- If a function parameter includes an
ObjectBindingPattern which uses computed property keys,
`hasParameterExpressions` in 9.2.12 is true, and this triggers the special
scoping behaviour even if initializers aren't used anywhere.

So, it's probably a good idea to record the presence of computed property keys
in the expression classifier somehow

[caitp (gmail), 2015-05-19 13:36:59]

On 2015/05/19 13:35:42, caitp wrote:
> Just a general note about this --- If a function parameter includes an
> ObjectBindingPattern which uses computed property keys,
> `hasParameterExpressions` in 9.2.12 is true, and this triggers the special
> scoping behaviour even if initializers aren't used anywhere.
> 
> So, it's probably a good idea to record the presence of computed property keys
> in the expression classifier somehow

On the other hand, you could use the weird scoping behaviour all the time and it
wouldn't really matter, but maybe there's a performance hit for doing that?

[Dmitry Lomov (no reviews), 2015-05-19 14:07:28]

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:286: (function TestComputedNames() {
On 2015/05/19 11:50:34, rossberg wrote:
> Add tests with computed accessor and method names. Do they actually work
> already?

Uhm, accessors and method names are not allowed in patterns :)

[rossberg, 2015-05-19 14:16:55]

On 2015/05/19 14:07:28, Dmitry Lomov (chromium) wrote:
>
https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
> File test/mjsunit/harmony/destructuring.js (right):
> 
>
https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
> test/mjsunit/harmony/destructuring.js:286: (function TestComputedNames() {
> On 2015/05/19 11:50:34, rossberg wrote:
> > Add tests with computed accessor and method names. Do they actually work
> > already?
> 
> Uhm, accessors and method names are not allowed in patterns :)

Oh dear. :)

LGTM

[arv (Not doing code reviews), 2015-05-19 14:19:56]

LGTM

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:344: }());
Can haz order tests?

var log = [];
var obj = {
  get x() {
    log.push('get x');
    return 1;
  },
  get y() {
    log.push('get y');
    return 2;
  },
};
function f(name) {
  log.push(`f ${name}`);
}
var {[f('x')]: x, [f('y')]: y} = obj;
assertArrayEquals(log, ['f x', 'f y', 'get x', get y']);

[arv (Not doing code reviews), 2015-05-19 14:36:56]

> assertArrayEquals(log, ['f x', 'f y', 'get x', get y']);

Should be

  assertArrayEquals(log, ['f x', 'get x', 'f y', 'get y']);

I double checked with the spec

[Dmitry Lomov (no reviews), 2015-05-19 14:52:00]

Landing

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1129083009/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:344: }());
On 2015/05/19 14:19:56, arv wrote:
> Can haz order tests?
> 
> var log = [];
> var obj = {
>   get x() {
>     log.push('get x');
>     return 1;
>   },
>   get y() {
>     log.push('get y');
>     return 2;
>   },
> };
> function f(name) {
>   log.push(`f ${name}`);
> }
> var {[f('x')]: x, [f('y')]: y} = obj;
> assertArrayEquals(log, ['f x', 'f y', 'get x', get y']);

Done.

[Dmitry Lomov (no reviews), 2015-05-19 14:52:04]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-05-19 14:52:05]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1129083009/#ps40001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-05-19 14:52:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129083009/40001

[commit-bot: I haz the power, 2015-05-19 15:06:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-19 15:06:01]

Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Dmitry Lomov (no reviews), 2015-05-19 16:03:05]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-05-19 16:03:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129083009/40001

[commit-bot: I haz the power, 2015-05-19 16:27:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-19 16:27:39]

Patchset 3 (id:??) landed as
https://crrev.com/3199077791ff1c062b92fe92614e8ee21b1f82d3
Cr-Commit-Position: refs/heads/master@{#28487}