Reland "Clean up promises and fix an edge case bug (patchset #4 id:60001 of https://codereview.chro…

src/js/promise.js

 // Copyright 2012 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.
 
 (function(global, utils, extrasUtils) {
 
 "use strict";
 
 %CheckIsBootstrapping();
 
@@ skipping 13 matching lines @@
 var promiseStatusSymbol = utils.ImportNow("promise_status_symbol");
 var promiseValueSymbol = utils.ImportNow("promise_value_symbol");
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
 
 utils.Import(function(from) {
   MakeTypeError = from.MakeTypeError;
 });
 
 // -------------------------------------------------------------------
 
 // Status values: 0 = pending, +1 = resolved, -1 = rejected

[Camillo Bruni, 2015/12/28 12:22:35]

Has adding named constants for the status values a performance impact? If not
that would help code readability...

 var lastMicrotaskId = 0;
 
 function CreateResolvingFunctions(promise) {
   var alreadyResolved = false;
 
   var resolve = function(value) {
     if (alreadyResolved === true) return;
     alreadyResolved = true;
-    if (value === promise) {
-      return PromiseReject(promise, MakeTypeError(kPromiseCyclic, value));
-    }
     PromiseResolve(promise, value);
   };
 
   var reject = function(reason) {
     if (alreadyResolved === true) return;
     alreadyResolved = true;
     PromiseReject(promise, reason);
   };
 
   return {
     __proto__: null,
     resolve: resolve,
     reject: reject
   };
 }
 
 
 var GlobalPromise = function Promise(resolver) {
   if (resolver === promiseRawSymbol) {
     return %NewObject(GlobalPromise, new.target);
   }
   if (IS_UNDEFINED(new.target)) throw MakeTypeError(kNotAPromise, this);
   if (!IS_CALLABLE(resolver))
     throw MakeTypeError(kResolverNotAFunction, resolver);

[Camillo Bruni, 2015/12/28 12:22:35]

please add braces here.

 
   var promise = PromiseInit(%NewObject(GlobalPromise, new.target));
 
   try {
     %DebugPushPromise(promise, Promise);
     var callbacks = CreateResolvingFunctions(promise);
     resolver(callbacks.resolve, callbacks.reject);
   } catch (e) {
     PromiseReject(promise, e);
   } finally {
@@ skipping 24 matching lines @@
 }
 
 function PromiseInit(promise) {
   return PromiseSet(
       promise, 0, UNDEFINED, new InternalArray, new InternalArray)
 }
 
 function PromiseDone(promise, status, value, promiseQueue) {
   if (GET_PRIVATE(promise, promiseStatusSymbol) === 0) {
     var tasks = GET_PRIVATE(promise, promiseQueue);
     if (tasks.length) PromiseEnqueue(value, tasks, status);

[Camillo Bruni, 2015/12/28 12:22:35]

According to the spec that's step 8, probably irrelevant, but unless critical I
would move it to a position that matches the spec (makes it easier for future
comparison).

     PromiseSet(promise, status, value);

[Camillo Bruni, 2015/12/28 12:22:35]

I'd make it explicit that you set the [[PromiseFulfillReactions]] and
[[PromiseRejectReactions]] to undefined here.

   }
 }
 
-function PromiseCoerce(constructor, x) {
-  if (!IsPromise(x) && IS_RECEIVER(x)) {
-    var then;
-    try {
-      then = x.then;
-    } catch(r) {
-      return %_Call(PromiseRejected, constructor, r);
-    }
-    if (IS_CALLABLE(then)) {
-      var deferred = NewPromiseCapability(constructor);
-      try {
-        %_Call(then, x, deferred.resolve, deferred.reject);
-      } catch(r) {
-        deferred.reject(r);
-      }
-      return deferred.promise;
-    }
-  }
-  return x;
-}
-
 function PromiseHandle(value, handler, deferred) {
   try {
     %DebugPushPromise(deferred.promise, PromiseHandle);
     var result = handler(value);
-    if (result === deferred.promise)
-      throw MakeTypeError(kPromiseCyclic, result);
-    else if (IsPromise(result))
-      %_Call(PromiseChain, result, deferred.resolve, deferred.reject);
-    else
-      deferred.resolve(result);
+    deferred.resolve(result);
   } catch (exception) {
     try { deferred.reject(exception); } catch (e) { }
   } finally {
     %DebugPopPromise();
   }
 }
 
 function PromiseEnqueue(value, tasks, status) {
   var id, name, instrumenting = DEBUG_IS_ACTIVE;
   %EnqueueMicrotask(function() {
@@ skipping 26 matching lines @@
 
 function IsPromise(x) {
   return IS_RECEIVER(x) && HAS_DEFINED_PRIVATE(x, promiseStatusSymbol);
 }
 
 function PromiseCreate() {
   return new GlobalPromise(PromiseNopResolver)
 }
 
 function PromiseResolve(promise, x) {
-  if (GET_PRIVATE(promise, promiseStatusSymbol) === 0) {
-    if (IS_RECEIVER(x)) {
-      // 25.4.1.3.2 steps 8-12
-      try {
-        var then = x.then;
-      } catch (e) {
-        return PromiseReject(promise, e);
+  if (x === promise) {
+    return PromiseReject(promise, MakeTypeError(kPromiseCyclic, x));
+  }
+  if (IS_RECEIVER(x)) {

[Camillo Bruni, 2015/12/28 12:22:35]

Early return would make this more readable, and easier to compare with the spec.

+    // 25.4.1.3.2 steps 8-12
+    try {
+      var then = x.then;
+    } catch (e) {
+      return PromiseReject(promise, e);
+    }
+    if (IS_CALLABLE(then)) {

[Camillo Bruni, 2015/12/28 12:22:35]

I'd say the same here ;) (yes, yes nit-picking :P)

+      // PromiseResolveThenableJob
+      var id, name, instrumenting = DEBUG_IS_ACTIVE;
+      %EnqueueMicrotask(function() {
+        if (instrumenting) {
+          %DebugAsyncTaskEvent({ type: "willHandle", id: id, name: name });
+        }
+        try {
+          var callbacks = CreateResolvingFunctions(promise);
+          %_Call(then, x, callbacks.resolve, callbacks.reject);
+        } catch (e) {
+          PromiseReject(promise, e);
+        }
+        if (instrumenting) {
+          %DebugAsyncTaskEvent({ type: "didHandle", id: id, name: name });
+        }
+      });
+      if (instrumenting) {
+        id = ++lastMicrotaskId;
+        name = "PromseResolveThenableJob";
+        %DebugAsyncTaskEvent({ type: "enqueue", id: id, name: name });
       }
-      if (IS_CALLABLE(then)) {
-        // PromiseResolveThenableJob
-        return %EnqueueMicrotask(function() {
-          try {
-            var callbacks = CreateResolvingFunctions(promise);
-            %_Call(then, x, callbacks.resolve, callbacks.reject);
-          } catch (e) {
-            PromiseReject(promise, e);
-          }
-        });
-      }
+      return;
     }
-    PromiseDone(promise, +1, x, promiseOnResolveSymbol);
   }
+  PromiseDone(promise, +1, x, promiseOnResolveSymbol);
 }
 
 function PromiseReject(promise, r) {
   // Check promise status to confirm that this reject has an effect.
   // Call runtime for callbacks to the debugger or for unhandled reject.
   if (GET_PRIVATE(promise, promiseStatusSymbol) == 0) {
     var debug_is_active = DEBUG_IS_ACTIVE;
     if (debug_is_active ||
         !HAS_DEFINED_PRIVATE(promise, promiseHasHandlerSymbol)) {
       %PromiseRejectEvent(promise, r, debug_is_active);
@@ skipping 10 matching lines @@
     var promise = PromiseInit(new GlobalPromise(promiseRawSymbol));
     var callbacks = CreateResolvingFunctions(promise);
     return {
       promise: promise,
       resolve: callbacks.resolve,
       reject: callbacks.reject
     };
   } else {
     var result = {promise: UNDEFINED, resolve: UNDEFINED, reject: UNDEFINED };
     result.promise = new C(function(resolve, reject) {
+      // TODO(littledan): Check for resolve and reject being not undefined
       result.resolve = resolve;
       result.reject = reject;
     });
     return result;
   }
 }
 
 function PromiseDeferred() {
   return NewPromiseCapability(this);
 }
 
 function PromiseResolved(x) {
-  if (this === GlobalPromise) {
-    // Optimized case, avoid extra closure.
-    return PromiseCreateAndSet(+1, x);
-  } else {
-    return new this(function(resolve, reject) { resolve(x) });
-  }
+  return %_Call(PromiseCast, this, x);
 }
 
 function PromiseRejected(r) {

[Camillo Bruni, 2015/12/28 12:22:35]

Dangerous name if you ask me :) PromiseReject vs. PromiseRejected

+  if (!IS_RECEIVER(this)) {
+    throw MakeTypeError(kCalledOnNonObject, PromiseRejected);
+  }
   var promise;
   if (this === GlobalPromise) {
     // Optimized case, avoid extra closure.
     promise = PromiseCreateAndSet(-1, r);
     // The debug event for this would always be an uncaught promise reject,
     // which is usually simply noise. Do not trigger that debug event.
     %PromiseRejectEvent(promise, r, false);
   } else {
     promise = new this(function(resolve, reject) { reject(r) });
   }
   return promise;
 }
 
-// Simple chaining.
+// Multi-unwrapped chaining with thenable coercion.
 
-// PromiseChain a.k.a. flatMap
-function PromiseChainInternal(constructor, onResolve, onReject) {
-  onResolve = IS_UNDEFINED(onResolve) ? PromiseIdResolveHandler : onResolve;
-  onReject = IS_UNDEFINED(onReject) ? PromiseIdRejectHandler : onReject;
+function PromiseThen(onResolve, onReject) {
+  var constructor = this.constructor;
+  onResolve = IS_CALLABLE(onResolve) ? onResolve : PromiseIdResolveHandler;
+  onReject = IS_CALLABLE(onReject) ? onReject : PromiseIdRejectHandler;
   var deferred = NewPromiseCapability(constructor);
   switch (GET_PRIVATE(this, promiseStatusSymbol)) {
     case UNDEFINED:
+      // TODO(littledan): The type check should be called before
+      // constructing NewPromiseCapability; this is observable when
+      // erroneously copying this method to other classes.
       throw MakeTypeError(kNotAPromise, this);
     case 0:  // Pending
       GET_PRIVATE(this, promiseOnResolveSymbol).push(onResolve, deferred);
       GET_PRIVATE(this, promiseOnRejectSymbol).push(onReject, deferred);
       break;
     case +1:  // Resolved
       PromiseEnqueue(GET_PRIVATE(this, promiseValueSymbol),
                      [onResolve, deferred],
                      +1);
       break;
     case -1:  // Rejected
       if (!HAS_DEFINED_PRIVATE(this, promiseHasHandlerSymbol)) {
         // Promise has already been rejected, but had no handler.
         // Revoke previously triggered reject event.
         %PromiseRevokeReject(this);
       }
       PromiseEnqueue(GET_PRIVATE(this, promiseValueSymbol),
                      [onReject, deferred],
                      -1);
       break;
   }
   // Mark this promise as having handler.
   SET_PRIVATE(this, promiseHasHandlerSymbol, true);

[Camillo Bruni, 2015/12/28 12:22:35]

Shouldn't this happen only in the rejected case? (spec step 25.4.5.3.1 9.e)

   if (DEBUG_IS_ACTIVE) {
     %DebugPromiseEvent({ promise: deferred.promise, parentPromise: this });
   }
   return deferred.promise;
 }
 
+// Chain is left around for now as an alias for then
 function PromiseChain(onResolve, onReject) {
-  return %_Call(PromiseChainInternal, this, this.constructor,
-                onResolve, onReject);
+  return %_Call(PromiseThen, this, onResolve, onReject);
 }
 
 function PromiseCatch(onReject) {
   return this.then(UNDEFINED, onReject);
 }
 
-// Multi-unwrapped chaining with thenable coercion.
-
-function PromiseThen(onResolve, onReject) {
-  onResolve = IS_CALLABLE(onResolve) ? onResolve : PromiseIdResolveHandler;
-  onReject = IS_CALLABLE(onReject) ? onReject : PromiseIdRejectHandler;
-  var that = this;
-  var constructor = this.constructor;
-  return %_Call(
-    PromiseChainInternal,
-    this,
-    constructor,
-    function(x) {
-      x = PromiseCoerce(constructor, x);
-      if (x === that) {
-        return onReject(MakeTypeError(kPromiseCyclic, x));
-      } else if (IsPromise(x)) {
-        return x.then(onResolve, onReject);
-      } else {
-        return onResolve(x);
-      }
-    },
-    onReject
-  );
-}
-
 // Combinators.
 
 function PromiseCast(x) {

[Camillo Bruni, 2015/12/28 12:22:35]

I know this is probably not very important, but as a new reader PromiseCast is
not a nice name :) Probably we would have to rename PromiseResolve to
PromisePrototypeResovle... and so forth, to rename PromiseCast to
PromiseResolve.

As a minimal aid I would put the spec section. (Yes it's work to maintain these
references ;), but if the spec changes we have to recheck our implementation)

+  if (!IS_RECEIVER(this)) {
+    throw MakeTypeError(kCalledOnNonObject, PromiseCast);
+  }
   if (IsPromise(x) && x.constructor === this) {
     return x;
   } else {
-    return new this(function(resolve) { resolve(x) });
+    return new this(function(resolve, reject) { resolve(x) });
   }
 }
 
 function PromiseAll(iterable) {
   var deferred = NewPromiseCapability(this);
   var resolutions = [];
   try {
     var count = 0;
     var i = 0;
     for (var value of iterable) {
@@ skipping 107 matching lines @@
 [PromiseChain, PromiseDeferred, PromiseResolved].forEach(
     fn => %FunctionRemovePrototype(fn));
 
 utils.Export(function(to) {
   to.PromiseChain = PromiseChain;
   to.PromiseDeferred = PromiseDeferred;
   to.PromiseResolved = PromiseResolved;
 });
 
 })