Fix re-indexing for literals in do-expressions

Fix re-indexing for literals in do-expressions

This became temporarily a big issue, because spreads are desugared
into do-expressions.  This patch fixes the problem with having
spreads as parameter initializers in arrow expressions, e.g., this
line would crash:

    [], ((x = [...[42]]) => x)();

R=rossberg@chromium.org
BUG=chromium:578038
LOG=N

Committed: https://crrev.com/9ed5596ade9f74b5604bbd1cb27e2f6265123556
Cr-Commit-Position: refs/heads/master@{#33365}

[nickie, 2016-01-15 17:44:24]

This is not 100% ready yet.
I'm getting this error when running the new regression test:

$ rt Optdebug mjsunit/harmony/regress/regress-crbug-578038
>>> Running tests for x64.Optdebug
=== mjsunit/harmony/regress/regress-crbug-578038 ===                    
Warning: unknown flag --enable-slow-asserts.
Try --help for options
============ Stress 1/2 ============
============ Stress 2/2 ============
#
# Fatal error in ../../src/objects-debug.cc, line 1323
# Check failed: old_target->kind() == new_target->kind() (2 vs. 4).
#

This does not happen if you execute the test individually once.

[caitp (gmail), 2016-01-15 17:48:00]

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

[caitp (gmail), 2016-01-15 17:48:00]

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
File src/ast/ast-literal-reindexer.cc (right):

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
src/ast/ast-literal-reindexer.cc:47: Visit(node->block());
The thing that worries me is, which function the reindexing is a part of. I'm
mostly concerned with things like initializers in arrow function formal
parameter lists.

This might be enough to make that work, or it might not be. I would say, make
sure to test that =)

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
src/ast/ast-literal-reindexer.cc:48: Visit(node->result());
`result` is always a VariableProxy, so it probably doesn't need to be visited

[nickie, 2016-01-15 20:18:29]

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
File src/ast/ast-literal-reindexer.cc (right):

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
src/ast/ast-literal-reindexer.cc:47: Visit(node->block());
On 2016/01/15 17:48:00, caitp wrote:
> The thing that worries me is, which function the reindexing is a part of. I'm
> mostly concerned with things like initializers in arrow function formal
> parameter lists.
> 
> This might be enough to make that work, or it might not be. I would say, make
> sure to test that =)

Initializers in arrow function parameters is exactly where this thing hit.  Now,
correct me if I'm wrong, but these two lines (modulo the next comment) are the
right thing to do here when traversing the tree.  What we should think about is
when this happens (as with the case of RewritableExpressions in destructuring
assignment, whose rewriting is postponed).  I'm not confident that I understand
this enough to express an opinion here.

There is a test that fails (a stress test, if that makes any difference); see my
first comment.

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
src/ast/ast-literal-reindexer.cc:48: Visit(node->result());
On 2016/01/15 17:48:00, caitp wrote:
> `result` is always a VariableProxy, so it probably doesn't need to be visited

I was of the impression that the result can be a temporary and (in other
visitors, not here I guess) it is important that it is indeed visited.  Anyway,
yes, this line can absolutely be removed.

Let me just comment that it would be much better (from the developer's point of
view, both when modifying AST nodes and when debugging, not for performance) if
it wasn't the case that each visitor did an AST traversal in a slightly
different way.

[caitp (gmail), 2016-01-15 20:25:30]

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
File src/ast/ast-literal-reindexer.cc (right):

https://codereview.chromium.org/1581403007/diff/1/src/ast/ast-literal-reindex...
src/ast/ast-literal-reindexer.cc:47: Visit(node->block());
On 2016/01/15 20:18:29, nickie wrote:
> On 2016/01/15 17:48:00, caitp wrote:
> > The thing that worries me is, which function the reindexing is a part of.
I'm
> > mostly concerned with things like initializers in arrow function formal
> > parameter lists.
> > 
> > This might be enough to make that work, or it might not be. I would say,
make
> > sure to test that =)
> 
> Initializers in arrow function parameters is exactly where this thing hit. 
Now,
> correct me if I'm wrong, but these two lines (modulo the next comment) are the
> right thing to do here when traversing the tree.  What we should think about
is
> when this happens (as with the case of RewritableExpressions in destructuring
> assignment, whose rewriting is postponed).  I'm not confident that I
understand
> this enough to express an opinion here.
> 
> There is a test that fails (a stress test, if that makes any difference); see
my
> first comment.

In a RewritableExpression, any ObjectLiteral or ArrayLiteral that is the
right-most RHS of a `=` Assignment, would represent a non-pattern to be
reindexed, as well as any that is on any side of any other form of Assignment.
As far as I'm aware, there's no other case that matters there, though.

[nickie, 2016-01-18 09:14:27]

The stress test error that I'm getting seems to be unrelated to arrays or
literal indexing in general.  You can get the same error by trying the function:

    (function testWithDoInArrowParameterInitializer() {
      ((x = (do { var e=0; while (0) {} })) => x)();
    })();

You still get:

    >>> Running tests for x64.Debug
    === mjsunit/harmony/regress/regress-crbug-578038 ===                    
    ============ Stress 1/2 ============
    ============ Stress 2/2 ============
    #
    # Fatal error in ../../src/objects-debug.cc, line 1323
    # Check failed: old_target->kind() == new_target->kind() (2 vs. 4).
    #

Bottom line: This is most probably a bug that is unrelated to spreads and that
was there ever since do expressions were implemented.  The problem now is that
the spread desugaring is based on this, so a lot of test cases that worked
before do not work now.

[nickie, 2016-01-18 14:30:06]

Description was changed from

==========
Fix re-indexing for literals in do-expressions

This became temporarily a big issue, because spreads are desugared
into do-expressions.  This patch fixes the problem with having
spreads as parameter initializers in arrow expressions, e.g., this
line would crash:

    [], ((x = [...[42]]) => x)();

R=rossberg@chromium.org
BUG=chromium:578038
==========

to

==========
Fix re-indexing for literals in do-expressions

This became temporarily a big issue, because spreads are desugared
into do-expressions.  This patch fixes the problem with having
spreads as parameter initializers in arrow expressions, e.g., this
line would crash:

    [], ((x = [...[42]]) => x)();

R=rossberg@chromium.org
BUG=chromium:578038
LOG=N
==========

[nickie, 2016-01-18 14:34:06]

The failing regression test used a variable declaration inside the do
expression, inside the parameter initializer of the arrow function, which is
known not to work.  On the other hand, spread desugaring uses a temporary
variable there, which works fine.  I'm fixing the regression test to focus on
spreads, leaving the more general problem of do expressions for the future.

[nickie, 2016-01-18 14:36:19]

Patchset #2 (id:20001) has been deleted

[rossberg, 2016-01-18 14:43:12]

lgtm

[nickie, 2016-01-18 14:55:40]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-01-18 14:55:48]

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

[commit-bot: I haz the power, 2016-01-18 15:20:55]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-18 15:21:04]

Description was changed from

==========
Fix re-indexing for literals in do-expressions

This became temporarily a big issue, because spreads are desugared
into do-expressions.  This patch fixes the problem with having
spreads as parameter initializers in arrow expressions, e.g., this
line would crash:

    [], ((x = [...[42]]) => x)();

R=rossberg@chromium.org
BUG=chromium:578038
LOG=N
==========

to

==========
Fix re-indexing for literals in do-expressions

This became temporarily a big issue, because spreads are desugared
into do-expressions.  This patch fixes the problem with having
spreads as parameter initializers in arrow expressions, e.g., this
line would crash:

    [], ((x = [...[42]]) => x)();

R=rossberg@chromium.org
BUG=chromium:578038
LOG=N

Committed: https://crrev.com/9ed5596ade9f74b5604bbd1cb27e2f6265123556
Cr-Commit-Position: refs/heads/master@{#33365}
==========

[commit-bot: I haz the power, 2016-01-18 15:21:05]

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