Analyzer should detect if improperly typed handler passed to Future.catchError.

[stereotype441]

Due to the fact that Future.catchError supports two kinds of callbacks, its callback parameter is typed Function, which means it gets very limited type checking from the analyzer.

Since this is a commonly used core method, it would be nice if the analyzer had some special case logic to type check it. I'm not sure whether it should be a hint or a lint (@bwilkerson, @pq, WDYT?)

Related issues: #35812, #34144, #31939, #35545.

[pq]

My inclination is that this should be a hint and agree it would be high value. 👍

[zanderso]

Is there any update on this? Thanks!

[pq]

Thanks for the nudge @zanderso.

@bwilkerson: any objection to treating this as a hint?

[srawlins]

With null safety, the priority on this is raised. See #44386. I'd like to implement it as a Hint. I think there would be zero false positives.

• The function must accept either one Object param, or one Object and one StackTrace param.
• The function must return Future<T>.

[lrhn]

If this can be extended to other Function-typed error handlers, then that would be nice. Not nearly as critical, though.

Most of the remaining error handlers are void-returning functions, so you'd only be checking that they accept (Object) or (Object, StackTrace), but that's still relevant for some situations. For example stream.handleError((FooException e) { ... }) where you know that the exception will be a FooException, but the function must still accept Object.
Or even stream.handleError((FooException e) => ..., test:(o) => o is FooException).

[bwilkerson]

Sorry, I missed seeing this earlier. I agree that this should be a hint.

[srawlins]

If this can be extended to other Function-typed error handlers, then that would be nice. Not nearly as critical, though.

+1

[srawlins]

Reporting on invalid returns is not as cut and dry as I had thought 😛 . At runtime, the VM and dart2js do not use the static type of any return values; only runtime type. For example:

void main() async {
  var x = await Future<int>.error(7).catchError((_) {
    num y = 1;
    return y;
  });
  print(x);
}

In a null-safe program, with --sound-null-safety, dart prints 1. So the analyzer could report on return y (because num is not assignable to FutureOr<int>), but it would then be reporting on valid code.

Reporting return y might be preferred because it requires the return statement to behave as if it were any other function with a return type of FutureOr<int> (requiring the user to cast with as int or something). Or reporting return y might not be preferable, because the program is still valid.

I'm not sure how I would allow return y and still catch return "". Under null safety, where implicit casts are not allowed, I think we'd always have to report return y.

[lrhn]

Correct. We don't check that the function argument is a FutureOr<T> Function(Object) or FutureOr<T> Function(Object, StackTrace), just that the parameters match. Then we check that the actual result value is a FutureOr<T>.
We do that because there is no inference to ensure the return type of a function literal, and it's far too easy to get something else.

If you check the actual return statement types, I think it would be fine to warn in the example above.
It seems like it should be non-breaking change to change the declaration to num y = 1.5;, but it is actually breaking.

[srawlins]

WDYT, @bwilkerson , should this still be a Hint, or a Lint? I think we can perfectly detect when the type of the expression in a return statement is not kosher, but we cannot detect perfectly what will be a runtime error. But this is not unlike some other static analyses. For example missing_return was always this safety guard that didn't perfectly align with runtime behavior (it was always legal for a function to end without a return statement, in which case null was returned. And it is not always known whether casts will fail at runtime, which is why we've added guards like implicit-casts: false.

[bwilkerson]

My understanding is that false positives will occur only in cases where the future on which catchError is being invoked returns a type T and there is a return of an expression whose static type is S where S is not assignable to T but where there is a subtype of both that is actually returned at runtime.

My intuition is that such cases, and hence false positives, will be rare, but we might want to gather some data to verify that.

[srawlins]

#44480 is a semi-blocking issue. I see code in Flutter engine where a Future<void> has a catchError callback with return;, which is currently illegal for FutureOr<void>. ☹️

I could perhaps make an initial landing of some checks which do not check return statements (though I think that is by far more useful than checking arrow function literals or function-typed expressions), or some checks which do not fire for Future<void> futures...

[srawlins]

I think the primary function-typed APIs from dart:async on which we could also report diagnostics include:

• Future.then(onError)
• Future.catchError(...)
• Stream.handleError(onError)
• Stream.listen(onError)
• StreamSubscription.onError(handleError)