[es6] support AssignmentPattern as LHS in for-in/of loops

[es6] support AssignmentPattern as LHS in for-in/of loops

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

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

[caitp (gmail), 2015-12-08 04:24:22]

quick hack to add this.

The bit for ForInStatement is probably the most iffy, and it's possible that
there are areas that need to be changed to handle this that I've missed.

[adamk, 2015-12-10 00:53:25]

The need for special handling in code generation is a red flag to me. I expected
a desugaring of the form:

for ([x, y] in whatever) body

->

for (temp in whatever) {
  [x, y] = temp;
  body;
}

But I expect I'm missing something; is there some reason the above wouldn't
work?

[caitp (gmail), 2015-12-10 09:38:22]

On 2015/12/10 00:53:25, adamk wrote:
> The need for special handling in code generation is a red flag to me. I
expected
> a desugaring of the form:
> 
> for ([x, y] in whatever) body
> 
> ->
> 
> for (temp in whatever) {
>   [x, y] = temp;
>   body;
> }
> 
> But I expect I'm missing something; is there some reason the above wouldn't
> work?

That's essentially what this implementation does. It's a bit more complicated to
put everything in a single Expression node, and not necessarily worth it

[caitp (gmail), 2015-12-10 10:12:31]

On 2015/12/10 09:38:22, caitp wrote:
> On 2015/12/10 00:53:25, adamk wrote:
> > The need for special handling in code generation is a red flag to me. I
> expected
> > a desugaring of the form:
> > 
> > for ([x, y] in whatever) body
> > 
> > ->
> > 
> > for (temp in whatever) {
> >   [x, y] = temp;
> >   body;
> > }
> > 
> > But I expect I'm missing something; is there some reason the above wouldn't
> > work?
> 
> That's essentially what this implementation does. It's a bit more complicated
to
> put everything in a single Expression node, and not necessarily worth it

I've changed it to be part of the body statement

[caitp (gmail), 2015-12-10 10:14:36]

Patchset #2 (id:20001) has been deleted

[caitp (gmail), 2015-12-10 10:17:04]

Description was changed from

==========
[es6] support AssignmentPattern as LHS in for-in/of loops

BUG=v8:811, v8:4599
LOG=N
R=bmeurer@chromium.org, adamk@chromium.org, rossberg@chromium.org
==========

to

==========
[es6] support AssignmentPattern as LHS in for-in/of loops

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

[caitp (gmail), 2015-12-10 10:17:04]

caitpotter88@gmail.com changed reviewers:
- bmeurer@chromium.org

[caitp (gmail), 2015-12-10 10:21:23]

Patchset #3 (id:60001) has been deleted

[caitp (gmail), 2015-12-10 10:36:36]

Patchset #3 (id:80001) has been deleted

[caitp (gmail), 2015-12-10 10:36:45]

Patchset #2 (id:40001) has been deleted

[caitp (gmail), 2015-12-10 10:36:53]

Patchset #2 (id:100001) has been deleted

[adamk, 2015-12-10 17:28:03]

Looking good. Seems like there's some opportunity for cleaning up the
PatternRewriter API.

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3386:
factory()->NewRewritableAssignmentExpression(assign_each);
Making the callers here and below construct a RewritableAssignmentExpression is
a little ugly. Can you add a new entry point to PatternRewriter that
encapsulates this, taking an Expression and returning the rewritten one?

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3389: DCHECK(ok);
It looks like all callers of RewriteDestructuringAssignment do a DCHECK. Can you
remove the ok argument and do that DCHECK to the end of
RewriteDestructuringAssignment?

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3402:
scope_->NewTemporary(ast_value_factory()->dot_result_string());
dot_result_string has a special usage (for completion value stuff). How about
just the empty string?

[caitp (gmail), 2015-12-10 18:19:36]

let me know how that looks --- I'm not 100% it's great that the two
RewriteDestructuringAssignment signatures are almost identical, but accept
different Expression subclasses (I'm forcing the new one to require an
Assignment rather than an Expression, in order to avoid accidents happening).

Works well enough here

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3386:
factory()->NewRewritableAssignmentExpression(assign_each);
On 2015/12/10 17:28:03, adamk wrote:
> Making the callers here and below construct a RewritableAssignmentExpression
is
> a little ugly. Can you add a new entry point to PatternRewriter that
> encapsulates this, taking an Expression and returning the rewritten one?

Done

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3389: DCHECK(ok);
On 2015/12/10 17:28:03, adamk wrote:
> It looks like all callers of RewriteDestructuringAssignment do a DCHECK. Can
you
> remove the ok argument and do that DCHECK to the end of
> RewriteDestructuringAssignment?

Done.

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3402:
scope_->NewTemporary(ast_value_factory()->dot_result_string());
On 2015/12/10 17:28:03, adamk wrote:
> dot_result_string has a special usage (for completion value stuff). How about
> just the empty string?

I think this is the completion-correct model, and matches what we do for for-of
loops. But, I've changed it to the empty string, let me know if you change your
mind

[adamk, 2015-12-10 19:23:34]

lgtm % test-parsing tests

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3402:
scope_->NewTemporary(ast_value_factory()->dot_result_string());
On 2015/12/10 18:19:36, caitp wrote:
> On 2015/12/10 17:28:03, adamk wrote:
> > dot_result_string has a special usage (for completion value stuff). How
about
> > just the empty string?
> 
> I think this is the completion-correct model, and matches what we do for
for-of
> loops. But, I've changed it to the empty string, let me know if you change
your
> mind

If there's a reason for it to be .result, I'd expect there's some test that
should catch it. Otherwise I'd always prefer empty string to make clear there's
no reason for .result.

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/1508933004/diff/140001/src/parsing/preparser....
src/parsing/preparser.cc:970: ValidateAssignmentPattern(&classifier, CHECK_OK);
Please add a few tests to test-parsing.cc to make sure we have preparser
coverage here.

https://codereview.chromium.org/1508933004/diff/180001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/1508933004/diff/180001/src/parsing/parser.h#n...
src/parsing/parser.h:1052: static Expression*
RewriteDestructuringAssignment(Parser* parser,
This looks fine to me.

[adamk, 2015-12-10 20:08:15]

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140
src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) {
Do you still need this check?

[caitp (gmail), 2015-12-10 20:32:25]

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140
src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) {
On 2015/12/10 20:08:15, adamk wrote:
> Do you still need this check?

I can avoid needing it, but it means an extra variable will be allocated

[adamk, 2015-12-10 22:44:17]

https://codereview.chromium.org/1508933004/diff/200001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1508933004/diff/200001/src/parsing/parser.cc#...
src/parsing/parser.cc:3410: each = factory()->NewVariableProxy(temp);
It looks like each is always a VariableProxy in this case. So when does your
check in ast.cc for IsValidReferenceExpression return false?

[adamk, 2015-12-10 23:03:31]

lgtm with a TODO

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140
src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) {
On 2015/12/10 20:32:25, caitp wrote:
> On 2015/12/10 20:08:15, adamk wrote:
> > Do you still need this check?
> 
> I can avoid needing it, but it means an extra variable will be allocated

A TODO to further separate ForIn and ForOf here would be nice.

[caitp (gmail), 2015-12-10 23:18:18]

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140
src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) {
On 2015/12/10 23:03:31, adamk wrote:
> On 2015/12/10 20:32:25, caitp wrote:
> > On 2015/12/10 20:08:15, adamk wrote:
> > > Do you still need this check?
> > 
> > I can avoid needing it, but it means an extra variable will be allocated
> 
> A TODO to further separate ForIn and ForOf here would be nice.

TODO() added

[caitp (gmail), 2015-12-10 23:18:33]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-12-10 23:18:33]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1508933004/#ps220001
(title: "slightly modify AssignFeedbackVectorSlots + add TODO")

[commit-bot: I haz the power, 2015-12-10 23:18:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1508933004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1508933004/220001

[caitp (gmail), 2015-12-10 23:29:44]

Patchset #8 (id:240001) has been deleted

[caitp (gmail), 2015-12-11 00:07:04]

On 2015/12/10 23:18:40, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1508933004/220001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1508933004/220001

bad accesses due to not null-terminating the context data arrays --- fixed

[caitp (gmail), 2015-12-11 00:07:09]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-12-11 00:07:09]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1508933004/#ps260001
(title: "Fix valgrind issue")

[commit-bot: I haz the power, 2015-12-11 00:07:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1508933004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1508933004/260001

[commit-bot: I haz the power, 2015-12-11 00:20:33]

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

[commit-bot: I haz the power, 2015-12-11 00:20:34]

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

[caitp (gmail), 2015-12-11 00:29:02]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-12-11 00:29:03]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1508933004/#ps280001
(title: "Fix test failures")

[commit-bot: I haz the power, 2015-12-11 00:29:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1508933004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1508933004/280001

[commit-bot: I haz the power, 2015-12-11 01:06:39]

Description was changed from

==========
[es6] support AssignmentPattern as LHS in for-in/of loops

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

to

==========
[es6] support AssignmentPattern as LHS in for-in/of loops

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

[commit-bot: I haz the power, 2015-12-11 01:06:39]

Committed patchset #9 (id:280001)

[commit-bot: I haz the power, 2015-12-11 01:06:54]

Description was changed from

==========
[es6] support AssignmentPattern as LHS in for-in/of loops

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

to

==========
[es6] support AssignmentPattern as LHS in for-in/of loops

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

Committed: https://crrev.com/e47bdb775564b2cd8365047425898ab4274190a6
Cr-Commit-Position: refs/heads/master@{#32773}
==========

[commit-bot: I haz the power, 2015-12-11 01:06:54]

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

[adamk, 2015-12-11 01:59:12]

A revert of this CL (patchset #9 id:280001) has been created in
https://codereview.chromium.org/1511773009/ by adamk@chromium.org.

The reason for reverting is: Hits unreachable code (found by fuzzer). Example
crasher:

"for(();;);".