Make `avoid_catching_errors` more linient

[goderbauer]

Issue https://github.com/dart-lang/linter/issues/3023 (dealing with avoid_catches_without_on_clauses) 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 https://github.com/dart-lang/sdk/commit/500a8c0a72851ac95f60685e66df035246ca97c6 in response to https://github.com/dart-lang/linter/issues/3023 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.

[goderbauer]

cc @Hixie @srawlins

[srawlins]

I'm in favor of keeping these consistent. I think the motivation behind each rule is similar in how Error might be handled.

[lrhn]

It flags when Errors are caught that are later rethrown after resetting some state.

Use finally for that.

It flags when Errors are caught to then throw a new Error with a more specific error message.

Shouldn't be necessary. Errors should not happen at all. (I can see how it can help debugging.) If it happens at a framework boundary, the catch should catch all values (no on clause). Use // ignore if needed.

Frameworks can run unrelated and untrusted code as a component, and may want to handle errors for that component. That's one of the cases where catching errors is OK, but only because it catches everything. (An unhandled Exception is an error. Not the exception itself, but that it wasn't handled.)

Catching a specific error is still a bad idea. It's not OK that an error was thrown, it shouldn't be "handled". It can be contained, but that shouldn't apply to specific individual errors.

It flags when Errors are caught and directly passed to an error processing function (e.g. FlutterError.reportError)

That sounds like a framework boundary. Catch any thrown object (no on clause) and use an // ignore if necessary.

I am wondering if the same lenience that was extended to avoid_catches_without_on_clauses in https://github.com/dart-lang/sdk/commit/500a8c0a72851ac95f60685e66df035246ca97c6 in response to https://github.com/dart-lang/linter/issues/3023 should also be applied to avoid_catching_errors?

I'd say no. A catch-all at a framework boundary makes sense. Catching individual errors is against the intent of the avoid_catching_errors lint.

[Hixie]

It flags when Errors are caught that are later rethrown after resetting some state.

Use finally for that.

That doesn't work for something like "count the number of Errors".

It flags when Errors are caught to then throw a new Error with a more specific error message.

Shouldn't be necessary. Errors should not happen at all. (I can see how it can help debugging.)

Debugging would be the main use case, yes. Debugging is one of the primary considerations of the Flutter framework. :-)

It flags when Errors are caught and directly passed to an error processing function (e.g. FlutterError.reportError)

That sounds like a framework boundary. Catch any thrown object (no on clause) and use an // ignore if necessary.

Sometimes you want to handle certain errors differently than others, e.g. Tween uses "on TypeError" to provide significantly more useful developer support.

Other examples where Flutter intentionally catches Errors: TextPainter catches an ArgumentError to handle the case where the caller provided invalid Unicode, replacing the built text with '\uFFFD' and reporting the error to the framework. The image cache catches FlutterErrors to evict the failed key and rethrow. Some debug code calls a function whose purpose is to throw AssertionErrors and then catches those errors and turns them into FlutterErrors.

Some of those could be implemented differently, but I think it's a bit of an overgeneralization to say they're all bad ideas.

[lrhn]

That doesn't work for something like "count the number of Errors".

The number of errors in production should be zero, so that should be easy :wink: If there are errors, and the program shouldn't crash on them, then its because untrusted code is being run, which suggests a framework boundary. Again I'd do a catch-all there. It's not like non-Error throws that haven't been handled are OK. They should be counted too. And a catch-all doesn't trigger the lint.

Sometimes you want to handle certain errors differently than others, e.g. Tween uses "on TypeError" to provide significantly more useful developer support.

That's a choice one can always make, but it is choosing to go directly against the lint intent. It's dynamic code, and optimisitic one at that, which is a reasonable reason for catching errors. It's a good use of // ignore with an explanation for why the lint is being ignored.

I'm not saying any these choices are inherently bad. I am saying that they should use // ignore, because they are (delibertely and reasonably) choosing to go against the recommended behavior. That's what // ignore is for.

I don't see a general enough rule that could be used as an exception, or enough uses that it's important to have one. If there was an exception, it would be something like "for adding debugging information". I don't think we can detet that intent programmtically, and I don't see a good way to write the intent into the program. (Having a rethrow somewhere isn't enough, IMO. A rethrow on all branches might.)