rewrite-static-analysis
rewrite-static-analysis copied to clipboard
Fix `CatchCauseOnlyRethrows` onlyRethrows check for multi catch statements
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 theonlyRethrows
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 casetryCanBeRemovedWithMultiCatch
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:
- The catch body contains a single statement of type J.Throw.
- The J.Throw exception type matches the catch parameter type.
- 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