rewrite-static-analysis icon indicating copy to clipboard operation
rewrite-static-analysis copied to clipboard

Fix `CatchCauseOnlyRethrows` onlyRethrows check for multi catch statements

Open nielsdebruin opened this issue 4 months ago • 0 comments

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

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

nielsdebruin avatar Oct 07 '24 10:10 nielsdebruin