[destructuring] More tests for object literal pattern

[destructuring] More tests for object literal pattern

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

Committed: https://crrev.com/cf9492ddd818f336a9dc1269cdb7557ec709d12c
Cr-Commit-Position: refs/heads/master@{#28457}

[Dmitry Lomov (no reviews), 2015-05-18 09:52:27]

ptal

[Dmitry Lomov (no reviews), 2015-05-18 09:52:57]

On 2015/05/18 09:52:27, Dmitry Lomov (chromium) wrote:
> ptal

(sorry seems like I forgot to push a button on Friday)

[Dmitry Lomov (no reviews), 2015-05-18 10:13:53]

The CQ bit was checked by dslomov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-18 10:14:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139773005/20001

[commit-bot: I haz the power, 2015-05-18 10:49:23]

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

[commit-bot: I haz the power, 2015-05-18 10:49:23]

Dry run: This issue passed the CQ dry run.

[caitp (gmail), 2015-05-18 13:11:05]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-05-18 13:11:06]

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

https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:140: assertThrows(function() { var {} =
null; }, TypeError);
do we want to test the other cases that would cause the
`RequireObjectCoercible(value)` test to throw? Including nested patterns

[arv (Not doing code reviews), 2015-05-18 13:12:54]

https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc#newcode4137
src/parser.cc:4137: // if (var === null || var === undefined ||
%_IsUndetectableObject(var))
Is this right? Why can I not do?

let {someId} = document.all;

The ES spec knows nothing about undetectable objects.

https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:220:
block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp),
In theory this is only need when we have an empty pattern.

let {x} = null;

=>

tmp = null;
tmp.x = x;  // null is not coercible

Maybe add a TODO?

https://codereview.chromium.org/1139773005/diff/20001/src/token.h
File src/token.h (right):

https://codereview.chromium.org/1139773005/diff/20001/src/token.h#newcode64
src/token.h:64: T(ASSIGN_ADD, "+ASSIGN_ADD=", 2)                                
  \
Why?

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

https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:26: var o = { get x() {
var o = {
  get x() {
    ...
  }
};

https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:37: var { x : x0, y : { z : z1 }, x : x1 }
= o;
who would have ever thought :-)

[Dmitry Lomov (no reviews), 2015-05-18 13:41:46]

Comments addressed. Sorry, I have no idea what is the meaning of 'undetectable'
:(

https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc#newcode4137
src/parser.cc:4137: // if (var === null || var === undefined ||
%_IsUndetectableObject(var))
On 2015/05/18 13:12:53, arv wrote:
> Is this right? Why can I not do?
> 
> let {someId} = document.all;
> 
> The ES spec knows nothing about undetectable objects.

The truth is, I have just copied logic from CHECK_OBJECT_COERCIBLE in macros.py.
Tell me what the right semantics is.

https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:220:
block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp),
On 2015/05/18 13:12:53, arv wrote:
> In theory this is only need when we have an empty pattern.
> 
> let {x} = null;
> 
> =>
> 
> tmp = null;
> tmp.x = x;  // null is not coercible
> 
> Maybe add a TODO?

Done.

https://codereview.chromium.org/1139773005/diff/20001/src/token.h
File src/token.h (right):

https://codereview.chromium.org/1139773005/diff/20001/src/token.h#newcode64
src/token.h:64: T(ASSIGN_ADD, "+ASSIGN_ADD=", 2)                                
  \
On 2015/05/18 13:12:53, arv wrote:
> Why?

Oops. Undone (it's pathetic that no tests fail :()

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

https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:26: var o = { get x() {
On 2015/05/18 13:12:53, arv wrote:
> var o = {
>   get x() {
>     ...
>   }
> };

Done.

https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:140: assertThrows(function() { var {} =
null; }, TypeError);
On 2015/05/18 13:11:06, caitp wrote:
> do we want to test the other cases that would cause the
> `RequireObjectCoercible(value)` test to throw? Including nested patterns

Done.

[Dmitry Lomov (no reviews), 2015-05-18 13:56:35]

Removed undetectable check. Whatever we do, we must be consistent betweet
  let {x} = foo
and
  let x = foo.x
The latter has no undetectable check, so the former shouldn't as well.

[arv (Not doing code reviews), 2015-05-18 14:00:27]

https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc#newcode4138
src/parser.cc:4138: //     return %ThrowNonCoercible(var);
Instead of creating a new runtime function you can use ThrowNewTypeError here.

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

https://codereview.chromium.org/1139773005/diff/80001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:72: let { x : x0, y : { z : z1 }, x : x1 }
= o;
This makes me wish we had a test that makes sure we do all the getters in the
right order.

[Dmitry Lomov (no reviews), 2015-05-18 14:25:56]

Thanks for the review, all great suggestions! PTAL

https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc#newcode4138
src/parser.cc:4138: //     return %ThrowNonCoercible(var);
On 2015/05/18 14:00:27, arv wrote:
> Instead of creating a new runtime function you can use ThrowNewTypeError here.

Done.

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

https://codereview.chromium.org/1139773005/diff/80001/test/mjsunit/harmony/de...
test/mjsunit/harmony/destructuring.js:72: let { x : x0, y : { z : z1 }, x : x1 }
= o;
On 2015/05/18 14:00:27, arv wrote:
> This makes me wish we had a test that makes sure we do all the getters in the
> right order.

Done.

[Dmitry Lomov (no reviews), 2015-05-18 14:26:31]

(sorry had to rebase because NewThrowTypeError changed under me)

[Dmitry Lomov (no reviews), 2015-05-18 14:30:34]

The CQ bit was checked by dslomov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-18 14:30:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139773005/100001

[arv (Not doing code reviews), 2015-05-18 14:43:43]

LGTM

Some small improvements

https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4148
src/parser.cc:4148: //     return throw /* type error kNonCoercible) */;
Just throw, no return.

https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4165
src/parser.cc:4165: factory()->NewReturnStatement(throw_type_error,
RelocInfo::kNoPosition),
NewExpressionStatement

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:222: if (pattern->properties()->length() == 0) {
That was easy

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:40: assertArrayEquals(["x", "y", "z",
"x"], log);
thanks

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:87: assertThrows(function() { x++; },
TypeError);
Can you add a test/message/ test too?

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:98: 
+1 newline for consistency

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:132: (function TestExceptions() {
How about:

(function TestExceptions() {
  for (var val of [null, undefined]) {
    assertThrows(function() { var {} = val; }, TypeError);
    assertThrows(function() { var {x} = val; }, TypeError);
    assertThrows(function() { var { x : {} } = { x : val }; }, TypeError);
    assertThrows(function() { 'use strict'; let {} = val; }, TypeError);
    assertThrows(function() { 'use strict'; let {x} = val; }, TypeError);
    assertThrows(function() { 'use strict'; let { x : {} } = { x : val }; },
                 TypeError);
  }
}());

[commit-bot: I haz the power, 2015-05-18 15:36:57]

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

[commit-bot: I haz the power, 2015-05-18 15:36:58]

Dry run: This issue passed the CQ dry run.

[rossberg, 2015-05-18 16:23:18]

LGTM modulo Arv's comments

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:223:
block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp),
Nit: I'd just inline this method

[Dmitry Lomov (no reviews), 2015-05-18 16:28:23]

Landing

https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4148
src/parser.cc:4148: //     return throw /* type error kNonCoercible) */;
On 2015/05/18 14:43:42, arv wrote:
> Just throw, no return.

Indeed, so much simpler! Awesomesauce. Done.

https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4165
src/parser.cc:4165: factory()->NewReturnStatement(throw_type_error,
RelocInfo::kNoPosition),
On 2015/05/18 14:43:42, arv wrote:
> NewExpressionStatement

Done.

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:222: if (pattern->properties()->length() == 0) {
On 2015/05/18 14:43:42, arv wrote:
> That was easy

:-) Esp. since undetectable garbage is gone.

https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c...
src/pattern-rewriter.cc:223:
block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp),
On 2015/05/18 16:23:18, rossberg wrote:
> Nit: I'd just inline this method

I think I will reuse it for ArrayPattern later on.

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:40: assertArrayEquals(["x", "y", "z",
"x"], log);
On 2015/05/18 14:43:42, arv wrote:
> thanks

Acknowledged.

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:87: assertThrows(function() { x++; },
TypeError);
On 2015/05/18 14:43:42, arv wrote:
> Can you add a test/message/ test too?

Done.

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:98: 
On 2015/05/18 14:43:42, arv wrote:
> +1 newline for consistency

Done.

https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d...
test/mjsunit/harmony/destructuring.js:132: (function TestExceptions() {
On 2015/05/18 14:43:42, arv wrote:
> How about:
> 
> (function TestExceptions() {
>   for (var val of [null, undefined]) {
>     assertThrows(function() { var {} = val; }, TypeError);
>     assertThrows(function() { var {x} = val; }, TypeError);
>     assertThrows(function() { var { x : {} } = { x : val }; }, TypeError);
>     assertThrows(function() { 'use strict'; let {} = val; }, TypeError);
>     assertThrows(function() { 'use strict'; let {x} = val; }, TypeError);
>     assertThrows(function() { 'use strict'; let { x : {} } = { x : val }; },
>                  TypeError);
>   }
> }());

Done.

[Dmitry Lomov (no reviews), 2015-05-18 16:28:28]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-05-18 16:28:28]

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/1139773005/#ps120001
(title: "Comments addressed, landing")

[commit-bot: I haz the power, 2015-05-18 16:28:37]

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

[arv (Not doing code reviews), 2015-05-18 16:32:05]

https://codereview.chromium.org/1139773005/diff/120001/test/message/destructu...
File test/message/destructuring-modify-const.js (right):

https://codereview.chromium.org/1139773005/diff/120001/test/message/destructu...
test/message/destructuring-modify-const.js:9: x++;
I was thinking a test where we have a failure in the pattern

const {x, y, x} = {x: 1, y: 2};

for example

[Dmitry Lomov (no reviews), 2015-05-18 17:30:29]

The CQ bit was unchecked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-05-18 17:30:29]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-05-18 17:30:40]

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

[commit-bot: I haz the power, 2015-05-18 20:15:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-18 22:12:23]

Patchset 7 (id:??) landed as
https://crrev.com/cf9492ddd818f336a9dc1269cdb7557ec709d12c
Cr-Commit-Position: refs/heads/master@{#28457}