Bug 1147371: Implement calling IteratorClose and "return" on iterators in yield*

Issue #74

10 files changed, 232 insertions, 45 deletions

diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp
index e2c906850..98679eac6 100644
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -4898,6 +4898,14 @@ BytecodeEmitter::emitIteratorClose(Maybe<JumpTarget> yieldStarTryStart, bool all
             return false;
         if (!ifReturnDone.emitIfElse())                   // ITER OLDRESULT FTYPE FVALUE RESULT
             return false;
+        if (!emitAtomOp(cx->names().value, JSOP_GETPROP)) // ITER OLDRESULT FTYPE FVALUE VALUE
+            return false;
+        if (!emitPrepareIteratorResult())                 // ITER OLDRESULT FTYPE FVALUE VALUE RESULT
+            return false;
+        if (!emit1(JSOP_SWAP))                            // ITER OLDRESULT FTYPE FVALUE RESULT VALUE
+            return false;
+        if (!emitFinishIteratorResult(true))              // ITER OLDRESULT FTYPE FVALUE RESULT
+            return false;
         if (!emit1(JSOP_DUP))                             // ITER OLDRESULT FTYPE FVALUE RESULT RESULT
             return false;
         if (!emit1(JSOP_SETRVAL))                         // ITER OLDRESULT FTYPE FVALUE RESULT
@@ -8059,61 +8067,77 @@ BytecodeEmitter::emitYieldStar(ParseNode* iter, ParseNode* gen)
 
     // Catch location.
     stackDepth = uint32_t(depth);                                // ITER RESULT
-    if (!emit1(JSOP_POP))                                        // ITER
-        return false;
-    // THROW? = 'throw' in ITER
-    if (!emit1(JSOP_EXCEPTION))                                  // ITER EXCEPTION
+    if (!emit1(JSOP_EXCEPTION))                                  // ITER RESULT EXCEPTION
         return false;
-    if (!emit1(JSOP_SWAP))                                       // EXCEPTION ITER
+    if (!emitDupAt(2))                                           // ITER RESULT EXCEPTION ITER
         return false;
-    if (!emit1(JSOP_DUP))                                        // EXCEPTION ITER ITER
+    if (!emit1(JSOP_DUP))                                        // ITER RESULT EXCEPTION ITER ITER
         return false;
-    if (!emitAtomOp(cx->names().throw_, JSOP_STRING))            // EXCEPTION ITER ITER "throw"
+    if (!emitAtomOp(cx->names().throw_, JSOP_CALLPROP))          // ITER RESULT EXCEPTION ITER THROW
         return false;
-    if (!emit1(JSOP_SWAP))                                       // EXCEPTION ITER "throw" ITER
+    if (!emit1(JSOP_DUP))                                        // ITER RESULT EXCEPTION ITER THROW THROW
         return false;
-    if (!emit1(JSOP_IN))                                         // EXCEPTION ITER THROW?
+    if (!emit1(JSOP_UNDEFINED))                                  // ITER RESULT EXCEPTION ITER THROW THROW UNDEFINED
         return false;
-    // if (THROW?) goto delegate
-    JumpList checkThrow;
-    if (!emitJump(JSOP_IFNE, &checkThrow))                       // EXCEPTION ITER
+    if (!emit1(JSOP_EQ))                                         // ITER RESULT EXCEPTION ITER THROW ?EQL
         return false;
-    if (!emit1(JSOP_POP))                                        // EXCEPTION
+
+    IfThenElseEmitter ifThrowMethodIsNotDefined(this);
+    if (!ifThrowMethodIsNotDefined.emitIf())                     // ITER RESULT EXCEPTION ITER THROW
         return false;
-    if (!emit1(JSOP_THROW))                                      // throw EXCEPTION
+    if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_ITERATOR_NO_THROW)) // throw
         return false;
-
-    if (!emitJumpTargetAndPatch(checkThrow))                     // delegate:
+    if (!ifThrowMethodIsNotDefined.emitEnd())                    // ITER OLDRESULT EXCEPTION ITER THROW
         return false;
-    // RESULT = ITER.throw(EXCEPTION)                            // EXCEPTION ITER
-    stackDepth = uint32_t(depth);
-    if (!emit1(JSOP_DUP))                                        // EXCEPTION ITER ITER
+    // ES 14.4.13, YieldExpression : yield * AssignmentExpression, step 5.b.iii.4.
+    // RESULT = ITER.throw(EXCEPTION)                            // ITER OLDRESULT EXCEPTION ITER THROW
+    if (!emit1(JSOP_SWAP))                                       // ITER OLDRESULT EXCEPTION THROW ITER
         return false;
-    if (!emit1(JSOP_DUP))                                        // EXCEPTION ITER ITER ITER
+    if (!emit2(JSOP_PICK, 2))                                    // ITER OLDRESULT THROW ITER EXCEPTION
         return false;
-    if (!emitAtomOp(cx->names().throw_, JSOP_CALLPROP))          // EXCEPTION ITER ITER THROW
+    if (!emitCall(JSOP_CALL, 1, iter))                           // ITER OLDRESULT RESULT
         return false;
-    if (!emit1(JSOP_SWAP))                                       // EXCEPTION ITER THROW ITER
+    checkTypeSet(JSOP_CALL);
+    if (!emitCheckIsObj(CheckIsObjectKind::IteratorThrow))       // ITER OLDRESULT RESULT
         return false;
-    if (!emit2(JSOP_PICK, 3))                                    // ITER THROW ITER EXCEPTION
+    if (!emit1(JSOP_SWAP))                                       // ITER RESULT OLDRESULT
         return false;
-    if (!emitCall(JSOP_CALL, 1, iter))                           // ITER RESULT
+    if (!emit1(JSOP_POP))                                        // ITER RESULT
         return false;
-    checkTypeSet(JSOP_CALL);
     MOZ_ASSERT(this->stackDepth == depth);
     JumpList checkResult;
+    // Note that there is no GOSUB to the finally block here. If the iterator has a
+    // "throw" method, it does not perform IteratorClose per
+    // ES 14.4.13, YieldExpression : yield * AssignmentExpression, step 5.b.ii.
     if (!emitJump(JSOP_GOTO, &checkResult))                      // goto checkResult
         return false;
 
-    // Catch epilogue.
+    // The finally block, IteratorClose logic.
+
+    JumpTarget finallyStart{ 0 };
+    if (!emitJumpTarget(&finallyStart))
+        return false;
+    if (!emit1(JSOP_FINALLY))                                    // ITER RESULT FTYPE FVALUE
+        return false;
+    if (!emitDupAt(3))                                           // ITER RESULT FTYPE FVALUE ITER
+        return false;
+    if (!emitIteratorClose(Some(tryStart)))                      // ITER RESULT FTYPE FVALUE
+        return false;
+    if (!emit1(JSOP_RETSUB))                                     // ITER RESULT
+        return false;
+
+    // Catch and finally epilogue.
 
     // This is a peace offering to ReconstructPCStack.  See the note in EmitTry.
     if (!emit1(JSOP_NOP))
         return false;
-    if (!tryNoteList.append(JSTRY_CATCH, depth, tryStart.offset + JSOP_TRY_LENGTH, tryEnd.offset))
+    size_t tryStartOffset = tryStart.offset + JSOP_TRY_LENGTH;
+    if (!tryNoteList.append(JSTRY_CATCH, depth, tryStartOffset, tryEnd.offset))
+        return false;
+    if (!tryNoteList.append(JSTRY_FINALLY, depth, tryStartOffset, finallyStart.offset))
         return false;
 
-    // After the try/catch block: send the received value to the iterator.
+    // After the try-catch-finally block: send the received value to the iterator.
     if (!emitJumpTargetAndPatch(send))                           // send:
         return false;
 
diff --git a/js/src/jit/BaselineCompiler.cpp b/js/src/jit/BaselineCompiler.cpp
index 2f0d41707..4524bae07 100644
--- a/js/src/jit/BaselineCompiler.cpp
+++ b/js/src/jit/BaselineCompiler.cpp
@@ -3975,7 +3975,7 @@ BaselineCompiler::emit_JSOP_MOREITER()
 }
 
 bool
-BaselineCompiler::emit_JSOP_ISNOITER()
+BaselineCompiler::emitIsMagicValue()
 {
     frame.syncStack(0);
 
@@ -3994,6 +3994,12 @@ BaselineCompiler::emit_JSOP_ISNOITER()
 }
 
 bool
+BaselineCompiler::emit_JSOP_ISNOITER()
+{
+    return emitIsMagicValue();
+}
+
+bool
 BaselineCompiler::emit_JSOP_ENDITER()
 {
     if (!emit_JSOP_JUMPTARGET())
@@ -4005,6 +4011,12 @@ BaselineCompiler::emit_JSOP_ENDITER()
 }
 
 bool
+BaselineCompiler::emit_JSOP_ISGENCLOSING()
+{
+    return emitIsMagicValue();
+}
+
+bool
 BaselineCompiler::emit_JSOP_GETRVAL()
 {
     frame.syncStack(0);
diff --git a/js/src/jit/BaselineCompiler.h b/js/src/jit/BaselineCompiler.h
index 60dac0966..18e56bcd4 100644
--- a/js/src/jit/BaselineCompiler.h
+++ b/js/src/jit/BaselineCompiler.h
@@ -203,6 +203,7 @@ namespace jit {
     _(JSOP_MOREITER)           \
     _(JSOP_ISNOITER)           \
     _(JSOP_ENDITER)            \
+    _(JSOP_ISGENCLOSING)       \
     _(JSOP_GENERATOR)          \
     _(JSOP_INITIALYIELD)       \
     _(JSOP_YIELD)              \
@@ -342,6 +343,8 @@ class BaselineCompiler : public BaselineCompilerSpecific
     MOZ_MUST_USE bool emitThrowConstAssignment();
     MOZ_MUST_USE bool emitUninitializedLexicalCheck(const ValueOperand& val);
 
+    MOZ_MUST_USE bool emitIsMagicValue();
+
     MOZ_MUST_USE bool addPCMappingEntry(bool addIndexEntry);
 
     MOZ_MUST_USE bool addYieldOffset();
diff --git a/js/src/js.msg b/js/src/js.msg
index 30c0d3035..8d492f523 100644
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -583,3 +583,4 @@ MSG_DEF(JSMSG_PROMISE_ERROR_IN_WRAPPED_REJECTION_REASON,0, JSEXN_INTERNALERR, "P
 
 // Iterator
 MSG_DEF(JSMSG_RETURN_NOT_CALLABLE,     0, JSEXN_TYPEERR, "property 'return' of iterator is not callable")
+MSG_DEF(JSMSG_ITERATOR_NO_THROW,       0, JSEXN_TYPEERR, "iterator does not have a 'throw' method")
diff --git a/js/src/tests/ecma_6/Generators/delegating-yield-2.js b/js/src/tests/ecma_6/Generators/delegating-yield-2.js
index 918fb33e1..34cb3f4a9 100644
--- a/js/src/tests/ecma_6/Generators/delegating-yield-2.js
+++ b/js/src/tests/ecma_6/Generators/delegating-yield-2.js
@@ -25,8 +25,8 @@ assertThrowsValue(function () { outer.throw(42) }, 42);
 inner = g1();
 outer = delegate(inner);
 assertIteratorNext(outer, 1);
-inner.throw = function(e) { return e*2; };
-assertEq(84, outer.throw(42));
+inner.throw = function(e) { return { value: e*2 }; };
+assertEq(84, outer.throw(42).value);
 assertIteratorDone(outer, undefined);
 
 // Monkeypatching inner.next.
@@ -41,7 +41,9 @@ outer = delegate(inner);
 assertIteratorNext(outer, 1);
 delete GeneratorObjectPrototype.throw;
 var outer_throw_42 = GeneratorObjectPrototype_throw.bind(outer, 42);
-assertThrowsValue(outer_throw_42, 42);
+// yield* protocol violation: no 'throw' method
+assertThrowsInstanceOf(outer_throw_42, TypeError);
+// Now done, so just throws.
 assertThrowsValue(outer_throw_42, 42);
 
 // Monkeypunch a different throw handler.
@@ -49,11 +51,11 @@ inner = g2();
 outer = delegate(inner);
 outer_throw_42 = GeneratorObjectPrototype_throw.bind(outer, 42);
 assertIteratorNext(outer, 1);
-GeneratorObjectPrototype.throw = function(e) { return e*2; }
-assertEq(84, outer_throw_42());
-assertEq(84, outer_throw_42());
+GeneratorObjectPrototype.throw = function(e) { return { value: e*2 }; }
+assertEq(84, outer_throw_42().value);
+assertEq(84, outer_throw_42().value);
 // This continues indefinitely.
-assertEq(84, outer_throw_42());
+assertEq(84, outer_throw_42().value);
 assertIteratorDone(outer, undefined);
 
 // The same, but restoring the original pre-monkey throw.
@@ -61,8 +63,8 @@ inner = g2();
 outer = delegate(inner);
 outer_throw_42 = GeneratorObjectPrototype_throw.bind(outer, 42);
 assertIteratorNext(outer, 1);
-assertEq(84, outer_throw_42());
-assertEq(84, outer_throw_42());
+assertEq(84, outer_throw_42().value);
+assertEq(84, outer_throw_42().value);
 GeneratorObjectPrototype.throw = GeneratorObjectPrototype_throw;
 assertIteratorResult(outer_throw_42(), 42, false);
 assertIteratorDone(outer, undefined);
diff --git a/js/src/tests/ecma_6/Generators/yield-star-iterator-close.js b/js/src/tests/ecma_6/Generators/yield-star-iterator-close.js
new file mode 100644
index 000000000..ec62dd86d
--- /dev/null
+++ b/js/src/tests/ecma_6/Generators/yield-star-iterator-close.js
@@ -0,0 +1,123 @@
+// Tests that the "return" method on iterators is called in yield*
+// expressions.
+
+function test() {
+    var returnCalled = 0;
+    var returnCalledExpected = 0;
+    var nextCalled = 0;
+    var nextCalledExpected = 0;
+    var iterable = {};
+    iterable[Symbol.iterator] = makeIterator({
+        next: function() {
+            nextCalled++;
+            return { done: false };
+        },
+        ret: function() {
+            returnCalled++;
+            return { done: true, value: "iter.return" };
+        }
+    });
+
+    function* y() {
+        yield* iterable;
+    }
+
+    // G.p.throw on an iterator without "throw" calls IteratorClose.
+    var g1 = y();
+    g1.next();
+    assertThrowsValue(function() {
+        g1.throw("foo");
+    }, "foo");
+    assertEq(returnCalled, ++returnCalledExpected);
+    assertEq(nextCalled, ++nextCalledExpected);
+    g1.next();
+    assertEq(nextCalled, nextCalledExpected);
+
+    // G.p.return calls "return", and if the result.done is true, return the
+    // result.
+    var g2 = y();
+    g2.next();
+    var v2 = g2.return("test return");
+    assertEq(v2.done, true);
+    assertEq(v2.value, "iter.return");
+    assertEq(returnCalled, ++returnCalledExpected);
+    assertEq(nextCalled, ++nextCalledExpected);
+    g2.next();
+    assertEq(nextCalled, nextCalledExpected);
+
+    // G.p.return calls "return", and if the result.done is false, continue
+    // yielding.
+    iterable[Symbol.iterator] = makeIterator({
+        next: function() {
+            nextCalled++;
+            return { done: false };
+        },
+        ret: function() {
+            returnCalled++;
+            return { done: false, value: "iter.return" };
+        }
+    });
+    var g3 = y();
+    g3.next();
+    var v3 = g3.return("test return");
+    assertEq(v3.done, false);
+    assertEq(v3.value, "iter.return");
+    assertEq(returnCalled, ++returnCalledExpected);
+    assertEq(nextCalled, ++nextCalledExpected);
+    g3.next();
+    assertEq(nextCalled, ++nextCalledExpected);
+
+    // G.p.return throwing does not re-call iter.return.
+    iterable[Symbol.iterator] = makeIterator({
+        ret: function() {
+            returnCalled++;
+            throw "in iter.return";
+        }
+    });
+    var g4 = y();
+    g4.next();
+    assertThrowsValue(function() {
+        g4.return("in test");
+    }, "in iter.return");
+    assertEq(returnCalled, ++returnCalledExpected);
+
+    // G.p.return expects iter.return to return an Object.
+    iterable[Symbol.iterator] = makeIterator({
+        ret: function() {
+            returnCalled++;
+            return 42;
+        }
+    });
+    var g5 = y();
+    g5.next();
+    assertThrowsInstanceOf(function() {
+        g5.return("foo");
+    }, TypeError);
+    assertEq(returnCalled, ++returnCalledExpected);
+
+    // IteratorClose expects iter.return to return an Object.
+    var g6 = y();
+    g6.next();
+    assertThrowsInstanceOf(function() {
+        g6.throw("foo");
+    }, TypeError);
+    assertEq(returnCalled, ++returnCalledExpected);
+
+    // G.p.return passes its argument to "return".
+    iterable[Symbol.iterator] = makeIterator({
+        ret: function(x) {
+            assertEq(x, "in test");
+            returnCalled++;
+            return { done: true };
+        }
+    });
+    var g7 = y();
+    g7.next();
+    g7.return("in test");
+    assertEq(returnCalled, ++returnCalledExpected);
+}
+
+test();
+
+if (typeof reportCompare === "function")
+    reportCompare(0, 0);
diff --git a/js/src/tests/ecma_6/shell.js b/js/src/tests/ecma_6/shell.js
index 1d067a282..4da9221d6 100644
--- a/js/src/tests/ecma_6/shell.js
+++ b/js/src/tests/ecma_6/shell.js
@@ -22,14 +22,17 @@
     /** Make an iterator with a return method. */
     global.makeIterator = function makeIterator(overrides) {
         var iterator = {
-            next: function() {
+            throw: function(e) {
+                throw e;
+            },
+            next: function(x) {
                 if (overrides && overrides.next)
-                    return overrides.next();
+                    return overrides.next(x);
                 return { done: false };
             },
-            return: function() {
+            return: function(x) {
                 if (overrides && overrides.ret)
-                    return overrides.ret();
+                    return overrides.ret(x);
                 return { done: true };
             }
         };
diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp
index 923c824ce..9a8c6777f 100644
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -1887,7 +1887,6 @@ CASE(EnableInterruptsPseudoOpcode)
 /* Various 1-byte no-ops. */
 CASE(JSOP_NOP)
 CASE(JSOP_NOP_DESTRUCTURING)
-CASE(JSOP_UNUSED187)
 CASE(JSOP_UNUSED192)
 CASE(JSOP_UNUSED209)
 CASE(JSOP_UNUSED210)
@@ -2182,6 +2181,13 @@ CASE(JSOP_ENDITER)
 }
 END_CASE(JSOP_ENDITER)
 
+CASE(JSOP_ISGENCLOSING)
+{
+    bool b = REGS.sp[-1].isMagic(JS_GENERATOR_CLOSING);
+    PUSH_BOOLEAN(b);
+}
+END_CASE(JSOP_ISGENCLOSING)
+
 CASE(JSOP_DUP)
 {
     MOZ_ASSERT(REGS.stackDepth() >= 1);
@@ -5074,6 +5080,10 @@ js::ThrowCheckIsObject(JSContext* cx, CheckIsObjectKind kind)
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                                   JSMSG_ITER_METHOD_RETURNED_PRIMITIVE, "return");
         break;
+      case CheckIsObjectKind::IteratorThrow:
+        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
+                                  JSMSG_ITER_METHOD_RETURNED_PRIMITIVE, "throw");
+        break;
       case CheckIsObjectKind::GetIterator:
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_GET_ITER_RETURNED_PRIMITIVE);
         break;
diff --git a/js/src/vm/Interpreter.h b/js/src/vm/Interpreter.h
index 3a24240fc..38e23ec06 100644
--- a/js/src/vm/Interpreter.h
+++ b/js/src/vm/Interpreter.h
@@ -563,6 +563,7 @@ ReportRuntimeRedeclaration(JSContext* cx, HandlePropertyName name, const char* r
 enum class CheckIsObjectKind : uint8_t {
     IteratorNext,
     IteratorReturn,
+    IteratorThrow,
     GetIterator
 };
 
diff --git a/js/src/vm/Opcodes.h b/js/src/vm/Opcodes.h
index b59b9388c..84f08c4d5 100644
--- a/js/src/vm/Opcodes.h
+++ b/js/src/vm/Opcodes.h
@@ -1916,8 +1916,16 @@
      *   Stack: => this
      */ \
     macro(JSOP_GLOBALTHIS,    186,"globalthis", NULL,     1,  0,  1,  JOF_BYTE) \
-    macro(JSOP_UNUSED187,     187,"unused187",  NULL,     1,  0,  0,  JOF_BYTE) \
-    \
+    /*
+     * Pushes a boolean indicating whether the top of the stack is
+     * MagicValue(JS_GENERATOR_CLOSING).
+     *
+     *   Category: Statements
+     *   Type: For-In Statement
+     *   Operands:
+     *   Stack: val => val, res
+     */ \
+    macro(JSOP_ISGENCLOSING,  187, "isgenclosing",   NULL,         1,  1,  2,  JOF_BYTE) \
     /*
      * Pushes unsigned 24-bit int immediate integer operand onto the stack.
      *   Category: Literals