Verify type of onError callbacks eagerly

[alexmarkov]

Currently, certain core library methods such as Future.then take Function onError callback.
They do not actually verify the type of the onError callback function until it is called.
As this is often used for error handling and errors are rare, it could be easily unnoticed that onError callback has a wrong type, which may cause flaky, hard to reproduce and hard to diagnose bugs.

Consider the following example:

import "dart:async";

Future<int> foo() async => throw 'hi';

main() {
  foo().then((x) => throw 'bye', onError: (e, s, t) {
    print('error');
  });
}

In this example onError callback has 3 arguments which is not noticed until the actual error happens. When it happens, it fails in the following way:

Unhandled exception:
type '(dynamic, dynamic, dynamic) => Null' is not a subtype of type '(Object) => dynamic'
#0      _FutureListener.handleError (dart:async/future_impl.dart:159:46)
#1      Future._propagateToListeners.handleError (dart:async/future_impl.dart:694:47)
#2      Future._propagateToListeners (dart:async/future_impl.dart:715:24)
#3      Future._completeError (dart:async/future_impl.dart:534:5)
#4      Future._asyncCompleteError.<anonymous closure> (dart:async/future_impl.dart:582:7)
#5      _microtaskLoop (dart:async/schedule_microtask.dart:43:21)
#6      _startMicrotaskLoop (dart:async/schedule_microtask.dart:52:5)
#7      _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:118:13)
#8      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:169:5)

This stack trace has zero information about the origin of the error. This stack trace doesn't even mention any user code.

I think we can improve usability of those onError callbacks by verifying their type eagerly when they are passed to the core library (e.g. on entry into Future.then).

This problem becomes even more severe with NNBD.
Previously, the following code was correct:

import "dart:async";

Future<int> foo() async => throw 'hi';

main() {
  foo().then((x) => throw 'bye', onError: (e, s) {
    print('error');
  });
}

It compiles and runs fine in weak mode.
However, in strong mode (with --null-safety) this example fails with

Unhandled exception:
type 'Null' is not a subtype of type 'Future<Never>'
#0      _FutureListener.handleError (dart:async/future_impl.dart:157:7)
#1      Future._propagateToListeners.handleError (dart:async/future_impl.dart:698:47)
#2      Future._propagateToListeners (dart:async/future_impl.dart:719:24)
#3      Future._completeError (dart:async/future_impl.dart:534:5)
#4      Future._asyncCompleteError.<anonymous closure> (dart:async/future_impl.dart:583:7)
#5      _microtaskLoop (dart:async/schedule_microtask.dart:41:21)
#6      _startMicrotaskLoop (dart:async/schedule_microtask.dart:50:5)
#7      _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:117:13)
#8      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:5)

Again, this stack trace doesn't mention the source of the problem and it is hard to track it down to a particular place in the code where the problem occurs (the fix would be to change .then to .then<void> so that future could hold return values of both onValue and onError).

/cc @lrhn @leafpetersen @munificent @liamappelbe

[leafpetersen]

@lrhn Looking at the implementation of Future.then it looks to me like the type of onError is validated eagerly if we are not in the root zone, and lazily if we are in the root zone. Is this intentional? Should we consider validating eagerly always (or at least in an assert if performance is a concern)?

[lrhn]

Not intentional.
It's an accident of implementation that we rely on the checks needed to pick the right register*Callback to also do the type checking of the function. The root zone does not need to call register callback (we know that the root implementations of register*Callback are identity functions).

So, we should probably do the type testing always, and then only call the register function when not in the root zone.

[lrhn]

The Future.then and Future.catchError functions now do err early on arguments which are neither Function(Object) not Function(Object, StackTrace). They still allow any return type and check that later, because the Function parameter type means that a function argument gets no type inference help to ensure the return type.

I'll add an early check for Stream.handleError too.

[alexmarkov]

flutter/flutter#81492 looks like caused by insufficient eager checks of return type of an error callback.

[lrhn]

I've tried adding eager checking of the return type of the onError/catchError Function values.
Predictably, some code started failing (including the SDK not building until I added extra types in some places).

Problems include:

• Functions that return dynamic because they just always have (we have a mock Function expectAsync(Function, {int count}) function in some tests which creates a new dynamic Function(dynamic) from an int Function(int) argument).
• Functions that return dynamic because of the lack of type inference. The actual value would be correct.
• Functions that return num instead of int because of lack of inference making int + x have type num. The actual value will be an int.
• Inferred return types for throwing functions that never mattered before.

It's likely to be significantly breaking to make a strong up-front requirement on the return type of a function in a position which gets no type inference, and which has so far been more permissive.

I'd propose to change the API to require a FutureOr<R> Function(Object, StackTrace) function and not allow unary functions. That's heavily breaking, but at least gets us to a better API in the long run, rather than having a bad API that strictly enforces an invisible constraint at runtime. That's just a pain.

(We already have the onError extension method on futures, as a replacement for catchError. Just recommend using that instead of catchError would likely go a long way. We could do similar things for then with it's onError and similar Stream functions. Then we can maybe deprecate the old API eventually.)