Fix errors in Future implementation around Future<Future<X>>.

Previously, when completing a future with another future,
we always chained to the other future. This effectively awaits
the latter future and then completes the former with its result.
This is incorrect behavior if the former future is, say,
a `Future<Future<int>>` and the latter is a `Future<int>`.
In that case we *must not* await the latter future
because we can't complete the former future with an `int`.
We should just complete the future directly with the latter future
as a value.

We now check first whether to chain a `Future<T>` to another
future, and only does so if the latter future is a `Future<T>`
(or it's not a `T`, which shouldn't happen,
but currently does in some places).

Add test for behavior.

Change-Id: I57e27111c2fc7b7792dcf4ae9b7c1d5d504d0c0f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/53602
Commit-Queue: Lasse R.H. Nielsen <lrn@google.com>
Reviewed-by: Nate Bosch <nbosch@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>

sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart

@@ -33,7 +33,7 @@ _async<T>(Function() initGenerator) {
       f = value;
     } else if (value is Future) {
       f = _Future();
-      _Future._chainForeignFuture(value, f);
+      f._chainForeignFuture(value);
     } else {
       f = _Future.value(value);
     }
@@ -100,7 +100,7 @@ _async<T>(Function() initGenerator) {
           if (value is _Future) {
             _Future._chainCoreFuture(value, asyncFuture);
           } else {
-            _Future._chainForeignFuture(value, asyncFuture);
+            asyncFuture._chainForeignFuture(value);
           }
         } else if (isRunningAsEvent) {
           asyncFuture._completeWithValue(JS('', '#', value));
@@ -356,7 +356,7 @@ class _AsyncStarImpl<T> {
       f = value;
     } else if (value is Future) {
       f = _Future();
-      _Future._chainForeignFuture(value, f);
+      f._chainForeignFuture(value);
     } else {
       f = _Future.value(value);
     }

sdk/lib/async/future_impl.dart

@@ -173,6 +173,11 @@ class _FutureListener<S, T> {
     assert(!handlesError);
     return _zone.run(_whenCompleteAction);
   }
+
+  // Whether the [value] future should be awaited and the [future] completed
+  // with its result, rather than just completing the [future] directly
+  // with the [value].
+  bool shouldChain(Future<dynamic> value) => value is Future<T> || value is! T;
 }
 
 class _Future<T> implements Future<T> {
@@ -457,27 +462,28 @@ class _Future<T> implements Future<T> {
     return prev;
   }
 
-  // Take the value (when completed) of source and complete target with that
+  // Take the value (when completed) of source and complete this future with that
   // value (or error). This function could chain all Futures, but is slower
   // for _Future than _chainCoreFuture, so you must use _chainCoreFuture
   // in that case.
-  static void _chainForeignFuture(Future source, _Future target) {
-    assert(!target._isComplete);
+  void _chainForeignFuture(Future source) {
+    assert(!_isComplete);
     assert(source is! _Future);
 
     // Mark the target as chained (and as such half-completed).
-    target._setPendingComplete();
+    _setPendingComplete();
     try {
       source.then((value) {
-        assert(target._isPendingComplete);
-        // The "value" may be another future if the foreign future
-        // implementation is mis-behaving,
-        // so use _complete instead of _completeWithValue.
-        target._clearPendingComplete(); // Clear this first, it's set again.
-        target._complete(value);
+        assert(_isPendingComplete);
+        _clearPendingComplete(); // Clear this first, it's set again.
+        try {
+          _completeWithValue(value as T);
+        } catch (error, stackTrace) {
+          _completeError(error, stackTrace);
+        }
       }, onError: (Object error, StackTrace stackTrace) {
-        assert(target._isPendingComplete);
-        target._completeError(error, stackTrace);
+        assert(_isPendingComplete);
+        _completeError(error, stackTrace);
       });
     } catch (e, s) {
       // This only happens if the `then` call threw synchronously when given
@@ -485,7 +491,7 @@ class _Future<T> implements Future<T> {
       // That requires a non-conforming implementation of the Future interface,
       // which should, hopefully, never happen.
       scheduleMicrotask(() {
-        target._completeError(e, s);
+        _completeError(e, s);
       });
     }
   }
@@ -514,7 +520,7 @@ class _Future<T> implements Future<T> {
       if (value is _Future<T>) {
         _chainCoreFuture(value, this);
       } else {
-        _chainForeignFuture(value, this);
+        _chainForeignFuture(value);
       }
     } else {
       _FutureListener? listeners = _removeListeners();
@@ -529,7 +535,6 @@ class _Future<T> implements Future<T> {
 
   void _completeWithValue(T value) {
     assert(!_isComplete);
-    assert(value is! Future<T>);
 
     _FutureListener? listeners = _removeListeners();
     _setValue(value);
@@ -589,7 +594,7 @@ class _Future<T> implements Future<T> {
       return;
     }
     // Just listen on the foreign future. This guarantees an async delay.
-    _chainForeignFuture(value, this);
+    _chainForeignFuture(value);
   }
 
   void _asyncCompleteError(Object error, StackTrace stackTrace) {
@@ -630,7 +635,7 @@ class _Future<T> implements Future<T> {
         nextListener = listener._nextListener;
       }
 
-      final sourceResult = source._resultOrListeners;
+      final dynamic sourceResult = source._resultOrListeners;
       // Do the actual propagation.
       // Set initial state of listenerHasError and listenerValueOrError. These
       // variables are updated with the outcome of potential callbacks.
@@ -740,9 +745,10 @@ class _Future<T> implements Future<T> {
         // If we changed zone, oldZone will not be null.
         if (oldZone != null) Zone._leave(oldZone);
 
-        // If the listener's value is a future we need to chain it. Note that
+        // If the listener's value is a future we *might* need to chain it. Note that
         // this can only happen if there is a callback.
-        if (listenerValueOrError is Future) {
+        if (listenerValueOrError is Future &&
+            listener.shouldChain(listenerValueOrError)) {
           Future chainSource = listenerValueOrError;
           // Shortcut if the chain-source is already completed. Just continue
           // the loop.
@@ -757,7 +763,7 @@ class _Future<T> implements Future<T> {
               _chainCoreFuture(chainSource, result);
             }
           } else {
-            _chainForeignFuture(chainSource, result);
+            result._chainForeignFuture(chainSource);
           }
           return;
         }

tests/lib/async/future_future_test.dart

@@ -0,0 +1,186 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Checks that `Future<Future<int>>` is a valid type
+// and that futures can contain and complete other futures.
+
+// This essentially checks that `FutureOr<X>` is treated correctly depending
+// on what `X` is.
+
+import 'dart:async';
+import 'package:async_helper/async_helper.dart';
+import "package:expect/expect.dart";
+
+main() {
+  asyncStart();
+
+  // Helper values and factories.
+  Future<Null> nullFuture = Future<Null>.value(null);
+
+  var stack = StackTrace.current;
+  var error = ArgumentError("yep");
+  Future<Null> errorFuture = Future<Null>.error(error, stack)
+    ..catchError((_) => null);
+  Future<int> fi(int n) => Future<int>.value(n);
+  Future<Future<int>> ffi(n) => Future<Future<int>>.value(fi(n));
+
+  // Tests that `Future<Future<int>>` can be created.
+  asyncTest(() {
+    return expectFutureFutureInt(ffi(0), 0);
+  });
+
+  // Check `Future.then`'s callback.
+
+  asyncTest(() {
+    Future<int> future = nullFuture.then<int>((_) => fi(1));
+    return expectFutureInt(future, 1);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future = nullFuture.then<Future<int>>((_) => fi(2));
+    return expectFutureFutureInt(future, 2);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future = nullFuture.then<Future<int>>((_) => ffi(3));
+    return expectFutureFutureInt(future, 3);
+  });
+
+  // Check `Future.then`'s `onError`.
+
+  asyncTest(() {
+    Future<int> future =
+        errorFuture.then<int>((_) => -1, onError: (_) => fi(4));
+    return expectFutureInt(future, 4);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future =
+        errorFuture.then<Future<int>>((_) => fi(-1), onError: (_) => fi(5));
+    return expectFutureFutureInt(future, 5);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future =
+        errorFuture.then<Future<int>>((_) => fi(-1), onError: (_) => ffi(6));
+    return expectFutureFutureInt(future, 6);
+  });
+
+  // Check `Future.catchError`.
+  // Its implied `FutureOr` return type is based on the original future's type.
+
+  asyncTest(() {
+    Future<int> errorFuture = Future<int>.error(error, stack);
+    Future<int> future = errorFuture.catchError((_) => fi(7));
+    return expectFutureInt(future, 7);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> errorFuture = Future<Future<int>>.error(error, stack);
+    Future<Future<int>> future = errorFuture.catchError((_) => fi(8));
+    return expectFutureFutureInt(future, 8);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> errorFuture = Future<Future<int>>.error(error, stack);
+    Future<Future<int>> future = errorFuture.catchError((_) => ffi(9));
+    return expectFutureFutureInt(future, 9);
+  });
+
+  // Check `Completer.complete`.
+
+  asyncTest(() {
+    var completer = Completer<int>()..complete(fi(10));
+    return expectFutureInt(completer.future, 10);
+  });
+
+  asyncTest(() {
+    var completer = Completer<Future<int>>()..complete(fi(11));
+    return expectFutureFutureInt(completer.future, 11);
+  });
+
+  asyncTest(() {
+    var completer = Completer<Future<int>>()..complete(ffi(12));
+    return expectFutureFutureInt(completer.future, 12);
+  });
+
+  // Future<Object> works correctly when Object is another Future.
+  asyncTest(() {
+    Future<Object> future = nullFuture.then<Object>((_) => fi(13));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    return future.then<void>((o) {
+      Expect.equals(13, o);
+    });
+  });
+
+  asyncTest(() {
+    Future<Object> future = nullFuture.then<Object>((_) => ffi(14));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    Expect.notType<Future<Future<int>>>(future);
+    return future.then<void>((v) => expectFutureInt(v, 14));
+  });
+
+  asyncTest(() {
+    Future<Object> future =
+        errorFuture.then<Object>((_) => -1, onError: (_) => fi(15));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    return future.then<void>((o) {
+      Expect.equals(15, o);
+    });
+  });
+
+  asyncTest(() {
+    Future<Object> future =
+        errorFuture.then<Object>((_) => -1, onError: (_) => ffi(16));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    Expect.notType<Future<Future<int>>>(future);
+    return future.then<void>((v) => expectFutureInt(v, 16));
+  });
+
+  asyncTest(() {
+    Future<Object> errorFuture = Future<Object>.error(error, stack);
+    Future<Object> future = errorFuture.catchError((_) => fi(17));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    return future.then<void>((o) {
+      Expect.equals(17, o);
+    });
+  });
+
+  asyncTest(() {
+    Future<Object> errorFuture = Future<Object>.error(error, stack);
+    Future<Object> future = errorFuture.catchError((_) => ffi(18));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    Expect.notType<Future<Future<int>>>(future);
+    return future.then<void>((v) => expectFutureInt(v, 18));
+  });
+
+  asyncEnd();
+}
+
+// Checks that future is a Future<Future<int>> containing the value Future<int>
+// which then contains the value 42.
+Future<void> expectFutureFutureInt(dynamic future, int n) {
+  Expect.type<Future<Future<int>>>(future);
+  asyncStart();
+  return future.then<void>((dynamic v) {
+    Expect.type<Future<int>>(v, "$n");
+    return expectFutureInt(v, n).then(asyncSuccess);
+  });
+}
+
+Future<void> expectFutureInt(dynamic future, int n) {
+  Expect.type<Future<int>>(future);
+  asyncStart();
+  return future.then<void>((dynamic v) {
+    Expect.type<int>(v, "$n");
+    Expect.equals(n, v);
+    asyncEnd();
+  });
+}

tests/lib/async/future_test.dart

@@ -1094,7 +1094,7 @@ void testFutureResult() {
     var f = new UglyFuture(5);
     // Sanity check that our future is as mis-behaved as we think.
     f.then((v) {
-      Expect.isTrue(v is Future);
+      Expect.equals(UglyFuture(4), v);
     });
 
     var v = await f;
@@ -1102,13 +1102,11 @@ void testFutureResult() {
     // suggests that it flattens. In practice it currently doesn't.
     // The specification doesn't say anything special, so v should be the
     // completion value of the UglyFuture future which is a future.
-    Expect.isTrue(v is Future);
+    Expect.equals(UglyFuture(4), v);
 
-    // This used to hit an assert in checked mode.
-    // The CL adding this test changed the behavior to actually flatten the
-    // the future returned by the then-callback.
+    // We no longer flatten recursively when completing a future.
     var w = new Future.value(42).then((_) => f);
-    Expect.equals(42, await w);
+    Expect.equals(UglyFuture(4), await w);
     asyncEnd();
   }();
 }
@@ -1259,8 +1257,10 @@ class BadFuture<T> implements Future<T> {
 // An evil future that completes with another future.
 class UglyFuture implements Future<dynamic> {
   final _result;
+  final int _badness;
   UglyFuture(int badness)
-      : _result = (badness == 0) ? 42 : new UglyFuture(badness - 1);
+      : _badness = badness,
+        _result = (badness == 0) ? 42 : new UglyFuture(badness - 1);
   Future<S> then<S>(action(value), {Function? onError}) {
     var c = new Completer<S>();
     c.complete(new Future<S>.microtask(() => action(_result)));
@@ -1282,4 +1282,10 @@ class UglyFuture implements Future<dynamic> {
   Future timeout(Duration duration, {onTimeout()?}) {
     return this;
   }
+
+  int get hashCode => _badness;
+  bool operator ==(Object other) =>
+      other is UglyFuture && _badness == other._badness;
+
+  String toString() => "UglyFuture($_badness)";
 }

tests/lib_2/async/future_future_test.dart

@@ -0,0 +1,187 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Checks that Future<Future<int>> is a valid type and that futures can contain
+// and complete other futures.
+
+// This essentially checks that `FutureOr<X>` is treated correctly depending
+// on what `X` is.
+
+import 'dart:async';
+import 'package:async_helper/async_helper.dart';
+import "package:expect/expect.dart";
+
+main() {
+  asyncStart();
+
+  // Helper values.
+  Future<Null> nullFuture = Future<Null>.value(null);
+
+  var stack = StackTrace.current;
+  var error = ArgumentError("yep");
+  Future<Null> errorFuture = Future<Null>.error(error, stack)
+    ..catchError((_) => null);
+  Future<int> fi(int n) => Future<int>.value(n);
+
+  // Tests that Future<Future<int>> can be created.
+  Future<Future<int>> ffi(n) => Future<Future<int>>.value(fi(n));
+
+  asyncTest(() {
+    return expectFutureFutureInt(ffi(0), 0);
+  });
+
+  // Check `Future.then`'s callback.
+
+  asyncTest(() {
+    Future<int> future = nullFuture.then<int>((_) => fi(1));
+    return expectFutureInt(future, 1);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future = nullFuture.then<Future<int>>((_) => fi(2));
+    return expectFutureFutureInt(future, 2);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future = nullFuture.then<Future<int>>((_) => ffi(3));
+    return expectFutureFutureInt(future, 3);
+  });
+
+  // Check `Future.then`'s `onError`.
+
+  asyncTest(() {
+    Future<int> future =
+        errorFuture.then<int>((_) => -1, onError: (_) => fi(4));
+    return expectFutureInt(future, 4);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future =
+        errorFuture.then<Future<int>>((_) => fi(-1), onError: (_) => fi(5));
+    return expectFutureFutureInt(future, 5);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> future =
+        errorFuture.then<Future<int>>((_) => fi(-1), onError: (_) => ffi(6));
+    return expectFutureFutureInt(future, 6);
+  });
+
+  // Checkc Future.catchError, it's FutureOr is based on the
+  // original future's type.
+
+  asyncTest(() {
+    Future<int> errorFuture = Future<int>.error(error, stack);
+    Future<int> future = errorFuture.catchError((_) => fi(7));
+    return expectFutureInt(future, 7);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> errorFuture = Future<Future<int>>.error(error, stack);
+    Future<Future<int>> future = errorFuture.catchError((_) => fi(8));
+    return expectFutureFutureInt(future, 8);
+  });
+
+  asyncTest(() {
+    Future<Future<int>> errorFuture = Future<Future<int>>.error(error, stack);
+    Future<Future<int>> future = errorFuture.catchError((_) => ffi(9));
+    return expectFutureFutureInt(future, 9);
+  });
+
+  // Check Completer.complete.
+
+  asyncTest(() {
+    var completer = Completer<int>()..complete(fi(10));
+    return expectFutureInt(completer.future, 10);
+  });
+
+  asyncTest(() {
+    var completer = Completer<Future<int>>()..complete(fi(11));
+    return expectFutureFutureInt(completer.future, 11);
+  });
+
+  asyncTest(() {
+    var completer = Completer<Future<int>>()..complete(ffi(12));
+    return expectFutureFutureInt(completer.future, 12);
+  });
+
+  // Future<Object> works correctly when Object is another Future.
+  asyncTest(() {
+    Future<Object> future = nullFuture.then<Object>((_) => fi(13));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    return future.then<void>((o) {
+      Expect.equals(13, o);
+    });
+  });
+
+  asyncTest(() {
+    Future<Object> future = nullFuture.then<Object>((_) => ffi(14));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    Expect.notType<Future<Future<int>>>(future);
+    return future.then<void>((v) => expectFutureInt(v, 14));
+  });
+
+  asyncTest(() {
+    Future<Object> future =
+        errorFuture.then<Object>((_) => -1, onError: (_) => fi(15));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    return future.then<void>((o) {
+      Expect.equals(15, o);
+    });
+  });
+
+  asyncTest(() {
+    Future<Object> future =
+        errorFuture.then<Object>((_) => -1, onError: (_) => ffi(16));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    Expect.notType<Future<Future<int>>>(future);
+    return future.then<void>((v) => expectFutureInt(v, 16));
+  });
+
+  asyncTest(() {
+    Future<Object> errorFuture = Future<Object>.error(error, stack);
+    Future<Object> future = errorFuture.catchError((_) => fi(17));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    return future.then<void>((o) {
+      Expect.equals(17, o);
+    });
+  });
+
+  asyncTest(() {
+    Future<Object> errorFuture = Future<Object>.error(error, stack);
+    Future<Object> future = errorFuture.catchError((_) => ffi(18));
+    Expect.type<Future<Object>>(future);
+    Expect.notType<Future<int>>(future);
+    Expect.notType<Future<Future<int>>>(future);
+    return future.then<void>((v) => expectFutureInt(v, 18));
+  });
+
+  asyncEnd();
+}
+
+// Checks that future is a Future<Future<int>> containing the value Future<int>
+// which then contains the value 42.
+Future<void> expectFutureFutureInt(dynamic future, int n) {
+  Expect.type<Future<Future<int>>>(future);
+  asyncStart();
+  return future.then<void>((dynamic v) {
+    Expect.type<Future<int>>(v, "$n");
+    return expectFutureInt(v, n).then(asyncSuccess);
+  });
+}
+
+Future<void> expectFutureInt(dynamic future, int n) {
+  Expect.type<Future<int>>(future);
+  asyncStart();
+  return future.then<void>((dynamic v) {
+    Expect.type<int>(v, "$n");
+    Expect.equals(n, v);
+    asyncEnd();
+  });
+}

tests/lib_2/async/future_test.dart

@@ -1087,7 +1087,7 @@ void testFutureResult() {
     var f = new UglyFuture(5);
     // Sanity check that our future is as mis-behaved as we think.
     f.then((v) {
-      Expect.isTrue(v is Future);
+      Expect.equals(UglyFuture(4), v);
     });
 
     var v = await f;
@@ -1095,13 +1095,11 @@ void testFutureResult() {
     // suggests that it flattens. In practice it currently doesn't.
     // The specification doesn't say anything special, so v should be the
     // completion value of the UglyFuture future which is a future.
-    Expect.isTrue(v is Future);
+    Expect.equals(UglyFuture(4), v);
 
-    // This used to hit an assert in checked mode.
-    // The CL adding this test changed the behavior to actually flatten the
-    // the future returned by the then-callback.
+    // We no longer flatten recursively in situations like this.
     var w = new Future.value(42).then((_) => f);
-    Expect.equals(42, await w);
+    Expect.equals(UglyFuture(4), await w);
     asyncEnd();
   }();
 }
@@ -1253,11 +1251,13 @@ class BadFuture<T> implements Future<T> {
 // An evil future that completes with another future.
 class UglyFuture implements Future<dynamic> {
   final _result;
+  final int _badness;
   UglyFuture(int badness)
-      : _result = (badness == 0) ? 42 : new UglyFuture(badness - 1);
+      : _badness = badness,
+        _result = (badness == 0) ? 42 : new UglyFuture(badness - 1);
   Future<S> then<S>(action(value), {Function onError}) {
     var c = new Completer<S>();
-    c.complete(new Future.microtask(() => action(_result)));
+    c.complete(new Future<S>.microtask(() => action(_result)));
     return c.future;
   }
 
@@ -1276,4 +1276,10 @@ class UglyFuture implements Future<dynamic> {
   Future timeout(Duration duration, {onTimeout()}) {
     return this;
   }
+
+  int get hashCode => _badness;
+  bool operator ==(Object other) =>
+      other is UglyFuture && _badness == other._badness;
+
+  String toString() => "UglyFuture($_badness)";
 }