From 1ea1ed151571a523d1c8016dcd314e12238cd785 Mon Sep 17 00:00:00 2001 From: janekptacijarabaci Date: Sat, 24 Mar 2018 12:27:00 +0100 Subject: Bug 1147371: Implement calling IteratorClose and "return" on iterators in yield* Issue #74 --- js/src/frontend/BytecodeEmitter.cpp | 80 +++++++++----- js/src/jit/BaselineCompiler.cpp | 14 ++- js/src/jit/BaselineCompiler.h | 3 + js/src/js.msg | 1 + .../tests/ecma_6/Generators/delegating-yield-2.js | 20 ++-- .../ecma_6/Generators/yield-star-iterator-close.js | 123 +++++++++++++++++++++ js/src/tests/ecma_6/shell.js | 11 +- js/src/vm/Interpreter.cpp | 12 +- js/src/vm/Interpreter.h | 1 + js/src/vm/Opcodes.h | 12 +- 10 files changed, 232 insertions(+), 45 deletions(-) create mode 100644 js/src/tests/ecma_6/Generators/yield-star-iterator-close.js 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 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); @@ -3993,6 +3993,12 @@ BaselineCompiler::emit_JSOP_ISNOITER() return true; } +bool +BaselineCompiler::emit_JSOP_ISNOITER() +{ + return emitIsMagicValue(); +} + bool BaselineCompiler::emit_JSOP_ENDITER() { @@ -4004,6 +4010,12 @@ BaselineCompiler::emit_JSOP_ENDITER() return emitOpIC(compiler.getStub(&stubSpace_)); } +bool +BaselineCompiler::emit_JSOP_ISGENCLOSING() +{ + return emitIsMagicValue(); +} + bool BaselineCompiler::emit_JSOP_GETRVAL() { 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 -- cgit v1.2.3