Fix: `CatchClauseOnlyRethrows` does not handle multi catch correctly

[nielsdebruin]

What's changed?

The CatchClauseOnlyRethrows will now correctly handle multicatch blocks that use the short hand syntax catch(Foo | Bar exception) {...}.

What's your motivation?

In issue https://github.com/openrewrite/rewrite/issues/4235, a bug was demonstrated that caused a catch statement to be incorrectly removed as a wider exception is specified later on. I found that the reason for this behavior is the incorrect handling of catch(Foo | Bar exception) when checking for a wider exception type. The current code only performs a direct comparison between the parameter type of the current catch statement and that of the next one. This works fine if the statement is of the form catch(Foo exception). However, in the case where the shorthand catch(Foo | Bar exception) is used, direct comparison of their parameter type no longer works. I added addition logic that will also correctly handle the check forJavaType.MultiCatch types.

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

[knutwannheden]

While there are still other improvements that are possible, I think they are out of scope for this PR. But I do get the impression that there is some sort of regression. The old implementation would have removed the first two catch blocks here (or also just the first one if you remove the middle one), whereas the new implementation doesn't seem to do that:

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

[nielsdebruin]

Hi @knutwannheden, thank you for the review! You are right. I updated the code and modified the order of the if statement to remedy the regression you mentioned. The modification, will now cause the first catch for FileNotFoundException in your example to be correctly deleted. However, it does preserve the IOException, which could be deleted. The cause behind this is that the onlyRethrows method is not able to handle a multicatch, and I am not sure how to fix this...I believe this PR will thus prevent code that should not be removed from being removed (as was the original issue). However, it might not remove all the code that can be removed, for that onlyRethrows need to be correctly modified (which the current implementation does not support either). As you mentioned perhaps a second issue/pr should be created for this.

My initial idea for a modified onlyRethrows would be as follows: A J.Try.Catch only rethrows IF: The body, of the catch only contains a J.Throw, which only contains a J.Identifier in its body, and this is the same identifier as the J.Try.Catch parameter.

private boolean onlyRethrows(J.Try.Catch aCatch) {
    if (aCatch.getBody().getStatements().size() != 1 ||
            !(aCatch.getBody().getStatements().get(0) instanceof J.Throw)) {
        return false;
    }

    Expression exception = ((J.Throw) aCatch.getBody().getStatements().get(0)).getException();
    if (exception instanceof J.Identifier) {
        return ((J.Identifier) exception).getSimpleName().equals(aCatch.getParameter().getTree().getVariables().get(0).getSimpleName());
    }

    return false;
}

[knutwannheden]

@nielsdebruin Thanks a lot for the improvement!