Better test error handler argument types.

Change-Id: I460a40dc9db096f3ae95602ba9c8dc86b4576c56
Reviewed-on: https://dart-review.googlesource.com/53208
Commit-Queue: Lasse R.H. Nielsen <lrn@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Leaf Petersen <leafp@google.com>

sdk/lib/async/async_error.dart

@@ -13,12 +13,3 @@ _invokeErrorHandler(
     return unaryErrorHandler(error);
   }
 }
-
-Function _registerErrorHandler<R>(Function errorHandler, Zone zone) {
-  if (errorHandler is ZoneBinaryCallback<dynamic, Null, Null>) {
-    return zone
-        .registerBinaryCallback<FutureOr<R>, Object, StackTrace>(errorHandler);
-  } else {
-    return zone.registerUnaryCallback<FutureOr<R>, Object>(errorHandler);
-  }
-}

sdk/lib/async/future.dart

@@ -552,10 +552,14 @@ abstract class Future<T> {
    *
    * If [onError] is provided, and this future completes with an error,
    * the `onError` callback is called with that error and its stack trace.
-   * The `onError` callback must accept either one argument or two arguments.
+   * The `onError` callback must accept either one argument or two arguments
+   * where the latter is a [StackTrace].
    * If `onError` accepts two arguments,
    * it is called with both the error and the stack trace,
    * otherwise it is called with just the error object.
+   * The `onError` callback must return a value or future that can be used
+   * to complete the returned future, so it must be something assignable to
+   * `FutureOr<R>`.
    *
    * Returns a new [Future]
    * which is completed with the result of the call to `onValue`
@@ -586,7 +590,7 @@ abstract class Future<T> {
    * has completed with an error then the error is reported as unhandled error.
    * See the description on [Future].
    */
-  Future<S> then<S>(FutureOr<S> onValue(T value), {Function onError});
+  Future<R> then<R>(FutureOr<R> onValue(T value), {Function onError});
 
   /**
    * Handles errors emitted by this [Future].

sdk/lib/async/future_impl.dart

@@ -137,12 +137,14 @@ class _FutureListener<S, T> {
   FutureOr<T> handleError(AsyncError asyncError) {
     assert(handlesError && hasErrorCallback);
     var errorCallback = this.errorCallback; // To enable promotion.
-    if (errorCallback is ZoneBinaryCallback<FutureOr<T>, Object, StackTrace>) {
+    // If the errorCallback returns something which is not a FutureOr<T>,
+    // this return statement throws, and the caller handles the error.
+    if (errorCallback is dynamic Function(Object, StackTrace)) {
       return _zone.runBinary(
           errorCallback, asyncError.error, asyncError.stackTrace);
     } else {
-      return _zone.runUnary<FutureOr<T>, Object>(
-          errorCallback, asyncError.error);
+      assert(errorCallback is dynamic Function(Object));
+      return _zone.runUnary<dynamic, Object>(errorCallback, asyncError.error);
     }
   }
 
@@ -232,15 +234,18 @@ class _Future<T> implements Future<T> {
     _resultOrListeners = source;
   }
 
-  Future<E> then<E>(FutureOr<E> f(T value), {Function onError}) {
+  Future<R> then<R>(FutureOr<R> f(T value), {Function onError}) {
     Zone currentZone = Zone.current;
     if (!identical(currentZone, _rootZone)) {
-      f = currentZone.registerUnaryCallback<FutureOr<E>, T>(f);
+      f = currentZone.registerUnaryCallback<FutureOr<R>, T>(f);
       if (onError != null) {
-        onError = _registerErrorHandler<E>(onError, currentZone);
+        // In checked mode, this checks that onError is assignable to one of:
+        //   dynamic Function(Object)
+        //   dynamic Function(Object, StackTrace)
+        onError = _registerErrorHandler(onError, currentZone);
       }
     }
-    return _thenNoZoneRegistration<E>(f, onError);
+    return _thenNoZoneRegistration<R>(f, onError);
   }
 
   // This method is used by async/await.
@@ -254,7 +259,7 @@ class _Future<T> implements Future<T> {
   Future<T> catchError(Function onError, {bool test(error)}) {
     _Future<T> result = new _Future<T>();
     if (!identical(result._zone, _rootZone)) {
-      onError = _registerErrorHandler<T>(onError, result._zone);
+      onError = _registerErrorHandler(onError, result._zone);
       if (test != null) test = result._zone.registerUnaryCallback(test);
     }
     _addListener(new _FutureListener<T, T>.catchError(result, onError, test));
@@ -739,3 +744,29 @@ class _Future<T> implements Future<T> {
     return result;
   }
 }
+
+/// Registers errorHandler in zone if it has the correct type.
+///
+/// Checks that the function accepts either an [Object] and a [StackTrace]
+/// or just one [Object]. Does not check the return type.
+/// The actually returned value must be `FutureOr<R>` where `R` is the
+/// value type of the future that the call will complete (either returned
+/// by [Future.then] or [Future.catchError]). We check the returned value
+/// dynamically because the functions are passed as arguments in positions
+/// without inference, so a function expression won't infer the return type.
+///
+/// Throws if the type is not valid.
+Function _registerErrorHandler(Function errorHandler, Zone zone) {
+  if (errorHandler is dynamic Function(Object, StackTrace)) {
+    return zone
+        .registerBinaryCallback<dynamic, Object, StackTrace>(errorHandler);
+  }
+  if (errorHandler is dynamic Function(Object)) {
+    return zone.registerUnaryCallback<dynamic, Object>(errorHandler);
+  }
+  throw new ArgumentError.value(
+      errorHandler,
+      "onError",
+      "Error handler must accept one Object or one Object and a StackTrace"
+      " as arguments, and return a a valid result");
+}

sdk/lib/async/stream_impl.dart

@@ -141,9 +141,15 @@ class _BufferingStreamSubscription<T>
 
   void onError(Function handleError) {
     if (handleError == null) handleError = _nullErrorHandler;
-    // We are not allowed to use 'void' as type argument for the generic type,
-    // so we use 'dynamic' instead.
-    _onError = _registerErrorHandler<dynamic>(handleError, _zone);
+    if (handleError is void Function(Object, StackTrace)) {
+      _onError = _zone
+          .registerBinaryCallback<dynamic, Object, StackTrace>(handleError);
+    } else if (handleError is void Function(Object)) {
+      _onError = _zone.registerUnaryCallback<dynamic, Object>(handleError);
+    } else {
+      throw new ArgumentError("handleError callback must take either an Object "
+          "(the error), or both an Object (the error) and a StackTrace.");
+    }
   }
 
   void onDone(void handleDone()) {
@@ -344,11 +350,11 @@ class _BufferingStreamSubscription<T>
       if (_isCanceled && !_waitsForCancel) return;
       _state |= _STATE_IN_CALLBACK;
       // TODO(floitsch): this dynamic should be 'void'.
-      if (_onError is ZoneBinaryCallback<dynamic, Object, StackTrace>) {
-        ZoneBinaryCallback<dynamic, Object, StackTrace> errorCallback =
-            _onError;
-        _zone.runBinaryGuarded(errorCallback, error, stackTrace);
+      var onError = _onError;
+      if (onError is void Function(Object, StackTrace)) {
+        _zone.runBinaryGuarded<Object, StackTrace>(onError, error, stackTrace);
       } else {
+        assert(_onError is void Function(Object));
         _zone.runUnaryGuarded<Object>(_onError, error);
       }
       _state &= ~_STATE_IN_CALLBACK;

sdk/lib/async/zone.dart

@@ -1419,25 +1419,33 @@ const _rootZone = const _RootZone();
 /**
  * Runs [body] in its own zone.
  *
- * Returns the result of invoking [body].
+ * Creates a new zone using [Zone.fork] based on [zoneSpecification] and
+ * [zoneValues], then runs [body] in that zone and returns the result.
  *
- * If [onError] is non-null the zone is considered an error zone. All uncaught
- * errors, synchronous or asynchronous, in the zone are caught and handled
- * by the callback. When the error is synchronous, throwing in the [onError]
- * handler, leads to a synchronous exception.
+ * If [onError] is provided, it must have one of the types
+ * * `void Function(Object)`
+ * * `void Function(Object, StackTrace)`
+ * and the [onError] handler is used *both* to handle asynchronous errors
+ * by overriding [ZoneSpecification.handleUncaughtError] in [zoneSpecification],
+ * if any, *and* to handle errors thrown synchronously by the call to [body].
  *
- * Returns `null` when [body] threw, and a provided [onError] function completed
- * without throwing.
+ * If an error occurs synchronously in [body],
+ * then throwing in the [onError] handler
+ * makes the call to `runZone` throw that error,
+ * and otherwise the call to `runZoned` returns `null`.
  *
- * Errors may never cross error-zone boundaries. This is intuitive for leaving
- * a zone, but it also applies for errors that would enter an error-zone.
- * Errors that try to cross error-zone boundaries are considered uncaught.
+ * If the zone specification has a `handleUncaughtError` value or the [onError]
+ * parameter is provided, the zone becomes an error-zone.
+ *
+ * Errors will never cross error-zone boundaries by themselves.
+ * Errors that try to cross error-zone boundaries are considered uncaught in
+ * their originating error zone.
  *
  *     var future = new Future.value(499);
  *     runZoned(() {
- *       future = future.then((_) { throw "error in first error-zone"; });
+ *       var future2 = future.then((_) { throw "error in first error-zone"; });
  *       runZoned(() {
- *         future = future.catchError((e) { print("Never reached!"); });
+ *         var future3 = future2.catchError((e) { print("Never reached!"); });
  *       }, onError: (e) { print("unused error handler"); });
  *     }, onError: (e) { print("catches error of first error-zone."); });
  *
@@ -1446,58 +1454,65 @@ const _rootZone = const _RootZone();
  *     runZoned(() {
  *       new Future(() { throw "asynchronous error"; });
  *     }, onError: print);  // Will print "asynchronous error".
+ *
+ * It is possible to manually pass an error from one error zone to another
+ * by re-throwing it in the new zone. If [onError] throws, that error will
+ * occur in the original zone where [runZoned] was called.
  */
 R runZoned<R>(R body(),
     {Map zoneValues, ZoneSpecification zoneSpecification, Function onError}) {
-  // TODO(floitsch): the return type should be `void` here.
-  if (onError != null &&
-      onError is! ZoneBinaryCallback<dynamic, Object, StackTrace> &&
-      onError is! ZoneUnaryCallback<dynamic, Object>) {
-    throw new ArgumentError("onError callback must take an Object (the error), "
-        "or an Object (the error) and a StackTrace");
+  if (onError == null) {
+    return _runZoned<R>(body, zoneValues, zoneSpecification);
   }
-  HandleUncaughtErrorHandler errorHandler;
-  if (onError != null) {
-    errorHandler = (Zone self, ZoneDelegate parent, Zone zone, error,
-        StackTrace stackTrace) {
-      try {
-        if (onError is void Function(Object, StackTrace)) {
-          self.parent.runBinary(onError, error, stackTrace);
-          return;
-        }
-        assert(onError is void Function(Object));
-        self.parent.runUnary(onError, error);
-      } catch (e, s) {
-        if (identical(e, error)) {
-          parent.handleUncaughtError(zone, error, stackTrace);
-        } else {
-          parent.handleUncaughtError(zone, e, s);
-        }
+  void Function(Object) unaryOnError;
+  void Function(Object, StackTrace) binaryOnError;
+  if (onError is void Function(Object)) {
+    unaryOnError = onError;
+  } else if (onError is void Function(Object, StackTrace)) {
+    binaryOnError = onError;
+  } else {
+    throw new ArgumentError("onError callback must take either an Object "
+        "(the error), or both an Object (the error) and a StackTrace.");
+  }
+  HandleUncaughtErrorHandler errorHandler = (Zone self, ZoneDelegate parent,
+      Zone zone, error, StackTrace stackTrace) {
+    try {
+      if (binaryOnError != null) {
+        self.parent.runBinary(binaryOnError, error, stackTrace);
+      } else {
+        assert(unaryOnError != null);
+        self.parent.runUnary(unaryOnError, error);
       }
-    };
-  }
+    } catch (e, s) {
+      if (identical(e, error)) {
+        parent.handleUncaughtError(zone, error, stackTrace);
+      } else {
+        parent.handleUncaughtError(zone, e, s);
+      }
+    }
+  };
   if (zoneSpecification == null) {
     zoneSpecification =
         new ZoneSpecification(handleUncaughtError: errorHandler);
-  } else if (errorHandler != null) {
+  } else {
     zoneSpecification = new ZoneSpecification.from(zoneSpecification,
         handleUncaughtError: errorHandler);
   }
-  Zone zone = Zone.current
-      .fork(specification: zoneSpecification, zoneValues: zoneValues);
-  if (onError != null) {
-    try {
-      return zone.run(body);
-    } catch (e, stackTrace) {
-      if (onError is ZoneBinaryCallback<R, Object, StackTrace>) {
-        zone.runBinary(onError, e, stackTrace);
-        return null;
-      }
-      assert(onError is ZoneUnaryCallback<R, Object>);
-      zone.runUnary(onError, e);
-      return null;
+  try {
+    return _runZoned<R>(body, zoneValues, zoneSpecification);
+  } catch (e, stackTrace) {
+    if (binaryOnError != null) {
+      binaryOnError(e, stackTrace);
+    } else {
+      assert(unaryOnError != null);
+      unaryOnError(e);
     }
-  } else {
-    return zone.run(body);
   }
+  return null;
 }
+
+/// Runs [body] in a new zone based on [zoneValues] and [specification].
+R _runZoned<R>(R body(), Map zoneValues, ZoneSpecification specification) =>
+    Zone.current
+        .fork(specification: specification, zoneValues: zoneValues)
+        .run<R>(body);

tests/co19/co19-co19.status

@@ -120,6 +120,9 @@ Language/Expressions/Additive_Expressions/syntax_t01/07: CompileTimeError
 LibTest/typed_data/Float32x4List/first_A01_t02: CompileTimeError # co19 issue 130 + type error
 LibTest/typed_data/Float32x4List/last_A01_t02: CompileTimeError # co19 issue 130 + type error
 
+[ $runtime != none && !$checked ]
+LibTest/async/Future/catchError_A03_t05: RuntimeError
+
 [ $runtime != none && !$strong ]
 LibTest/typed_data/Float32x4List/first_A01_t02: RuntimeError # co19 issue 130
 LibTest/typed_data/Float32x4List/last_A01_t02: RuntimeError # co19 issue 130

tests/co19_2/co19_2-runtime.status

@@ -2,6 +2,9 @@
 # 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.
 
+[ $runtime != none && !$checked ]
+LibTest/async/Future/catchError_A03_t05: RuntimeError
+
 [ $runtime == dart_precompiled || $runtime == flutter || $runtime == vm ]
 LayoutTests/fast/*: SkipByDesign # DOM not supported on VM.
 LibTest/html/*: SkipByDesign # dart:html not supported on VM.

tests/lib_2/lib_2_dart2js.status

@@ -491,9 +491,6 @@ html/js_function_getter_trust_types_test: Skip # --trust-type-annotations incomp
 [ $compiler == dart2js && $checked && $fasta ]
 async/stream_listen_zone_test: RuntimeError
 
-[ $compiler == dart2js && !$checked ]
-async/async_await_sync_completer_test: RuntimeError
-
 [ $compiler == dart2js && $csp && $fasta && $minified ]
 collection/list_test: RuntimeError
 

tests/lib_2/lib_2_dartdevc.status

@@ -36,7 +36,6 @@ html/fileapi_entry_test: Pass, RuntimeError # Issue 31019
 html/xhr_test: Skip # Times out. Issue 21527
 
 [ $compiler == dartdevc || $compiler == dartdevk ]
-async/async_await_sync_completer_test: RuntimeError # Issue 29922
 async/async_await_zones_test: RuntimeError # Issue 29922
 async/future_or_bad_type_test/implements: RuntimeError # Issue 29922
 async/future_or_bad_type_test/none: RuntimeError # Issue 29922
@@ -45,7 +44,6 @@ async/slow_consumer_test: Pass, Timeout # Issue 29922
 async/stream_controller_async_test: RuntimeError
 async/stream_distinct_test: RuntimeError # Issue 29922
 async/stream_join_test: RuntimeError
-async/stream_subscription_as_future_test: RuntimeError
 async/timer_not_available_test: RuntimeError
 convert/base64_test/01: Fail, OK # Uses bit-wise operations to detect invalid values. Some large invalid values accepted by DDC/dart2js.
 convert/chunked_conversion_utf88_test: Slow, Pass

tests/lib_2/lib_2_kernel.status

@@ -94,11 +94,9 @@ mirrors/reflected_type_generics_test/02: Pass
 
 # ===== dartk + vm status lines =====
 [ $compiler == dartk && $runtime == vm && $strong ]
-async/async_await_sync_completer_test: RuntimeError
 async/slow_consumer2_test: CompileTimeError # Issue 31402 (Invocation arguments)
 async/stream_controller_async_test: CompileTimeError # Issue 31402 (Invocation arguments)
 async/stream_join_test: CompileTimeError # Issue 31402 (Invocation arguments)
-async/stream_subscription_as_future_test: CompileTimeError # Issue 31402 (Invocation arguments)
 async/timer_not_available_test: RuntimeError
 convert/streamed_conversion_json_utf8_decode_test: Pass, Slow # Infrequent timeouts.
 html/*: SkipByDesign # dart:html not supported on VM.
@@ -219,7 +217,6 @@ async/slow_consumer2_test: RuntimeError # Issue 31402 (Invocation arguments)
 async/stream_controller_async_test: RuntimeError
 async/stream_distinct_test: RuntimeError
 async/stream_join_test: RuntimeError
-async/stream_subscription_as_future_test: RuntimeError
 isolate/issue_22778_test: Crash
 isolate/kill_self_synchronously_test: RuntimeError
 isolate/message_test: RuntimeError