Implement iterator finalization in array destructuring.

src/parsing/pattern-rewriter.cc

Index: src/parsing/pattern-rewriter.cc
diff --git a/src/parsing/pattern-rewriter.cc b/src/parsing/pattern-rewriter.cc
index 2a194ee14b32d9145bddd82f26285ab86ad81241..0afa757b83bd8e014ad4c6d7efdcac79f51994de 100644
--- a/src/parsing/pattern-rewriter.cc
+++ b/src/parsing/pattern-rewriter.cc
@@ -407,16 +407,27 @@ void Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* node) {
 
 void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node,
                                                 Variable** temp_var) {
-  auto temp = *temp_var = CreateTempVar(current_value_);
-
-  block_->statements()->Add(parser_->BuildAssertIsCoercible(temp), zone());
+  // TODO(neis): DCHECK(block_->ignore_completion_value());
 
+  auto temp = *temp_var = CreateTempVar(current_value_);
   auto iterator = CreateTempVar(parser_->GetIterator(
       factory()->NewVariableProxy(temp), factory(), RelocInfo::kNoPosition));
   auto done = CreateTempVar(
       factory()->NewBooleanLiteral(false, RelocInfo::kNoPosition));
   auto result = CreateTempVar();
   auto v = CreateTempVar();
+  auto completion = CreateTempVar();
+  auto nopos = RelocInfo::kNoPosition;
+
+  // For the purpose of iterator finalization, we temporarily set block_ to a
+  // new block.  In the main body of this function, we write to block_ (both
+  // explicitly and implicitly via recursion).  At the end of the function, we
+  // wrap this new block in a try-finally statement, restore block_ to its
+  // original value, and add the try-finally statement to block_.
+  auto target = block_;
+  if (FLAG_harmony_iterator_close) {
+    block_ = factory()->NewBlock(nullptr, 8, true, nopos);
+  }
 
   Spread* spread = nullptr;
   for (Expression* value : *node->values()) {
@@ -426,86 +437,193 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node,
     }
 
     PatternContext context = SetInitializerContextIfNeeded(value);
+
     // if (!done) {
     //   result = IteratorNext(iterator);
     //   v = (done = result.done) ? undefined : result.value;
     // }
-    auto next_block =
-        factory()->NewBlock(nullptr, 2, true, RelocInfo::kNoPosition);
-    next_block->statements()->Add(factory()->NewExpressionStatement(
-                                      parser_->BuildIteratorNextResult(
-                                          factory()->NewVariableProxy(iterator),
-                                          result, RelocInfo::kNoPosition),
-                                      RelocInfo::kNoPosition),
-                                  zone());
-
-    auto assign_to_done = factory()->NewAssignment(
-        Token::ASSIGN, factory()->NewVariableProxy(done),
-        factory()->NewProperty(
-            factory()->NewVariableProxy(result),
-            factory()->NewStringLiteral(ast_value_factory()->done_string(),
-                                        RelocInfo::kNoPosition),
-            RelocInfo::kNoPosition),
-        RelocInfo::kNoPosition);
-    auto next_value = factory()->NewConditional(
-        assign_to_done, factory()->NewUndefinedLiteral(RelocInfo::kNoPosition),
-        factory()->NewProperty(
-            factory()->NewVariableProxy(result),
-            factory()->NewStringLiteral(ast_value_factory()->value_string(),
-                                        RelocInfo::kNoPosition),
-            RelocInfo::kNoPosition),
-        RelocInfo::kNoPosition);
-    next_block->statements()->Add(
-        factory()->NewExpressionStatement(
-            factory()->NewAssignment(Token::ASSIGN,
-                                     factory()->NewVariableProxy(v), next_value,
-                                     RelocInfo::kNoPosition),
-            RelocInfo::kNoPosition),
-        zone());
+    Statement* if_statement;
+    {
+      auto next_block =
+          factory()->NewBlock(nullptr, 2, true, nopos);

[adamk, 2016/03/07 20:28:05]

Can you leave stylistic changes out of this patch? The diff is already pretty
big, and this just makes the change harder to review (for me, anyway).

[neis, 2016/03/08 13:40:10]

Ok, I removed them.

+      next_block->statements()->Add(
+          factory()->NewExpressionStatement(
+              parser_->BuildIteratorNextResult(
+                  factory()->NewVariableProxy(iterator), result, nopos),
+              nopos),
+          zone());
 
-    auto if_statement = factory()->NewIfStatement(
-        factory()->NewUnaryOperation(Token::NOT,
-                                     factory()->NewVariableProxy(done),
-                                     RelocInfo::kNoPosition),
-        next_block, factory()->NewEmptyStatement(RelocInfo::kNoPosition),
-        RelocInfo::kNoPosition);
+      auto assign_to_done = factory()->NewAssignment(
+          Token::ASSIGN, factory()->NewVariableProxy(done),
+          factory()->NewProperty(
+              factory()->NewVariableProxy(result),
+              factory()->NewStringLiteral(ast_value_factory()->done_string(),
+                                          nopos),
+              nopos),
+          nopos);
+      auto next_value = factory()->NewConditional(
+          assign_to_done, factory()->NewUndefinedLiteral(nopos),
+          factory()->NewProperty(
+              factory()->NewVariableProxy(result),
+              factory()->NewStringLiteral(ast_value_factory()->value_string(),
+                                          nopos),
+              nopos),
+          nopos);
+      next_block->statements()->Add(
+          factory()->NewExpressionStatement(
+              factory()->NewAssignment(
+                  Token::ASSIGN, factory()->NewVariableProxy(v), next_value,
+                  nopos),
+              nopos),
+          zone());
+
+      if_statement = factory()->NewIfStatement(
+          factory()->NewUnaryOperation(Token::NOT,
+                                       factory()->NewVariableProxy(done),
+                                       nopos),
+          next_block, factory()->NewEmptyStatement(nopos),
+          nopos);
+    }
     block_->statements()->Add(if_statement, zone());
 
     if (!(value->IsLiteral() && value->AsLiteral()->raw_value()->IsTheHole())) {
+      if (FLAG_harmony_iterator_close) {
+        // completion = ABRUPT;
+        Expression* proxy = factory()->NewVariableProxy(completion);
+        Expression* assignment = factory()->NewAssignment(
+            Token::ASSIGN, proxy,
+            factory()->NewSmiLiteral(ABRUPT, nopos),
+            nopos);
+        block_->statements()->Add(
+            factory()->NewExpressionStatement(assignment, nopos), zone());
+      }
+
       RecurseIntoSubpattern(value, factory()->NewVariableProxy(v));
+
+      if (FLAG_harmony_iterator_close) {
+        // completion = NORMAL;
+        Expression* proxy = factory()->NewVariableProxy(completion);
+        Expression* assignment = factory()->NewAssignment(
+            Token::ASSIGN, proxy, factory()->NewSmiLiteral(NORMAL, nopos),
+            nopos);
+        block_->statements()->Add(
+            factory()->NewExpressionStatement(assignment, nopos), zone());
+      }
     }
     set_context(context);
   }
 
   if (spread != nullptr) {
-    // array = [];
-    // if (!done) %concat_iterable_to_array(array, iterator);
+    // A spread can only occur as the last component.  It is not handled by
+    // RecurseIntoSubpattern above.
+    //
+    // let array = [];
+    // while (!done) {

[adamk, 2016/03/07 20:28:05]

This change is already giant (and thus tricky to review); could you leave the
call out to %concat_iterable_to_array() in this one?

We'll always exhaust the iterator when we hit a spread, so I don't think there's
any interaction between this stuff and the new code you're adding for iterator
close.

[neis, 2016/03/08 13:40:10]

Except that 'done' must be set to true at the end.  I changed the code back to
using concat_iterable_to_array and added an extra assignment.

+    //   result = IteratorNext(iterator);
+    //   if (!(done = result.done)) %AppendElement(array, result.value);
+    // }
+    // <.completion = ABRUPT>

[adamk, 2016/03/07 20:28:05]

This doesn't seem right to me. The recursion is over a whole different iterator,
which will be exhausted by the time we get here. Won't that be handled by a
generated try-finally inside the recurision?

[neis, 2016/03/08 13:40:10]

Oops, these two assignments were wrong here indeed.

+    // #recurse
+    // <.completion = NORMAL>
+
     auto empty_exprs = new (zone()) ZoneList<Expression*>(0, zone());
     auto array = CreateTempVar(factory()->NewArrayLiteral(
         empty_exprs,
         // Reuse pattern's literal index - it is unused since there is no
         // actual literal allocated.
-        node->literal_index(), RelocInfo::kNoPosition));
-
-    auto arguments = new (zone()) ZoneList<Expression*>(2, zone());
-    arguments->Add(factory()->NewVariableProxy(array), zone());
-    arguments->Add(factory()->NewVariableProxy(iterator), zone());
-    auto spread_into_array_call =
-        factory()->NewCallRuntime(Context::CONCAT_ITERABLE_TO_ARRAY_INDEX,
-                                  arguments, RelocInfo::kNoPosition);
-
-    auto if_statement = factory()->NewIfStatement(
-        factory()->NewUnaryOperation(Token::NOT,
-                                     factory()->NewVariableProxy(done),
-                                     RelocInfo::kNoPosition),
-        factory()->NewExpressionStatement(spread_into_array_call,
-                                          RelocInfo::kNoPosition),
-        factory()->NewEmptyStatement(RelocInfo::kNoPosition),
-        RelocInfo::kNoPosition);
-    block_->statements()->Add(if_statement, zone());
+        node->literal_index(), nopos));
+
+    // result = IteratorNext(iterator);
+    Statement* get_next = factory()->NewExpressionStatement(
+        parser_->BuildIteratorNextResult(
+            factory()->NewVariableProxy(iterator), result, nopos),
+        nopos);
+
+    // done = result.done
+    auto set_done = factory()->NewAssignment(
+        Token::ASSIGN, factory()->NewVariableProxy(done),
+        factory()->NewProperty(
+            factory()->NewVariableProxy(result),
+            factory()->NewStringLiteral(ast_value_factory()->done_string(),
+                                        nopos),
+            nopos), nopos);
+
+    // result.value
+    Expression* result_value = factory()->NewProperty(
+        factory()->NewVariableProxy(result),
+        factory()->NewStringLiteral(ast_value_factory()->value_string(), nopos),
+         nopos);
+
+    // %AppendElement(array, result.value);
+    Statement* append_element;
+    {
+      auto args = new (zone()) ZoneList<Expression*>(2, zone());
+      args->Add(factory()->NewVariableProxy(array), zone());
+      args->Add(result_value, zone());
+
+      append_element = factory()->NewExpressionStatement(
+          factory()->NewCallRuntime(Runtime::kAppendElement, args, nopos),
+          nopos);
+    }
 
+    // if (!#set_done) #append_element;
+    auto maybe_append = factory()->NewIfStatement(
+        factory()->NewUnaryOperation(Token::NOT, set_done, nopos),
+        append_element, factory()->NewEmptyStatement(nopos), nopos);
+
+    // while (#not_done) {
+    //   #get_next;
+    //   #maybe_append;
+    // }
+    WhileStatement* loop = factory()->NewWhileStatement(nullptr, nopos);
+    {
+      Expression* condition = factory()->NewUnaryOperation(
+          Token::NOT, factory()->NewVariableProxy(done), nopos);
+      Block* body = factory()->NewBlock(nullptr, 2, true, nopos);
+      body->statements()->Add(get_next, zone());
+      body->statements()->Add(maybe_append, zone());
+      loop->Initialize(condition, body);
+    }
+
+    // completion = ABRUPT;
+    Statement* set_aborted;
+    if (FLAG_harmony_iterator_close) {
+      Expression* proxy = factory()->NewVariableProxy(completion);
+      Expression* assignment = factory()->NewAssignment(
+          Token::ASSIGN, proxy,
+          factory()->NewSmiLiteral(ABRUPT, nopos), nopos);
+      set_aborted =
+          factory()->NewExpressionStatement(assignment, nopos);
+    }
+
+    // completion = NORMAL;
+    Statement* set_completed;
+    if (FLAG_harmony_iterator_close) {
+      Expression* proxy = factory()->NewVariableProxy(completion);
+      Expression* assignment = factory()->NewAssignment(
+          Token::ASSIGN, proxy,
+          factory()->NewSmiLiteral(NORMAL, nopos), nopos);
+      set_completed =
+          factory()->NewExpressionStatement(assignment, nopos);
+    }
+
+    block_->statements()->Add(loop, zone());
+    if (FLAG_harmony_iterator_close) {
+      block_->statements()->Add(set_aborted, zone());
+    }
     RecurseIntoSubpattern(spread->expression(),
                           factory()->NewVariableProxy(array));
+    if (FLAG_harmony_iterator_close) {
+      block_->statements()->Add(set_completed, zone());
+    }
+  }
+
+  if (FLAG_harmony_iterator_close) {
+    Expression* condition = factory()->NewUnaryOperation(
+        Token::NOT, factory()->NewVariableProxy(done), nopos);
+    parser_->FinalizeIteratorUse(
+        completion, condition, iterator, block_, target);
+    block_ = target;
   }
 }