From 5f67e34aed279d3239f66f8fb0a26e650ec7bb79 Mon Sep 17 00:00:00 2001 From: littledan Date: Wed, 24 Feb 2016 10:21:32 -0800 Subject: [PATCH] Fix priority of exceptions being thrown from for-of loops In the for-of desugaring, IteratorClose is a subtle thing to get right. When return exists, the logic for which exception to throw is as follows: 1. Get the 'return' property and property any exception that might come from the property read 2. Call return, not yet propagating an exception if it's thrown. 3. If we are closing the iterator due to an exception, propagate that error. 4. If return threw, propagate that error. 5. Check if return's return value was not an object, and throw if so Previously, we were effectively doing step 5 even if an exception "had already been thrown" by step 3. Because this took place in a finally block, the exception "won the race" and was the one propagated to the user. The fix is a simple change to the desugaring to do step 5 only if step 3 didn't happen. R=rossberg BUG=v8:4775 LOG=Y Review URL: https://codereview.chromium.org/1728973002 Cr-Commit-Position: refs/heads/master@{#34261} --- src/parsing/parser.cc | 111 +++++++++++++------------ test/mjsunit/harmony/iterator-close.js | 10 ++- test/test262/test262.status | 12 --- 3 files changed, 62 insertions(+), 71 deletions(-) diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc index 8005479a32..46f6bc22f2 100644 --- a/src/parsing/parser.cc +++ b/src/parsing/parser.cc @@ -6577,16 +6577,17 @@ void ParserTraits::BuildIteratorCloseForCompletion( // // let iteratorReturn = iterator.return; // if (!IS_NULL_OR_UNDEFINED(iteratorReturn)) { - // let output; // if (completion === BODY_THREW) { // if (!IS_CALLABLE(iteratorReturn)) { // throw MakeTypeError(kReturnMethodNotCallable); // } - // try { output = %_Call(iteratorReturn, iterator) } catch (_) { } + // try { %_Call(iteratorReturn, iterator) } catch (_) { } // } else { - // output = %_Call(iteratorReturn, iterator); + // let output = %_Call(iteratorReturn, iterator); + // if (!IS_RECEIVER(output)) { + // %ThrowIterResultNotAnObject(output); + // } // } - // if (!IS_RECEIVER(output)) %ThrowIterResultNotAnObject(output); // } // @@ -6596,11 +6597,9 @@ void ParserTraits::BuildIteratorCloseForCompletion( auto scope = parser_->scope_; auto zone = parser_->zone(); - // let output; - Variable* var_output = scope->NewTemporary(avfactory->empty_string()); // let iteratorReturn = iterator.return; - Variable* var_return = var_output; // Reusing the output variable. + Variable* var_return = scope->NewTemporary(avfactory->empty_string()); Statement* get_return; { Expression* iterator_proxy = factory->NewVariableProxy(iterator); @@ -6625,22 +6624,7 @@ void ParserTraits::BuildIteratorCloseForCompletion( check_return_callable = CheckCallable(var_return, throw_expr); } - // output = %_Call(iteratorReturn, iterator); - Statement* call_return; - { - auto args = new (zone) ZoneList(2, zone); - args->Add(factory->NewVariableProxy(var_return), zone); - args->Add(factory->NewVariableProxy(iterator), zone); - Expression* call = - factory->NewCallRuntime(Runtime::kInlineCall, args, nopos); - - Expression* output_proxy = factory->NewVariableProxy(var_output); - Expression* assignment = factory->NewAssignment( - Token::ASSIGN, output_proxy, call, nopos); - call_return = factory->NewExpressionStatement(assignment, nopos); - } - - // try { output = %_Call(iteratorReturn, iterator) } catch (_) { } + // try { %_Call(iteratorReturn, iterator) } catch (_) { } Statement* try_call_return; { auto args = new (zone) ZoneList(2, zone); @@ -6649,12 +6633,10 @@ void ParserTraits::BuildIteratorCloseForCompletion( Expression* call = factory->NewCallRuntime(Runtime::kInlineCall, args, nopos); - Expression* assignment = factory->NewAssignment( - Token::ASSIGN, factory->NewVariableProxy(var_output), call, nopos); Block* try_block = factory->NewBlock(nullptr, 1, false, nopos); - try_block->statements()->Add( - factory->NewExpressionStatement(assignment, nopos), zone); + try_block->statements()->Add(factory->NewExpressionStatement(call, nopos), + zone); Block* catch_block = factory->NewBlock(nullptr, 0, false, nopos); @@ -6667,29 +6649,27 @@ void ParserTraits::BuildIteratorCloseForCompletion( try_block, catch_scope, catch_variable, catch_block, nopos); } - // if (completion === ABRUPT_THROW) { - // #check_return_callable; - // #try_call_return; - // } else { - // #call_return; + // let output = %_Call(iteratorReturn, iterator); + // if (!IS_RECEIVER(output)) { + // %ThrowIteratorResultNotAnObject(output); // } - Statement* call_return_carefully; + Block* validate_return; { - Expression* condition = factory->NewCompareOperation( - Token::EQ_STRICT, factory->NewVariableProxy(completion), - factory->NewSmiLiteral(BODY_THREW, nopos), nopos); + Variable* var_output = scope->NewTemporary(avfactory->empty_string()); + Statement* call_return; + { + auto args = new (zone) ZoneList(2, zone); + args->Add(factory->NewVariableProxy(var_return), zone); + args->Add(factory->NewVariableProxy(iterator), zone); + Expression* call = + factory->NewCallRuntime(Runtime::kInlineCall, args, nopos); - Block* then_block = factory->NewBlock(nullptr, 2, false, nopos); - then_block->statements()->Add(check_return_callable, zone); - then_block->statements()->Add(try_call_return, zone); + Expression* output_proxy = factory->NewVariableProxy(var_output); + Expression* assignment = + factory->NewAssignment(Token::ASSIGN, output_proxy, call, nopos); + call_return = factory->NewExpressionStatement(assignment, nopos); + } - call_return_carefully = - factory->NewIfStatement(condition, then_block, call_return, nopos); - } - - // if (!IS_RECEIVER(output)) %ThrowIteratorResultNotAnObject(output); - Statement* validate_output; - { Expression* is_receiver_call; { auto args = new (zone) ZoneList(1, zone); @@ -6707,8 +6687,32 @@ void ParserTraits::BuildIteratorCloseForCompletion( throw_call = factory->NewExpressionStatement(call, nopos); } - validate_output = factory->NewIfStatement( + Statement* check_return = factory->NewIfStatement( is_receiver_call, factory->NewEmptyStatement(nopos), throw_call, nopos); + + validate_return = factory->NewBlock(nullptr, 2, false, nopos); + validate_return->statements()->Add(call_return, zone); + validate_return->statements()->Add(check_return, zone); + } + + // if (completion === BODY_THREW) { + // #check_return_callable; + // #try_call_return; + // } else { + // #validate_return; + // } + Statement* call_return_carefully; + { + Expression* condition = factory->NewCompareOperation( + Token::EQ_STRICT, factory->NewVariableProxy(completion), + factory->NewSmiLiteral(BODY_THREW, nopos), nopos); + + Block* then_block = factory->NewBlock(nullptr, 2, false, nopos); + then_block->statements()->Add(check_return_callable, zone); + then_block->statements()->Add(try_call_return, zone); + + call_return_carefully = + factory->NewIfStatement(condition, then_block, validate_return, nopos); } // if (!IS_NULL_OR_UNDEFINED(iteratorReturn)) { ... } @@ -6718,12 +6722,9 @@ void ParserTraits::BuildIteratorCloseForCompletion( Token::EQ, factory->NewVariableProxy(var_return), factory->NewNullLiteral(nopos), nopos); - Block* block = factory->NewBlock(nullptr, 2, false, nopos); - block->statements()->Add(call_return_carefully, zone); - block->statements()->Add(validate_output, zone); - - maybe_call_return = factory->NewIfStatement( - condition, factory->NewEmptyStatement(nopos), block, nopos); + maybe_call_return = + factory->NewIfStatement(condition, factory->NewEmptyStatement(nopos), + call_return_carefully, nopos); } @@ -6746,7 +6747,7 @@ Statement* ParserTraits::FinalizeForOfStatement(ForOfStatement* loop, int pos) { // throw e; // } finally { // if (!(completion === BODY_COMPLETED || IS_UNDEFINED(#iterator))) { - // #BuildIteratorClose(#iterator, completion) // See above. + // #BuildIteratorCloseForCompletion(#iterator, completion) // } // } // @@ -6793,7 +6794,7 @@ Statement* ParserTraits::FinalizeForOfStatement(ForOfStatement* loop, int pos) { } // if (!(completion === BODY_COMPLETED || IS_UNDEFINED(#iterator))) { - // #BuildIteratorClose(#iterator, completion) + // #BuildIteratorCloseForCompletion(#iterator, completion) // } Block* maybe_close; { diff --git a/test/mjsunit/harmony/iterator-close.js b/test/mjsunit/harmony/iterator-close.js index 94785de51f..3cc4412e8a 100644 --- a/test/mjsunit/harmony/iterator-close.js +++ b/test/mjsunit/harmony/iterator-close.js @@ -87,13 +87,15 @@ function* g() { yield 42; return 88 }; for (x of g()) { break; } }, TypeError); - assertThrows(() => { + // Throw from the body of a for loop 'wins' vs throw + // originating from a bad 'return' value. + assertThrowsEquals(() => { for (let x of g()) { throw 666; } - }, TypeError); + }, 666); - assertThrows(() => { + assertThrowsEquals(() => { for (x of g()) { throw 666; } - }, TypeError); + }, 666); assertThrows(() => { for (let x of g()) { return 666; } diff --git a/test/test262/test262.status b/test/test262/test262.status index 2c0a8c8a57..9e3b4b7fad 100644 --- a/test/test262/test262.status +++ b/test/test262/test262.status @@ -55,18 +55,6 @@ # https://code.google.com/p/v8/issues/detail?id=4163 'built-ins/GeneratorPrototype/next/context-constructor-invocation': [FAIL], - # https://bugs.chromium.org/p/v8/issues/detail?id=4775 - 'built-ins/Array/from/iter-map-fn-err': [FAIL], - 'built-ins/Map/iterator-close-after-set-failure': [FAIL], - 'built-ins/Map/iterator-item-first-entry-returns-abrupt': [FAIL], - 'built-ins/Map/iterator-item-second-entry-returns-abrupt': [FAIL], - 'built-ins/Promise/race/iter-close': [PASS, FAIL], - 'built-ins/Set/set-iterator-close-after-add-failure': [FAIL], - 'built-ins/WeakMap/iterator-close-after-set-failure': [FAIL], - 'built-ins/WeakMap/iterator-item-first-entry-returns-abrupt': [FAIL], - 'built-ins/WeakMap/iterator-item-second-entry-returns-abrupt': [FAIL], - 'built-ins/WeakSet/iterator-close-after-add-failure': [FAIL], - # https://code.google.com/p/v8/issues/detail?id=4348 'built-ins/String/prototype/Symbol.iterator/this-val-non-obj-coercible': [FAIL],