Add a lint for avoiding .catchError()

[DanTup]

An issue that's been coming up more and more recently in VS Code is the debugger breaking on exceptions that the user considers "caught" but the Dart VM does not. For example this code:

Future<void> main() async {
  await throwErr().catchError((e) => print('Caught!'));
}

Future<void> throwErr() async {
  throw 'a';
}

Here, when the exception is thrown, the VM examines the stack and sees no catch block so considers the error uncaught. This breaks the debugger (when using the default "break on uncaught exceptions" mode) and is very confusing (it's not clear to the user if there's some uncaught error they need to handle).

There are often suggestions of workarounds for this in VS Code, but they all have some drawbacks. In my opinion, a much better option is to update the libraries to not use catchError() and instead catch. Having a lint would make it easier to enforce this (and it would be much easier to send PRs to packages that enables and fixes the lint, without fear that they'd accidentally put more .catchError() calls in in the future).

(this was also mentioned at #34144, so there may be other benefits to this too)

[natebosch]

I worry that this would be a lint with a very high number of false positives. If this applies in methods that are not async I'm not sure if there is a refactoring that preserves semantics. Same for intentionally unawaited futures in async methods...

[DanTup]

I don't think a refactor/fix should be a requirement for a lint (though it would be nice to have one). If you have a lot of code that intentionally doesn't await futures, you could not use the lint (or just ignore it where it makes sense).

The debugging issue seems to come up a lot - users don't understand why the VM keeps breaking on caught exceptions (and even though it can be explained, it doesn't change that it seems weird we can't break only on uncaught exceptions).

Here a few issues related to this (some requesting all sorts of strange workarounds in the editor - though none work well because the editor also doesn't know when an exception-pause is really caught or uncaught so anytime we avoid pausing is the potential for masking a real exception):

• Hard to read exception Dart-Code/Dart-Code#2664
• confusing experience debugging with caught exceptions in .pub-cache -- skipFiles option? Dart-Code/Dart-Code#2701
• [Feature Request]: Ignore exception and continue Dart-Code/Dart-Code#2670
• Debugger pauses on exception that will be caught Dart-Code/Dart-Code#2063
• Flutter debugging in Visual Code throws handled exceptions as uncaught flutter/flutter#33427
• Cancelling Google Sign In on Android & iOS throws exception instead of returning null flutter/flutter#26705
• IDE uncaught exception breakpoint stopped unexpectedly when using a combination of Future and Completer #37268
• Dart debugging in Visual Code throws handled exceptions as uncaught #37953

Most of these seem to come from code using catchError() that doesn't need to. If we can't make the debugging work better, it seems like generally guiding people to use try/catch and not .catchError() is the next best option and having a lint seems like the only good way to avoid them accidentally creeping back in.

[natebosch]

What I'm worried about is the cost/benefit. How many projects will be able to use this vs not be able to use it because of the noise?

Also, if we push users towards enabling by default this so they have a less confusing experience around debugging, what will our guidelines be when they do run into the situations where they need .catchError? I'd strongly to prefer not to encourage // ignore in any official docs.

[DanTup]

How many projects will be able to use this vs not be able to use it because of the noise?

I don't know for sure, but I just cloned the dio project (which is mentioned in at least three of the issues above) and ignoring tests, it only has a few uses of catchError and I believe all are trivial to change to await/try/catch. It's not a useful sample, but it's at least one project that I think would significantly benefit from this (as it's confused/frustrated at least some people).

if we push users towards enabling by default this so they have a less confusing experience around debugging, what will our guidelines be when they do run into the situations where they need .catchError?

I'm not sure (though I also don't know how common this is). I don't think a lint needs to have a 0% false positive rate to be useful. I'd personally prefer to have this lint and turn it on based on the debugging issue alone, and I'd happily use // ignore if there are really places I need to use it. Right now authors of packages use this completely unaware of the frustration it might be causing users of their packages (in one of the examples above, this was happening in a loop, so it just immediately paused every time they clicked continue - and there was no unhandled error - it's a confusing and frustrating experience).

Just having the lint (for those that would prefer it - like me!) and documenting why it's useful would be helpful even without official encouragement to use it. Right now people raise bugs that the debugger doesn't work properly and the response is "it's because the package is doing this thing and it would be better if they did this other thing" but there's no documentation or guidance anywhere except some random comments in some GH issues. It feels a bit like an excuse for not fixing the debugger (which I don't think can reasonably be fixed). At least if there was a lint that documented it (even if it wasn't specifically recommended), it would give a more official explanation to link to and an easy way for people to enforce it if they wanted.

I'd prefer the lint wasn't needed - but since it doesn't seem like the debugger could be made to handle this, it's the best I've come up with. Without either, the debugging experience just feels broken if you hit these cases. People are suggesting all sorts of workarounds for the IDEs (like auto-resuming when we bit exceptions that meet certain conditions, or auto-resuming all exceptions and just printing them to a separate log) in the absence of anything better.

If anyone has other suggestions, I'd definitely like to hear them :-)

[glacials]

I don't know enough to have suggestions, but as a new Dart/Flutter user hitting this issue I 100% agree with @DanTup's assessment of the problem -- I was confused about why my debugger was stopping at this exception, became convinced for a time that my catch wasn't actually working and the exception was in fact uncaught, then eventually learned to settle into hammering "Continue" until my app starts doing its work again.

That said, I'm not convinced linting people towards try/catch is a holistic enough solution to be worth the cost -- one thing I haven't seen mentioned is the Flutter FutureBuilder use case, where catchError is not even explicitly used and so cannot be easily substituted.

return FutureBuilder<Widget>(
  future: f,
  builder: (BuildContext context, AsyncSnapshot<String> snapshot) {
    if (snapshot.hasData) {
      return SuccessfulWidget();
    } else if (snapshot.hasError) {
      return ErrorMessageWidget(snapshot.error);
    }

    return LoadingSpinnerWidget();
  }
);

This pattern of checking hasError to see if the future threw an exception is the official example use of FutureBuilder and would still trip this debugger issue without any ability to fix it. (This exact use case is how I arrived here via Google 😄)

[DanTup]

That said, I'm not convinced linting people towards try/catch is a holistic enough solution to be worth the cost -- one thing I haven't seen mentioned is the Flutter FutureBuilder use case, where catchError is not even explicitly used and so cannot be easily substituted.

Good point - I hadn't considered cases like this, where the future is created in a different place and the event handler added later. I'm not sure there's going to be any way to improve situations like that.

I decided to do some testing of how this works in JavaScript, and I found that its debugger handles this a little better. Given the following code, the debugger pauses - treating the exception as unhandled (this is similar to what's happening in Dart):

async function throwError() {
	throw new Error('err');
}

function foo() {
	throwError().catch((e) => console.log('caught!'));
}

In this case, the throw is occurring before catch is called (since throwError is synchronous, even though it's marked async and returns a rejected promise.

However, if the throw is delayed, the debugger does not pause:

async function throwError() {
	await new Promise(setImmediate);
	throw new Error('err');
}

function foo() {
	throwError().catch((e) => console.log('caught!'));
}

Here, the error handler has been attached before the error is thrown. It seems like the Node debugger is able to check this, and not treat the error as uncaught. I wonder whether that Dart debugger would be able to do something similar - it seems like that should fix most cases. I'll open an SDK issue for discussion.

[DanTup]

My node example above was a bit flawed - it was still throwing synchronously. If I change it to:

async function throwError() {
	await new Promise(setImmediate); // Added this, similar to the .catch() example
	throw new Error('err');
}

async function foo() {
	await throwError();
}

foo();

The debugger does break, but it's no longer inside throwError, it's on a reject call further up the stack. So it's not quite as good as I thought - but you can still force it to break on the throw by enabling pause on caught exceptions, so it still feels like a reasonable improvement.

[DanTup]

With #37953, errors thrown that will be handled by a catchError are treated as caught, so the original reason for this has been solved :-)