Skip to content

Fix CatchCauseOnlyRethrows onlyRethrows check for multi catch statements #355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timtebeek merged 3 commits into from
Oct 22, 2024
Merged

Fix CatchCauseOnlyRethrows onlyRethrows check for multi catch statements #355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e8d04a
Fix CatchCauseOnlyRethrows rethrows check for multi catch statements
Oct 7, 2024
7364f62
Merge branch 'main' into multi-catch-only-throws-support
timtebeek Oct 7, 2024
6b39457
Specifically check for `JavaType.MultiCatch`
timtebeek Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/main/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrows.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,6 @@ private boolean onlyRethrows(J.Try.Catch aCatch) {
}

Expression exception = ((J.Throw) aCatch.getBody().getStatements().get(0)).getException();
JavaType.FullyQualified catchType = TypeUtils.asFullyQualified(aCatch.getParameter().getType());
if (catchType == null || !catchType.equals(exception.getType())) {
return false;
}
Copy link
Member

@timtebeek timtebeek Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nielsdebruin nielsdebruin Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Laurens-W Laurens-W Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor Author

@nielsdebruin nielsdebruin Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

Copy link
Contributor

@Laurens-W Laurens-W Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (exception instanceof J.Identifier) {
return ((J.Identifier) exception).getSimpleName().equals(aCatch.getParameter().getTree().getVariables().get(0).getSimpleName());
}
Expand Down
39 changes: 39 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/CatchClauseOnlyRethrowsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,45 @@ void foo() throws IOException {
);
}

@Test
void tryCanBeRemovedWithMultiCatch() {
rewriteRun(
//language=java
java(
"""
import java.io.FileReader;
import java.io.IOException;
import java.io.FileNotFoundException;

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

class A {
void foo() throws IOException {
new FileReader("").read();
}
}
"""
)
);
}

@Test
void tryShouldBePreservedBecauseFinally() {
rewriteRun(
Expand Down