Fix CatchCauseOnlyRethrows onlyRethrows check for multi catch statements

[nielsdebruin]

What's changed?

This pull request includes changes to the CatchClauseOnlyRethrows class and its corresponding test class to improve the handling of multi-catch blocks and simplify the code.

Improvements to CatchClauseOnlyRethrows class:

• src/main/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrows.java: Removed type checks for the caught exception and its type, simplifying the onlyRethrows method, and allowing multi catch statement to be correctly handeled.

Enhancements to test coverage:

• src/test/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrowsTest.java: Added a new test case tryCanBeRemovedWithMultiCatch to ensure that try-catch blocks with multi catch exceptions are correctly handled and can be removed when appropriate.

What's your motivation?

Consider the following input:

class A {
  void foo() throws IOException {
      try {
          new FileReader("").read();
      } catch (FileNotFoundException e) {
          throw e;
      } catch(IOException | ArrayIndexOutOfBoundsException e) {
          throw e;
      }
  }
}

Currently, no suggestions are provided to remove the redundant catch (FileNotFoundException e) block. The issue stems from the onlyRethrows method not functioning correctly with multi-catch statements. For a J.Try.Catch the onlyRethrows method will perform the following check:

1. The catch body contains a single statement of type J.Throw.
2. The J.Throw exception type matches the catch parameter type.
3. The J.Throw statement contains an identifier that matches the catch parameter.

The problem lies with the second check. For catch (IOException e), the check passes since e is assigned to IOException. However, for catch (IOException | ArrayIndexOutOfBoundsException e), the type of e becomes Exception, causing the check to fail.

Fortunately, I believe the second check is unnecessary. If the throw contains only a single statement of type J.Throw (check 1), and this throws only contains a J.Identifier pointing to the catch parameter, there’s no chance of modifying e. Hence checking for type changes is not required. This change resolves the issue with multi-catch statements and improves method efficiency.

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Instead of removing this check, I wonder if we should add an additional check with something like if (catchType instanceof JavaType.MultiCatch) { ... check if any of the thrown exceptions matches ... }. I'd feel slightly more secure with such an approach then dropping this requirement.

[nielsdebruin]

Hi Tim, Thanks for the review! I am not sure how to implement this check....

In the base case e.g. catch(A ex) {throw ex} type A is assigned to ex in the body. Hence we can compare the type of the catch parameter ex that of the body. In the multicatch, catch (A | B ex) {throw ex} both ex in the body and parameter will be assigned type Exception, as it is not know if they will be of type A or B. Using JavaType.MultiCatch.getThrowableTypes I can retrieve the possible types the parameter ex can take, but the I can't compare them with the type of ex in the body as this will still be of type Exception.

[Laurens-W]

If we can't determine it based on type then maybe in case of a MultiCatch we could check whether exception is an identifier and if it is whether it's the same identifier as aCatch.getParameter()?

[nielsdebruin]

Isn't that already checked on line 114?

if (exception instanceof J.Identifier) {
    return ((J.Identifier) exception).getSimpleName().equals(aCatch.getParameter().getTree().getVariables().get(0).getSimpleName());
}

[Laurens-W]

Ah yes, missed that part! Though checking by name is potentially more dangerous than checking by reference.
Not sure what direction to go here then

[timtebeek]

@nielsdebruin & @Laurens-W would you agree with the change I pushed in 6b39457 ?

[github-actions]

Some suggestions could not be made:

• src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
  • lines 35-35
• src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java
  • lines 28-28
  • lines 59-60

[nielsdebruin]

@nielsdebruin & @Laurens-W would you agree with the change I pushed in 6b39457 ?

As mentioned in the PR description I don't think this extra check is required, however, it certainly cannot be harmful. So I agree with it.

[timtebeek]

@nielsdebruin & @Laurens-W would you agree with the change I pushed in 6b39457 ?

As mentioned in the PR description I don't think this extra check is required, however, it certainly cannot be harmful. So I agree with it.

Yeah it's happened a few too many times that I thought "we don't need that" only to later realize we did indeed need that, we'd just lacked a test for it. So figured be slightly more conservative with the change for now. Thanks though!