[Promise combinators] Spec change: check "resolve" early

Promise.{all,allSettled,any,race} should check resolve is a function before
opening their iteratable.

PR: https://github.com/tc39/ecma262/pull/1912

PR for Promise.any: https://github.com/tc39/proposal-promise-any/pull/65

This CL includes the following cleanup changes:
- Made it more explicit that the constructor is a Constructor.
- Removed unnecessary nested try blocks (a try can have both a catch and a label).
- Moved commonly used definitions out of promise-race.tq where they don't belong.
- Made the parameter order of PerformPromiseAll match the spec.


Bug: v8:10578
Change-Id: I9deb5d5106db7350a0d0ad52f165ff2469e7074b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2232544
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68260}

src/builtins/base.tq

@@ -546,6 +546,8 @@ extern transitioning macro HasProperty_Inline(implicit context: Context)(
 extern builtin LoadIC(
     Context, JSAny, JSAny, TaggedIndex, FeedbackVector): JSAny;
 
+extern macro SetPropertyStrict(Context, Object, Object, Object): Object;
+
 extern macro ThrowRangeError(implicit context: Context)(
     constexpr MessageTemplate): never;
 extern macro ThrowRangeError(implicit context: Context)(

src/builtins/promise-abstract-operations.tq

@@ -24,6 +24,16 @@ PromiseRejectEventFromStack(implicit context: Context)(JSPromise, JSAny): JSAny;
 
 // https://tc39.es/ecma262/#sec-promise-abstract-operations
 namespace promise {
+
+extern macro PromiseForwardingHandlerSymbolConstant(): Symbol;
+const kPromiseForwardingHandlerSymbol: Symbol =
+    PromiseForwardingHandlerSymbolConstant();
+extern macro PromiseHandledBySymbolConstant(): Symbol;
+const kPromiseHandledBySymbol: Symbol = PromiseHandledBySymbolConstant();
+extern macro ResolveStringConstant(): String;
+const kResolveString: String = ResolveStringConstant();
+extern macro IsPromiseResolveProtectorCellInvalid(): bool;
+
 extern macro AllocateFunctionWithMapAndContext(
     Map, SharedFunctionInfo, Context): JSFunction;
 
@@ -503,6 +513,41 @@ PromiseGetCapabilitiesExecutor(
   return Undefined;
 }
 
+macro IsPromiseResolveLookupChainIntact(implicit context: Context)(
+    nativeContext: NativeContext, constructor: JSReceiver): bool {
+  if (IsForceSlowPath()) return false;
+  const promiseFun = UnsafeCast<JSFunction>(
+      nativeContext[NativeContextSlot::PROMISE_FUNCTION_INDEX]);
+  return promiseFun == constructor && !IsPromiseResolveProtectorCellInvalid();
+}
+
+// https://tc39.es/ecma262/#sec-getpromiseresolve
+transitioning macro GetPromiseResolve(implicit context: Context)(
+    nativeContext: NativeContext, constructor: Constructor): JSAny {
+  // 1. Assert: IsConstructor(constructor) is true.
+
+  // We can skip the "resolve" lookup on {constructor} if it's the
+  // Promise constructor and the Promise.resolve protector is intact,
+  // as that guards the lookup path for the "resolve" property on the
+  // Promise constructor. In this case, promiseResolveFunction is undefined,
+  // and when CallResolve is called with it later, it will call Promise.resolve.
+  let promiseResolveFunction: JSAny = Undefined;
+
+  if (!IsPromiseResolveLookupChainIntact(nativeContext, constructor)) {
+    let promiseResolve: JSAny;
+
+    // 2. Let promiseResolve be ? Get(constructor, "resolve").
+    promiseResolve = GetProperty(constructor, kResolveString);
+
+    // 3. If IsCallable(promiseResolve) is false, throw a TypeError exception.
+    promiseResolveFunction =
+        Cast<Callable>(promiseResolve) otherwise ThrowTypeError(
+            MessageTemplate::kCalledNonCallable, 'resolve');
+  }
+  // 4. return promiseResolve.
+  return promiseResolveFunction;
+}
+
 transitioning macro CallResolve(implicit context: Context)(
     constructor: Constructor, resolve: JSAny, value: JSAny): JSAny {
   // Undefined can never be a valid value for the resolve function,

src/builtins/promise-any.tq

@@ -135,14 +135,13 @@ PromiseAnyRejectElementClosure(
 }
 
 transitioning macro PerformPromiseAny(implicit context: Context)(
-    iteratorRecord: iterator::IteratorRecord, constructor: Constructor,
-    resultCapability: PromiseCapability): JSAny labels
+    nativeContext: NativeContext, iteratorRecord: iterator::IteratorRecord,
+    constructor: Constructor, resultCapability: PromiseCapability,
+    promiseResolveFunction: JSAny): JSAny labels
 Reject(Object) {
   // 1. Assert: ! IsConstructor(constructor) is true.
   // 2. Assert: resultCapability is a PromiseCapability Record.
 
-  const nativeContext = LoadNativeContext(context);
-
   // 3. Let errors be a new empty List. (Do nothing: errors is
   // initialized lazily when the first Promise rejects.)
 
@@ -155,21 +154,6 @@ Reject(Object) {
   let index: Smi = 1;
 
   try {
-    // We can skip the "resolve" lookup on {constructor} if it's the
-    // Promise constructor and the Promise.resolve protector is intact,
-    // as that guards the lookup path for the "resolve" property on the
-    // Promise constructor.
-    let promiseResolveFunction: JSAny = Undefined;
-    if (!IsPromiseResolveLookupChainIntact(nativeContext, constructor))
-      deferred {
-        // 6. Let promiseResolve be ? Get(constructor, `"resolve"`).
-        const promiseResolve = GetProperty(constructor, kResolveString);
-        // 7. If IsCallable(promiseResolve) is false, throw a
-        // TypeError exception.
-        promiseResolveFunction = Cast<Callable>(promiseResolve)
-            otherwise ThrowTypeError(
-            MessageTemplate::kCalledNonCallable, 'resolve');
-      }
     const fastIteratorResultMap = UnsafeCast<Map>(
         nativeContext[NativeContextSlot::ITERATOR_RESULT_MAP_INDEX]);
     // 8. Repeat,
@@ -312,6 +296,8 @@ Reject(Object) {
 transitioning javascript builtin
 PromiseAny(
     js-implicit context: Context, receiver: JSAny)(iterable: JSAny): JSAny {
+  const nativeContext = LoadNativeContext(context);
+
   // 1. Let C be the this value.
   const receiver = Cast<JSReceiver>(receiver)
       otherwise ThrowTypeError(MessageTemplate::kCalledOnNonObject, 'Promise.any');
@@ -319,37 +305,42 @@ PromiseAny(
   // 2. Let promiseCapability be ? NewPromiseCapability(C).
   const capability = NewPromiseCapability(receiver, False);
 
-  // NewPromiseCapability guarantees that receiver is Constructor
+  // NewPromiseCapability guarantees that receiver is Constructor.
   assert(Is<Constructor>(receiver));
   const constructor = UnsafeCast<Constructor>(receiver);
 
   try {
-    let iteratorRecord: iterator::IteratorRecord;
-    try {
-      // 3. Let iteratorRecord be GetIterator(iterable).
+    // 3. Let promiseResolve be GetPromiseResolve(C).
+    // 4. IfAbruptRejectPromise(promiseResolve, promiseCapability).
+    // (catch below)
+    const promiseResolveFunction =
+        GetPromiseResolve(nativeContext, constructor);
 
-      // 4. IfAbruptRejectPromise(iteratorRecord, promiseCapability).
-      // (catch below)
-      iteratorRecord = iterator::GetIterator(iterable);
+    // 5. Let iteratorRecord be GetIterator(iterable).
 
-      // 5. Let result be PerformPromiseAny(iteratorRecord, C,
-      // promiseCapability).
+    // 6. IfAbruptRejectPromise(iteratorRecord, promiseCapability).
+    // (catch below)
+    const iteratorRecord = iterator::GetIterator(iterable);
 
-      // 6. If result is an abrupt completion, then
+    // 7. Let result be PerformPromiseAny(iteratorRecord, C,
+    // promiseCapability).
 
-      //   a. If iteratorRecord.[[Done]] is false, set result to
-      //   IteratorClose(iteratorRecord, result).
+    // 8. If result is an abrupt completion, then
 
-      //   b. IfAbruptRejectPromise(result, promiseCapability).
+    //   a. If iteratorRecord.[[Done]] is false, set result to
+    //   IteratorClose(iteratorRecord, result).
 
-      // [Iterator closing handled by PerformPromiseAny]
+    //   b. IfAbruptRejectPromise(result, promiseCapability).
 
-      // 7. Return Completion(result).
-      return PerformPromiseAny(iteratorRecord, constructor, capability)
-          otherwise Reject;
-    } catch (e) deferred {
-      goto Reject(e);
-    }
+    // [Iterator closing handled by PerformPromiseAny]
+
+    // 9. Return Completion(result).
+    return PerformPromiseAny(
+        nativeContext, iteratorRecord, constructor, capability,
+        promiseResolveFunction)
+        otherwise Reject;
+  } catch (e) deferred {
+    goto Reject(e);
   } label Reject(e: Object) deferred {
     // Exception must be bound to a JS value.
     assert(e != TheHole);

src/builtins/promise-race.tq

@@ -6,24 +6,6 @@
 
 namespace promise {
 
-extern macro PromiseForwardingHandlerSymbolConstant(): Symbol;
-const kPromiseForwardingHandlerSymbol: Symbol =
-    PromiseForwardingHandlerSymbolConstant();
-extern macro PromiseHandledBySymbolConstant(): Symbol;
-const kPromiseHandledBySymbol: Symbol = PromiseHandledBySymbolConstant();
-extern macro ResolveStringConstant(): String;
-const kResolveString: String = ResolveStringConstant();
-extern macro SetPropertyStrict(Context, Object, Object, Object): Object;
-extern macro IsPromiseResolveProtectorCellInvalid(): bool;
-
-macro IsPromiseResolveLookupChainIntact(implicit context: Context)(
-    nativeContext: NativeContext, constructor: JSReceiver): bool {
-  if (IsForceSlowPath()) return false;
-  const promiseFun = UnsafeCast<JSFunction>(
-      nativeContext[NativeContextSlot::PROMISE_FUNCTION_INDEX]);
-  return promiseFun == constructor && !IsPromiseResolveProtectorCellInvalid();
-}
-
 // https://tc39.es/ecma262/#sec-promise.race
 transitioning javascript builtin
 PromiseRace(
@@ -31,6 +13,8 @@ PromiseRace(
   const receiver = Cast<JSReceiver>(receiver)
       otherwise ThrowTypeError(MessageTemplate::kCalledOnNonObject, 'Promise.race');
 
+  const nativeContext = LoadNativeContext(context);
+
   // Let promiseCapability be ? NewPromiseCapability(C).
   // Don't fire debugEvent so that forwarding the rejection through all does
   // not trigger redundant ExceptionEvents
@@ -39,6 +23,10 @@ PromiseRace(
   const reject = capability.reject;
   const promise = capability.promise;
 
+  // NewPromiseCapability guarantees that receiver is Constructor.
+  assert(Is<Constructor>(receiver));
+  const constructor = UnsafeCast<Constructor>(receiver);
+
   // For catch prediction, don't treat the .then calls as handling it;
   // instead, recurse outwards.
   if (IsDebugActive()) deferred {
@@ -46,10 +34,15 @@ PromiseRace(
     }
 
   try {
-    // Let iterator be GetIterator(iterable).
-    // IfAbruptRejectPromise(iterator, promiseCapability).
+    let promiseResolveFunction: JSAny;
     let i: iterator::IteratorRecord;
     try {
+      // Let promiseResolve be GetPromiseResolve(C).
+      // IfAbruptRejectPromise(promiseResolve, promiseCapability).
+      promiseResolveFunction = GetPromiseResolve(nativeContext, constructor);
+
+      // Let iterator be GetIterator(iterable).
+      // IfAbruptRejectPromise(iterator, promiseCapability).
       i = iterator::GetIterator(iterable);
     } catch (e) deferred {
       goto Reject(e);
@@ -57,24 +50,6 @@ PromiseRace(
 
     // Let result be PerformPromiseRace(iteratorRecord, C, promiseCapability).
     try {
-      // We can skip the "resolve" lookup on {constructor} if it's the
-      // Promise constructor and the Promise.resolve protector is intact,
-      // as that guards the lookup path for the "resolve" property on the
-      // Promise constructor.
-      const nativeContext = LoadNativeContext(context);
-      let promiseResolveFunction: JSAny = Undefined;
-      if (!IsPromiseResolveLookupChainIntact(nativeContext, receiver))
-        deferred {
-          // 3. Let _promiseResolve_ be ? Get(_constructor_, `"resolve"`).
-          const resolve = GetProperty(receiver, kResolveString);
-
-          // 4. If IsCallable(_promiseResolve_) is *false*, throw a
-          // *TypeError* exception.
-          promiseResolveFunction = Cast<Callable>(resolve)
-              otherwise ThrowTypeError(
-              MessageTemplate::kCalledNonCallable, 'resolve');
-        }
-
       const fastIteratorResultMap = UnsafeCast<Map>(
           nativeContext[NativeContextSlot::ITERATOR_RESULT_MAP_INDEX]);
       while (true) {
@@ -96,9 +71,8 @@ PromiseRace(
         }
         // Let nextPromise be ? Call(constructor, _promiseResolve_, «
         // nextValue »).
-        const nextPromise = CallResolve(
-            UnsafeCast<Constructor>(receiver), promiseResolveFunction,
-            nextValue);
+        const nextPromise =
+            CallResolve(constructor, promiseResolveFunction, nextValue);
 
         // Perform ? Invoke(nextPromise, "then", « resolveElement,
         //                  resultCapability.[[Reject]] »).

test/mjsunit/es6/promise-all-resolve-not-callable.js

@@ -6,16 +6,12 @@
 
 load('test/mjsunit/test-async.js');
 
-// Promise.all should call IteratorClose if Promise.resolve is not callable.
+// Promise.all should not call GetIterator if Promise.resolve is not callable.
 
-let returnCount = 0;
+let getIteratorCount = 0;
 let iter = {
-  [Symbol.iterator]() {
-    return {
-      return() {
-        returnCount++;
-      }
-    };
+  get [Symbol.iterator]() {
+    ++getIteratorCount;
   }
 };
 
@@ -25,6 +21,6 @@ testAsync(assert => {
   assert.plan(2);
   Promise.all(iter).then(assert.unreachable, reason => {
     assert.equals(true, reason instanceof TypeError);
-    assert.equals(1, returnCount);
+    assert.equals(0, getIteratorCount);
   });
 });

test/mjsunit/es6/promise-allsettled-resolve-not-callable.js

@@ -6,16 +6,12 @@
 
 load('test/mjsunit/test-async.js');
 
-// Promise.allSettled should call IteratorClose if Promise.resolve is not callable.
+// Promise.allSettled should not call GetIterator if Promise.resolve is not callable.
 
-let returnCount = 0;
+let getIteratorCount = 0;
 let iter = {
-  [Symbol.iterator]() {
-    return {
-      return() {
-        returnCount++;
-      }
-    };
+  get [Symbol.iterator]() {
+    ++getIteratorCount;
   }
 };
 
@@ -25,6 +21,6 @@ testAsync(assert => {
   assert.plan(2);
   Promise.allSettled(iter).then(assert.unreachable, reason => {
     assert.equals(true, reason instanceof TypeError);
-    assert.equals(1, returnCount);
+    assert.equals(0, getIteratorCount);
   });
 });

test/mjsunit/es6/promise-race-resolve-not-callable.js

@@ -6,16 +6,12 @@
 
 load('test/mjsunit/test-async.js');
 
-// Promise.race should call IteratorClose if Promise.resolve is not callable.
+// Promise.race should not call GetIterator if Promise.resolve is not callable.
 
-let returnCount = 0;
+let getIteratorCount = 0;
 let iter = {
-  [Symbol.iterator]() {
-    return {
-      return() {
-        returnCount++;
-      }
-    };
+  get [Symbol.iterator]() {
+    ++getIteratorCount;
   }
 };
 
@@ -25,6 +21,6 @@ testAsync(assert => {
   assert.plan(2);
   Promise.race(iter).then(assert.unreachable, reason => {
     assert.equals(true, reason instanceof TypeError);
-    assert.equals(1, returnCount);
+    assert.equals(0, getIteratorCount);
   });
 });

test/mjsunit/harmony/promise-any-resolve-not-callable.js

@@ -8,14 +8,10 @@ load('test/mjsunit/test-async.js');
 
 // Promise.any should call IteratorClose if Promise.resolve is not callable.
 
-let returnCount = 0;
+let getIteratorCount = 0;
 let iter = {
-  [Symbol.iterator]() {
-    return {
-      return() {
-        returnCount++;
-      }
-    };
+  get [Symbol.iterator]() {
+    ++getIteratorCount;
   }
 };
 
@@ -25,6 +21,6 @@ testAsync(assert => {
   assert.plan(2);
   Promise.any(iter).then(assert.unreachable, reason => {
     assert.equals(true, reason instanceof TypeError);
-    assert.equals(1, returnCount);
+    assert.equals(0, getIteratorCount);
   });
 });

test/test262/test262.status

@@ -646,6 +646,17 @@
   'harness/detachArrayBuffer': [SKIP],
   'harness/detachArrayBuffer-host-detachArrayBuffer': [SKIP],
 
+  # Invalid due to a recent spec change (will be updated when we roll test262).
+  # https://bugs.chromium.org/p/v8/issues/detail?id=10578
+  'built-ins/Promise/all/invoke-resolve-get-error-close': [SKIP],
+  'built-ins/Promise/all/resolve-not-callable-close': [SKIP],
+  'built-ins/Promise/allSettled/invoke-resolve-get-error-close': [SKIP],
+  'built-ins/Promise/allSettled/resolve-not-callable-close': [SKIP],
+  'built-ins/Promise/any/invoke-resolve-get-error-close': [SKIP],
+  'built-ins/Promise/any/resolve-not-callable-close': [SKIP],
+  'built-ins/Promise/race/invoke-resolve-get-error-close': [SKIP],
+  'built-ins/Promise/race/resolve-not-callable-close': [SKIP],
+
   ############################ SKIPPED TESTS #############################
 
   # These tests take a looong time to run.