Future.catchError unsafe if you don't know the runtimeType of the Future it is called on

[christopherfujino]

Update April 3, 2024

The Future.onError extension method now solves this problem--that is, this code works without throwing an Error:

Future<void> main() async {
  await func().onError(handler);
}

Null handler(_, __) => null;

Future<Object?> func() => Future<bool>.error('oops');

---

Consider the following code:

Future<void> main() async {
  await func().catchError(handler);
}

Null handler(_, __) => null;

Future<Object?> func() => Future<bool>.error('oops');

This code appears to be statically correct. Because the return type of handler satisfies the return type of func(), it looks like this code should work. However, at runtime, the .catchError method will actually be called on a Future<bool>, and now our handler function is no longer a valid callback to Future<bool>.catchError(), and we get an ArgumentError:

Unhandled exception:
Invalid argument(s) (onError): The error handler of Future.catchError must return a value of the future's type
#0      _FutureListener.handleError (dart:async/future_impl.dart:181:7)
#1      Future._propagateToListeners.handleError (dart:async/future_impl.dart:779:47)
#2      Future._propagateToListeners (dart:async/future_impl.dart:800:13)
#3      Future._completeError (dart:async/future_impl.dart:575:5)
#4      Future._asyncCompleteError.<anonymous closure> (dart:async/future_impl.dart:666:7)
#5      _microtaskLoop (dart:async/schedule_microtask.dart:40:21)
#6      _startMicrotaskLoop (dart:async/schedule_microtask.dart:49:5)
#7      _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:123:13)
#8      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:193:5)

Even though the onError extension method features a strictly-typed callback, it still leads to a runtime-only error:

Future<void> main() async {
  await func().onError(handler);
}

Null handler(_, __) => null;

Future<Object?> func() => Future<bool>.error('oops');

Leads to the same stacktrace.

Interestingly, passing our handler to the onError parameter of .then() does not have the runtime error:

Future<void> main() async {
  await func().then(
    (Object? obj) => obj,
    onError: handler,
  );
}

Null handler(_, __) => null;

Future<Object?> func() => Future<bool>.error('oops');

This has lead to a series of very difficult to track down crashes in the Flutter CLI tool (especially since the stacktrace is opaque), such as flutter/flutter#114031

In this particular case, we were doing essentially:

import 'dart:io' as io;

Future<void> main() async {
  final process = await io.Process.start('interactive-script.sh', <String>[]);
  // stdin is an IOSink
  process.stdin
      // IOSink.addStream's static type is Future<dynamic> Function(Stream<List<int>>)
      // however, the actual runtimeType of what is returned is `Future<Socket>`
      .addStream(io.stdin)
      .catchError((dynamic err, __) => print(err));
}

[christopherfujino]

Related issue: #50430

[lrhn]

Correct.

First of all, the parameter of catchError has type Function, so it's possible to pass in arbitrarily wrong functions.

Even if it had a useful type like FutureOr<T> Function(Object, StackTrace), that argument type has a T in covariant position, so it's still possible to get a run-time error for an argument that looks statically safe - because of unsafe covariant generics.

A solution would be to introduce super-bounded type parameters, and make catchError be

  Future<R> catchError<R super T>(FutureOr<R> Function(Object, StackTrace) handleError, ...)

We're not close to getting that, so not a solution.

[natebosch]

We have the extension method Future.onError which has a better static type than Future.catchError. We could make .onError resilient to this by changing the definition to include a call to .then<T>((value) => value).catchError(...).

Ideally this shouldn't impact much code because it would use try{await} catch, but that doesn't work for unawaited cases like the stdin example.

Is the bug that we should be more careful that the runtime type for a Future matches the static type at API boundaries?

[christopherfujino]

We have the extension method Future.onError which has a better static type than Future.catchError. We could make .onError resilient to this by changing the definition to include a call to .then<T>((value) => value).catchError(...).

Ooh, I think this would fix our use case. With this change, I think .onError would be a statically safe replacement for Future.catchError.

[lrhn]

The onError is an extension method, which means it's based on the static type, which is precisely what we need here.

Rewriting it to be even more type safe should be possible, preferably more efficiently than introducing an intermediate future.
(We effectively have a Future<E>.onError<R super E>(FutureOr<R> Function...) signature, because R is the static element type of a Future<E>, which by covariance is a supertype of E. The R type is introduced by upcasting the receiver instead of bounding a parameter, but it works and we still get access to R at runtime. We should just ensure that the internal operations used by onError can use that knowledge efficiently and effectively, and not just call catchError, which should be possible since it's in the same library as the _Future implementation.)

[natebosch]

which should be possible since it's in the same library as the _Future implementation

We will need a backup implementation for non _Future.

[lrhn]

True, the backup could do the then-based upcast if necessary. Or just use then directly instead of catchError, if it doesn't already. You only need the one call:

extension _<T> on Future<T> {
  Future<T> onError<E extends Object>(FutureOr<T> Function(E, StackTrace) onError, [bool Function(E)? test]) =>
    this.then<T>((T value) => value, onError: (Object error, StackTrace stack) {
      if (error is! E || test != null && !test(error)) throw error; // Rethrow.
      return onError(error, stack);
    });
}

I should just change it to that. The biggest inefficiency here is allocating the (T value) => value closure.

[christopherfujino]

True, the backup could do the then-based upcast if necessary. Or just use then directly instead of catchError, if it doesn't already. You only need the one call:

extension _<T> on Future<T> {
  Future<T> onError<E extends Object>(FutureOr<T> Function(E, StackTrace) onError, [bool Function(E)? test]) =>
    this.then<T>((T value) => value, onError: (Object error, StackTrace stack) {
      if (error is! E || test != null && !test(error)) throw error; // Rethrow.
      return onError(error, stack);
    });
}

I should just change it to that. The biggest inefficiency here is allocating the (T value) => value closure.

Would be great if you made this change. I basically implemented that code inline in all the places we were using .catchError() in the tool.

[lrhn]

(The Future.onError extension methods was updated, and should now be as safe as possible, also for non-_Futures.)

[christopherfujino]

(The Future.onError extension methods was updated, and should now be as safe as possible, also for non-_Futures.)

Thanks!