Future.catchError is error prone

[matanlurey]

(Forked from an internal thread; @munificent and others have additional context)

I noticed in code reviews that developers often do: return someFuture.catchError((e) => log(...));

But this pattern actually swallows the error, and instead propagates a null to the caller, which may cause an NPE or worse down the future chain. This pattern is only good if catchError returned future is a terminal (="orphaned") future (which is often an error-prone pattern too).

@munificent writes, in response:

In most cases, the easy fix is to avoid using catchError() at all and use async/await instead. That way, you can use a regular catch clause where people (hopefully!) already know how to log and rethrow exceptions correctly.

Compare with Java Futures, which doesn't have his errorprone pattern, because its catching function takes a Function<?, V>, not Function, and in Java, and the void output of print would not match with V.

We can't do that in Dart because catchError() accepts either of two kinds of functions: a single-parameter function that just accepts the error, or a two-parameter one that accepts the error and stack trace. We don't have overloading in Dart, so we have to loosen catchError()'s parameter type to Function to accept both kinds.

(Java, of course, doesn't have this problem because it puts the stack trace directly on the exception object instead of passing it around separately.)

---

You can see a minimal example of this problem in action in the following code sample:

void main() {
  invokeLazy(() => aVoidMethod());
}

// We need this to actually return a String, but we can't specify the type due to lack of overloads.
// See https://api.dartlang.org/stable/2.0.0/dart-async/Future/catchError.html.
void invokeLazy(Function aString) {
  print(aString);
}

void aVoidMethod() {}

API reference for <Future>.catchError: https://api.dartlang.org/stable/2.0.0/dart-async/Future/catchError.html

EDIT: I believe there is another bug, somewhere, about the hidden errors you get if you write:

// (ArgumentError) => void is not a (dynamic) => dynamic.
future.catchError((ArgumentError e) => ...);

[matanlurey]

And sorry, some questions here:

• Should we change the definition of catchError and/or split it into two methods in Dart 3?
• Should we invest in a lint/hint that would catch obvious issues, whether:
  • avoid_catch_error: Suggests using async and await (with try/catch) instead
  • avoid_catch_error_with_void: Catches this particular case

/cc @MichaelRFairhurst

[MichaelRFairhurst]

One interesting way we could roll this out "incrementally" would be to add a flag that:

• adds new members "catchErrorOnly" and "catchErrorAndTrace"
• deprecates old members

This way code with the flag could use code without the flag*, and its not a breaking change (for those that implement the Future API) without the flag.

This could be a new annotation @behindFlag("safer-catch-error"), @deprecatedBehindFlag("safer-catch-error").

We could then use it internally, though unless we can make code without this flag use code with it, any published internal code or code used by published internal code (which, I suppose, transitively, is published) would not be able to use it.

*it would be an error for code with the flag to import a library that implements Future without the flag, though

[matanlurey]

@MichaelRFairhurst:

One interesting way we could roll this out "incrementally" would be to add a flag...

Funny enough I had a similar request here: https://github.com/dart-lang/sdk/issues/30811:

abstract class List<T> implements ... {
  @notImplemented
  T whoop();
}

...

class BetterList<T> implements List<T> {
  // ... every method implemented but whoop() ...
}

I think the main objection was adding @notImplemented, even informally, is that it would require changes to the CFE, backends, analyzer, etc, and was non trivial enough where you might as well implement default methods.

[MichaelRFairhurst]

That makes sense.

+1 to default methods!

[lrhn]

Re.: Rolling out changes incrementally. It would be really nice if we had a way to provide two different platform APIs for the same class, with a manual opt-in to use the "new" API instead of the old API. Then code could be migrated from old to new over time, and then we could remove the old API after a long deprecation period.

(That, or add overloading).

There is no way to change the current behavior in a non-breaking way without such a feature (or interface default methods, or extension methods, or any other way to add methods to an interface without breaking existing implementations), so unless we get that first, this cannot be changed until Dart 3, and even then it's unlikely that we can change the interface of Future in a breaking way.

[matanlurey]

Ping. I think this needs to be addressed in some reasonable way, and we can't wait years to do it.

[lrhn]

I'm all for fixing this, but I don't actually see a possible way to do so right now.

We cannot change the Future API. That is a breaking change, and we cannot do breaking changes. Too many classes implement Future, and too much code uses it, so we can neither make the API more general nor more specific. In Dart 2, any change to the API of a public class is a breaking change. Maybe in Dart 3.0, but I'm not even sure we can break something as fundamental as Future there, if anything at all.

The only solution I see is to introduce language features which make some currently breaking API changes into non-breaking changes, e.g., interface default members for adding new members, but definitely not restricted to that. Maybe proper type-based overloading would allow us to have both full signatures, and potentially still allow existing subclasses to implement both signatures with their current single implementation. In any case, that'll be a larger feature and will take time to design and implement.

I do want to make such language features a priority, precisely because Future isn't the only class which is stuck with a backwards compatible type which was designed for Dart 1, not Dart 2. I hope that won't take years. Until such time, there is nothing we can do to this issue. Marking the issue as P1 won't change that.

(As a side not, if we had had sealed as a language feature from the beginning, we would likely have sealed a lot of SDK classes including at least Future, Stream, Zone, Symbol, Type, DateTime, Interval, and RegExp. Then we would be able to change the Future API to be more permissive because all the classes implementing Future wouldn't exist.).

[matanlurey]

I'm all for fixing this, but I don't actually see a possible way to do so right now.

I can think of at least one option, though it hurts terseness a bit temporarily:

• Add top-level/static methods that delegate to <Future>.catchError:
  • Future.catch(Future, void Function(Object))
  • Future.catchTrace(Future, void Function(Object, StackTrace)).
• Move all uses of future.catchError((e) { ... } to the new catch method:

  - future.catchError((e) { ... })
  + Future.catch(future, (e) { ... })

• Move all uses of future.catchError((e, s) { ... } to the new catchTrace method:

  - future.catchError((e, s) { ... })
  + Future.catchTrace(future, (e, s) { ... })

• Mark Future.catchError as @Deprecated(), pending removal in 3.0.

If we wanted to add them as instance methods in 3.0, then we'll need to dance a bit (add new methods to the approximately ~100-200 sub-classes of Future in the wild). We could do this after Future.catchError is @Deprecated(), and then use another automated process to move back the static methods to instance methods fairly easily.

I'd sign up for work inside of Google and Flutter on this one.

As a side note, if we had had sealed as a language feature from the beginning, we would likely have sealed a lot of SDK classes including at least Future, Stream, Zone, Symbol, Type, DateTime, Interval, and RegExp. Then we would be able to change the Future API to be more permissive because all the classes implementing Future wouldn't exist.

It might not be too late, at least on some of them (Zone, Symbol, Type, DateTime, Interval, RegExp - I don't see any sub-classes of these within google3). For Future (191 occurrences) and Stream (280 occurrences), it would definitely be more work, but I'd consider it user-hostile to seal these classes - they are basic data structures and the nearly 500 custom classes make me believe the SDK's implementation is not suitable for a large set of clients.

For Stream specifically, it looks like it is extended (228) more than implemented (56). If we could make the requirement extends, it would make future evolution much easier. You could imagine a similar approach for Future - it looks like almost every implementation delegates to another Future.

[matanlurey]

We could also add a lint, i.e. avoid_future_catch_error to suggest using async/await or the new static methods, and try to enable that lint in Flutter and Google. That would at least stop the bleeding of this broken method.

[natebosch]

is it possible to add a static warning/lint for the specific incorrect usage without changing the implementation in the SDK? Can we hardcode some SDK apis within analyzer to have it treat them slightly differently than they are written.

[matanlurey]

I think there was a proposal from @bwilkerson (or @pq?) for a poor-man's union type.

For example, something like this:

Future<S> catchError<S>(@Union2<S Function(Object), S Function(Object, StackTrace)> Function onError)

Unfortunately there was a lot of opposition, and it would need to modify the SDK :(

[lrhn]

Add top-level/static methods that delegate to <Future>.catchError ... Those would need to be generic, otherwise they won't be able to ensure the typing anyway. Also, catch is a reserved word (trust me, otherwise we'd have used it already).

Future<T> onErrorOnly<T>(Future<T> future, FutureOr<T> handler(Object error), 
[bool test(Object error)]) => future.catchError(handler, test);
Future<T> onError<T>(Future<T> future, FutureOr<T> handler(Object error, StackTrace stack), 
[bool test(Object error])) => future.catchError(handler, test);

That's definitely doable, but not user friendly. I'd rather wait for extension methods or interface default methods than start adding top-level static helper functions for existing more-usable methods, and then try to migrate people to the less usable version.

It also fails to have any of the improvements that I'd actually want on catchError. Say (on Future<T>):

Future<T> onError<E>(FutureOr<T> handler(E error, StackTrace stack), [bool test(E error)]) ...

That would allow simple type checks to be handled without a test-argument, and even infer the type, so:

  future.onError((StateError e) { ... if (e.something) return null; });

would automatically only catch state errors.

Going to an intermediate "solution" which is less usable that what we have, and not getting any of the potential advantages either, that seems like a lot of work for little effect, and it won't help all the other functions that have similar issues.

More interesting solution would be to allow FutureOr<T> Function to denote the class of functions with FutureOr<T> as return type. As a type annotation, it would reject any function with a return type that is not assignable to FutureOr<T>. A value with that type could be "dynamically callable" like Function because there is no known argument signature, but the result type would still be known. Parsing gets harder, again, and we'll likely have to reserve the Function identifier, which is a breaking change, but a much more restricted one than changing the Future API.

It would still be breaking to change catchError to:

Future<T> catchError(FutureOr<T> Function handler, [bool test(Object error)]) ...

because sub-classes are not implementing it, though :(

(My point about sealing Future wasn't that I wanted to do it now, but that I definitely would have at the time it was written, and be validated in that choice by my ability to change the API now (along with the performance improvements it would have allowed). I would then have prevented the 500 subclasses that people have made since, but I doubt any one of them would have made me unseal the class anyway, since I rely on it being sealed for performance. That's the real danger of having sealed in your language. :wink:)

[matanlurey]

@lrhn:

That's definitely doable, but not user friendly. I'd rather wait for extension methods or interface default methods than start adding top-level static helper functions for existing more-usable methods, and then try to migrate people to the less usable version.

Are we expected to get either of those in an upcoming release (read: soon)? If not, I don't think its fair to make all current and especially new users of Dart and Flutter suffer because hypothetical features might come out at some point in time.

Going to an intermediate "solution" which is less usable that what we have, and not getting any of the potential advantages either, that seems like a lot of work for little effect, and it won't help all the other functions that have similar issues.

I don't think its less usable just because, as you write:

It also fails to have any of the improvements that I'd actually want on catchError

That is, not having improvements you want doesn't make it less usable.

---

This is a serious usability issue. I realize you might not write 1000s of lines of application Dart code, so I want to make it clear from users that do, this is a super thorny issue and not easily solvable unless you have a lot of intimate knowledge of Dart, the type system, and core libraries.

I did propose a couple of alternatives:

1. Add static warnings to the analyzer specifically for incorrect use of Future.catchError
2. Add lints banning Future.catchError and to always use async/await+try/catch
3. Add static methods to use from >= 2.1.0 <4.0.0
   • Then, make the stack trace always required for Future.catchError in 3.0
   • Or, add another method, i.e. catchErrorOnly or similar

I do not think it is reasonable to say we can't make any improvements at all, anywhere.

[lrhn]

I'm sure we can make some improvements. This is filed as a library issue, and I'm saying I don't think we can make meaningful library improvements with the current Dart feature-set. I don't think static helper functions are viable in practice. Maybe inside Google where we can force people to use them and poterntially fix all the existing code, but for everybody else, there is no discoverability and bad ergonomics.

An analyzer special-case is a much more promising. That was what the analyzer did for Future.then until we introduced FutureOr. If the analyzer knows that arguments to Future<T>.catchError must return FutureOr<T>, then it can flag any function that doesn't. It won't provide type inference for the function, so the users will have to make the return type explicit in the code, but that is doable.

[natebosch]

An analyzer special-case is a much more promising.

In my opinion this should be our focus. @bwilkerson - can you weigh in on the feasibility?

[matanlurey]

Ping @bwilkerson @pq thoughts?

[bwilkerson]

Sorry, I missed this issue somehow. If we want to flag either all uses of catchError or only those whose argument does not have a return type of FutureOr, then that sounds like it should be easy.

[matanlurey]

I think flag:

• Callback does not have a return type of FutureOr<T> for a given Future<T>
• Callback does not match Function(Object) or Function(Object, StackTrace) contravariantly

[matanlurey]

I would take a shot at starting this but I don't know much about the analyzer code base or where I would write such a check. If there are any pointers or docs I could give it a shot or at least a first pass.

[bwilkerson]

Are we talking about a hint (on by default for everyone) or a lint (opt in)?

[matanlurey]

I imagine a hint (or even a warning if the spec can allow it), given that I don't think any developer wants to add code that will 100% of the time throw a CastError (well, any developer that isn't writing tests for the SDK). Consider this:

import 'dart:async';

void main() {
  final name = Future.value('Brian W.');
  name.catchError((int foo) => foo);
}

This code will never succeed at runtime. The only reason it doesn't fail statically is because the language lacks a way to express that static check (either union types, overloads, etc). Given that it's part of the core (I mean, Future is basically a primitive in Dart), it's reasonable to add a warning.

[vsmenon]

@lrhn @leafpetersen - would extension methods give us a non-breaking way to do this?

[leafpetersen]

This is filed as a library issue, and I'm saying I don't think we can make meaningful library improvements with the current Dart feature-set. I don't think static helper functions are viable in practice. Maybe inside Google where we can force people to use them and poterntially fix all the existing code, but for everybody else, there is no discoverability and bad ergonomics.

Extension methods would change the discoverability and ergonomics here. We can't change the catchError signature, but we can add two extension methods with new names and the tighter types.

[vsmenon]

@aadilmaan - we should mark this as blocked by extension methods and do it once we have the support. It's a P1 ask for google and flutter.

[lrhn]

Is this a request to add an extension method to the platform libraries?

I guess it would be something like:

extension FutureExt<T> on Future<T> {
  Future<T> onError<E>(FutureOr<T> handleError(E error, StackTrace? stack), 
                      [bool test(E error)?]) async {
    try {
      return await this;
    } on E catch (e, s) {
      if (test == null || test(e)) {
        return handleError(e, s);
      } 
      rethrow;
    }
} 

Then again, anyone can write this extension, it doens't have to be in the platform libraries.

(I'd love if that was the catchError function, but that would be a very breaking change).

[vsmenon]

Is this a request to add an extension method to the platform libraries?

Yes. Adding it to the platform would allow us to mark catchError as deprecated and point to the alternative.

[matanlurey]

How does one deprecate an instance method in favor of an extension method?

[DanTup]

Should we invest in a lint/hint that would catch obvious issues, whether:

• avoid_catch_error: Suggests using async and await (with try/catch) instead

This would have other benefits too. Something that's been coming up a lot in VS Code lately is that the debugger can't tell when an exception is "caught" if it uses .catchError(), so it frequently pausing on what the user considers a "caught error". This can be quite confusing (it's not clear whether there's an error they need to handle or not).

I've had lots of workarounds suggested, but they all introduce other issues (for example automatically resuming from exceptions - but that would happen for real-uncaught errors too, in which case you may as well just turn off breaking on exceptions). I think encouraging try/catch over .catchError() generally would be a great idea.

(I filed an issue in the linter repo here -> https://github.com/dart-lang/sdk/issues/58218)

[vsmenon]

Ping. Now that we have extension methods, do we want to revisit this? If not, is this still a P1?

[lrhn]

I do want to do something, like https://dart-review.googlesource.com/c/sdk/+/151512.