Fix desugaring of let bindings in for loops to handle continue properly

src/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/v8.h"
 
 #include "src/api.h"
 #include "src/ast.h"
 #include "src/bailout-reason.h"
 #include "src/base/platform/platform.h"
@@ skipping 2965 matching lines @@
                        result_done,
                        assign_each);
   } else {
     stmt->Initialize(each, subject, body);
   }
 }
 
 
 Statement* Parser::DesugarLetBindingsInForStatement(
     Scope* inner_scope, ZoneList<const AstRawString*>* names,
-    ForStatement* loop, Statement* init, Expression* cond, Statement* next,
+    ForStatement* loop, Statement* init, Expression* cond, Expression* next,
     Statement* body, bool* ok) {
   // ES6 13.6.3.4 specifies that on each loop iteration the let variables are
   // copied into a new environment. After copying, the "next" statement of the
   // loop is executed to update the loop variables. The loop condition is
   // checked and the loop body is executed.
   //
   // We rewrite a for statement of the form
   //
-  //  for (let x = i; cond; next) body
+  //  labels: for (let x = i; cond; next) body
   //
   // into
   //
   //  {
-  //     let x = i;
-  //     temp_x = x;
-  //     flag = 1;
-  //     for (;;) {
-  //        let x = temp_x;
-  //        if (flag == 1) {
-  //          flag = 0;
+  //    let x = i;
+  //    temp_x = x;
+  //    first = 1;
+  //    outer: for (;;) {
+  //      let x = temp_x;
+  //      if (first == 1) {
+  //        first = 0;
+  //      } else {
+  //        next;
+  //      }
+  //      flag = 1;
+  //      labels: for (; flag == 1; temp_x = x, flag = 0) {
+  //        if (cond) {
+  //          body
   //        } else {
-  //          next;
+  //          break outer;
   //        }
-  //        if (cond) {
-  //          <empty>
-  //        } else {
-  //          break;
-  //        }
-  //        b
-  //        temp_x = x;
-  //     }
+  //      }
+  //      if (flag == 1) {
+  //        break;
+  //      }
+  //    }
   //  }
 
   DCHECK(names->length() > 0);
   Scope* for_scope = scope_;
   ZoneList<Variable*> temps(names->length(), zone());
 
-  Block* outer_block = factory()->NewBlock(NULL, names->length() + 3, false,
+  Block* outer_block = factory()->NewBlock(NULL, names->length() + 2, false,
                                            RelocInfo::kNoPosition);
+
   outer_block->AddStatement(init, zone());
 
   const AstRawString* temp_name = ast_value_factory()->dot_for_string();
 
   // For each let variable x:
   //   make statement: temp_x = x.
   for (int i = 0; i < names->length(); i++) {
     VariableProxy* proxy =
         NewUnresolved(names->at(i), LET, Interface::NewValue());
     Variable* temp = scope_->DeclarationScope()->NewTemporary(temp_name);
     VariableProxy* temp_proxy = factory()->NewVariableProxy(temp);
     Assignment* assignment = factory()->NewAssignment(
         Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition);
     Statement* assignment_statement = factory()->NewExpressionStatement(
         assignment, RelocInfo::kNoPosition);
     outer_block->AddStatement(assignment_statement, zone());
     temps.Add(temp, zone());
   }
 
-  Variable* flag = scope_->DeclarationScope()->NewTemporary(temp_name);
-  // Make statement: flag = 1.
-  {
-    VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
-    Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
-    Assignment* assignment = factory()->NewAssignment(
-        Token::ASSIGN, flag_proxy, const1, RelocInfo::kNoPosition);
-    Statement* assignment_statement = factory()->NewExpressionStatement(
-        assignment, RelocInfo::kNoPosition);
-    outer_block->AddStatement(assignment_statement, zone());
-  }
+  // Make statement: outer: for (;;)
+  // Note that we don't actually create the label, or set this loop up as an
+  // explicit break target, instead handing it directly to those nodes that
+  // need to know about it. This should be safe because we don't run any code
+  // in this function that looks up break targets.
+  ForStatement* outer_loop =
+      factory()->NewForStatement(NULL, RelocInfo::kNoPosition);
+  outer_block->AddStatement(outer_loop, zone());
 
-  outer_block->AddStatement(loop, zone());
   outer_block->set_scope(for_scope);
   scope_ = inner_scope;
 
-  Block* inner_block = factory()->NewBlock(NULL, 2 * names->length() + 3,
-                                           false, RelocInfo::kNoPosition);
+  Block* inner_block = factory()->NewBlock(NULL, names->length() + 3, false,
+                                           RelocInfo::kNoPosition);
   int pos = scanner()->location().beg_pos;
   ZoneList<Variable*> inner_vars(names->length(), zone());
 
   // For each let variable x:
   //    make statement: let x = temp_x.
   for (int i = 0; i < names->length(); i++) {
     VariableProxy* proxy =
         NewUnresolved(names->at(i), LET, Interface::NewValue());
     Declaration* declaration =
         factory()->NewVariableDeclaration(proxy, LET, scope_, pos);
     Declare(declaration, true, CHECK_OK);
     inner_vars.Add(declaration->proxy()->var(), zone());
     VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i));
     Assignment* assignment = factory()->NewAssignment(
         Token::INIT_LET, proxy, temp_proxy, pos);
     Statement* assignment_statement = factory()->NewExpressionStatement(
         assignment, pos);
     proxy->var()->set_initializer_position(pos);
     inner_block->AddStatement(assignment_statement, zone());
   }
 
-  // Make statement: if (flag == 1) { flag = 0; } else { next; }.
-  if (next) {
+  Variable* flag = scope_->DeclarationScope()->NewTemporary(temp_name);
+  // Make statement: flag = 1.
+  {
+    VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
+    Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);

[arv (Not doing code reviews), 2014/11/13 16:15:43]

Maybe we should make these default to RelocInfo::kNoPosition? Would clean up the
code a bit.

+    Assignment* assignment = factory()->NewAssignment(
+        Token::ASSIGN, flag_proxy, const1, RelocInfo::kNoPosition);
+    Statement* assignment_statement =
+        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
+    inner_block->AddStatement(assignment_statement, zone());
+  }
+
+  // Make cond expression for main loop: flag == 1.
+  Expression* flag_cond = NULL;
+  {
+    Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
+    VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
+    flag_cond = factory()->NewCompareOperation(Token::EQ, flag_proxy, const1,
+                                               RelocInfo::kNoPosition);
+  }
+
+  // Create chain of expressions "next, temp_x = x, ..., flag = 0"
+  Statement* compound_next_statement = NULL;
+  {
+    Expression* compound_next = next;
+
+    // Make the comma-separated list of temp_x = x assignments.
+    for (int i = 0; i < names->length(); i++) {
+      VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i));
+      VariableProxy* proxy =
+          factory()->NewVariableProxy(inner_vars.at(i), RelocInfo::kNoPosition);
+      Assignment* assignment = factory()->NewAssignment(
+          Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition);
+      if (compound_next) {
+        compound_next = factory()->NewBinaryOperation(
+            Token::COMMA, compound_next, assignment, RelocInfo::kNoPosition);
+      } else {
+        compound_next = assignment;
+      }
+    }
+
+    // Make expression: flag = 0.
+    {
+      VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
+      Expression* const0 = factory()->NewSmiLiteral(0, RelocInfo::kNoPosition);
+      Assignment* assignment = factory()->NewAssignment(
+          Token::ASSIGN, flag_proxy, const0, RelocInfo::kNoPosition);
+      compound_next = factory()->NewBinaryOperation(
+          Token::COMMA, compound_next, assignment, RelocInfo::kNoPosition);
+    }
+
+    compound_next_statement = factory()->NewExpressionStatement(
+        compound_next, next ? next->position() : RelocInfo::kNoPosition);
+  }
+
+  // Make statement: if (cond) { body; } else { break aux; }
+  Statement* if_cond_body_else_stop = NULL;
+  {
+    Statement* stop =
+        factory()->NewBreakStatement(outer_loop, RelocInfo::kNoPosition);
+    if_cond_body_else_stop =
+        factory()->NewIfStatement(cond, body, stop, cond->position());
+  }
+
+  // TODO(adamk): Does the order matter?
+  loop->Initialize(NULL, flag_cond, compound_next_statement,
+                   if_cond_body_else_stop);
+  inner_block->AddStatement(loop, zone());
+
+  // Make statement: if (flag == 1) { break; }
+  {
+    Statement* empty = factory()->NewEmptyStatement(RelocInfo::kNoPosition);
     Expression* compare = NULL;
     // Make compare expresion: flag == 1.
     {
       Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
       VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
-      compare = factory()->NewCompareOperation(
-          Token::EQ, flag_proxy, const1, pos);
+      compare =
+          factory()->NewCompareOperation(Token::EQ, flag_proxy, const1, pos);
     }
-    Statement* clear_flag = NULL;
-    // Make statement: flag = 0.
-    {
-      VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
-      Expression* const0 = factory()->NewSmiLiteral(0, RelocInfo::kNoPosition);
-      Assignment* assignment = factory()->NewAssignment(
-          Token::ASSIGN, flag_proxy, const0, RelocInfo::kNoPosition);
-      clear_flag = factory()->NewExpressionStatement(assignment, pos);
-    }
-    Statement* clear_flag_or_next = factory()->NewIfStatement(
-        compare, clear_flag, next, RelocInfo::kNoPosition);
-    inner_block->AddStatement(clear_flag_or_next, zone());
-  }
-
-
-  // Make statement: if (cond) { } else { break; }.
-  if (cond) {
-    Statement* empty = factory()->NewEmptyStatement(RelocInfo::kNoPosition);
-    BreakableStatement* t = LookupBreakTarget(NULL, CHECK_OK);
-    Statement* stop = factory()->NewBreakStatement(t, RelocInfo::kNoPosition);
-    Statement* if_not_cond_break = factory()->NewIfStatement(
-        cond, empty, stop, cond->position());
-    inner_block->AddStatement(if_not_cond_break, zone());
-  }
-
-  inner_block->AddStatement(body, zone());
-
-  // For each let variable x:
-  //   make statement: temp_x = x;
-  for (int i = 0; i < names->length(); i++) {
-    VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i));
-    int pos = scanner()->location().end_pos;
-    VariableProxy* proxy = factory()->NewVariableProxy(inner_vars.at(i), pos);
-    Assignment* assignment = factory()->NewAssignment(
-        Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition);
-    Statement* assignment_statement = factory()->NewExpressionStatement(
-        assignment, RelocInfo::kNoPosition);
-    inner_block->AddStatement(assignment_statement, zone());
+    Statement* stop =
+        factory()->NewBreakStatement(outer_loop, RelocInfo::kNoPosition);
+    Statement* if_flag_break =
+        factory()->NewIfStatement(compare, stop, empty, RelocInfo::kNoPosition);
+    inner_block->AddStatement(if_flag_break, zone());
   }
 
   inner_scope->set_end_position(scanner()->location().end_pos);
   inner_block->set_scope(inner_scope);
   scope_ = for_scope;
 
-  loop->Initialize(NULL, NULL, NULL, inner_block);
+  outer_loop->Initialize(NULL, NULL, NULL, inner_block);
   return outer_block;
 }
 
 
 Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
                                      bool* ok) {
   // ForStatement ::
   //   'for' '(' Expression? ';' Expression? ';' Expression? ')' Statement
 
   int stmt_pos = peek_position();
@@ skipping 161 matching lines @@
     inner_scope->set_start_position(scanner()->location().beg_pos);
     scope_ = inner_scope;
   }
 
   Expression* cond = NULL;
   if (peek() != Token::SEMICOLON) {
     cond = ParseExpression(true, CHECK_OK);
   }
   Expect(Token::SEMICOLON, CHECK_OK);
 
-  Statement* next = NULL;
+  Expression* next = NULL;
   if (peek() != Token::RPAREN) {
-    int next_pos = position();
-    Expression* exp = ParseExpression(true, CHECK_OK);
-    next = factory()->NewExpressionStatement(exp, next_pos);
+    next = ParseExpression(true, CHECK_OK);
   }
   Expect(Token::RPAREN, CHECK_OK);
 
   Statement* body = ParseStatement(NULL, CHECK_OK);
 
   Statement* result = NULL;
   if (let_bindings.length() > 0) {
     scope_ = for_scope;
     result = DesugarLetBindingsInForStatement(inner_scope, &let_bindings, loop,
                                               init, cond, next, body, CHECK_OK);
     scope_ = saved_scope;
     for_scope->set_end_position(scanner()->location().end_pos);
   } else {
+    Statement* next_stmt = NULL;
+    if (next) {
+      next_stmt = factory()->NewExpressionStatement(next, next->position());
+    }
     scope_ = saved_scope;
     for_scope->set_end_position(scanner()->location().end_pos);
     for_scope = for_scope->FinalizeBlockScope();
     if (for_scope) {
       // Rewrite a for statement of the form
       //   for (const x = i; c; n) b
       //
       // into
       //
       //   {
       //     const x = i;
       //     for (; c; n) b
       //   }
       DCHECK(init != NULL);
       Block* block =
           factory()->NewBlock(NULL, 2, false, RelocInfo::kNoPosition);
       block->AddStatement(init, zone());
       block->AddStatement(loop, zone());
       block->set_scope(for_scope);
-      loop->Initialize(NULL, cond, next, body);
+      loop->Initialize(NULL, cond, next_stmt, body);
       result = block;
     } else {
-      loop->Initialize(init, cond, next, body);
+      loop->Initialize(init, cond, next_stmt, body);
       result = loop;
     }
   }
   return result;
 }
 
 
 DebuggerStatement* Parser::ParseDebuggerStatement(bool* ok) {
   // In ECMA-262 'debugger' is defined as a reserved keyword. In some browser
   // contexts this is used as a statement which invokes the debugger as i a
@@ skipping 1648 matching lines @@
 
   // We cannot internalize on a background thread; a foreground task will take
   // care of calling Parser::Internalize just before compilation.
 
   if (compile_options() == ScriptCompiler::kProduceParserCache) {
     if (result != NULL) *info_->cached_data() = recorder.GetScriptData();
     log_ = NULL;
   }
 }
 } }  // namespace v8::internal