invalid_return_type_for_catch_error does not detect implicit null returns

[AlexV525]

This tracker is for issues related to Analyzer.

Summary

Some exceptions were thrown due to unknown reasons, we managed to catch them but the stack trace was unable to locate the root cause of the issue.

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)
#1      Future._propagateToListeners.handleError (dart:async/future_impl.dart:778)
#2      Future._propagateToListeners (dart:async/future_impl.dart:799)
#3      Future._completeError (dart:async/future_impl.dart:574)
#4      _SyncCompleter._completeError (dart:async/future_impl.dart:51)
#5      _Completer.completeError (dart:async/future_impl.dart:23)
#6      Future.any.onError (dart:async/future.dart:616)
#7      _rootRunBinary (dart:async/zone.dart:1450)
#8      _CustomZone.runBinary (dart:async/zone.dart:1342)
#9      _FutureListener.handleError (dart:async/future_impl.dart:162)
#10     Future._propagateToListeners.handleError (dart:async/future_impl.dart:778)
#11     Future._propagateToListeners (dart:async/future_impl.dart:799)
#12     Future._completeError (dart:async/future_impl.dart:574)
#13     Future._chainForeignFuture.<anonymous closure> (dart:async/future_impl.dart:519)
#14     _rootRun (dart:async/zone.dart:1426)
#15     _CustomZone.run (dart:async/zone.dart:1328)
#16     _CustomZone.runGuarded (dart:async/zone.dart:1236)
#17     _CustomZone.bindCallbackGuarded.<anonymous closure> (dart:async/zone.dart:1276)
#18     _microtaskLoop (dart:async/schedule_microtask.dart:40)
#19     _startMicrotaskLoop (dart:async/schedule_microtask.dart:49)

Expect behavior

Reports the precise cause of the error.

[mraleph]

Somewhere in your program you are using catchError which produces a value of wrong type. Unfortunately Future.catchError is very loosely typed so the error is not caught by compiler. Analyzer should emit a diagnostic for it though:

// -- /tmp/x.dart ----
void main() async {
  await Future<int>.error('error').catchError((e) => '');
}

$ dart /tmp/x.dart
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:778:47)
#2      Future._propagateToListeners (dart:async/future_impl.dart:799:13)
#3      Future._completeError (dart:async/future_impl.dart:574:5)
#4      Future._asyncCompleteError.<anonymous closure> (dart:async/future_impl.dart:665: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)
$ dart analyze /tmp/x.dart
Analyzing x.dart...                    0.5s

   info • x.dart:2:54 • A value of type 'String' can't be returned by the 'onError' handler because it must be assignable to 'FutureOr<int>'. •
          invalid_return_type_for_catch_error

1 issue found.

/cc @lrhn

[lrhn]

I agree with @mraleph.

We are deliberately a little permissive around catchError's onError argument because the lack of context type means that it won't necessarily infer the correct type for a function literal. So, we check that the actual value returned by it has the required type, which we can't do until it has actually been called. Because of that, we have no useful stack trace to show.

That makes it very hard to improve the experience.

[AlexV525]

Somewhere in your program you are using catchError which produces a value of wrong type.

Yes. The only place I use catchError is:

xxxRequest<void>(something).catchError(
  (dynamic e, StackTrace? s) {
    someHandlerHere();
  },
);

It did not return a wrong type (as I thought) but the arguments might be wrong here. We didn't see anything in the analyzer, so it became a runtime exception eventually. And yes, I removed once it occurred because I realized it's the exception from catchError, but futures are used frequently and we can't make promises that everyone can handle it before exceptions are delivered to productions.

[lrhn]

If the future's type is indeed Future<void>, you cannot return a wrong value.
The function accepts Object and StackTrace as arguments, so that should be valid too.

Are you sure about the actual runtime-type of the future returned by xxxRequest<void>(something)?
If it happens to be Future<Object> instead, you get an error trying to return null.

[AlexV525]

Are you sure about the actual runtime-type of the future returned by xxxRequest<void>(something)? If it happens to be Future<Object> instead, you get an error trying to return null.

So I rewrote the method from the left to the right then no exception occurred again, and I can confirm the issue was resolved.

P.S. The rewrote is done before I wrote the issue.

[mraleph]

_dio.post<void>() returns Future<Response<void>> rather than Future<void> which explains the issue.

I am still surprised that analyzer does not highlight this issue.

[vsmenon]

Do we want to file an analyzer issue here? ( @bwilkerson @srawlins )

[mraleph]

Ah-ha. I see, the analyzer seems to only validate return statements.

void main() async {
  await Future<int>.error('error').catchError((e) { return null; });
  await Future<int>.error('error').catchError((e) { });
}

dart analyze /tmp/x.dart
Analyzing x.dart...                    0.5s

   info • x.dart:2:60 • A value of type 'Null' can't be returned by the 'onError' handler because it must be assignable to 'FutureOr<int>'. •
          invalid_return_type_for_catch_error

1 issue found.

Though actually both of these should produce an error in null-safety mode.

It's definitely an analyzer issue.

[mraleph]

Updated the title and area accordingly.

[AlexV525]

Well, my original proposal is a better stack trace, which seems unable to resolve currently?

[vsmenon]

@AlexV525 - this won't show on a normal sync stack trace as the line with the error is no longer on the stack when the runtime error is triggered.

@mraleph @lrhn - is this something we could / should show in an async trace?

[srawlins]

I've implemented the analyzer bit in https://dart-review.googlesource.com/c/sdk/+/247863, but it will take some time to land as I have to clean up Flutter first.

[oprypin]

@srawlins Could you please also update documentation?

• There is no mention of this hint at https://dart.dev/tools/diagnostic-messages#body_might_complete_normally_catch_error

• The main example of catchError doesn't demonstrate a usage when the Future has a value, so it looks like it's OK to not return any value
  

  sdk/sdk/lib/async/future.dart

  Lines 810 to 812 in c1afa7f

  	/// }).catchError((err) {
  	/// print('Error: $err'); // Prints 401.
  	/// }, test: (error) {

[srawlins]

@bwilkerson would you like to take a stab at writing docs for body_might_complete_normally_catch_error, or should I?

The main example of catchError doesn't demonstrate a usage when the Future has a value, so it looks like it's OK to not return any value

I've filed #50430. Please comment if I misunderstood your request here.

[bwilkerson]

... would you like to take a stab at writing docs for body_might_complete_normally_catch_error, or should I?

The docs are already in the pipeline, waiting for a review from the documentation team.

[flenairMacif]

Hi, no news since November ?

[bwilkerson]

The documentation landed, so I'm not sure whether there's any work that still needs to be done.

[srawlins]

I think this is good to go then. Thanks @bwilkerson !