Error prone ExceptionSpecificity check to avoid catching too broadly

[carterkozak]

Before this PR

try {
   doesNotThrowChecked();
- } catch (Exception e) {
+ } catch (RuntimeException e) {
    log.error("failure", e);
}

try {
   doesNotThrowChecked();
- } catch (Throwable t) {
+ } catch (RuntimeException | Error t) {
    log.error("failure", e);
}

try {
   doesNotThrowChecked();
} catch (RuntimeException e) {
    throw e
- } catch (Exception e) { // unreachable
-    log.error("failure", e);
}

After this PR

==COMMIT_MSG==
Error prone ExceptionSpecificity check to avoid catching too broadly

Prefer more specific catch types than Exception and Throwable.
When methods are updated to throw new checked exceptions they expect
callers to handle failure types explicitly. Catching broad types defeats
the type system. By catching the most specific types possible we
leverage existing compiler functionality to detect unreachable code.
==COMMIT_MSG==

Possible downsides?

Possible to create a large diff in projects which don't follow best practices.
As always there's a chance the rewrite is not completely accurate.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Error prone ExceptionSpecificity check to avoid catching too broadly

Prefer more specific catch types than Exception and Throwable.
When methods are updated to throw new checked exceptions they expect
callers to handle failure types explicitly. Catching broad types defeats
the type system. By catching the most specific types possible we
leverage existing compiler functionality to detect unreachable code.

Check the box to generate changelog(s)

• Generate changelog entry

[robert3005]

Small flag for people to be aware - scala treats all exceptions as unchecked so this autofix can break things that integrate with some scala code where you catch an exception (however, java already yells at you for catching checked exception where one is not defined)

[iamdanfox]

Wondering if it would be clearer if this lived in a separate class? That way we'd have two error-prone checks that just do one thing, e.g. CatchSpecificity and ThrowsSpecificity?

[carterkozak]

I could go either way on this, there are two manifestations of the same conceptual issue. I suppose it boils down to whether we want bug checkers mapped to concepts, or implementation/output?

I was thinking about this yesterday, specifically for the assertj cleanup checkers -- it might be easiest for consumers to understand if all assertj refactor checks were part of a single 'AssertjUsage' check.

[iamdanfox]

I think I'm mainly thinking about this from a maintenance perspective, as shorter classes that just do one thing are easier to understand as I can be sure each private method is somehow relevant to the one purpose of the class.

[carterkozak]

That's a good point

[iamdanfox]

I think i'm still struggling with this method - there's quite a lot of intricacy in the de-duping and selecting only exceptions&throwables that is all woven together to get this all done in one pass.

what about if we did a loop through all the possible tree.getCatches() to accumulate the encounteredTypes, then could call out to a dedicated function to process each single catch?

[iamdanfox]

also the possibility of fully returning from deep inside a loop & if condition feels a bit weird - are we saying if we hit one catch(Exception) then a later catch(Throwable) might not get processed?

[carterkozak]

are we saying if we hit one catch(Exception) then a later catch(Throwable) might not get processed?

Yep, we have a test for that case which takes two rounds to resolve. We can do it in one loop, but I worried about the additional complexity. I can take a look at implementing it and we can decide from there.

[iamdanfox]

Personally I'm fine with it just doing one thing at a time - minimum complexity = happy dan. Just wanted to check my understanding.

[carterkozak]

Turns out it's straightforward to fix 'em all at once. I think that blocks us from splitting into multiple loops though because the changes mutate state, and we would have to keep expected handled types in sync with the code that builds suggested fixes, leading us back to the original model.

[iamdanfox]

Also is this just a strictly better version of https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/ThrowSpecificExceptions.java ?

[carterkozak]

Oh wow is state.errorProneOptions().isTestOnlyTarget() new?

I think there's a lot of overlap, ThrowSpecificExceptions.java recommends changing throw <throwable> blocks to use more specific types, where this check produces no runtime changes, only provides stronger type safety by narrowing declared throws types on methods.