Correctly set the closing condition in array patterns.

This fixes a bug where the iterable's .return method gets called when it
shouldn't.

R=littledan@chromium.org
BUG=v8:4952
LOG=n

Review-Url: https://codereview.chromium.org/1927073002
Cr-Commit-Position: refs/heads/master@{#35850}

src/parsing/pattern-rewriter.cc

@@ -546,11 +546,11 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node,
 
     // let array = [];
     // while (!done) {
+    //   done = true;  // If .next, .done or .value throws, don't close.
     //   result = IteratorNext(iterator);
-    //   if (result.done) {
-    //     done = true;
-    //   } else {
+    //   if (!result.done) {
     //     %AppendElement(array, result.value);
+    //     done = false;
     //   }
     // }
 
@@ -565,12 +565,6 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node,
           node->literal_index(), RelocInfo::kNoPosition));
     }
 
-    // result = IteratorNext(iterator);
-    Statement* get_next = factory()->NewExpressionStatement(
-        parser_->BuildIteratorNextResult(factory()->NewVariableProxy(iterator),
-                                         result, nopos),
-        nopos);
-
     // done = true;
     Statement* set_done = factory()->NewExpressionStatement(
         factory()->NewAssignment(
@@ -578,6 +572,12 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node,
             factory()->NewBooleanLiteral(true, nopos), nopos),
         nopos);
 
+    // result = IteratorNext(iterator);
+    Statement* get_next = factory()->NewExpressionStatement(
+        parser_->BuildIteratorNextResult(factory()->NewVariableProxy(iterator),
+                                         result, nopos),
+        nopos);
+
     // %AppendElement(array, result.value);
     Statement* append_element;
     {
@@ -594,29 +594,44 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node,
           nopos);
     }
 
-    // if (result.done) { #set_done } else { #append_element }
-    Statement* set_done_or_append;
+    // done = false;
+    Statement* unset_done = factory()->NewExpressionStatement(
+        factory()->NewAssignment(
+            Token::ASSIGN, factory()->NewVariableProxy(done),
+            factory()->NewBooleanLiteral(false, nopos), nopos),
+        nopos);
+
+    // if (!result.done) { #append_element; #unset_done }
+    Statement* maybe_append_and_unset_done;
     {
       Expression* result_done =
           factory()->NewProperty(factory()->NewVariableProxy(result),
                                  factory()->NewStringLiteral(
                                      ast_value_factory()->done_string(), nopos),
                                  nopos);
-      set_done_or_append = factory()->NewIfStatement(result_done, set_done,
-                                                     append_element, nopos);
+
+      Block* then = factory()->NewBlock(nullptr, 2, true, nopos);
+      then->statements()->Add(append_element, zone());
+      then->statements()->Add(unset_done, zone());
+
+      maybe_append_and_unset_done = factory()->NewIfStatement(
+          factory()->NewUnaryOperation(Token::NOT, result_done, nopos), then,
+          factory()->NewEmptyStatement(nopos), nopos);
     }
 
     // while (!done) {
+    //   #set_done;
     //   #get_next;
-    //   #set_done_or_append;
+    //   #maybe_append_and_unset_done;
     // }
     WhileStatement* loop = factory()->NewWhileStatement(nullptr, nopos);
     {
       Expression* condition = factory()->NewUnaryOperation(
           Token::NOT, factory()->NewVariableProxy(done), nopos);
-      Block* body = factory()->NewBlock(nullptr, 2, true, nopos);
+      Block* body = factory()->NewBlock(nullptr, 3, true, nopos);
+      body->statements()->Add(set_done, zone());
       body->statements()->Add(get_next, zone());
-      body->statements()->Add(set_done_or_append, zone());
+      body->statements()->Add(maybe_append_and_unset_done, zone());
       loop->Initialize(condition, body);
     }
 

test/mjsunit/harmony/iterator-close.js

@@ -1007,6 +1007,26 @@ function* g() { yield 42; return 88 };
   assertThrowsEquals(() => {
     (([x]) => x)(g());
   }, 666);
+
+  assertThrowsEquals(() => {
+    var [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    let [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    const [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    (([...x]) => x)(g());
+  }, 666);
 }
 
 
@@ -1051,6 +1071,26 @@ function* g() { yield 42; return 88 };
   assertThrowsEquals(() => {
     (([x]) => x)(g());
   }, 666);
+
+  assertThrowsEquals(() => {
+    var [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    let [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    const [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    (([...x]) => x)(g());
+  }, 666);
 }
 
 
@@ -1095,6 +1135,26 @@ function* g() { yield 42; return 88 };
   assertThrowsEquals(() => {
     (([x]) => x)(g());
   }, 666);
+
+  assertThrowsEquals(() => {
+    var [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    let [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    const [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    [...x] = g();
+  }, 666);
+
+  assertThrowsEquals(() => {
+    (([...x]) => x)(g());
+  }, 666);
 }