Casting `Future<Foo>` to `Future<Foo?>` leads to hard-to-debug errors too easily

[yjbanov]

To reproduce dart run the following code:

import 'dart:async';

void main() {
  final Completer<int> completer = Completer<int>();
  final Future<int?> future = completer.future; // cast Future<int> to Future<int?>
  future.catchError((_) {
    return null;  // this only looks correct because the future is allowed to return null
  });
  completer.completeError('Oops!');
}

Expected result

The stack trace should point to the code containing the programming error, i.e. (future.catchError). Consider the following non-async code containing a similar error:

void main() {
  final List<int> list = <int>[1, 2, 3];
  final List<int?> nullList = list;
  nullList.add(null);
}

The result is:

Unhandled exception:
type 'Null' is not a subtype of type 'int' of 'value'
#0      List.add (dart:core-patch/growable_array.dart)
#1      main (file:///usr/local/google/home/yjbanov/code/tmp/null_future.dart:4:12)
#2      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192:12)

The stack trace points to the line 4 in the file null_future.dart that I wrote, which is helpful.

Actual result

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:194: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:610: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:122:13)
#8      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:193:5)

This stack trace is not helpful at all. The best method I could find is pause on exceptions in the debugger, wait until it pauses on this line, then eval $T. This gives you the type that it expected. Then you grep your source code for futures and completers that use this type (hopefully there aren't too many places) and try to spot the bug.

As a concrete example, here's the bug in the Flutter Web engine tool, which was a conspiracy of three separate lines of code:

• Completer<BrowserEngine> instantiated.
• The completer's .future is implicitly cast to Future<BrowserEngine?>.
• A catchError attempts to return null.

No matter what Dart program you write you see the exact same stack trace, making it very hard to debug.

[lrhn]

We're not testing the arguments to catchError as eagerly as we could because the argument is typed as Function.
That means that we don't get type inference on the type argument. which again means that people risk getting worse types than they expect. Because of that, we are a somewhat more lenient about what we accept than we would otherwise be, like allowing a function returning dynamic to be used, and only erring if it actually returns something wrong.

That also prevents us from giving the error inside catchError.

Adding earlier and stricter checks is definitely possible, but also very likely to break existing code.

[yjbanov]

Can the closure "remember" the location in code where it was defined, so when there's need to complain about it the error message could point to it. Example:

Unhandled exception:
Invalid argument(s) (onError): The error handler of Future.catchError must return a value of the future's type. The catchError function is defined in main (file:///usr/local/google/home/yjbanov/code/tmp/null_future.dart:4:12).

[lrhn]

The code definitely already knows that the function came from either then(..,. onError: HERE) or `catchError(HERE), and we already use that: https://github.com/dart-lang/sdk/blob/main/sdk/lib/async/future_impl.dart#L183

Remembering where it came from is a much tougher request. We could capture the stack trace for every then and catchError call, but that's exceedingly expensive for something that is unlikely to ever be used (it's only for errors, so production code will never use it).

Maybe we could capture the stack trace only in debug mode (capture and assign it inside an assert). We'd still increase the overhead of the class by one extra field, even when it isn't used. I'd prefer to avoid that for something used as often as _FutureListener.

Or we could wrap the error handler function where it's provided, maybe only if it doesn't have the correct type, and let the wrapper do the type checking. Then it failing would throw in code that was, well, just inside then or catchError, which we already knew, but we wouldn't have any better stacktrace anyway.

[yjbanov]

IIRC the DevTools team implemented a kernel transformer that remembers where widgets are instantiated. This way there's no overhead at runtime as the location is hard-coded in the compiled code. Can the same approach be used here? Also, while the full stack could be even more helpful, just the source location alone would already help a lot.

[lrhn]

The source location is in user code, so we can't capture that in Dart code.
It amounts to, effectively, capturing one stack frame instead of the entire stack, but that's still not something we can currently do. (JavaScript disallowed .caller in strict code, which would be exactly the functionality we'd want.)
We'd still risk a measurable overhead on every error handler. If we could restrict that do debugging mode, then it might be viable, but still not something we can easily support in JavaScript.

Honestly, I'd much rather breakingly change the API to only accept FutureOr<R> Function(Object, StackTrace), then force everybody to migrate their code. That's the correct long-term fix for this problem. Also a big migration.

(Would it be meaningful to have a lint checking that the on-error handler has the required type? It'll probably end up being impractical to satisfy the return type for a function literal without a helpful context type.)

[yjbanov]

I was thinking of a compile-time solution rather than a runtime one. Instead of checking the stack at runtime, the compiler could attach the line information to the closure/tear-off. Then at runtime the line information can be read back and printed as part of the exception (perhaps dart:developer could have a function SourceLocation? getInitializationSourceLocation(object)).

I'm not sure how a lint can help. The static types are correct. The problem is with the Future<Foo> => Future<Foo?> cast, which is allowed by the language.

[lrhn]

Yes, casting Future<Foo> to Future<Foo?>, or List<int> to List<int?> is valid and often useful. Dart generics are unsoundly covariant, and Future is mostly covariant, but there are exceptions like catchError.

Until we get variance annotations, up-casts of classes that are not completely covariant are always going to be risky.
It's not different than List<num> l = <int>[1]; l.add(2.5); giving you a run-time error, but no static warning.

If it was only the up-cast that was a problem, but it's really every use of catchError with a non-void/top future value type.
You can write Future<int>.error("gotcha").catchError((x) => logger..log("badness")..log(x)); and you get a run-time error when the returned logger is not an int. No static warning from the language (but we could potentially special case that mistake).

Accepting Function for error handlers is inherently unsafe, but it's the only way to accept both unary and binary error handlers, which we have inherited from Dart 1 where it was not a problem.
Adding stricter, earlier checks on the return type of the function is breaking because code relies on the typing being permissive because it gets no inference help to set the return type from the Function parameter type.

So, checking the actual value is currently the best we can do, and we do that. At least we check the error handler callback parameter types eagerly. I don't think there is anything more we can do here, short of a breaking change.
(And then I'd prefer a very breaking change of changing the parameter type to FutureOr<T> Function(Object, StackTrace) instead of Function). Until then, use the onError extension method.

[yjbanov]

What do you think of a compiler-based solution, where one can extract the source location of the closure/tear-off instantiation using something like dart:developer and include it in the error message? Something like this can be useful in other contexts, e.g. a class can be annotations with a pragma instructing the compiler to record locations of all constructor calls, enabling debugging scenarios when the culprit is detached from the location that throws the error.

[yjbanov]

This is biting us again: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8817129316426320929/+/u/test:_Run_tests_on_iOS_Safari/stdout

[lrhn]

The solution to unsound covariance is variance annotations, which we don't have yet, and likely won't be able to attach to existing technically-invariant SDK types anyway.
I have given up on doing something with the actual function arguments, to give earlier errors, because it causes too many new errors (because of the lack of useful type inference on the Function typed argument).

Attaching location information to errors related to function values is doable. Heck, I'd let the VM put the location information directly into the function's toString.
I don't want to make the general cross-platform code depend on dart:developer features that only work on the VM,.

[eernstg]

The statically checked declaration-site feature (cf. dart-lang/language#524) is not fully implemented, but much of it is there already.

How about offering a tiny preview of this feature: We could have an annotation @typeParametersAreInvariant (long name to avoid name clashes), and we could add that to Future. We could have a lint that flags every situation where an upcast has source Future<T> for some T and destination Future<S> for some S which is a proper supertype of T, and similarly for any other class with metadata @typeParametersAreInvariant. This lint would kick in at the location where ..

The completer's .future is implicitly cast to Future<BrowserEngine?>.

This is not the complete declaration-site variance feature, not even for the special case where every type parameter of a specific class would have the variance modifier inout, but it's a self-contained subset of the feature which is just about flagging the locations where it is statically known that a non-trivial covariance relation is introduced by an upcast.

That lint could be enabled during debugging of the kind of problem described here.

The lint might even be enabled permanently in certain libraries where futures are used in an invariant manner. But it might also be used in a debugging-only manner. That's definitely not going to break anything, so we don't have to worry about questions like "would it break the world to make Future invariant?!".

It is not sound, of course, so we still need the actual feature in order to get the real thing:

Y f<X extends Y, Y>(X x) => x;

void main() {
  Future<int?> fut = f(Future<int>.value(42)); // No lint.
}

With real declaration-site variance, and assuming that Future is made invariant, Future<int> <: Future<int?> does not hold (those two types are unrelated), so the example would fail to compile. The very simple approximation that I proposed above as a lint fails to detect this case, but the cases that it does lint is a subset of the cases where real declaration-site variance would emit a compile-time error.