Remove duplicated ForOfStatement init code code from RewriteSpreads

src/parsing/parser.cc

 // Copyright 2012 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/parsing/parser.h"
 
 #include "src/api.h"
 #include "src/ast/ast.h"
 #include "src/ast/ast-expression-rewriter.h"
 #include "src/ast/ast-expression-visitor.h"
@@ skipping 3239 matching lines @@
   throw_arguments->Add(result_proxy_again, zone());
   Expression* throw_call = factory()->NewCallRuntime(
       Runtime::kThrowIteratorResultNotAnObject, throw_arguments, pos);
 
   return factory()->NewBinaryOperation(
       Token::AND,
       factory()->NewUnaryOperation(Token::NOT, is_spec_object_call, pos),
       throw_call, pos);
 }
 
-
 void Parser::InitializeForEachStatement(ForEachStatement* stmt,
                                         Expression* each, Expression* subject,
-                                        Statement* body,
-                                        bool is_destructuring) {
-  DCHECK(!is_destructuring || allow_harmony_destructuring_assignment());
+                                        Statement* body) {
   ForOfStatement* for_of = stmt->AsForOfStatement();
-
   if (for_of != NULL) {
-    Variable* iterator = scope_->NewTemporary(
-        ast_value_factory()->dot_iterator_string());
-    Variable* result = scope_->NewTemporary(
-        ast_value_factory()->dot_result_string());
-
-    Expression* assign_iterator;
-    Expression* next_result;
-    Expression* result_done;
-    Expression* assign_each;
-
-    // iterator = subject[Symbol.iterator]()
-    // Hackily disambiguate o from o.next and o [Symbol.iterator]().
-    // TODO(verwaest): Come up with a better solution.
-    assign_iterator = factory()->NewAssignment(
-        Token::ASSIGN, factory()->NewVariableProxy(iterator),
-        GetIterator(subject, factory(), subject->position() - 2),
-        subject->position());
-
-    // !%_IsJSReceiver(result = iterator.next()) &&
-    //     %ThrowIteratorResultNotAnObject(result)
-    {
-      // result = iterator.next()
-      Expression* iterator_proxy = factory()->NewVariableProxy(iterator);
-      // Hackily disambiguate o from o.next and o [Symbol.iterator]().
-      // TODO(verwaest): Come up with a better solution.
-      next_result = BuildIteratorNextResult(iterator_proxy, result,
-                                            subject->position() - 1);
-    }
-
-    // result.done
-    {
-      Expression* done_literal = factory()->NewStringLiteral(
-          ast_value_factory()->done_string(), RelocInfo::kNoPosition);
-      Expression* result_proxy = factory()->NewVariableProxy(result);
-      result_done = factory()->NewProperty(
-          result_proxy, done_literal, RelocInfo::kNoPosition);
-    }
-
-    // each = result.value
-    {
-      Expression* value_literal = factory()->NewStringLiteral(
-          ast_value_factory()->value_string(), RelocInfo::kNoPosition);
-      Expression* result_proxy = factory()->NewVariableProxy(result);
-      Expression* result_value = factory()->NewProperty(
-          result_proxy, value_literal, RelocInfo::kNoPosition);
-      assign_each = factory()->NewAssignment(Token::ASSIGN, each, result_value,
-                                             RelocInfo::kNoPosition);
-      if (is_destructuring) {
-        assign_each = PatternRewriter::RewriteDestructuringAssignment(
-            this, assign_each->AsAssignment(), scope_);
-      }
-    }
-
-    for_of->Initialize(each, subject, body,
-                       iterator,
-                       assign_iterator,
-                       next_result,
-                       result_done,
-                       assign_each);
+    InitializeForOfStatement(for_of, each, subject, body, true);
   } else {
-    if (is_destructuring) {
+    if (each->IsArrayLiteral() || each->IsObjectLiteral()) {
+      DCHECK(allow_harmony_destructuring_assignment());
       Variable* temp =
           scope_->NewTemporary(ast_value_factory()->empty_string());
       VariableProxy* temp_proxy = factory()->NewVariableProxy(temp);
       Expression* assign_each = PatternRewriter::RewriteDestructuringAssignment(
           this, factory()->NewAssignment(Token::ASSIGN, each, temp_proxy,
                                          RelocInfo::kNoPosition),
           scope_);
       auto block =
           factory()->NewBlock(nullptr, 2, false, RelocInfo::kNoPosition);
       block->statements()->Add(factory()->NewExpressionStatement(
                                    assign_each, RelocInfo::kNoPosition),
                                zone());
       block->statements()->Add(body, zone());
       body = block;
       each = factory()->NewVariableProxy(temp);
     }
     stmt->Initialize(each, subject, body);
   }
 }
 
+void Parser::InitializeForOfStatement(ForOfStatement* for_of, Expression* each,
+                                      Expression* subject, Statement* body,
+                                      bool use_position_hack) {

[nickie, 2016/03/04 14:15:13]

As a fix, I suggest that you replace "bool use_position_hack" with "int
iterable_position".  The default value would be something illegal as a position
(e.g., -1) and this would trigger the "position hack".

+  Variable* iterator =
+      scope_->NewTemporary(ast_value_factory()->dot_iterator_string());
+  Variable* result =
+      scope_->NewTemporary(ast_value_factory()->dot_result_string());
+
+  Expression* assign_iterator;
+  Expression* next_result;
+  Expression* result_done;
+  Expression* assign_each;
+
+  // Hackily disambiguate o from o.next and o [Symbol.iterator]().
+  // TODO(verwaest): Come up with a better solution.
+  int get_iterator_pos = subject->position() - (use_position_hack * 2);
+  int next_result_pos = subject->position() - (use_position_hack * 1);

[nickie, 2016/03/04 14:15:13]

These two would become:

int get_iterator_pos =
  iterable_position < 0 ? subject->position() - 2 : iterable_position;
int next_result_pos =
  iterable_position < 0 ? subject->position() - 1 : iterable_position;

+
+  // iterator = subject[Symbol.iterator]()
+  assign_iterator = factory()->NewAssignment(
+      Token::ASSIGN, factory()->NewVariableProxy(iterator),
+      GetIterator(subject, factory(), get_iterator_pos), subject->position());
+
+  // !%_IsJSReceiver(result = iterator.next()) &&
+  //     %ThrowIteratorResultNotAnObject(result)
+  {
+    // result = iterator.next()
+    Expression* iterator_proxy = factory()->NewVariableProxy(iterator);
+    next_result =
+        BuildIteratorNextResult(iterator_proxy, result, next_result_pos);
+  }
+
+  // result.done
+  {
+    Expression* done_literal = factory()->NewStringLiteral(
+        ast_value_factory()->done_string(), RelocInfo::kNoPosition);
+    Expression* result_proxy = factory()->NewVariableProxy(result);
+    result_done = factory()->NewProperty(result_proxy, done_literal,
+                                         RelocInfo::kNoPosition);
+  }
+
+  // each = result.value
+  {
+    Expression* value_literal = factory()->NewStringLiteral(
+        ast_value_factory()->value_string(), RelocInfo::kNoPosition);
+    Expression* result_proxy = factory()->NewVariableProxy(result);
+    Expression* result_value = factory()->NewProperty(
+        result_proxy, value_literal, RelocInfo::kNoPosition);
+    assign_each = factory()->NewAssignment(Token::ASSIGN, each, result_value,
+                                           RelocInfo::kNoPosition);
+    if (each->IsArrayLiteral() || each->IsObjectLiteral()) {
+      DCHECK(allow_harmony_destructuring_assignment());
+      assign_each = PatternRewriter::RewriteDestructuringAssignment(
+          this, assign_each->AsAssignment(), scope_);
+    }
+  }
+
+  for_of->Initialize(each, subject, body, iterator, assign_iterator,
+                     next_result, result_done, assign_each);
+}
+
 Statement* Parser::DesugarLexicalBindingsInForStatement(
     Scope* inner_scope, VariableMode mode, ZoneList<const AstRawString*>* names,
     ForStatement* loop, Statement* init, Expression* cond, Statement* next,
     Statement* body, bool* ok) {
   // ES6 13.7.4.8 specifies that on each loop iteration the let variables are
   // copied into a new environment.  Moreover, the "next" statement must be
   // evaluated not in the environment of the just completed iteration but in
   // that of the upcoming one.  We achieve this with the following desugaring.
   // Extra care is needed to preserve the completion value of the original loop.
   //
@@ skipping 362 matching lines @@
                 each_initialization_block, &descriptor, &decl,
                 IsLexicalVariableMode(descriptor.mode) ? &lexical_bindings
                                                        : nullptr,
                 CHECK_OK);
           }
 
           body_block->statements()->Add(each_initialization_block, zone());
           body_block->statements()->Add(body, zone());
           VariableProxy* temp_proxy =
               factory()->NewVariableProxy(temp, each_beg_pos, each_end_pos);
-          InitializeForEachStatement(loop, temp_proxy, enumerable, body_block,
-                                     false);
+          InitializeForEachStatement(loop, temp_proxy, enumerable, body_block);
         }
         body_scope->set_end_position(scanner()->location().end_pos);
         body_scope = body_scope->FinalizeBlockScope();
         body_block->set_scope(body_scope);
 
         // Create a TDZ for any lexically-bound names.
         if (IsLexicalVariableMode(parsing_result.descriptor.mode)) {
           DCHECK_NULL(init_block);
 
           init_block =
@@ skipping 87 matching lines @@
         // lexical binding is introduced which overlaps with the for-in/of,
         // expressions in head of the loop should actually have variables
         // resolved in the outer scope.
         Scope* body_scope = NewScope(for_scope, BLOCK_SCOPE);
         {
           BlockState block_state(&scope_, body_scope);
           Block* block =
               factory()->NewBlock(NULL, 1, false, RelocInfo::kNoPosition);
           Statement* body = ParseSubStatement(NULL, CHECK_OK);
           block->statements()->Add(body, zone());
-          InitializeForEachStatement(loop, expression, enumerable, block,
-                                     is_destructuring);
+          InitializeForEachStatement(loop, expression, enumerable, block);
           body_scope->set_end_position(scanner()->location().end_pos);
           body_scope = body_scope->FinalizeBlockScope();
           block->set_scope(body_scope);
         }
 
         Statement* final_loop = loop->IsForOfStatement()
             ? FinalizeForOfStatement(
                 loop->AsForOfStatement(), RelocInfo::kNoPosition)
             : loop;
 
@@ skipping 1807 matching lines @@
               factory()->NewCallRuntime(Runtime::kAppendElement,
                                         append_element_args,
                                         RelocInfo::kNoPosition),
               RelocInfo::kNoPosition),
           zone());
     } else {
       // If it's a spread, we're adding a for/of loop iterating through it.
       Variable* each =
           scope_->NewTemporary(ast_value_factory()->dot_for_string());
       Expression* subject = spread->expression();
-      Variable* iterator =
-          scope_->NewTemporary(ast_value_factory()->dot_iterator_string());
-      Variable* element =
-          scope_->NewTemporary(ast_value_factory()->dot_result_string());
-      // iterator = subject[Symbol.iterator]()
-      Expression* assign_iterator = factory()->NewAssignment(
-          Token::ASSIGN, factory()->NewVariableProxy(iterator),
-          GetIterator(subject, factory(), spread->expression_position()),

[nickie, 2016/03/04 14:15:13]

There was another tricky point but it was a bit hidden and I missed it in my
first comment.  In the line above, it's spread->expression_position() and not
spread->expression->position().  There's an extra field for spreads keeping the
starting position of the expression after the dots; this is the one used here.

So, for "...42(undefined)", spread->expression->position() would have been the
opening parenthesis.  Instead, the original code was taking
spread->expression_position() which was the digit 4.  As far as I understand,
this is what guides the error messaging system to report the right expression
that's guilty. 

-          subject->position());
-      // !%_IsJSReceiver(element = iterator.next()) &&
-      //     %ThrowIteratorResultNotAnObject(element)
-      Expression* next_element;
-      {
-        // element = iterator.next()
-        Expression* iterator_proxy = factory()->NewVariableProxy(iterator);
-        next_element = BuildIteratorNextResult(iterator_proxy, element,
-                                               spread->expression_position());

[nickie, 2016/03/04 14:15:13]

Here too...

-      }
-      // element.done
-      Expression* element_done;
-      {
-        Expression* done_literal = factory()->NewStringLiteral(
-            ast_value_factory()->done_string(), RelocInfo::kNoPosition);
-        Expression* element_proxy = factory()->NewVariableProxy(element);
-        element_done = factory()->NewProperty(element_proxy, done_literal,
-                                              RelocInfo::kNoPosition);
-      }
-      // each = element.value
-      Expression* assign_each;
-      {
-        Expression* value_literal = factory()->NewStringLiteral(
-            ast_value_factory()->value_string(), RelocInfo::kNoPosition);
-        Expression* element_proxy = factory()->NewVariableProxy(element);
-        Expression* element_value = factory()->NewProperty(
-            element_proxy, value_literal, RelocInfo::kNoPosition);
-        assign_each = factory()->NewAssignment(
-            Token::ASSIGN, factory()->NewVariableProxy(each), element_value,
-            RelocInfo::kNoPosition);
-      }
       // %AppendElement($R, each)
       Statement* append_body;
       {
         ZoneList<Expression*>* append_element_args =
             NewExpressionList(2, zone());
         append_element_args->Add(factory()->NewVariableProxy(result), zone());
         append_element_args->Add(factory()->NewVariableProxy(each), zone());
         append_body = factory()->NewExpressionStatement(
             factory()->NewCallRuntime(Runtime::kAppendElement,
                                       append_element_args,
                                       RelocInfo::kNoPosition),
             RelocInfo::kNoPosition);
       }
       // for (each of spread) %AppendElement($R, each)
       ForEachStatement* loop = factory()->NewForEachStatement(
           ForEachStatement::ITERATE, nullptr, RelocInfo::kNoPosition);
-      ForOfStatement* for_of = loop->AsForOfStatement();
-      for_of->Initialize(factory()->NewVariableProxy(each), subject,
-                         append_body, iterator, assign_iterator, next_element,
-                         element_done, assign_each);
-      do_block->statements()->Add(for_of, zone());
+      InitializeForOfStatement(loop->AsForOfStatement(),
+                               factory()->NewVariableProxy(each), subject,
+                               append_body);
+      do_block->statements()->Add(loop, zone());
     }
   }
   // Now, rewind the original array literal to truncate everything from the
   // first spread (included) until the end. This fixes $R's initialization.
   lit->RewindSpreads();
   return factory()->NewDoExpression(do_block, result, lit->position());
 }
 
 
 void ParserTraits::QueueDestructuringAssignmentForRewriting(Expression* expr) {
@@ skipping 1200 matching lines @@
   Expression* do_each =
       factory->NewDoExpression(new_assign_each, var_each, nopos);
   loop->set_assign_each(do_each);
 
   return final_loop;
 }
 
 
 }  // namespace internal
 }  // namespace v8