From d22346de40f27157ea81f94479ba42bfc0122f5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 23 Mar 2017 12:53:08 +0100 Subject: [PATCH] deps: fix async await desugaring in V8 This is a backport of https://codereview.chromium.org/2672313003/. The patch did not land in V8 because it was superseded by another one but it is much easier to backport to V8 5.5, was reviewed and passed tests. Original commit message: [async await] Fix async function desugaring Previously we rewrote the return statement in an async function from `return expr;` to `return %ResolvePromise(.promise, expr), .promise`. This can lead to incorrect behavior in the presence of try-finally. This patch stores the `expr` of the return statement in a temporary variable, resolves and returns the promise at the end of the finally block. BUG=v8:5896 PR-URL: https://github.com/nodejs/node/pull/12004 Fixes: https://github.com/nodejs/node/issues/11960 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- deps/v8/include/v8-version.h | 2 +- deps/v8/src/parsing/parser-base.h | 15 +- deps/v8/src/parsing/parser.cc | 194 +++++++++++++++---- deps/v8/src/parsing/parser.h | 9 +- deps/v8/test/mjsunit/regress/regress-5896.js | 14 ++ 5 files changed, 187 insertions(+), 47 deletions(-) create mode 100644 deps/v8/test/mjsunit/regress/regress-5896.js diff --git a/deps/v8/include/v8-version.h b/deps/v8/include/v8-version.h index 2751bb168981ad..759767d7eb55cb 100644 --- a/deps/v8/include/v8-version.h +++ b/deps/v8/include/v8-version.h @@ -11,7 +11,7 @@ #define V8_MAJOR_VERSION 5 #define V8_MINOR_VERSION 5 #define V8_BUILD_NUMBER 372 -#define V8_PATCH_LEVEL 42 +#define V8_PATCH_LEVEL 43 // Use 1 for candidates and 0 otherwise. // (Boolean macro values are not supported by all preprocessors.) diff --git a/deps/v8/src/parsing/parser-base.h b/deps/v8/src/parsing/parser-base.h index 1ebbee4959e8f2..86da9e0c50a36c 100644 --- a/deps/v8/src/parsing/parser-base.h +++ b/deps/v8/src/parsing/parser-base.h @@ -411,7 +411,7 @@ class ParserBase { } void set_promise_variable(typename Types::Variable* variable) { - DCHECK(variable != NULL); + DCHECK_NOT_NULL(variable); DCHECK(IsAsyncFunction(kind())); promise_variable_ = variable; } @@ -419,6 +419,15 @@ class ParserBase { return promise_variable_; } + void set_async_return_variable(typename Types::Variable* variable) { + DCHECK_NOT_NULL(variable); + DCHECK(IsAsyncFunction(kind())); + async_return_variable_ = variable; + } + typename Types::Variable* async_return_variable() const { + return async_return_variable_; + } + const ZoneList& destructuring_assignments_to_rewrite() const { return destructuring_assignments_to_rewrite_; @@ -488,6 +497,8 @@ class ParserBase { // For async functions, this variable holds a temporary for the Promise // being created as output of the async function. Variable* promise_variable_; + Variable* async_return_variable_; + Variable* is_rejection_variable_; FunctionState** function_state_stack_; FunctionState* outer_function_state_; @@ -1468,6 +1479,8 @@ ParserBase::FunctionState::FunctionState( expected_property_count_(0), generator_object_variable_(nullptr), promise_variable_(nullptr), + async_return_variable_(nullptr), + is_rejection_variable_(nullptr), function_state_stack_(function_state_stack), outer_function_state_(*function_state_stack), destructuring_assignments_to_rewrite_(16, scope->zone()), diff --git a/deps/v8/src/parsing/parser.cc b/deps/v8/src/parsing/parser.cc index 2933f7d5470b4c..a0648473f95cfa 100644 --- a/deps/v8/src/parsing/parser.cc +++ b/deps/v8/src/parsing/parser.cc @@ -1701,10 +1701,17 @@ Expression* Parser::RewriteReturn(Expression* return_value, int pos) { return_value = factory()->NewConditional(is_undefined, ThisExpression(pos), is_object_conditional, pos); } + if (is_generator()) { return_value = BuildIteratorResult(return_value, true); } else if (is_async_function()) { - return_value = BuildResolvePromise(return_value, return_value->position()); + // In an async function, + // return expr; + // is rewritten as + // return .async_return_value = expr; + return_value = factory()->NewAssignment( + Token::ASSIGN, factory()->NewVariableProxy(AsyncReturnVariable()), + return_value, kNoSourcePosition); } return return_value; } @@ -2991,60 +2998,152 @@ Block* Parser::BuildParameterInitializationBlock( return init_block; } +Block* Parser::BuildRejectPromiseOnExceptionForParameters(Block* inner_block) { + // .promise = %AsyncFunctionPromiseCreate(); + // try { + // + // } catch (.catch) { + // return %RejectPromise(.promise, .catch), .promise; + // } + Block* result = factory()->NewBlock(nullptr, 2, true, kNoSourcePosition); + { + // .promise = %AsyncFunctionPromiseCreate(); + Expression* create_promise = factory()->NewCallRuntime( + Context::ASYNC_FUNCTION_PROMISE_CREATE_INDEX, + new (zone()) ZoneList(0, zone()), kNoSourcePosition); + Assignment* assign_promise = factory()->NewAssignment( + Token::INIT, factory()->NewVariableProxy(PromiseVariable()), + create_promise, kNoSourcePosition); + Statement* set_promise = + factory()->NewExpressionStatement(assign_promise, kNoSourcePosition); + result->statements()->Add(set_promise, zone()); + } + // catch (.catch) { + // return %RejectPromise(.promise, .catch), .promise; + // } + Scope* catch_scope = NewScope(CATCH_SCOPE); + catch_scope->set_is_hidden(); + Variable* catch_variable = + catch_scope->DeclareLocal(ast_value_factory()->dot_catch_string(), VAR, + kCreatedInitialized, NORMAL_VARIABLE); + Block* catch_block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); + { + // return %RejectPromise(.promise, .catch), .promise; + Expression* reject_return_promise = factory()->NewBinaryOperation( + Token::COMMA, BuildRejectPromise(catch_variable), + factory()->NewVariableProxy(PromiseVariable(), kNoSourcePosition), + kNoSourcePosition); + catch_block->statements()->Add( + factory()->NewReturnStatement(reject_return_promise, kNoSourcePosition), + zone()); + } + TryStatement* try_catch_statement = + factory()->NewTryCatchStatementForAsyncAwait(inner_block, catch_scope, + catch_variable, catch_block, + kNoSourcePosition); + result->statements()->Add(try_catch_statement, zone()); + return result; +} + Block* Parser::BuildRejectPromiseOnException(Block* inner_block, bool* ok) { + // .is_rejection = false; // .promise = %AsyncFunctionPromiseCreate(); // try { // // } catch (.catch) { - // %RejectPromise(.promise, .catch); - // return .promise; + // .is_rejection = true; + // .async_return_value = .catch; // } finally { + // .is_rejection + // ? %RejectPromise(.promise, .async_return_value) + // : %ResolvePromise(.promise, .async_return_value); // %AsyncFunctionPromiseRelease(.promise); + // return .promise; // } - Block* result = factory()->NewBlock(nullptr, 2, true, kNoSourcePosition); + Block* result = factory()->NewBlock(nullptr, 3, true, kNoSourcePosition); - // .promise = %AsyncFunctionPromiseCreate(); - Statement* set_promise; + Variable* is_rejection_var = + scope()->NewTemporary(ast_value_factory()->empty_string()); { + // .is_rejection = false; + Assignment* set_is_rejection = factory()->NewAssignment( + Token::INIT, factory()->NewVariableProxy(is_rejection_var), + factory()->NewBooleanLiteral(false, kNoSourcePosition), + kNoSourcePosition); + result->statements()->Add( + factory()->NewExpressionStatement(set_is_rejection, kNoSourcePosition), + zone()); + // .promise = %AsyncFunctionPromiseCreate(); Expression* create_promise = factory()->NewCallRuntime( Context::ASYNC_FUNCTION_PROMISE_CREATE_INDEX, new (zone()) ZoneList(0, zone()), kNoSourcePosition); Assignment* assign_promise = factory()->NewAssignment( Token::INIT, factory()->NewVariableProxy(PromiseVariable()), create_promise, kNoSourcePosition); - set_promise = + Statement* set_promise = factory()->NewExpressionStatement(assign_promise, kNoSourcePosition); + result->statements()->Add(set_promise, zone()); } - result->statements()->Add(set_promise, zone()); - // catch (.catch) { return %RejectPromise(.promise, .catch), .promise } + // catch (.catch) { + // .is_rejection = true; + // .async_return_value = .catch; + // } Scope* catch_scope = NewScope(CATCH_SCOPE); catch_scope->set_is_hidden(); Variable* catch_variable = catch_scope->DeclareLocal(ast_value_factory()->dot_catch_string(), VAR, kCreatedInitialized, NORMAL_VARIABLE); Block* catch_block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); - - Expression* promise_reject = BuildRejectPromise( - factory()->NewVariableProxy(catch_variable), kNoSourcePosition); - ReturnStatement* return_promise_reject = - factory()->NewReturnStatement(promise_reject, kNoSourcePosition); - catch_block->statements()->Add(return_promise_reject, zone()); + { + // .is_rejection = true; + DCHECK_NOT_NULL(is_rejection_var); + Assignment* set_is_rejection = factory()->NewAssignment( + Token::ASSIGN, factory()->NewVariableProxy(is_rejection_var), + factory()->NewBooleanLiteral(true, kNoSourcePosition), + kNoSourcePosition); + catch_block->statements()->Add( + factory()->NewExpressionStatement(set_is_rejection, kNoSourcePosition), + zone()); + // .async_return_value = .catch; + Assignment* set_async_return_var = factory()->NewAssignment( + Token::ASSIGN, factory()->NewVariableProxy(AsyncReturnVariable()), + factory()->NewVariableProxy(catch_variable), kNoSourcePosition); + catch_block->statements()->Add(factory()->NewExpressionStatement( + set_async_return_var, kNoSourcePosition), + zone()); + } TryStatement* try_catch_statement = factory()->NewTryCatchStatementForAsyncAwait(inner_block, catch_scope, catch_variable, catch_block, kNoSourcePosition); - - // There is no TryCatchFinally node, so wrap it in an outer try/finally Block* outer_try_block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); outer_try_block->statements()->Add(try_catch_statement, zone()); - // finally { %AsyncFunctionPromiseRelease(.promise) } + // finally { + // .is_rejection + // ? %RejectPromise(.promise, .async_return_value) + // : %ResolvePromise(.promise, .async_return_value); + // %AsyncFunctionPromiseRelease(.promise); + // return .promise; + // } Block* finally_block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); { + // .is_rejection + // ? %RejectPromise(.promise, .async_return_value) + // : %ResolvePromise(.promise, .async_return_value); + Expression* resolve_or_reject_promise = + factory()->NewConditional(factory()->NewVariableProxy(is_rejection_var), + BuildRejectPromise(AsyncReturnVariable()), + BuildResolvePromise(), kNoSourcePosition); + finally_block->statements()->Add( + factory()->NewExpressionStatement(resolve_or_reject_promise, + kNoSourcePosition), + zone()); + // %AsyncFunctionPromiseRelease(.promise); ZoneList* args = new (zone()) ZoneList(1, zone()); args->Add(factory()->NewVariableProxy(PromiseVariable()), zone()); Expression* call_promise_release = factory()->NewCallRuntime( @@ -3052,11 +3151,15 @@ Block* Parser::BuildRejectPromiseOnException(Block* inner_block, bool* ok) { Statement* promise_release = factory()->NewExpressionStatement( call_promise_release, kNoSourcePosition); finally_block->statements()->Add(promise_release, zone()); + + // return .promise; + Statement* return_promise = factory()->NewReturnStatement( + factory()->NewVariableProxy(PromiseVariable()), kNoSourcePosition); + finally_block->statements()->Add(return_promise, zone()); } Statement* try_finally_statement = factory()->NewTryFinallyStatement( outer_try_block, finally_block, kNoSourcePosition); - result->statements()->Add(try_finally_statement, zone()); return result; } @@ -3072,31 +3175,25 @@ Expression* Parser::BuildCreateJSGeneratorObject(int pos, FunctionKind kind) { pos); } -Expression* Parser::BuildResolvePromise(Expression* value, int pos) { - // %ResolvePromise(.promise, value), .promise +Expression* Parser::BuildResolvePromise() { + // %ResolvePromise(.promise, .async_return_variable), .promise ZoneList* args = new (zone()) ZoneList(2, zone()); args->Add(factory()->NewVariableProxy(PromiseVariable()), zone()); - args->Add(value, zone()); - Expression* call_runtime = - factory()->NewCallRuntime(Context::PROMISE_RESOLVE_INDEX, args, pos); - return factory()->NewBinaryOperation( - Token::COMMA, call_runtime, - factory()->NewVariableProxy(PromiseVariable()), pos); + args->Add(factory()->NewVariableProxy(AsyncReturnVariable()), zone()); + return factory()->NewCallRuntime(Context::PROMISE_RESOLVE_INDEX, args, + kNoSourcePosition); } -Expression* Parser::BuildRejectPromise(Expression* value, int pos) { - // %RejectPromiseNoDebugEvent(.promise, value, true), .promise +Expression* Parser::BuildRejectPromise(Variable* value) { + // %RejectPromiseNoDebugEvent(.promise, .value, true) // The NoDebugEvent variant disables the additional debug event for the // rejection since a debug event already happened for the exception that got // us here. ZoneList* args = new (zone()) ZoneList(2, zone()); args->Add(factory()->NewVariableProxy(PromiseVariable()), zone()); - args->Add(value, zone()); - Expression* call_runtime = factory()->NewCallRuntime( - Context::REJECT_PROMISE_NO_DEBUG_EVENT_INDEX, args, pos); - return factory()->NewBinaryOperation( - Token::COMMA, call_runtime, - factory()->NewVariableProxy(PromiseVariable()), pos); + args->Add(factory()->NewVariableProxy(value), zone()); + return factory()->NewCallRuntime( + Context::REJECT_PROMISE_NO_DEBUG_EVENT_INDEX, args, kNoSourcePosition); } Variable* Parser::PromiseVariable() { @@ -3111,6 +3208,19 @@ Variable* Parser::PromiseVariable() { return promise; } +Variable* Parser::AsyncReturnVariable() { + // Based on the various compilation paths, there are many different + // code paths which may be the first to access the return value + // temporary. Whichever comes first should create it and stash it in + // the FunctionState. + Variable* async_return = function_state_->async_return_variable(); + if (async_return == nullptr) { + async_return = scope()->NewTemporary(ast_value_factory()->empty_string()); + function_state_->set_async_return_variable(async_return); + } + return async_return; +} + Expression* Parser::BuildInitialYield(int pos, FunctionKind kind) { Expression* allocation = BuildCreateJSGeneratorObject(pos, kind); VariableProxy* init_proxy = @@ -3235,7 +3345,7 @@ ZoneList* Parser::ParseEagerFunctionBody( // TODO(littledan): Merge the two rejection blocks into one if (IsAsyncFunction(kind)) { - init_block = BuildRejectPromiseOnException(init_block, CHECK_OK); + init_block = BuildRejectPromiseOnExceptionForParameters(init_block); } DCHECK_NOT_NULL(init_block); @@ -4176,14 +4286,16 @@ void Parser::RewriteAsyncFunctionBody(ZoneList* body, Block* block, // .generator_object = %CreateGeneratorObject(); // BuildRejectPromiseOnException({ // ... block ... - // return %ResolvePromise(.promise, expr), .promise; + // .async_return_var = expr; // }) // } - return_value = BuildResolvePromise(return_value, return_value->position()); - block->statements()->Add( - factory()->NewReturnStatement(return_value, return_value->position()), - zone()); + Assignment* set_async_return_var = factory()->NewAssignment( + Token::ASSIGN, factory()->NewVariableProxy(AsyncReturnVariable()), + return_value, kNoSourcePosition); + block->statements()->Add(factory()->NewExpressionStatement( + set_async_return_var, kNoSourcePosition), + zone()); block = BuildRejectPromiseOnException(block, CHECK_OK_VOID); body->Add(block, zone()); } diff --git a/deps/v8/src/parsing/parser.h b/deps/v8/src/parsing/parser.h index 418bedf81b4156..6ff4336f9b44a3 100644 --- a/deps/v8/src/parsing/parser.h +++ b/deps/v8/src/parsing/parser.h @@ -491,6 +491,7 @@ class Parser : public ParserBase { Block* BuildParameterInitializationBlock( const ParserFormalParameters& parameters, bool* ok); Block* BuildRejectPromiseOnException(Block* block, bool* ok); + Block* BuildRejectPromiseOnExceptionForParameters(Block* block); // Consumes the ending }. ZoneList* ParseEagerFunctionBody( @@ -576,9 +577,10 @@ class Parser : public ParserBase { Expression* BuildInitialYield(int pos, FunctionKind kind); Expression* BuildCreateJSGeneratorObject(int pos, FunctionKind kind); - Expression* BuildResolvePromise(Expression* value, int pos); - Expression* BuildRejectPromise(Expression* value, int pos); + Expression* BuildResolvePromise(); + Expression* BuildRejectPromise(Variable* value); Variable* PromiseVariable(); + Variable* AsyncReturnVariable(); // Generic AST generator for throwing errors from compiled code. Expression* NewThrowError(Runtime::FunctionId function_id, @@ -983,8 +985,7 @@ class Parser : public ParserBase { auto* init_block = BuildParameterInitializationBlock(parameters, ok); if (!*ok) return; if (is_async) { - init_block = BuildRejectPromiseOnException(init_block, ok); - if (!*ok) return; + init_block = BuildRejectPromiseOnExceptionForParameters(init_block); } if (init_block != nullptr) body->Add(init_block, zone()); } diff --git a/deps/v8/test/mjsunit/regress/regress-5896.js b/deps/v8/test/mjsunit/regress/regress-5896.js new file mode 100644 index 00000000000000..acd9a32dfa7d49 --- /dev/null +++ b/deps/v8/test/mjsunit/regress/regress-5896.js @@ -0,0 +1,14 @@ +// Copyright 2017 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// Flags: --allow-natives-syntax +async function foo() { + try { + return 1; + } finally { + return Promise.resolve().then(() => { return 2; }); + } +} +foo() + .then(value => { found = value; assertEquals(2, value) }) + .catch((e) => { %AbortJS(''+e); });