Fix re-indexing for literals in do-expressions

src/ast/ast-literal-reindexer.cc

Index: src/ast/ast-literal-reindexer.cc
diff --git a/src/ast/ast-literal-reindexer.cc b/src/ast/ast-literal-reindexer.cc
index fce33e70b8f7caf9aec7a25aaf3fe1ebfa00344e..6db9a61b9b48e235b3d06020b39aae448b93dde9 100644
--- a/src/ast/ast-literal-reindexer.cc
+++ b/src/ast/ast-literal-reindexer.cc
@@ -44,7 +44,8 @@ void AstLiteralReindexer::VisitNativeFunctionLiteral(
 
 
 void AstLiteralReindexer::VisitDoExpression(DoExpression* node) {
-  // TODO(caitp): literals in do expressions need re-indexing too.
+  Visit(node->block());

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

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 =)

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

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.

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

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.

+  Visit(node->result());

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

`result` is always a VariableProxy, so it probably doesn't need to be visited

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

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.

 }