[es6] Fix completion values of for loops with lexical variables

Currently, the desugaring of for loops of the form for
(let/const ...; bla; bla) causes them to always have a
completion value of 1, regardless of whether the loop body
is executed or not. This CL fixes this, realigning
initializer blocks as a more general purpose way to avoid
the completion value rewriter (since that's all they really
do anyway).

BUG=

Review URL: https://codereview.chromium.org/1177053006

Cr-Commit-Position: refs/heads/master@{#29108}

diff --git a/src/ast.h b/src/ast.h
index 7bdd520bbaf6e6559e8dae2f273facfb6bf1969f..8eae849358dd5de1c3d168b0cd8d9b42baf38454 100644 (file)
--- a/src/ast.h
+++ b/src/ast.h
@@ -484,7 +484,7 @@ class Block final : public BreakableStatement {
   }
 
   ZoneList<Statement*>* statements() { return &statements_; }
-  bool is_initializer_block() const { return is_initializer_block_; }
+  bool ignore_completion_value() const { return ignore_completion_value_; }
 
   static int num_ids() { return parent_num_ids() + 1; }
   BailoutId DeclsId() const { return BailoutId(local_id(0)); }
@@ -499,10 +499,10 @@ class Block final : public BreakableStatement {
 
  protected:
   Block(Zone* zone, ZoneList<const AstRawString*>* labels, int capacity,
-        bool is_initializer_block, int pos)
+        bool ignore_completion_value, int pos)
       : BreakableStatement(zone, labels, TARGET_FOR_NAMED_ONLY, pos),
         statements_(capacity, zone),
-        is_initializer_block_(is_initializer_block),
+        ignore_completion_value_(ignore_completion_value),
         scope_(NULL) {}
   static int parent_num_ids() { return BreakableStatement::num_ids(); }
 
@@ -510,7 +510,7 @@ class Block final : public BreakableStatement {
   int local_id(int n) const { return base_id() + parent_num_ids() + n; }
 
   ZoneList<Statement*> statements_;
-  bool is_initializer_block_;
+  bool ignore_completion_value_;
   Scope* scope_;
 };
 
@@ -3286,12 +3286,10 @@ class AstNodeFactory final BASE_EMBEDDED {
     return new (zone_) ExportDeclaration(zone_, proxy, scope, pos);
   }
 
-  Block* NewBlock(ZoneList<const AstRawString*>* labels,
-                  int capacity,
-                  bool is_initializer_block,
-                  int pos) {
+  Block* NewBlock(ZoneList<const AstRawString*>* labels, int capacity,
+                  bool ignore_completion_value, int pos) {
     return new (zone_)
-        Block(zone_, labels, capacity, is_initializer_block, pos);
+        Block(zone_, labels, capacity, ignore_completion_value, pos);
   }
 
 #define STATEMENT_WITH_LABELS(NodeType)                                     \

diff --git a/src/parser.cc b/src/parser.cc
index 41a5d36b04ca84962ac1e40fa9c44b4c32e351e3..7e0d9366116f3b8bd7619a78ceb39e6a6901cd1e 100644 (file)
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3230,14 +3230,21 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
   //    let/const x = i;
   //    temp_x = x;
   //    first = 1;
+  //    undefined;
   //    outer: for (;;) {
-  //      let/const x = temp_x;
-  //      if (first == 1) {
-  //        first = 0;
-  //      } else {
-  //        next;
+  //      { // This block's only function is to ensure that the statements it
+  //        // contains do not affect the normal completion value. This is
+  //        // accomplished by setting its ignore_completion_value bit.
+  //        // No new lexical scope is introduced, so lexically scoped variables
+  //        // declared here will be scoped to the outer for loop.
+  //        let/const x = temp_x;
+  //        if (first == 1) {
+  //          first = 0;
+  //        } else {
+  //          next;
+  //        }
+  //        flag = 1;
   //      }
-  //      flag = 1;
   //      labels: for (; flag == 1; flag = 0, temp_x = x) {
   //        if (cond) {
   //          body
@@ -3290,6 +3297,13 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
     outer_block->AddStatement(assignment_statement, zone());
   }
 
+  // make statement: undefined;
+  outer_block->AddStatement(
+      factory()->NewExpressionStatement(
+          factory()->NewUndefinedLiteral(RelocInfo::kNoPosition),
+          RelocInfo::kNoPosition),
+      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
@@ -3302,8 +3316,10 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
   outer_block->set_scope(for_scope);
   scope_ = inner_scope;
 
-  Block* inner_block = factory()->NewBlock(NULL, names->length() + 4, false,
-                                           RelocInfo::kNoPosition);
+  Block* inner_block =
+      factory()->NewBlock(NULL, 3, false, RelocInfo::kNoPosition);
+  Block* ignore_completion_block = factory()->NewBlock(
+      NULL, names->length() + 2, true, RelocInfo::kNoPosition);
   ZoneList<Variable*> inner_vars(names->length(), zone());
   // For each let variable x:
   //    make statement: let/const x = temp_x.
@@ -3322,7 +3338,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
         factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
     DCHECK(init->position() != RelocInfo::kNoPosition);
     proxy->var()->set_initializer_position(init->position());
-    inner_block->AddStatement(assignment_statement, zone());
+    ignore_completion_block->AddStatement(assignment_statement, zone());
   }
 
   // Make statement: if (first == 1) { first = 0; } else { next; }
@@ -3348,7 +3364,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
     }
     Statement* clear_first_or_next = factory()->NewIfStatement(
         compare, clear_first, next, RelocInfo::kNoPosition);
-    inner_block->AddStatement(clear_first_or_next, zone());
+    ignore_completion_block->AddStatement(clear_first_or_next, zone());
   }
 
   Variable* flag = scope_->DeclarationScope()->NewTemporary(temp_name);
@@ -3360,9 +3376,9 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
         Token::ASSIGN, flag_proxy, const1, RelocInfo::kNoPosition);
     Statement* assignment_statement =
         factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
-    inner_block->AddStatement(assignment_statement, zone());
+    ignore_completion_block->AddStatement(assignment_statement, zone());
   }
-
+  inner_block->AddStatement(ignore_completion_block, zone());
   // Make cond expression for main loop: flag == 1.
   Expression* flag_cond = NULL;
   {
@@ -3410,7 +3426,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
   }
 
   // Make statement: labels: for (; flag == 1; flag = 0, temp_x = x)
-  // Note that we re-use the original loop node, which retains it labels
+  // Note that we re-use the original loop node, which retains its labels
   // and ensures that any break or continue statements in body point to
   // the right place.
   loop->Initialize(NULL, flag_cond, compound_next_statement, body_or_stop);

diff --git a/src/prettyprinter.cc b/src/prettyprinter.cc
index 284beadfeedb5a7dc64472a6372a4091e6a42eff..198ca9dfec8539f31cdf7feb2a9e65bacd65f438 100644 (file)
--- a/src/prettyprinter.cc
+++ b/src/prettyprinter.cc
@@ -433,10 +433,10 @@ PrettyPrinter::~PrettyPrinter() {
 
 
 void PrettyPrinter::VisitBlock(Block* node) {
-  if (!node->is_initializer_block()) Print("{ ");
+  if (!node->ignore_completion_value()) Print("{ ");
   PrintStatements(node->statements());
   if (node->statements()->length() > 0) Print(" ");
-  if (!node->is_initializer_block()) Print("}");
+  if (!node->ignore_completion_value()) Print("}");
 }
 
 
@@ -1146,7 +1146,8 @@ void AstPrinter::PrintArguments(ZoneList<Expression*>* arguments) {
 
 
 void AstPrinter::VisitBlock(Block* node) {
-  const char* block_txt = node->is_initializer_block() ? "BLOCK INIT" : "BLOCK";
+  const char* block_txt =
+      node->ignore_completion_value() ? "BLOCK NOCOMPLETIONS" : "BLOCK";
   IndentedScope indent(this, block_txt);
   PrintStatements(node->statements());
 }

diff --git a/src/rewriter.cc b/src/rewriter.cc
index ee08d885f602f2d110e60a540cf7a0e4281ce241..c901653a2ba3c6aff53a4e13fb463f0a667c7ddf 100644 (file)
--- a/src/rewriter.cc
+++ b/src/rewriter.cc
@@ -83,7 +83,7 @@ void Processor::VisitBlock(Block* node) {
   // with some JS VMs: For instance, using smjs, print(eval('var x = 7'))
   // returns 'undefined'. To obtain the same behavior with v8, we need
   // to prevent rewriting in that case.
-  if (!node->is_initializer_block()) Process(node->statements());
+  if (!node->ignore_completion_value()) Process(node->statements());
 }
 
 

diff --git a/test/mjsunit/es6/block-for.js b/test/mjsunit/es6/block-for.js
index b91af0116cadb8a1f68b6240d7c6b494d6e52d16..420c41e610cce619cdfd7e5a549b2d680979a52b 100644 (file)
--- a/test/mjsunit/es6/block-for.js
+++ b/test/mjsunit/es6/block-for.js
@@ -158,7 +158,7 @@ closure_in_for_next();
 
 
 // In a for-in statement the iteration variable is fresh
-// for earch iteration.
+// for each iteration.
 function closures3(x) {
   let a = [];
   for (let p in x) {
@@ -171,3 +171,28 @@ function closures3(x) {
   }
 }
 closures3({a : [0], b : 1, c : {v : 1}, get d() {}, set e(x) {}});
+
+// Check normal for statement completion values.
+assertEquals(1, eval("for (let i = 0; i < 10; i++) { 1; }"));
+assertEquals(9, eval("for (let i = 0; i < 10; i++) { i; }"));
+assertEquals(undefined, eval("for (let i = 0; false;) { }"));
+assertEquals(undefined, eval("for (const i = 0; false;) { }"));
+assertEquals(undefined, eval("for (let i = 0; i < 10; i++) { }"));
+assertEquals(undefined, eval("for (let i = 0; false;) { i; }"));
+assertEquals(undefined, eval("for (const i = 0; false;) { i; }"));
+assertEquals(undefined, eval("for (let i = 0; true;) { break; }"));
+assertEquals(undefined, eval("for (const i = 0; true;) { break; }"));
+assertEquals(undefined, eval("for (let i = 0; i < 10; i++) { continue; }"));
+assertEquals(undefined, eval("for (let i = 0; true;) { break; i; }"));
+assertEquals(undefined, eval("for (const i = 0; true;) { break; i; }"));
+assertEquals(undefined, eval("for (let i = 0; i < 10; i++) { continue; i; }"));
+assertEquals(0, eval("for (let i = 0; true;) { i; break; }"));
+assertEquals(0, eval("for (const i = 0; true;) { i; break; }"));
+assertEquals(9, eval("for (let i = 0; i < 10; i++) { i; continue; }"));
+assertEquals(3, eval("for (let i = 0; true; i++) { i; if (i >= 3) break; }"));
+assertEquals(2, eval("for (let i = 0; true; i++) { if (i >= 3) break; i; }"));
+assertEquals(
+  2, eval("for (let i = 0; i < 10; i++) { if (i >= 3) continue; i; }"));
+assertEquals(undefined, eval("foo: for (let i = 0; true;) { break foo; }"));
+assertEquals(undefined, eval("foo: for (const i = 0; true;) { break foo; }"));
+assertEquals(3, eval("foo: for (let i = 3; true;) { i; break foo; }"));