Fix re-indexing for literals in do-expressions

src/ast/ast-literal-reindexer.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/ast/ast-literal-reindexer.h"
 
 #include "src/ast/ast.h"
 #include "src/ast/scopes.h"
 
 namespace v8 {
@@ skipping 26 matching lines @@
 
 
 void AstLiteralReindexer::VisitDebuggerStatement(DebuggerStatement* node) {}
 
 
 void AstLiteralReindexer::VisitNativeFunctionLiteral(
     NativeFunctionLiteral* node) {}
 
 
 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.

 }
 
 
 void AstLiteralReindexer::VisitLiteral(Literal* node) {}
 
 
 void AstLiteralReindexer::VisitRegExpLiteral(RegExpLiteral* node) {
   UpdateIndex(node);
 }
 
@@ skipping 264 matching lines @@
 void AstLiteralReindexer::VisitFunctionLiteral(FunctionLiteral* node) {
   // We don't recurse into the declarations or body of the function literal:
 }
 
 
 void AstLiteralReindexer::Reindex(Expression* pattern) {
   pattern->Accept(this);
 }
 }  // namespace internal
 }  // namespace v8