proposal: `avoid_future_catcherror`

christopherfujino commented 1 year ago

avoid_future_catcherror

Description

Avoid calling Future.catchError.

Details

Calling the catchError method on a Future can lead to type errors at runtime if the runtime type of the Future is more specific than that of the variable or argument that references it.

Kind

Does this enforce style advice? Guard against errors? Other?

Bad Examples

void doSomething() {
  unawaited(doSomethingAsync().catchError((_) => null));
}

Future<Object?> doSomethingAsync() => Future<Object?>.value(1);

Good Examples

void doSomething() {
  unawaited(doSomethingAsync().then(
    (Object? obj) => obj,
    onError: (_) => null,
  ));
}

Future<Object?> doSomethingAsync() async => Future<Object?>.value(1);

Discussion

This is the SDK issue indicating tracking that statically correct usage of Future.catchError can lead to a runtime ArgumentError: https://github.com/dart-lang/sdk/issues/51248. This investigation was initiated by https://github.com/flutter/flutter/issues/114031, which was a runtime crasher in the Flutter CLI tool that was very difficult to track down.

I intend to implement this lint in the Flutter CLI tool, and will discuss enabling it across the Flutter SDK.

The "Bad example" above will not hit this runtime issue. Should I have included an example that would crash at runtime? A trivial example of this would be:

void doSomething() {
  unawaited(doSomethingAsync().catchError((_) => 'hi'));
}

Future<Object?> doSomethingAsync() => Future<bool>.error(true);

Discussion checklist

[ ] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
[x] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
[ ] If there's any prior art (e.g., in other linters), please add references here.
[x] If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
[x] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

christopherfujino commented 1 year ago

In addition I created a draft PR for this: https://github.com/dart-lang/linter/actions/runs/4177158649/jobs/7234383340

I used it to create the following Flutter CLI tool PR https://github.com/flutter/flutter/pull/120637

srawlins commented 1 year ago

I don't think this behavior is restricted to Future.catchError. This is true of any API and using subtypes. For example:

void main() {
  List<Object?> list = <bool>[true, false];
  list.add('hello');
}

This code also always has a runtime type error, but no static error.

I suspect it would be a mistake to implement such checks piecemeal.

bwilkerson commented 1 year ago

I suspect it would be a mistake to implement such checks piecemeal.

We certainly don't want to do so without due consideration.

To start the conversation, one question that I'd want to ask is how many false positives would a general lint have compared to a more specific lint. I haven't thought about it long enough to have an answer, but someone else might have.

christopherfujino commented 1 year ago

To start the conversation, one question that I'd want to ask is how many false positives would a general lint have compared to a more specific lint. I haven't thought about it long enough to have an answer, but someone else might have.

What would a more general lint look like? The only thing I can think of would be to warn when assigning a List<bool> to a more generally typed variable, however that wouldn't have prevented https://github.com/flutter/flutter/issues/114031 as the Future in question originated in the SDK.

srawlins commented 1 year ago

What would a more general lint look like?

Yeah I have no idea what a more general lint would look like. The broader issues which have been filed are https://github.com/dart-lang/linter/issues/1133 and https://github.com/dart-lang/sdk/issues/33372. There is a lot of text there so it will take me a while to re-load it into my brain :)

christopherfujino commented 1 year ago

As an aside, it sounds like @lrhn is going to update Future.onError(handler) to delegate to this.then(..., onError: handler) https://github.com/dart-lang/sdk/issues/51248#issuecomment-1431800690, in which case (if this lint could instead recommend always using Future.onError().

christopherfujino commented 3 months ago

@srawlins so, Future.onError has been updated to be a more type-safe alternative to Future.catchError (https://github.com/dart-lang/sdk/issues/51248#issuecomment-2035079904), what do you think about a lint that bans the latter in favor of the former? If not, I will just add it to the tool custom rules.

srawlins commented 3 months ago

@lrhn What do you think? Is FutureExtension.onError a better-in-every-way alternative to Future.catchError? Can we deprecate Future.catchError? I believe we could make a data-driven fix, if deprecated, to replace, e.g.

  void f(Future fut) {
-   fut.catchError((e, s) {});
+   fut.onError((e, s) {});

+   // Add a dead parameter.
-   fut.catchError((e) {});
+   fut.onError((e, _) {});

   void callback(Object e) {}
+   // Wrap a non-function-expression.
-   fut.catchError(callback);
+   fut.catchError((e, _) => callback(e));
  }

Such that dart fix would fix users code.

If there are any reasons, or enough reasons, to keep Future.catchError, then what do you think about a lint rule that always recommends using FutureExtension.onError? Are there circumstances where onError would not be an appropriate replacement for catchError, or could a lint rule be very broad?

Also cc @iinozemtsev who has been looking at similar issues internally, IIRC.

lrhn commented 3 months ago

Switching to using onError for all future catches sound be safe.

Changing existing catches sound be OK if possible.

The main difference is that catchError allows a one-argument error handler, where onError does not. That's necessitated by wanting the parameter to be typed. That does mean that not every current handler can be used with onError without either:

adding a stack trace parameter, if its a literal function.
wrap in (var error, StackTrace _) => oldHandler(error) if not a literal.

There are also some type checks that aren't done until the error has been caught. To make those type statically, we might have to widen the type the handler accepts. If we can do a type based migration, this should all be possible. A few might need a dynamic downcast.

eernstg commented 3 months ago

catchError does have one special power, being an instance member of Future: It can return a new future with the same type argument. This could be important in cases where that future is being handled in a non-trivial manner (that is, anything other than being awaited immediately). For example:

import 'dart:math';

void main() async {
  // Introduce covariance: Static and dynamic type arguments differ.
  Future<Object> future = Future<int>.error("Ouch!");

  var result = Random().nextBool()
      ? future.catchError((_, __) => 42)
      : future.onError((_, __) => 42);

  if (result is Future<int>) {
    print('Future<int>'); // Will arrive here if we used `catchError`.
  } else {
    print('Future<Object>'); // Will arrive here if we used `onError`.
  }
}

It could be argued that this example is unrealistic, because we implicitly claim to have "forgotten" that the type argument of the future is int, and yet we are lucky enough to provide an int as the value to use in case of an error.

However, it is actually possible to align the actual type argument of the future with the value which is used in case of an error, and even the function object can have the correct return type (catchError doesn't care about the function return type, it just checks the type of the returned value, but in general we'd need to pay attention to function types as well).

One way to do this is to bundle the future and the value together at a location in the code where the future has just been created (such that we know the precise type argument and we can choose a value of that type, or provide some devices that will allows us to obtain such a value when needed).

OK, developers probably aren't going to do this in real life, but it illustrates that it is possible to use techniques whereby the type safety is guaranteed even in the presence of covariance (as long as we use that technique consistently).

import 'dart:math';

class Bundle<X> {
  final Future<X> future;
  final X value;
  Bundle(this.future, this.value);
  X onError(_, __) => value;
}

void main() async {
  // Introduce covariance: Static and dynamic type arguments differ.
  Bundle<Object> bundle = Bundle<int>(Future.error("Ouch!"), 42);

  var result = Random().nextBool()
      ? bundle.future.catchError(bundle.onError)
      : bundle.future.onError(bundle.onError);

  if (result is Future<int>) {
    print('Future<int>'); // Will arrive here if we used `catchError`.
  } else {
    print('Future<Object>'); // Will arrive here if we used `onError`.
  }
}

The point is simply that if somebody insists on doing something like this then perhaps they wouldn't want to switch from catchError to onError.

iinozemtsev commented 3 months ago

Ideally I would love to be able to ban all covariant assignments for certain types (like Completer, Future, Sink), but I'm also voting with both hands for the ability to ban catchError in particular :)

The tricky behavior is indeed possible, but I think this just means that automated fixes should be carefully reviewed (and it would still be possible to ignore this lint via // ignore:).

lrhn commented 2 months ago

@eernstg If you have an expression with static type Future<Object> and it matters whether it's a Future<int> or not, then the code is not being generic in the type parameter. It's doing introspection to try to find the subtype. Then it might as well do that check before the branch, to use the most specific known type at all times.

There are very few situations where you want to store a Future along with any other data. Either the data is used before, or it's used after the future completes, which means inside the callback/after the await. It's incredibly rare to want to store a future side-by-side with any other data. And if it does, and that data is also covariantly typed by the same type parameter, then the correct place to put code that uses the data is inside the same object as a method with access to the actual type variable. If it's upcast, it's because it's OK to upcast.

eernstg commented 2 months ago

@lrhn wrote:

If you have an expression with static type Future<Object> and it matters whether it's a Future<int> or not, then the code is not being generic in the type parameter. It's doing introspection to try to find the subtype. Then it might as well do that check before the branch, to use the most specific known type at all times.

We could have a long style discussion about this. I was just pointing out the fact that if a future is typed covariantly (that is, the statically known actual type argument at Future differs from the run-time value) then it does make a difference that catchError is an instance member and onError is an extension member.

I'm sure we can come up with a hundred reasons why most futures aren't typed covariantly, but I'd still claim that it is plausible that some futures are typed covariantly in a reasonably well designed Dart program. Due to the complexity of some applications it is not quite fair to say "it might as well do that check before the branch" (meaning "reasonably well designed Dart code will never encounter a situation where a future is typed covariantly, and the run-time type argument of that future does matter for the application logic").

So let's consider the situation based on the premise that we have encountered a covariantly typed future whose run-time type does matter to the application logic.

In that situation, the semantics of catchError differs from the semantics of onError in a way that we can't compensate for in an invocation of onError. That is, we can't choose to pass explicit type arguments or modify the invocation in any other way such that onError will be able to do what catchError does.

There are very few situations where you want to store a Future along with any other data.

You can use many different coding techniques to obtain a value of a type which is only known by an upper bound, e.g., you could use a factory or keep an actual value around. I used the latter because it's very simple to express, and the actual bundling into a separate instance of type Bundle makes it very easy to see that the two entities (the future and the error fallback value) are correctly paired up.

[If you store a future side-by-side with any other data then ..] that data is also covariantly typed by the same type parameter, then the correct place to put code that uses the data is inside the same object as a method with access to the actual type variable.

You're just restating the fact that an instance member has a special power, namely the ability to denote (and hence use) a given type parameter.

So that's true, but the whole point of this discussion is that onError (being an extension member) does not have that special power, and we can't make it an instance member because that's a too-breaking change.

So your advice is good, but it can be unrealistic.

Anyway, as I mentioned, it is definitely possible that the scenario I described will be considered esoteric and we should just ignore it.

However, we could also take it into account and mention in the lint message emitted by avoid_future_catcherror that there is this difference between catchError and onError, and the developer might need to take it into account.

lrhn commented 2 months ago

I think we should ignore the difference, and I support using onError everywhere, and not using catchError ever again. (Not just because the latter has horrible static typing properties, but also becuase of that.)

Using the static type instead of the future's inherent type is difference, but that's a benficial feature in most cases. If everybody switched to using onError instead, with a proper rewrite of the function argument to now being typed, I seriously doubt we'd see any errors outside of the SDK's tests/lib/async directory.

(I sometimes wish I could have the effect of extension methods, using the static type of the receiver, in instance methods.)

dart-lang / linter