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: #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.).