Check return type of error handler passed to `Future.then`, like we already do for `Future.catchError`

[yanok]

Error handlers passed to both Future.catchError and Future.then and intentionally typed too wide, to allow passing both 1- and 2- argument handlers. This unfortunately could lead to run-time type errors when these error handlers are executed.

Analyzer already provides checks for error handlers passed to these (and some other) methods. Unfortunately while the check for catchError checks the return type, the check for then does not. I propose to extend the latter to also cover the return type. In fact, I don't see why the two couldn't be performed by the same code, as the requirements seem to be the same.

I've tried to unify the two checks in https://dart-review.googlesource.com/c/sdk/+/352362 but it comes with some issues:

• the existing check for catchError seems too strict and not fully consistent: if an error handler returns dynamic, it fits everywhere (with stict casts disabled), while if it returns Future<dynamic> the check complains about Future<dynamic> not being assignable to FutureOr<T>. Looking at the actual Future implementation we probably want to unwrap the future type before checking if the return type is assignable. This seems like something that should be fixed.
• void is not assignable to Null. This is correct, since void technically can be anything. But results in many annoying cases where Future argument type was inferred to be Null, but the error handler has declared return type void (even though it will be null at runtime). Here I'm undecided. Technically this is correct, but will require lots of cleanup. And the cleanup is not that great, adding explicit type arguments where they could perfectly be inferred. OTOH, this was already done for catchError, so maybe it won't be that bad for then...
• both finding code and message are specifically about catchError. What should I do:
  • keep the code, make the message more generic;
  • change both code and message to be more generic;
  • add a new code for then onError handler return type?

[yanok]

cc @srawlins

[srawlins]

Looking at the actual Future implementation we probably want to unwrap the future type before checking if the return type is assignable. This seems like something that should be fixed.

This sounds right to me.

But results in many annoying cases where Future argument type was inferred to be Null, but the error handler has declared return type void (even though it will be null at runtime). Here I'm undecided. Technically this is correct, but will require lots of cleanup. And the cleanup is not that great, adding explicit type arguments where they could perfectly be inferred.

Ugh, yeah this is annoying. We could punt on it and leave a TODO. Since already the story is that we don't report enough things; nothing wrong with incrementally reporting one more category, then one more...

Are many Future type arguments inferred to be Null because they are something like Future.delayed or Future.value with no value? I'm hoping we can get some better "constructors" / static methods like Future.void and Completer.completeVoid. Maybe Future.voidDelayed. https://github.com/dart-lang/sdk/issues/53364

both finding code and message are specifically about catchError. What should I do: add a new code for then onError handler return type?

Yeah I think that would be idiomatic to the rest of analyzer's errors/warnings: if it's not a combinatoric explosion of individual codes, we like to have separate codes with separate text. Unless the change in text is just "catchError" --> "then", in which case we can use interpolation and a single code.

[lrhn]

It depends on whether we want type checks of the error handler, or the value it returns. Which the analyzer obviously can't.

The most correct check is to check that the onError callback for a .then<R>(...) call has type FutureOr<R> Function(Object) or FutureOr<R> Function(Object, StackTrace) The context type of Function makes it very hard to infer the necessary return type, even if you want to, so we don't actually do that at runtime.

So we check that the onError handler at least has type Object? Function(Object) or Object? Function(Object, StackTrace), so we can call it, and delay checks on the result.

Here we can again chose to check for FutureOr<R> or we can check for R or Future<Object?>, and wait for the Future and then check if its value is an R. (If not, the future will complete with a type error.) I don't remember what we do here, probably check FutureOr<R> because we just try to complete the then call's return future with the returned value, which does that check. We could change it, but it's not like we want to allow any Future to be returned.

I wish that we could change the type of the onError to FutureOr<R> Function(Object, StackTrace). Same for catchError. That's probably going to be quite breaking, so it's a hard sell.

Until then, I'd be happy if the analyzer complained about any return type which isn't FutureOr<R>, it's just sometimes hard to satisfy. (Still better than failing to satisfy it and return a non-R, like null when R is non-nullable.)

[yanok]

Ugh, yeah this is annoying. We could punt on it and leave a TODO. Since already the story is that we don't report enough things; nothing wrong with incrementally reporting one more category, then one more...

Well, ideally I'd like to use the same code for catchError and then (maybe others?) and catchError already enforces this, so things like

void handle(dynamic err) {}

future.then((_) {} /* this makes R=Null */).catchError(handler);  // 'void' isn't assignable to 'FutureOr<Null>'

already cause complains. So, sure, we can fix one thing at a time, but unfortunately that prevents us from merging the code that checks catchError and then.

Are many Future type arguments inferred to be Null because they are something like Future.delayed or Future.value with no value? I'm hoping we can get some better "constructors" / static methods like Future.void and Completer.completeVoid. Maybe Future.voidDelayed. #53364

I don't have any representative statistics, but in my experience it was mostly about inline void callbacks, that result in overall inferred type Future<Null>. I think Future.value() is actually Future<dynamic> BTW.

Yeah I think that would be idiomatic to the rest of analyzer's errors/warnings: if it's not a combinatoric explosion of individual codes, we like to have separate codes with separate text. Unless the change in text is just "catchError" --> "then", in which case we can use interpolation and a single code.

Got it, thanks! Yep, probably interpolation would be a good fit here.

[yanok]

@lrhn Yep, we want to check the static type of the handler, and as part of it we want to check it returns FutureOr<R>. I just thought I found two corner cases:

1. error handler returning Future<dynamic>. Now this complains if expected type is not Future<dynamic>. And I thought this could be relaxed, because the implementation awaits the handler result before trying to cast it to the expected type. But I was wrong, the implementation actually immediately tries to return the result, which implies casting it to FutureOr<R>, so complaining about a handler returning Future<dynamic> is correct and desired. So this is not a corner case after all and the existing behavior is good.
2. Futures that are logically meant to be Future<void> often get inferred type Future<Null>, so we start barking on void returning handlers, because technically void is not assignable to FutureOr<Null>. This is correct but somewhat annoying behavior.

I wish that we could change the type of the onError to FutureOr<R> Function(Object, StackTrace). Same for catchError. That's probably going to be quite breaking, so it's a hard sell.

I'd love to see that happen, but yeah, it's going to break all 1-argument handlers.