avoid_catches_without_on_clauses false positives

[Hixie]

One valid use for catching all exceptions and errors is to forward them to some error reporting logic, for example, the way that Flutter frequently catches all exceptions and forwards them to FlutterError.reportError.

It would be nice if a function like FlutterError.reportError could be annotated as a valid reason to catch exceptions, such that if the catch block contains a call to such a function, it isn't flagged with avoid_catches_without_on_clauses.

[Hixie]

Forwarding to Completer.completeError would be another example of a valid reason to catch all exceptions.

[Hixie]

Another example of a false positive is catch blocks that unconditionally call rethrow.

[Hixie]

Another false positive is when a new exception is thrown, wrapping the original one.

[Hixie]

Another false positive is when the catch block does return Future.error with the exception.

[bwilkerson]

How common are the false positive cases? Would it be better to fix them all, or to just stop using it?

[Hixie]

It did catch some problems, so it has value. flutter/flutter#91642 is the PR where I'm trying to turn it on (along with a few other lints), if you want to take a look.

[bwilkerson]

I wonder whether there's a better way to formulate the lint (something other than "don't do this broad thing except in these 5 cases"). Maybe one or more specific use cases that should unconditionally be avoided.

[Hixie]

The two problems I personally care about catching here are (1) cases where people willy-nilly swallow all exceptions because they just want to push all problems under the carpet, and (2) cases where someone wants to catch a specific exception but casts a much wider net than necessary (e.g. they just need to catch FormatException but they actually catch everything). Because Flutter is a framework it also does a lot of passing exceptions around, but I think that's pretty uncommon in apps.

[Hixie]

FWIW, due to the number of false positives, we've decided in flutter/flutter#91642 to disable the lint for Flutter's framework for now, but if we can ever enable it without all the //ignores I would be very happy because it catches some insidious patterns that a lot of people (including me) accidentally use and that are very hard to fix after the fact.

[srawlins]

I wonder whether there's a better way to formulate the lint (something other than "don't do this broad thing except in these 5 cases"). Maybe one or more specific use cases that should unconditionally be avoided.

(2) cases where someone wants to catch a specific exception but casts a much wider net than necessary (e.g. they just need to catch FormatException but they actually catch everything).

This sounds like it would be hard to detect accurately. I think it may be hard to invert the rule's logic to target this case.

On the other hand, might not be to hard to just make some allowances for the above exceptions like:

• If the caught error is passed to FlutterError.reportError, Completer.completeError, or Future.error, allow it.
• If the caught error is passed as an argument to any function, whose return value is immediately thrown (i.e. the invocation is the expression in a ThrowStatement), allow it.
• If the body of the catch has any rethrow, allow it.

[bwilkerson]

... just make some allowances for the above exceptions like ...

That sounds reasonable to me.

We might also consider adding

• If the caught error is passed as an argument to any function whose return type is Never.

[srawlins]

Ooh good thought. I can add that one.

[Hixie]

My only note would be that instead of hard-coding FlutterError.reportError, Completer.completeError, and Future.error, there be an annotation that those things have that gives them the magic ability. That way someone who develops a new framework can get the same integration with the lint.

[srawlins]

It would take years to get that annotation into dart:core or a pragma annotating Completer.completeError and Future.error so I'll just hardcode them.

[srawlins]

Fixed with 500a8c0

[goderbauer]

This bug was also linked as a blocker for enabling the avoid_catching_errors lint in Flutter and when enabling it I am essentially running into the same classes of problems:

• It flags when Errors are caught that are later rethrown after resetting some state.
• It flags when Errors are caught to then throw a new Error with a more specific error message.
• It flags when Errors are caught and directly passed to an error processing function (e.g. FlutterError.reportError)

I am wondering if the same lenience that was extended to avoid_catches_without_on_clauses in 500a8c0 should also be applied to avoid_catching_errors?

FWIW, most of the uncovered reports of avoid_catching_errors are inside asserts to improve the debugging experience when those Errors happen during development. I wonder if catches in asserts should generally be exempt from the avoid_catching_errors lint or whether just those specific categories mentioned above should be exempt from the lint.