From af4ff8c71fafa3db25441cf69f5a16242238feda Mon Sep 17 00:00:00 2001 From: Sathya Gunasekaran Date: Thu, 7 Sep 2017 10:00:23 -0700 Subject: [PATCH] [ESNext] Update Promise.prototype.finally to match latest spec The spec calls out to Promise.prototype.then and also passes around the constructor of the receiver to Promise.prototype.finally. Adds a new constructor slot to PromiseFinallyContext enum and this is used to create a new promise in the thenFinally/catchFinally callbacks. Created a new PromiseResolve TFS builtin refactored from the existing PromiseResolve builtin. PromiseResolveWrapper calls out to this TFS Builtin and is now exposed as Promise.resolve. The thenFinally and catchFinally callbacks also call out to the PromiseResolve TFS builtin. Spec -- https://tc39.github.io/proposal-promise-finally/ Bug: v8:5967 Change-Id: I2ce89f14d3b149619d11e424b6e37062e466c4d5 Reviewed-on: https://chromium-review.googlesource.com/652026 Reviewed-by: Georg Neis Commit-Queue: Sathya Gunasekaran Cr-Commit-Position: refs/heads/master@{#47898} --- src/bootstrapper.cc | 4 +- src/builtins/builtins-definitions.h | 3 +- src/builtins/builtins-promise-gen.cc | 192 ++++++++++-------- src/builtins/builtins-promise-gen.h | 3 +- .../harmony/promise-prototype-finally.js | 81 ++++++++ 5 files changed, 193 insertions(+), 90 deletions(-) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 3445835b49..47b47d9466 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -2221,8 +2221,8 @@ void Genesis::InitializeGlobal(Handle global_object, SimpleInstallFunction(promise_fun, "race", Builtins::kPromiseRace, 1, true); - SimpleInstallFunction(promise_fun, "resolve", Builtins::kPromiseResolve, 1, - true); + SimpleInstallFunction(promise_fun, "resolve", + Builtins::kPromiseResolveWrapper, 1, true); SimpleInstallFunction(promise_fun, "reject", Builtins::kPromiseReject, 1, true); diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 8eb94bf253..b1c13f5fc8 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -754,7 +754,8 @@ namespace internal { TFJ(PromiseHandle, 5, kValue, kHandler, kDeferredPromise, \ kDeferredOnResolve, kDeferredOnReject) \ /* ES #sec-promise.resolve */ \ - TFJ(PromiseResolve, 1, kValue) \ + TFJ(PromiseResolveWrapper, 1, kValue) \ + TFS(PromiseResolve, kConstructor, kValue) \ /* ES #sec-promise.reject */ \ TFJ(PromiseReject, 1, kReason) \ TFJ(InternalPromiseReject, 3, kPromise, kReason, kDebugEvent) \ diff --git a/src/builtins/builtins-promise-gen.cc b/src/builtins/builtins-promise-gen.cc index f1df9676dd..0aa618e3cc 100644 --- a/src/builtins/builtins-promise-gen.cc +++ b/src/builtins/builtins-promise-gen.cc @@ -1385,60 +1385,68 @@ TF_BUILTIN(PromiseCatch, PromiseBuiltinsAssembler) { } } -TF_BUILTIN(PromiseResolve, PromiseBuiltinsAssembler) { +TF_BUILTIN(PromiseResolveWrapper, PromiseBuiltinsAssembler) { // 1. Let C be the this value. Node* receiver = Parameter(Descriptor::kReceiver); Node* value = Parameter(Descriptor::kValue); Node* context = Parameter(Descriptor::kContext); - Isolate* isolate = this->isolate(); // 2. If Type(C) is not Object, throw a TypeError exception. ThrowIfNotJSReceiver(context, receiver, MessageTemplate::kCalledOnNonObject, "PromiseResolve"); + // 3. Return ? PromiseResolve(C, x). + Return(CallBuiltin(Builtins::kPromiseResolve, context, receiver, value)); +} + +TF_BUILTIN(PromiseResolve, PromiseBuiltinsAssembler) { + Node* constructor = Parameter(Descriptor::kConstructor); + Node* value = Parameter(Descriptor::kValue); + Node* context = Parameter(Descriptor::kContext); + Isolate* isolate = this->isolate(); + Node* const native_context = LoadNativeContext(context); Node* const promise_fun = LoadContextElement(native_context, Context::PROMISE_FUNCTION_INDEX); - Label if_valueisnativepromise(this), if_valueisnotnativepromise(this), - if_valueisnotpromise(this); + Label if_value_is_native_promise(this), + if_value_or_constructor_are_not_native_promise(this), + if_need_to_allocate(this); - // 3.If IsPromise(x) is true, then - GotoIf(TaggedIsSmi(value), &if_valueisnotpromise); + GotoIf(TaggedIsSmi(value), &if_need_to_allocate); // This shortcircuits the constructor lookups. - GotoIfNot(HasInstanceType(value, JS_PROMISE_TYPE), &if_valueisnotpromise); + GotoIfNot(HasInstanceType(value, JS_PROMISE_TYPE), &if_need_to_allocate); // This adds a fast path as non-subclassed native promises don't have // an observable constructor lookup. - BranchIfFastPath(native_context, promise_fun, value, &if_valueisnativepromise, - &if_valueisnotnativepromise); + BranchIfFastPath(native_context, promise_fun, value, + &if_value_is_native_promise, + &if_value_or_constructor_are_not_native_promise); - BIND(&if_valueisnativepromise); + BIND(&if_value_is_native_promise); { - GotoIfNot(WordEqual(promise_fun, receiver), &if_valueisnotnativepromise); + GotoIfNot(WordEqual(promise_fun, constructor), + &if_value_or_constructor_are_not_native_promise); Return(value); } - // At this point, value or/and receiver are not native promises, but + // At this point, value or/and constructor are not native promises, but // they could be of the same subclass. - BIND(&if_valueisnotnativepromise); + BIND(&if_value_or_constructor_are_not_native_promise); { - // 3.a Let xConstructor be ? Get(x, "constructor"). - // The constructor lookup is observable. - Node* const constructor = + Node* const xConstructor = GetProperty(context, value, isolate->factory()->constructor_string()); - // 3.b If SameValue(xConstructor, C) is true, return x. - GotoIfNot(SameValue(constructor, receiver), &if_valueisnotpromise); + GotoIfNot(SameValue(xConstructor, constructor), &if_need_to_allocate); Return(value); } - BIND(&if_valueisnotpromise); + BIND(&if_need_to_allocate); { Label if_nativepromise(this), if_notnativepromise(this); - Branch(WordEqual(promise_fun, receiver), &if_nativepromise, + Branch(WordEqual(promise_fun, constructor), &if_nativepromise, &if_notnativepromise); // This adds a fast path for native promises that don't need to @@ -1452,16 +1460,13 @@ TF_BUILTIN(PromiseResolve, PromiseBuiltinsAssembler) { BIND(&if_notnativepromise); { - // 4. Let promiseCapability be ? NewPromiseCapability(C). - Node* const capability = NewPromiseCapability(context, receiver); + Node* const capability = NewPromiseCapability(context, constructor); - // 5. Perform ? Call(promiseCapability.[[Resolve]], undefined, « x »). Callable call_callable = CodeFactory::Call(isolate); Node* const resolve = LoadObjectField(capability, PromiseCapability::kResolveOffset); CallJS(call_callable, context, resolve, UndefinedConstant(), value); - // 6. Return promiseCapability.[[Promise]]. Node* const result = LoadObjectField(capability, PromiseCapability::kPromiseOffset); Return(result); @@ -1562,11 +1567,13 @@ TF_BUILTIN(InternalPromiseReject, PromiseBuiltinsAssembler) { } std::pair PromiseBuiltinsAssembler::CreatePromiseFinallyFunctions( - Node* on_finally, Node* native_context) { + Node* on_finally, Node* constructor, Node* native_context) { Node* const promise_context = CreatePromiseContext(native_context, kPromiseFinallyContextLength); StoreContextElementNoWriteBarrier(promise_context, kOnFinallySlot, on_finally); + StoreContextElementNoWriteBarrier(promise_context, kConstructorSlot, + constructor); Node* const map = LoadContextElement( native_context, Context::STRICT_FUNCTION_WITHOUT_PROTOTYPE_MAP_INDEX); Node* const then_finally_info = LoadContextElement( @@ -1607,30 +1614,37 @@ TF_BUILTIN(PromiseThenFinally, PromiseBuiltinsAssembler) { Node* const value = Parameter(Descriptor::kValue); Node* const context = Parameter(Descriptor::kContext); + // 1. Let onFinally be F.[[OnFinally]]. Node* const on_finally = LoadContextElement(context, kOnFinallySlot); - // 2.a Let result be ? Call(onFinally, undefined). + // 2. Assert: IsCallable(onFinally) is true. + CSA_ASSERT(this, IsCallable(on_finally)); + + // 3. Let result be ? Call(onFinally). Callable call_callable = CodeFactory::Call(isolate()); - Node* result = + Node* const result = CallJS(call_callable, context, on_finally, UndefinedConstant()); - // 2.b Let promise be ! PromiseResolve( %Promise%, result). - Node* const promise = AllocateAndInitJSPromise(context); - InternalResolvePromise(context, promise, result); + // 4. Let C be F.[[Constructor]]. + Node* const constructor = LoadContextElement(context, kConstructorSlot); - // 2.c Let valueThunk be equivalent to a function that returns value. + // 5. Assert: IsConstructor(C) is true. + CSA_ASSERT(this, IsConstructor(constructor)); + + // 6. Let promise be ? PromiseResolve(C, result). + Node* const promise = + CallBuiltin(Builtins::kPromiseResolve, context, constructor, result); + + // 7. Let valueThunk be equivalent to a function that returns value. Node* native_context = LoadNativeContext(context); Node* const value_thunk = CreateValueThunkFunction(value, native_context); - // 2.d Let promiseCapability be ! NewPromiseCapability( %Promise%). - Node* const promise_capability = AllocateAndInitJSPromise(context, promise); - - // 2.e Return PerformPromiseThen(promise, valueThunk, undefined, - // promiseCapability). - InternalPerformPromiseThen(context, promise, value_thunk, UndefinedConstant(), - promise_capability, UndefinedConstant(), - UndefinedConstant()); - Return(promise_capability); + // 8. Return ? Invoke(promise, "then", « valueThunk »). + Node* const promise_then = + GetProperty(context, promise, factory()->then_string()); + Node* const result_promise = CallJS(call_callable, context, + promise_then, promise, value_thunk); + Return(result_promise); } TF_BUILTIN(PromiseThrowerFinally, PromiseBuiltinsAssembler) { @@ -1661,30 +1675,37 @@ TF_BUILTIN(PromiseCatchFinally, PromiseBuiltinsAssembler) { Node* const reason = Parameter(Descriptor::kReason); Node* const context = Parameter(Descriptor::kContext); + // 1. Let onFinally be F.[[OnFinally]]. Node* const on_finally = LoadContextElement(context, kOnFinallySlot); - // 2.a Let result be ? Call(onFinally, undefined). + // 2. Assert: IsCallable(onFinally) is true. + CSA_ASSERT(this, IsCallable(on_finally)); + + // 3. Let result be ? Call(onFinally). Callable call_callable = CodeFactory::Call(isolate()); Node* result = - CallJS(call_callable, context, on_finally, UndefinedConstant()); + CallJS(call_callable, context, on_finally, UndefinedConstant()); - // 2.b Let promise be ! PromiseResolve( %Promise%, result). - Node* const promise = AllocateAndInitJSPromise(context); - InternalResolvePromise(context, promise, result); + // 4. Let C be F.[[Constructor]]. + Node* const constructor = LoadContextElement(context, kConstructorSlot); - // 2.c Let thrower be equivalent to a function that throws reason. + // 5. Assert: IsConstructor(C) is true. + CSA_ASSERT(this, IsConstructor(constructor)); + + // 6. Let promise be ? PromiseResolve(C, result). + Node* const promise = + CallBuiltin(Builtins::kPromiseResolve, context, constructor, result); + + // 7. Let thrower be equivalent to a function that throws reason. Node* native_context = LoadNativeContext(context); Node* const thrower = CreateThrowerFunction(reason, native_context); - // 2.d Let promiseCapability be ! NewPromiseCapability( %Promise%). - Node* const promise_capability = AllocateAndInitJSPromise(context, promise); - - // 2.e Return PerformPromiseThen(promise, thrower, undefined, - // promiseCapability). - InternalPerformPromiseThen(context, promise, thrower, UndefinedConstant(), - promise_capability, UndefinedConstant(), - UndefinedConstant()); - Return(promise_capability); + // 8. Return ? Invoke(promise, "then", « thrower »). + Node* const promise_then = + GetProperty(context, promise, factory()->then_string()); + Node* const result_promise = CallJS(call_callable, context, + promise_then, promise, thrower); + Return(result_promise); } TF_BUILTIN(PromiseFinally, PromiseBuiltinsAssembler) { @@ -1699,26 +1720,43 @@ TF_BUILTIN(PromiseFinally, PromiseBuiltinsAssembler) { ThrowIfNotInstanceType(context, promise, JS_PROMISE_TYPE, "Promise.prototype.finally"); + // 3. Let C be ? SpeciesConstructor(promise, %Promise%). + Node* const native_context = LoadNativeContext(context); + Node* const promise_fun = + LoadContextElement(native_context, Context::PROMISE_FUNCTION_INDEX); + Node* const constructor = SpeciesConstructor(context, promise, promise_fun); + + // 4. Assert: IsConstructor(C) is true. + CSA_ASSERT(this, IsConstructor(constructor)); + VARIABLE(var_then_finally, MachineRepresentation::kTagged); VARIABLE(var_catch_finally, MachineRepresentation::kTagged); Label if_notcallable(this, Label::kDeferred), perform_finally(this); - // 3. Let thenFinally be ! CreateThenFinally(onFinally). - // 4. Let catchFinally be ! CreateCatchFinally(onFinally). GotoIf(TaggedIsSmi(on_finally), &if_notcallable); - Node* const on_finally_map = LoadMap(on_finally); - GotoIfNot(IsCallableMap(on_finally_map), &if_notcallable); + GotoIfNot(IsCallable(on_finally), &if_notcallable); - Node* const native_context = LoadNativeContext(context); + // 6. Else, + // a. Let thenFinally be a new built-in function object as defined + // in ThenFinally Function. + // b. Let catchFinally be a new built-in function object as + // defined in CatchFinally Function. + // c. Set thenFinally and catchFinally's [[Constructor]] internal + // slots to C. + // d. Set thenFinally and catchFinally's [[OnFinally]] internal + // slots to onFinally. Node* then_finally = nullptr; Node* catch_finally = nullptr; std::tie(then_finally, catch_finally) = - CreatePromiseFinallyFunctions(on_finally, native_context); + CreatePromiseFinallyFunctions(on_finally, constructor, native_context); var_then_finally.Bind(then_finally); var_catch_finally.Bind(catch_finally); Goto(&perform_finally); + // 5. If IsCallable(onFinally) is not true, + // a. Let thenFinally be onFinally. + // b. Let catchFinally be onFinally. BIND(&if_notcallable); { var_then_finally.Bind(on_finally); @@ -1726,32 +1764,14 @@ TF_BUILTIN(PromiseFinally, PromiseBuiltinsAssembler) { Goto(&perform_finally); } - // 5. Return PerformPromiseThen(promise, valueThunk, undefined, - // promiseCapability). + // 7. Return ? Invoke(promise, "then", « thenFinally, catchFinally »). BIND(&perform_finally); - Label if_nativepromise(this), if_custompromise(this, Label::kDeferred); - BranchIfFastPath(context, promise, &if_nativepromise, &if_custompromise); - - BIND(&if_nativepromise); - { - Node* deferred_promise = AllocateAndInitJSPromise(context, promise); - InternalPerformPromiseThen(context, promise, var_then_finally.value(), - var_catch_finally.value(), deferred_promise, - UndefinedConstant(), UndefinedConstant()); - Return(deferred_promise); - } - - BIND(&if_custompromise); - { - Node* const then = - GetProperty(context, promise, isolate()->factory()->then_string()); - Callable call_callable = CodeFactory::Call(isolate()); - // 5. Return ? Invoke(promise, "then", « thenFinally, catchFinally »). - Node* const result = - CallJS(call_callable, context, then, promise, var_then_finally.value(), - var_catch_finally.value()); - Return(result); - } + Node* const promise_then = + GetProperty(context, promise, factory()->then_string()); + Node* const result_promise = + CallJS(CodeFactory::Call(isolate()), context, promise_then, promise, + var_then_finally.value(), var_catch_finally.value()); + Return(result_promise); } TF_BUILTIN(ResolveNativePromise, PromiseBuiltinsAssembler) { diff --git a/src/builtins/builtins-promise-gen.h b/src/builtins/builtins-promise-gen.h index 5240da182d..c2cadecfd2 100644 --- a/src/builtins/builtins-promise-gen.h +++ b/src/builtins/builtins-promise-gen.h @@ -57,11 +57,11 @@ class PromiseBuiltinsAssembler : public CodeStubAssembler { // This is used by the Promise.prototype.finally builtin to store // onFinally callback and the Promise constructor. - // TODO(gsathya): Add extra slot for Promise constructor. // TODO(gsathya): For native promises we can create a variant of // this without extra space for the constructor to save memory. enum PromiseFinallyContextSlot { kOnFinallySlot = Context::MIN_CONTEXT_SLOTS, + kConstructorSlot, kPromiseFinallyContextLength, }; @@ -156,6 +156,7 @@ class PromiseBuiltinsAssembler : public CodeStubAssembler { void InternalPromiseReject(Node* context, Node* promise, Node* value, Node* debug_event); std::pair CreatePromiseFinallyFunctions(Node* on_finally, + Node* constructor, Node* native_context); Node* CreateValueThunkFunction(Node* value, Node* native_context); diff --git a/test/mjsunit/harmony/promise-prototype-finally.js b/test/mjsunit/harmony/promise-prototype-finally.js index d80065f10d..3668ab5538 100644 --- a/test/mjsunit/harmony/promise-prototype-finally.js +++ b/test/mjsunit/harmony/promise-prototype-finally.js @@ -524,3 +524,84 @@ assertTrue(descriptor.configurable); assertFalse(descriptor.enumerable); assertEquals("finally", Promise.prototype.finally.name); assertEquals(1, Promise.prototype.finally.length); + +var count = 0; +class FooPromise extends Promise { + constructor(resolve, reject) { + count++; + return super(resolve, reject); + } +} + +testAsync(assert => { + assert.plan(1); + count = 0; + + new FooPromise(r => r()).finally(() => {}).then(() => { + assert.equals(6, count); + }); +}, "finally/speciesconstructor"); + +testAsync(assert => { + assert.plan(1); + count = 0; + + FooPromise.resolve().finally(() => {}).then(() => { + assert.equals(6, count); + }) +}, "resolve/finally/speciesconstructor"); + +testAsync(assert => { + assert.plan(1); + count = 0; + + FooPromise.reject().finally(() => {}).catch(() => { + assert.equals(6, count); + }) +}, "reject/finally/speciesconstructor"); + +testAsync(assert => { + assert.plan(2); + + class MyPromise extends Promise { + static get [Symbol.species]() { return Promise; } + } + + var p = Promise + .resolve() + .finally(() => MyPromise.resolve()); + + assert.equals(true, p instanceof Promise); + assert.equals(false, p instanceof MyPromise); +}, "finally/Symbol.Species"); + +testAsync(assert => { + assert.plan(3); + let resolve; + let value = 0; + + let p = new Promise(r => { resolve = r }); + + Promise.resolve() + .finally(() => { + return p; + }) + .then(() => { + value = 1; + }); + + // This makes sure we take the fast path in PromiseResolve that just + // returns the promise it receives as value. If we had to create + // another wrapper promise, that would cause an additional tick in + // the microtask queue. + Promise.resolve() + // onFinally has run. + .then(() => { resolve(); }) + // thenFinally has run. + .then(() => assert.equals(0, value)) + // promise returned by .finally has been resolved. + .then(() => assert.equals(0, value)) + // onFulfilled callback of .then() has run. + .then(() => assert.equals(1, value)); + +}, "PromiseResolve-ordering");