From eab8f49487e5f4b2ce3fd53df1a4c86993fb6eb3 Mon Sep 17 00:00:00 2001 From: stefanod Date: Sat, 21 Jun 2025 23:30:56 +0200 Subject: [PATCH 01/17] (RSPEC-S2221) SpecifyGenericExceptionCatches recipe --- .../SpecifyGenericExceptionCatches.java | 184 ++++++++++ .../SpecifyGenericExceptionCatchesTest.java | 335 ++++++++++++++++++ 2 files changed, 519 insertions(+) create mode 100644 src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java create mode 100644 src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java new file mode 100644 index 0000000000..f00afbfea5 --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java @@ -0,0 +1,184 @@ +package org.openrewrite.staticanalysis; + +import org.openrewrite.Cursor; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.ShortenFullyQualifiedTypeReferences; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.JavaType.FullyQualified; +import org.openrewrite.java.tree.TypeUtils; + +import java.util.*; +import java.util.stream.Collectors; + + +public class SpecifyGenericExceptionCatches extends Recipe { + + @Override + public String getDisplayName() { + return "Replace `catch(Exception)` with specific exceptions thrown in the try block"; + } + + @Override + public String getDescription() { + return "Replaces `catch(Exception e)` blocks with a multi-catch block " + + "(`catch (SpecificException1 | SpecificException2 e)`) containing only the checked exceptions " + + "explicitly thrown by method or constructor invocations within the `try` block that are not " + + "already caught by more specific `catch` clauses. If no checked exceptions are found that " + + "require catching, the generic `catch` block is removed."; + } + + @Override + public Set getTags() { + return Collections.unmodifiableSet(new HashSet<>(Arrays.asList("CWE-396", "RSPEC-S2221"))); + } + + @Override + public JavaVisitor getVisitor() { + return new GenericExceptionCatchVisitor(); + } + + private static class GenericExceptionCatchVisitor extends JavaIsoVisitor { + + private static final String JAVA_LANG_EXCEPTION = "java.lang.Exception"; + private static final String MULTI_CATCH_SEPARATOR = "|"; + private static final String TRY_CATCH_TEMPLATE = "try {} catch (%s %s) {}"; + + @Override + public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { + J.Try t = super.visitTry(aTry, ctx); + + if (hasGenericCatch(t)) { + Set caughtExceptions = getCaughtExceptions(t); + Set thrownExceptions = getThrownExceptions(t); + thrownExceptions.removeAll(caughtExceptions); // Remove exceptions that are already specifically caught + + if (!thrownExceptions.isEmpty()) { + List updatedCatches = t.getCatches().stream() + .map(c -> updateCatchIfGeneric(c, thrownExceptions)) + .collect(Collectors.toList()); + + return t.withCatches(updatedCatches); + } + } + + return t; + } + + private static boolean isGenericCatch(J.Try.Catch aCatch) { + FullyQualified fq = TypeUtils.asFullyQualified(aCatch.getParameter().getType()); + if (fq != null) { + String fqn = fq.getFullyQualifiedName(); + return TypeUtils.fullyQualifiedNamesAreEqual(JAVA_LANG_EXCEPTION, fqn); + } + return false; + } + + private boolean hasGenericCatch(J.Try aTry) { + for (J.Try.Catch c : aTry.getCatches()) { + if (isGenericCatch(c)) { + return true; + } + } + return false; + } + + private Set getCaughtExceptions(J.Try aTry) { + Set caughtExceptions = new HashSet<>(); + + for (J.Try.Catch c : aTry.getCatches()) { + if (c.getParameter().getType() != null) { + caughtExceptions.add(c.getParameter().getType()); + } + } + + return caughtExceptions; + } + + /** + * Collects all checked exceptions that are explicitly thrown by method invocations + * and constructor calls within the try block. + * + * @param aTry the try block to analyze + * @return a set of exception types that may be thrown by code in the try block + */ + private Set getThrownExceptions(J.Try aTry) { + Set thrownExceptions = new HashSet<>(); + new JavaIsoVisitor>() { + @Override + public J.NewClass visitNewClass(J.NewClass nc, Set set) { + if (nc.getConstructorType() != null) { + set.addAll(nc.getConstructorType().getThrownExceptions()); + } + return super.visitNewClass(nc, set); + } + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Set set) { + if (mi.getMethodType() != null) { + set.addAll(mi.getMethodType().getThrownExceptions()); + } + return super.visitMethodInvocation(mi, set); + } + }.visit(aTry.getBody(), thrownExceptions); + return thrownExceptions; + } + + /** + * Updates a generic catch block (catching java.lang.Exception) to catch only the specific + * exception types that are actually thrown within the try block. + * + *

This method transforms generic catch blocks like: + * 

{@code
+         * catch (Exception e) { ... }
+         * }
+ * + * into specific single or multi-catch blocks like: + * 
{@code
+         * catch (IOException e) { ... }
+         * // or
+         * catch (IOException | SQLException e) { ... }
+         * }
+ * + * @param aCatch the catch block to potentially update + * @param thrownExceptions the set of specific exception types thrown in the try block + * that are not already caught by other catch clauses + * @return the original catch block if it doesn't catch java.lang.Exception, + * otherwise a new catch block with specific exception types + */ + private J.Try.Catch updateCatchIfGeneric(J.Try.Catch aCatch, Set thrownExceptions) { + if (!isGenericCatch(aCatch)) { + return aCatch; + } + + // Preserve the existing variable name from the original generic catch block + String variableName = aCatch.getParameter().getTree().getVariables().get(0).getSimpleName(); + + String throwableTypes = thrownExceptions.stream() + .map(TypeUtils::asFullyQualified) + .filter(Objects::nonNull) + .sorted(Comparator.comparing(FullyQualified::getClassName)) + .map(FullyQualified::getFullyQualifiedName) + .collect(Collectors.joining(MULTI_CATCH_SEPARATOR)); + + J.Try aTry = getCursor().firstEnclosing(J.Try.class); + assert aTry != null; + + J.Try tempTry = JavaTemplate.builder(String.format(TRY_CATCH_TEMPLATE, throwableTypes, variableName)) + .contextSensitive() + .build() + .apply( + new Cursor(getCursor(), aTry), + aTry.getCoordinates().replace() + ); + + J.ControlParentheses cp = tempTry.getCatches().get(0).getParameter(); + doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(cp.getTree())); + return aCatch.withParameter(cp); + } + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java new file mode 100644 index 0000000000..88a7cc6af6 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -0,0 +1,335 @@ +package org.openrewrite.staticanalysis; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; +import org.openrewrite.test.SourceSpecs; + +import static org.openrewrite.java.Assertions.java; + +class SpecifyGenericExceptionCatchesTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new SpecifyGenericExceptionCatches()); + } + + // Helper SourceSpecs for mock classes that throw checked exceptions + private static final SourceSpecs MOCK_THROWING_CLASS = java(""" + package com.example; + + public class MockThrowingClass { + + public MockThrowingClass() { + } + + public MockThrowingClass(boolean throwSQLException) throws java.sql.SQLException { + if (throwSQLException) { + throw new java.sql.SQLException("Test Constructor SQL Exception"); + } + } + + public void throwsIOException() throws java.io.IOException { + throw new java.io.IOException("Test IO Exception"); + } + + public void throwsSQLException() throws java.sql.SQLException { + throw new java.sql.SQLException("Test SQL Exception"); + } + + public void throwsBothIOAndSQL() throws java.io.IOException, java.sql.SQLException { + throw new java.io.IOException("Test IO or SQL Exception"); + } + + public void throwsNothing() { + // This method throws no checked exceptions + } + + public void throwsRuntimeException() throws RuntimeException { + throw new RuntimeException("Test Runtime Exception"); + } + } + """); + + @DocumentExample + @Test + void shouldReplaceExceptionWithSingleCheckedException() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.io.IOException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + } catch (IOException e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldReplaceExceptionWithMultipleCheckedExceptions() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + new MockThrowingClass().throwsSQLException(); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + new MockThrowingClass().throwsSQLException(); + } catch (IOException | SQLException e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldReplaceExceptionWithMultipleCheckedExceptionsFromSingleCall() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException | SQLException e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldKeepSpecificCatchAndRemoveGenericWhenCovered() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException e) { + System.out.println("Caught IO: " + e.getMessage()); + } catch (Exception e) { + System.out.println("Caught generic: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException e) { + System.out.println("Caught IO: " + e.getMessage()); + } catch (SQLException e) { + System.out.println("Caught generic: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldNotModifyWhenOnlySpecificCatchesExist() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + import java.io.IOException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + } catch (IOException e) { + System.out.println("Caught IO: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldNotModifyWhenNoCheckedExceptionIsThrown() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass(); // Nothing thrown + } catch (Exception e) { + System.out.println("Caught something unusual: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldHandleConstructorThrowingException() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + public class MyService { + public void createObject() { + try { + new MockThrowingClass(true); // Throws SQLException + } catch (Exception e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.sql.SQLException; + + public class MyService { + public void createObject() { + try { + new MockThrowingClass(true); // Throws SQLException + } catch (SQLException e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldHandleConstructorThrowingSpecificExceptionAndAnotherGenericCatch() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + import java.io.IOException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (Exception e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (IOException | SQLException e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } + } + } + """)); + } + + @Test + void shouldHandleConstructorThrowingExceptionWithExistingSpecificCatch() { + rewriteRun(MOCK_THROWING_CLASS, java(""" + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (SQLException e) { + System.out.println("Caught sql exception: " + e.getMessage()); + } catch (Exception e) { + System.out.println("Caught generic: " + e.getMessage()); + } + } + } + """, """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (SQLException e) { + System.out.println("Caught sql exception: " + e.getMessage()); + } catch (IOException e) { + System.out.println("Caught generic: " + e.getMessage()); + } + } + } + """)); + } +} From e61f000b672c31fe787f0562ec4ff6817ded2e03 Mon Sep 17 00:00:00 2001 From: Stefano Dalla Palma Date: Sat, 21 Jun 2025 23:48:09 +0200 Subject: [PATCH 02/17] Update src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../SpecifyGenericExceptionCatches.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java index f00afbfea5..2fdfe9a5f8 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java +++ b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java @@ -1,3 +1,18 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.staticanalysis; import org.openrewrite.Cursor; From 3d18c3725c561f8f3a75f3c2c1f9d05ed44c09c2 Mon Sep 17 00:00:00 2001 From: Stefano Dalla Palma Date: Sat, 21 Jun 2025 23:48:18 +0200 Subject: [PATCH 03/17] Update src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../SpecifyGenericExceptionCatchesTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index 88a7cc6af6..0e2acaa5b9 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -1,3 +1,18 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.staticanalysis; import org.junit.jupiter.api.Test; From 00e9021ec0efed0143b21037d6abee41909b5963 Mon Sep 17 00:00:00 2001 From: Stefano Dalla Palma Date: Sat, 21 Jun 2025 23:50:50 +0200 Subject: [PATCH 04/17] Apply suggestions from code review Applied suggestions Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../SpecifyGenericExceptionCatchesTest.java | 71 ++++++++++++------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index 0e2acaa5b9..7b03749112 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -31,7 +31,8 @@ public void defaults(RecipeSpec spec) { } // Helper SourceSpecs for mock classes that throw checked exceptions - private static final SourceSpecs MOCK_THROWING_CLASS = java(""" + private static final SourceSpecs MOCK_THROWING_CLASS = java( + """ package com.example; public class MockThrowingClass { @@ -65,8 +66,9 @@ public void throwsRuntimeException() throws RuntimeException { throw new RuntimeException("Test Runtime Exception"); } } - """); - + """ ); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @DocumentExample @Test void shouldReplaceExceptionWithSingleCheckedException() { @@ -82,7 +84,8 @@ public void doSomething() { } } } - """, """ + """, + """ package com.example; import java.io.IOException; @@ -96,8 +99,9 @@ public void doSomething() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldReplaceExceptionWithMultipleCheckedExceptions() { @@ -114,7 +118,8 @@ public void doSomething() { } } } - """, """ + """, + """ package com.example; import java.io.IOException; @@ -130,8 +135,9 @@ public void doSomething() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldReplaceExceptionWithMultipleCheckedExceptionsFromSingleCall() { @@ -147,7 +153,8 @@ public void doSomething() { } } } - """, """ + """, + """ package com.example; import java.io.IOException; @@ -162,8 +169,9 @@ public void doSomething() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldKeepSpecificCatchAndRemoveGenericWhenCovered() { @@ -184,7 +192,8 @@ public void doSomething() { } } } - """, """ + """, + """ package com.example; import java.io.IOException; @@ -201,8 +210,9 @@ public void doSomething() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldNotModifyWhenOnlySpecificCatchesExist() { @@ -220,8 +230,9 @@ public void doSomething() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldNotModifyWhenNoCheckedExceptionIsThrown() { @@ -237,8 +248,9 @@ public void doSomething() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldHandleConstructorThrowingException() { @@ -254,7 +266,8 @@ public void createObject() { } } } - """, """ + """, + """ package com.example; import java.sql.SQLException; @@ -268,8 +281,9 @@ public void createObject() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldHandleConstructorThrowingSpecificExceptionAndAnotherGenericCatch() { @@ -288,7 +302,8 @@ public void createAndHandle() { } } } - """, """ + """, + """ package com.example; import java.io.IOException; @@ -304,8 +319,9 @@ public void createAndHandle() { } } } - """)); - } + """ )); + rewriteRun(MOCK_THROWING_CLASS, java( + """ @Test void shouldHandleConstructorThrowingExceptionWithExistingSpecificCatch() { @@ -327,7 +343,8 @@ public void createAndHandle() { } } } - """, """ + """, + """ package com.example; import java.io.IOException; @@ -345,6 +362,6 @@ public void createAndHandle() { } } } - """)); + """ )); } } From 0a7c667bfad3029e9451b741aca677b806a4c8e2 Mon Sep 17 00:00:00 2001 From: Stefano Dalla Palma Date: Sat, 21 Jun 2025 22:03:49 +0000 Subject: [PATCH 05/17] Fixed conflict introduced by applying batch suggestions --- .../SpecifyGenericExceptionCatchesTest.java | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index 7b03749112..f02384a91c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -67,8 +67,7 @@ public void throwsRuntimeException() throws RuntimeException { } } """ ); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + @DocumentExample @Test void shouldReplaceExceptionWithSingleCheckedException() { @@ -100,8 +99,7 @@ public void doSomething() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldReplaceExceptionWithMultipleCheckedExceptions() { @@ -136,8 +134,7 @@ public void doSomething() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldReplaceExceptionWithMultipleCheckedExceptionsFromSingleCall() { @@ -170,8 +167,7 @@ public void doSomething() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldKeepSpecificCatchAndRemoveGenericWhenCovered() { @@ -211,8 +207,7 @@ public void doSomething() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldNotModifyWhenOnlySpecificCatchesExist() { @@ -231,8 +226,7 @@ public void doSomething() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldNotModifyWhenNoCheckedExceptionIsThrown() { @@ -249,8 +243,7 @@ public void doSomething() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldHandleConstructorThrowingException() { @@ -282,8 +275,7 @@ public void createObject() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldHandleConstructorThrowingSpecificExceptionAndAnotherGenericCatch() { @@ -320,8 +312,7 @@ public void createAndHandle() { } } """ )); - rewriteRun(MOCK_THROWING_CLASS, java( - """ + } @Test void shouldHandleConstructorThrowingExceptionWithExistingSpecificCatch() { From b45b78629732b5f1bc4e1e63df9b2b14a74ef6da Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 24 Jun 2025 14:21:23 +0200 Subject: [PATCH 06/17] Apply formatter --- .../SpecifyGenericExceptionCatchesTest.java | 543 ++++++++++-------- 1 file changed, 290 insertions(+), 253 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index f02384a91c..dd04be6a1e 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -17,9 +17,9 @@ import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; +import org.openrewrite.java.JavaParser; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; -import org.openrewrite.test.SourceSpecs; import static org.openrewrite.java.Assertions.java; @@ -27,332 +27,369 @@ class SpecifyGenericExceptionCatchesTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new SpecifyGenericExceptionCatches()); + spec.recipe(new SpecifyGenericExceptionCatches()) + .parser(JavaParser.fromJavaVersion().dependsOn( + //language=java + """ + package com.example; + + public class MockThrowingClass { + + public MockThrowingClass() { + } + + public MockThrowingClass(boolean throwSQLException) throws java.sql.SQLException { + if (throwSQLException) { + throw new java.sql.SQLException("Test Constructor SQL Exception"); + } + } + + public void throwsIOException() throws java.io.IOException { + throw new java.io.IOException("Test IO Exception"); + } + + public void throwsSQLException() throws java.sql.SQLException { + throw new java.sql.SQLException("Test SQL Exception"); + } + + public void throwsBothIOAndSQL() throws java.io.IOException, java.sql.SQLException { + throw new java.io.IOException("Test IO or SQL Exception"); + } + + public void throwsNothing() { + // This method throws no checked exceptions + } + + public void throwsRuntimeException() throws RuntimeException { + throw new RuntimeException("Test Runtime Exception"); + } + } + """ + ) + ); } - // Helper SourceSpecs for mock classes that throw checked exceptions - private static final SourceSpecs MOCK_THROWING_CLASS = java( - """ - package com.example; - - public class MockThrowingClass { - - public MockThrowingClass() { - } - - public MockThrowingClass(boolean throwSQLException) throws java.sql.SQLException { - if (throwSQLException) { - throw new java.sql.SQLException("Test Constructor SQL Exception"); - } - } - - public void throwsIOException() throws java.io.IOException { - throw new java.io.IOException("Test IO Exception"); - } - - public void throwsSQLException() throws java.sql.SQLException { - throw new java.sql.SQLException("Test SQL Exception"); - } - - public void throwsBothIOAndSQL() throws java.io.IOException, java.sql.SQLException { - throw new java.io.IOException("Test IO or SQL Exception"); - } - - public void throwsNothing() { - // This method throws no checked exceptions - } - - public void throwsRuntimeException() throws RuntimeException { - throw new RuntimeException("Test Runtime Exception"); - } - } - """ ); - @DocumentExample @Test void shouldReplaceExceptionWithSingleCheckedException() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsIOException(); - } catch (Exception e) { - System.out.println("Caught exception: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } } } - } - """, - """ - package com.example; + """, + """ + package com.example; - import java.io.IOException; + import java.io.IOException; - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsIOException(); - } catch (IOException e) { - System.out.println("Caught exception: " + e.getMessage()); + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + } catch (IOException e) { + System.out.println("Caught exception: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldReplaceExceptionWithMultipleCheckedExceptions() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsIOException(); - new MockThrowingClass().throwsSQLException(); - } catch (Exception e) { - System.out.println("Caught exception: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + new MockThrowingClass().throwsSQLException(); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } } } - } - """, - """ - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsIOException(); - new MockThrowingClass().throwsSQLException(); - } catch (IOException | SQLException e) { - System.out.println("Caught exception: " + e.getMessage()); + """, + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + new MockThrowingClass().throwsSQLException(); + } catch (IOException | SQLException e) { + System.out.println("Caught exception: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldReplaceExceptionWithMultipleCheckedExceptionsFromSingleCall() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsBothIOAndSQL(); - } catch (Exception e) { - System.out.println("Caught exception: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } } } - } - """, - """ - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsBothIOAndSQL(); - } catch (IOException | SQLException e) { - System.out.println("Caught exception: " + e.getMessage()); + """, + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException | SQLException e) { + System.out.println("Caught exception: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldKeepSpecificCatchAndRemoveGenericWhenCovered() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsBothIOAndSQL(); - } catch (IOException e) { - System.out.println("Caught IO: " + e.getMessage()); - } catch (Exception e) { - System.out.println("Caught generic: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException e) { + System.out.println("Caught IO: " + e.getMessage()); + } catch (Exception e) { + System.out.println("Caught generic: " + e.getMessage()); + } } } - } - """, - """ - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsBothIOAndSQL(); - } catch (IOException e) { - System.out.println("Caught IO: " + e.getMessage()); - } catch (SQLException e) { - System.out.println("Caught generic: " + e.getMessage()); + """, + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException e) { + System.out.println("Caught IO: " + e.getMessage()); + } catch (SQLException e) { + System.out.println("Caught generic: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldNotModifyWhenOnlySpecificCatchesExist() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; + rewriteRun( + java( + """ + package com.example; - import java.io.IOException; + import java.io.IOException; - public class MyService { - public void doSomething() { - try { - new MockThrowingClass().throwsIOException(); - } catch (IOException e) { - System.out.println("Caught IO: " + e.getMessage()); + public class MyService { + public void doSomething() { + try { + new MockThrowingClass().throwsIOException(); + } catch (IOException e) { + System.out.println("Caught IO: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldNotModifyWhenNoCheckedExceptionIsThrown() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - public class MyService { - public void doSomething() { - try { - new MockThrowingClass(); // Nothing thrown - } catch (Exception e) { - System.out.println("Caught something unusual: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + public class MyService { + public void doSomething() { + try { + new MockThrowingClass(); // Nothing thrown + } catch (Exception e) { + System.out.println("Caught something unusual: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldHandleConstructorThrowingException() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - public class MyService { - public void createObject() { - try { - new MockThrowingClass(true); // Throws SQLException - } catch (Exception e) { - System.out.println("Caught constructor exception: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + public class MyService { + public void createObject() { + try { + new MockThrowingClass(true); // Throws SQLException + } catch (Exception e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } } } - } - """, - """ - package com.example; + """, + """ + package com.example; - import java.sql.SQLException; + import java.sql.SQLException; - public class MyService { - public void createObject() { - try { - new MockThrowingClass(true); // Throws SQLException - } catch (SQLException e) { - System.out.println("Caught constructor exception: " + e.getMessage()); + public class MyService { + public void createObject() { + try { + new MockThrowingClass(true); // Throws SQLException + } catch (SQLException e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldHandleConstructorThrowingSpecificExceptionAndAnotherGenericCatch() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - import java.io.IOException; - - public class MyService { - public void createAndHandle() { - try { - new MockThrowingClass(true); // Throws SQLException - new MockThrowingClass().throwsIOException(); - } catch (Exception e) { - System.out.println("Caught constructor exception: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + import java.io.IOException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (Exception e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } } } - } - """, - """ - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void createAndHandle() { - try { - new MockThrowingClass(true); // Throws SQLException - new MockThrowingClass().throwsIOException(); - } catch (IOException | SQLException e) { - System.out.println("Caught constructor exception: " + e.getMessage()); + """, + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (IOException | SQLException e) { + System.out.println("Caught constructor exception: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } @Test void shouldHandleConstructorThrowingExceptionWithExistingSpecificCatch() { - rewriteRun(MOCK_THROWING_CLASS, java(""" - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void createAndHandle() { - try { - new MockThrowingClass(true); // Throws SQLException - new MockThrowingClass().throwsIOException(); - } catch (SQLException e) { - System.out.println("Caught sql exception: " + e.getMessage()); - } catch (Exception e) { - System.out.println("Caught generic: " + e.getMessage()); + rewriteRun( + java( + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (SQLException e) { + System.out.println("Caught sql exception: " + e.getMessage()); + } catch (Exception e) { + System.out.println("Caught generic: " + e.getMessage()); + } } } - } - """, - """ - package com.example; - - import java.io.IOException; - import java.sql.SQLException; - - public class MyService { - public void createAndHandle() { - try { - new MockThrowingClass(true); // Throws SQLException - new MockThrowingClass().throwsIOException(); - } catch (SQLException e) { - System.out.println("Caught sql exception: " + e.getMessage()); - } catch (IOException e) { - System.out.println("Caught generic: " + e.getMessage()); + """, + """ + package com.example; + + import java.io.IOException; + import java.sql.SQLException; + + public class MyService { + public void createAndHandle() { + try { + new MockThrowingClass(true); // Throws SQLException + new MockThrowingClass().throwsIOException(); + } catch (SQLException e) { + System.out.println("Caught sql exception: " + e.getMessage()); + } catch (IOException e) { + System.out.println("Caught generic: " + e.getMessage()); + } } } - } - """ )); + """ + ) + ); } } From 3bdf1ac9367657d98dfb8eefa5e388b601ff0423 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 24 Jun 2025 14:24:51 +0200 Subject: [PATCH 07/17] No need for a named internal class --- .../SpecifyGenericExceptionCatches.java | 246 +++++++++--------- 1 file changed, 122 insertions(+), 124 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java index 2fdfe9a5f8..c7f5765391 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java +++ b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java @@ -54,146 +54,144 @@ public Set getTags() { @Override public JavaVisitor getVisitor() { - return new GenericExceptionCatchVisitor(); - } - - private static class GenericExceptionCatchVisitor extends JavaIsoVisitor { - - private static final String JAVA_LANG_EXCEPTION = "java.lang.Exception"; - private static final String MULTI_CATCH_SEPARATOR = "|"; - private static final String TRY_CATCH_TEMPLATE = "try {} catch (%s %s) {}"; - - @Override - public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { - J.Try t = super.visitTry(aTry, ctx); - - if (hasGenericCatch(t)) { - Set caughtExceptions = getCaughtExceptions(t); - Set thrownExceptions = getThrownExceptions(t); - thrownExceptions.removeAll(caughtExceptions); // Remove exceptions that are already specifically caught - - if (!thrownExceptions.isEmpty()) { - List updatedCatches = t.getCatches().stream() - .map(c -> updateCatchIfGeneric(c, thrownExceptions)) - .collect(Collectors.toList()); - - return t.withCatches(updatedCatches); + return new JavaIsoVisitor() { + private static final String JAVA_LANG_EXCEPTION = "java.lang.Exception"; + private static final String MULTI_CATCH_SEPARATOR = "|"; + private static final String TRY_CATCH_TEMPLATE = "try {} catch (%s %s) {}"; + + @Override + public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { + J.Try t = super.visitTry(aTry, ctx); + + if (hasGenericCatch(t)) { + Set caughtExceptions = getCaughtExceptions(t); + Set thrownExceptions = getThrownExceptions(t); + thrownExceptions.removeAll(caughtExceptions); // Remove exceptions that are already specifically caught + + if (!thrownExceptions.isEmpty()) { + List updatedCatches = t.getCatches().stream() + .map(c -> updateCatchIfGeneric(c, thrownExceptions)) + .collect(Collectors.toList()); + + return t.withCatches(updatedCatches); + } } - } - return t; - } + return t; + } - private static boolean isGenericCatch(J.Try.Catch aCatch) { - FullyQualified fq = TypeUtils.asFullyQualified(aCatch.getParameter().getType()); - if (fq != null) { - String fqn = fq.getFullyQualifiedName(); - return TypeUtils.fullyQualifiedNamesAreEqual(JAVA_LANG_EXCEPTION, fqn); + private boolean isGenericCatch(J.Try.Catch aCatch) { + FullyQualified fq = TypeUtils.asFullyQualified(aCatch.getParameter().getType()); + if (fq != null) { + String fqn = fq.getFullyQualifiedName(); + return TypeUtils.fullyQualifiedNamesAreEqual(JAVA_LANG_EXCEPTION, fqn); + } + return false; } - return false; - } - private boolean hasGenericCatch(J.Try aTry) { - for (J.Try.Catch c : aTry.getCatches()) { - if (isGenericCatch(c)) { - return true; + private boolean hasGenericCatch(J.Try aTry) { + for (J.Try.Catch c : aTry.getCatches()) { + if (isGenericCatch(c)) { + return true; + } } + return false; } - return false; - } - private Set getCaughtExceptions(J.Try aTry) { - Set caughtExceptions = new HashSet<>(); + private Set getCaughtExceptions(J.Try aTry) { + Set caughtExceptions = new HashSet<>(); - for (J.Try.Catch c : aTry.getCatches()) { - if (c.getParameter().getType() != null) { - caughtExceptions.add(c.getParameter().getType()); + for (J.Try.Catch c : aTry.getCatches()) { + if (c.getParameter().getType() != null) { + caughtExceptions.add(c.getParameter().getType()); + } } + + return caughtExceptions; } - return caughtExceptions; - } - - /** - * Collects all checked exceptions that are explicitly thrown by method invocations - * and constructor calls within the try block. - * - * @param aTry the try block to analyze - * @return a set of exception types that may be thrown by code in the try block - */ - private Set getThrownExceptions(J.Try aTry) { - Set thrownExceptions = new HashSet<>(); - new JavaIsoVisitor>() { - @Override - public J.NewClass visitNewClass(J.NewClass nc, Set set) { - if (nc.getConstructorType() != null) { - set.addAll(nc.getConstructorType().getThrownExceptions()); + /** + * Collects all checked exceptions that are explicitly thrown by method invocations + * and constructor calls within the try block. + * + * @param aTry the try block to analyze + * @return a set of exception types that may be thrown by code in the try block + */ + private Set getThrownExceptions(J.Try aTry) { + Set thrownExceptions = new HashSet<>(); + new JavaIsoVisitor>() { + @Override + public J.NewClass visitNewClass(J.NewClass nc, Set set) { + if (nc.getConstructorType() != null) { + set.addAll(nc.getConstructorType().getThrownExceptions()); + } + return super.visitNewClass(nc, set); } - return super.visitNewClass(nc, set); - } - @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Set set) { - if (mi.getMethodType() != null) { - set.addAll(mi.getMethodType().getThrownExceptions()); + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Set set) { + if (mi.getMethodType() != null) { + set.addAll(mi.getMethodType().getThrownExceptions()); + } + return super.visitMethodInvocation(mi, set); } - return super.visitMethodInvocation(mi, set); - } - }.visit(aTry.getBody(), thrownExceptions); - return thrownExceptions; - } - - /** - * Updates a generic catch block (catching java.lang.Exception) to catch only the specific - * exception types that are actually thrown within the try block. - * - *

This method transforms generic catch blocks like: - * 

{@code
-         * catch (Exception e) { ... }
-         * }
- * - * into specific single or multi-catch blocks like: - * 
{@code
-         * catch (IOException e) { ... }
-         * // or
-         * catch (IOException | SQLException e) { ... }
-         * }
- * - * @param aCatch the catch block to potentially update - * @param thrownExceptions the set of specific exception types thrown in the try block - * that are not already caught by other catch clauses - * @return the original catch block if it doesn't catch java.lang.Exception, - * otherwise a new catch block with specific exception types - */ - private J.Try.Catch updateCatchIfGeneric(J.Try.Catch aCatch, Set thrownExceptions) { - if (!isGenericCatch(aCatch)) { - return aCatch; + }.visit(aTry.getBody(), thrownExceptions); + return thrownExceptions; } - // Preserve the existing variable name from the original generic catch block - String variableName = aCatch.getParameter().getTree().getVariables().get(0).getSimpleName(); - - String throwableTypes = thrownExceptions.stream() - .map(TypeUtils::asFullyQualified) - .filter(Objects::nonNull) - .sorted(Comparator.comparing(FullyQualified::getClassName)) - .map(FullyQualified::getFullyQualifiedName) - .collect(Collectors.joining(MULTI_CATCH_SEPARATOR)); - - J.Try aTry = getCursor().firstEnclosing(J.Try.class); - assert aTry != null; - - J.Try tempTry = JavaTemplate.builder(String.format(TRY_CATCH_TEMPLATE, throwableTypes, variableName)) - .contextSensitive() - .build() - .apply( - new Cursor(getCursor(), aTry), - aTry.getCoordinates().replace() - ); - - J.ControlParentheses cp = tempTry.getCatches().get(0).getParameter(); - doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(cp.getTree())); - return aCatch.withParameter(cp); - } + /** + * Updates a generic catch block (catching java.lang.Exception) to catch only the specific + * exception types that are actually thrown within the try block. + * + *

This method transforms generic catch blocks like: + * 

{@code
+             * catch (Exception e) { ... }
+             * }
+ * + * into specific single or multi-catch blocks like: + * 
{@code
+             * catch (IOException e) { ... }
+             * // or
+             * catch (IOException | SQLException e) { ... }
+             * }
+ * + * @param aCatch the catch block to potentially update + * @param thrownExceptions the set of specific exception types thrown in the try block + * that are not already caught by other catch clauses + * @return the original catch block if it doesn't catch java.lang.Exception, + * otherwise a new catch block with specific exception types + */ + private J.Try.Catch updateCatchIfGeneric(J.Try.Catch aCatch, Set thrownExceptions) { + if (!isGenericCatch(aCatch)) { + return aCatch; + } + + // Preserve the existing variable name from the original generic catch block + String variableName = aCatch.getParameter().getTree().getVariables().get(0).getSimpleName(); + + String throwableTypes = thrownExceptions.stream() + .map(TypeUtils::asFullyQualified) + .filter(Objects::nonNull) + .sorted(Comparator.comparing(FullyQualified::getClassName)) + .map(FullyQualified::getFullyQualifiedName) + .collect(Collectors.joining(MULTI_CATCH_SEPARATOR)); + + J.Try aTry = getCursor().firstEnclosing(J.Try.class); + assert aTry != null; + + J.Try tempTry = JavaTemplate.builder(String.format(TRY_CATCH_TEMPLATE, throwableTypes, variableName)) + .contextSensitive() + .build() + .apply( + new Cursor(getCursor(), aTry), + aTry.getCoordinates().replace() + ); + + J.ControlParentheses cp = tempTry.getCatches().get(0).getParameter(); + doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(cp.getTree())); + return aCatch.withParameter(cp); + } + }; } + } From f393b2052eddc0cfb5bf2e0a3df5f56d6f38804a Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 18 Jul 2025 12:58:41 +0200 Subject: [PATCH 08/17] Show difference between declared and undeclared runtime exceptions --- .../SpecifyGenericExceptionCatchesTest.java | 168 ++++++++++-------- 1 file changed, 95 insertions(+), 73 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index dd04be6a1e..a6def548df 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -31,36 +31,38 @@ public void defaults(RecipeSpec spec) { .parser(JavaParser.fromJavaVersion().dependsOn( //language=java """ - package com.example; - public class MockThrowingClass { - public MockThrowingClass() { + MockThrowingClass() { } - public MockThrowingClass(boolean throwSQLException) throws java.sql.SQLException { + MockThrowingClass(boolean throwSQLException) throws java.sql.SQLException { if (throwSQLException) { throw new java.sql.SQLException("Test Constructor SQL Exception"); } } - public void throwsIOException() throws java.io.IOException { + void throwsIOException() throws java.io.IOException { throw new java.io.IOException("Test IO Exception"); } - public void throwsSQLException() throws java.sql.SQLException { + void throwsSQLException() throws java.sql.SQLException { throw new java.sql.SQLException("Test SQL Exception"); } - public void throwsBothIOAndSQL() throws java.io.IOException, java.sql.SQLException { + void throwsBothIOAndSQL() throws java.io.IOException, java.sql.SQLException { throw new java.io.IOException("Test IO or SQL Exception"); } - public void throwsNothing() { + void throwsNothing() { // This method throws no checked exceptions } - public void throwsRuntimeException() throws RuntimeException { + void throwsDeclaredRuntimeException() throws RuntimeException { + throw new RuntimeException("Test Runtime Exception"); + } + + void throwsUndeclaredRuntimeException() { throw new RuntimeException("Test Runtime Exception"); } } @@ -75,10 +77,8 @@ void shouldReplaceExceptionWithSingleCheckedException() { rewriteRun( java( """ - package com.example; - - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsIOException(); } catch (Exception e) { @@ -88,12 +88,10 @@ public void doSomething() { } """, """ - package com.example; - import java.io.IOException; - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsIOException(); } catch (IOException e) { @@ -111,10 +109,8 @@ void shouldReplaceExceptionWithMultipleCheckedExceptions() { rewriteRun( java( """ - package com.example; - - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsIOException(); new MockThrowingClass().throwsSQLException(); @@ -125,13 +121,11 @@ public void doSomething() { } """, """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsIOException(); new MockThrowingClass().throwsSQLException(); @@ -150,10 +144,8 @@ void shouldReplaceExceptionWithMultipleCheckedExceptionsFromSingleCall() { rewriteRun( java( """ - package com.example; - - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsBothIOAndSQL(); } catch (Exception e) { @@ -163,13 +155,11 @@ public void doSomething() { } """, """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsBothIOAndSQL(); } catch (IOException | SQLException e) { @@ -187,13 +177,11 @@ void shouldKeepSpecificCatchAndRemoveGenericWhenCovered() { rewriteRun( java( """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsBothIOAndSQL(); } catch (IOException e) { @@ -205,13 +193,11 @@ public void doSomething() { } """, """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsBothIOAndSQL(); } catch (IOException e) { @@ -231,12 +217,10 @@ void shouldNotModifyWhenOnlySpecificCatchesExist() { rewriteRun( java( """ - package com.example; - import java.io.IOException; - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass().throwsIOException(); } catch (IOException e) { @@ -254,10 +238,8 @@ void shouldNotModifyWhenNoCheckedExceptionIsThrown() { rewriteRun( java( """ - package com.example; - - public class MyService { - public void doSomething() { + class MyService { + void doSomething() { try { new MockThrowingClass(); // Nothing thrown } catch (Exception e) { @@ -275,10 +257,8 @@ void shouldHandleConstructorThrowingException() { rewriteRun( java( """ - package com.example; - - public class MyService { - public void createObject() { + class MyService { + void createObject() { try { new MockThrowingClass(true); // Throws SQLException } catch (Exception e) { @@ -288,12 +268,10 @@ public void createObject() { } """, """ - package com.example; - import java.sql.SQLException; - public class MyService { - public void createObject() { + class MyService { + void createObject() { try { new MockThrowingClass(true); // Throws SQLException } catch (SQLException e) { @@ -311,12 +289,10 @@ void shouldHandleConstructorThrowingSpecificExceptionAndAnotherGenericCatch() { rewriteRun( java( """ - package com.example; - import java.io.IOException; - public class MyService { - public void createAndHandle() { + class MyService { + void createAndHandle() { try { new MockThrowingClass(true); // Throws SQLException new MockThrowingClass().throwsIOException(); @@ -327,13 +303,11 @@ public void createAndHandle() { } """, """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void createAndHandle() { + class MyService { + void createAndHandle() { try { new MockThrowingClass(true); // Throws SQLException new MockThrowingClass().throwsIOException(); @@ -352,13 +326,11 @@ void shouldHandleConstructorThrowingExceptionWithExistingSpecificCatch() { rewriteRun( java( """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void createAndHandle() { + class MyService { + void createAndHandle() { try { new MockThrowingClass(true); // Throws SQLException new MockThrowingClass().throwsIOException(); @@ -371,13 +343,11 @@ public void createAndHandle() { } """, """ - package com.example; - import java.io.IOException; import java.sql.SQLException; - public class MyService { - public void createAndHandle() { + class MyService { + void createAndHandle() { try { new MockThrowingClass(true); // Throws SQLException new MockThrowingClass().throwsIOException(); @@ -392,4 +362,56 @@ public void createAndHandle() { ) ); } + + @Test + void declaredRuntimeException() { + rewriteRun( + java( + """ + class MyService { + void doSomething() { + try { + new MockThrowingClass().throwsDeclaredRuntimeException(); + } catch (Exception e) { + // This is a generic catch block that should be replaced + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """, + """ + class MyService { + void doSomething() { + try { + new MockThrowingClass().throwsDeclaredRuntimeException(); + } catch (java.lang.RuntimeException e) { + // This is a generic catch block that should be replaced + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """ + ) + ); + } + + @Test + void undeclaredRuntimeException() { + rewriteRun( + java( + """ + class MyService { + void doSomething() { + try { + new MockThrowingClass().throwsUndeclaredRuntimeException(); + } catch (Exception e) { + // This is a generic catch block that should be replaced + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """ + ) + ); + } } From 6614563b150ed2d485c39073d218ba447631a570 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 18 Jul 2025 13:01:17 +0200 Subject: [PATCH 09/17] Use `reduce` --- .../staticanalysis/SpecifyGenericExceptionCatches.java | 6 ++---- .../staticanalysis/SpecifyGenericExceptionCatchesTest.java | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java index c7f5765391..924d9f86b7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java +++ b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java @@ -118,8 +118,7 @@ private Set getCaughtExceptions(J.Try aTry) { * @return a set of exception types that may be thrown by code in the try block */ private Set getThrownExceptions(J.Try aTry) { - Set thrownExceptions = new HashSet<>(); - new JavaIsoVisitor>() { + return new JavaIsoVisitor>() { @Override public J.NewClass visitNewClass(J.NewClass nc, Set set) { if (nc.getConstructorType() != null) { @@ -135,8 +134,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Set()); } /** diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index a6def548df..d2eaa96aaa 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -31,7 +31,7 @@ public void defaults(RecipeSpec spec) { .parser(JavaParser.fromJavaVersion().dependsOn( //language=java """ - public class MockThrowingClass { + class MockThrowingClass { MockThrowingClass() { } From cde05edd28529a8c12d0dc0750f9820fd9f9aa76 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 18 Jul 2025 17:53:09 +0200 Subject: [PATCH 10/17] Use `ListUtils.map` --- .../SpecifyGenericExceptionCatches.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java index 924d9f86b7..dba06b9f04 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java +++ b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java @@ -18,6 +18,7 @@ import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; @@ -65,15 +66,12 @@ public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { if (hasGenericCatch(t)) { Set caughtExceptions = getCaughtExceptions(t); - Set thrownExceptions = getThrownExceptions(t); + Set thrownExceptions = getDeclaredThrownExceptions(t); thrownExceptions.removeAll(caughtExceptions); // Remove exceptions that are already specifically caught if (!thrownExceptions.isEmpty()) { - List updatedCatches = t.getCatches().stream() - .map(c -> updateCatchIfGeneric(c, thrownExceptions)) - .collect(Collectors.toList()); - - return t.withCatches(updatedCatches); + return t.withCatches(ListUtils.map(t.getCatches(), c -> + updateCatchIfGeneric(c, thrownExceptions))); } } @@ -100,13 +98,11 @@ private boolean hasGenericCatch(J.Try aTry) { private Set getCaughtExceptions(J.Try aTry) { Set caughtExceptions = new HashSet<>(); - for (J.Try.Catch c : aTry.getCatches()) { if (c.getParameter().getType() != null) { caughtExceptions.add(c.getParameter().getType()); } } - return caughtExceptions; } @@ -117,7 +113,7 @@ private Set getCaughtExceptions(J.Try aTry) { * @param aTry the try block to analyze * @return a set of exception types that may be thrown by code in the try block */ - private Set getThrownExceptions(J.Try aTry) { + private Set getDeclaredThrownExceptions(J.Try aTry) { return new JavaIsoVisitor>() { @Override public J.NewClass visitNewClass(J.NewClass nc, Set set) { From 7d4cfe211a029053a4eedad066df529bcdf65fe9 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 11:46:38 +0200 Subject: [PATCH 11/17] Handle existing multi catch that's incomplete --- .../SpecifyGenericExceptionCatches.java | 65 ++++++++----------- .../SpecifyGenericExceptionCatchesTest.java | 39 +++++++++++ 2 files changed, 65 insertions(+), 39 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java index dba06b9f04..e1fb962341 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java +++ b/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java @@ -15,6 +15,7 @@ */ package org.openrewrite.staticanalysis; +import org.jspecify.annotations.Nullable; import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; @@ -36,21 +37,20 @@ public class SpecifyGenericExceptionCatches extends Recipe { @Override public String getDisplayName() { - return "Replace `catch(Exception)` with specific exceptions thrown in the try block"; + return "Replace `catch(Exception)` with specific declared exceptions thrown in the try block"; } @Override public String getDescription() { return "Replaces `catch(Exception e)` blocks with a multi-catch block " + - "(`catch (SpecificException1 | SpecificException2 e)`) containing only the checked exceptions " + - "explicitly thrown by method or constructor invocations within the `try` block that are not " + - "already caught by more specific `catch` clauses. If no checked exceptions are found that " + - "require catching, the generic `catch` block is removed."; + "(`catch (SpecificException1 | SpecificException2 e)`) containing only the exceptions declared " + + "thrown by method or constructor invocations within the `try` block that are not already caught " + + "by more specific `catch` clauses."; } @Override public Set getTags() { - return Collections.unmodifiableSet(new HashSet<>(Arrays.asList("CWE-396", "RSPEC-S2221"))); + return new HashSet<>(Arrays.asList("CWE-396", "RSPEC-S2221")); } @Override @@ -65,28 +65,18 @@ public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { J.Try t = super.visitTry(aTry, ctx); if (hasGenericCatch(t)) { - Set caughtExceptions = getCaughtExceptions(t); Set thrownExceptions = getDeclaredThrownExceptions(t); - thrownExceptions.removeAll(caughtExceptions); // Remove exceptions that are already specifically caught + thrownExceptions.removeAll(getCaughtExceptions(t)); // Remove exceptions that are already specifically caught if (!thrownExceptions.isEmpty()) { return t.withCatches(ListUtils.map(t.getCatches(), c -> - updateCatchIfGeneric(c, thrownExceptions))); + updateCatchIfGeneric(c, thrownExceptions))); } } return t; } - private boolean isGenericCatch(J.Try.Catch aCatch) { - FullyQualified fq = TypeUtils.asFullyQualified(aCatch.getParameter().getType()); - if (fq != null) { - String fqn = fq.getFullyQualifiedName(); - return TypeUtils.fullyQualifiedNamesAreEqual(JAVA_LANG_EXCEPTION, fqn); - } - return false; - } - private boolean hasGenericCatch(J.Try aTry) { for (J.Try.Catch c : aTry.getCatches()) { if (isGenericCatch(c)) { @@ -96,39 +86,36 @@ private boolean hasGenericCatch(J.Try aTry) { return false; } + private boolean isGenericCatch(J.Try.Catch aCatch) { + FullyQualified fq = TypeUtils.asFullyQualified(aCatch.getParameter().getType()); + if (fq != null) { + String fqn = fq.getFullyQualifiedName(); + return TypeUtils.fullyQualifiedNamesAreEqual(JAVA_LANG_EXCEPTION, fqn); + } + return false; + } + private Set getCaughtExceptions(J.Try aTry) { Set caughtExceptions = new HashSet<>(); for (J.Try.Catch c : aTry.getCatches()) { - if (c.getParameter().getType() != null) { - caughtExceptions.add(c.getParameter().getType()); + JavaType type = c.getParameter().getType(); + if (type instanceof JavaType.MultiCatch) { + caughtExceptions.addAll(((JavaType.MultiCatch) type).getThrowableTypes()); + } else if (type != null) { + caughtExceptions.add(type); } } return caughtExceptions; } - /** - * Collects all checked exceptions that are explicitly thrown by method invocations - * and constructor calls within the try block. - * - * @param aTry the try block to analyze - * @return a set of exception types that may be thrown by code in the try block - */ private Set getDeclaredThrownExceptions(J.Try aTry) { return new JavaIsoVisitor>() { @Override - public J.NewClass visitNewClass(J.NewClass nc, Set set) { - if (nc.getConstructorType() != null) { - set.addAll(nc.getConstructorType().getThrownExceptions()); - } - return super.visitNewClass(nc, set); - } - - @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Set set) { - if (mi.getMethodType() != null) { - set.addAll(mi.getMethodType().getThrownExceptions()); + public @Nullable JavaType visitType(@Nullable JavaType javaType, Set javaTypes) { + if (javaType instanceof JavaType.Method) { + javaTypes.addAll(((JavaType.Method) javaType).getThrownExceptions()); } - return super.visitMethodInvocation(mi, set); + return super.visitType(javaType, javaTypes); } }.reduce(aTry.getBody(), new HashSet<>()); } diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java index d2eaa96aaa..662b97dda6 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java @@ -363,6 +363,45 @@ void createAndHandle() { ); } + @Test + void shouldHandleExistingMultiCatch() { + rewriteRun( + java( + """ + import java.io.IOException; + + class MyService { + void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException | NullPointerException e) { + System.out.println("Multi exception: " + e.getMessage()); + } catch (Exception e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """, + """ + import java.io.IOException; + import java.sql.SQLException; + + class MyService { + void doSomething() { + try { + new MockThrowingClass().throwsBothIOAndSQL(); + } catch (IOException | NullPointerException e) { + System.out.println("Multi exception: " + e.getMessage()); + } catch (SQLException e) { + System.out.println("Caught exception: " + e.getMessage()); + } + } + } + """ + ) + ); + } + @Test void declaredRuntimeException() { rewriteRun( From 7c88c9a9a2ab5a499a94b83bb4909c676cb78640 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 11:48:23 +0200 Subject: [PATCH 12/17] Rename recipe to make intention clear --- ...ExceptionCatches.java => OnlyCatchDeclaredExceptions.java} | 2 +- ...nCatchesTest.java => OnlyCatchDeclaredExceptionsTest.java} | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) rename src/main/java/org/openrewrite/staticanalysis/{SpecifyGenericExceptionCatches.java => OnlyCatchDeclaredExceptions.java} (99%) rename src/test/java/org/openrewrite/staticanalysis/{SpecifyGenericExceptionCatchesTest.java => OnlyCatchDeclaredExceptionsTest.java} (99%) diff --git a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java similarity index 99% rename from src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java rename to src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java index e1fb962341..580434be4d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatches.java +++ b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java @@ -33,7 +33,7 @@ import java.util.stream.Collectors; -public class SpecifyGenericExceptionCatches extends Recipe { +public class OnlyCatchDeclaredExceptions extends Recipe { @Override public String getDisplayName() { diff --git a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java b/src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java similarity index 99% rename from src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java rename to src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java index 662b97dda6..cb447cef28 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SpecifyGenericExceptionCatchesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java @@ -23,11 +23,11 @@ import static org.openrewrite.java.Assertions.java; -class SpecifyGenericExceptionCatchesTest implements RewriteTest { +class OnlyCatchDeclaredExceptionsTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new SpecifyGenericExceptionCatches()) + spec.recipe(new OnlyCatchDeclaredExceptions()) .parser(JavaParser.fromJavaVersion().dependsOn( //language=java """ From f1da21caa1340995a49382113065c9cf5d1d3dfb Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 12:06:50 +0200 Subject: [PATCH 13/17] Drop need for `hasGenericCatch` --- .../OnlyCatchDeclaredExceptions.java | 52 ++++++------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java index 580434be4d..dee138f393 100644 --- a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java +++ b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java @@ -30,8 +30,8 @@ import org.openrewrite.java.tree.TypeUtils; import java.util.*; -import java.util.stream.Collectors; +import static java.util.stream.Collectors.joining; public class OnlyCatchDeclaredExceptions extends Recipe { @@ -57,33 +57,21 @@ public Set getTags() { public JavaVisitor getVisitor() { return new JavaIsoVisitor() { private static final String JAVA_LANG_EXCEPTION = "java.lang.Exception"; - private static final String MULTI_CATCH_SEPARATOR = "|"; - private static final String TRY_CATCH_TEMPLATE = "try {} catch (%s %s) {}"; @Override public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { J.Try t = super.visitTry(aTry, ctx); - - if (hasGenericCatch(t)) { - Set thrownExceptions = getDeclaredThrownExceptions(t); - thrownExceptions.removeAll(getCaughtExceptions(t)); // Remove exceptions that are already specifically caught - - if (!thrownExceptions.isEmpty()) { - return t.withCatches(ListUtils.map(t.getCatches(), c -> - updateCatchIfGeneric(c, thrownExceptions))); - } - } - - return t; - } - - private boolean hasGenericCatch(J.Try aTry) { - for (J.Try.Catch c : aTry.getCatches()) { + return t.withCatches(ListUtils.map(t.getCatches(), c -> { if (isGenericCatch(c)) { - return true; + // Find declared thrown exceptions that are not already specifically caught + Set declaredThrown = getDeclaredThrownExceptions(t); + declaredThrown.removeAll(getCaughtExceptions(t)); + if (!declaredThrown.isEmpty()) { + return updateCatch(c, declaredThrown); + } } - } - return false; + return c; + })); } private boolean isGenericCatch(J.Try.Catch aCatch) { @@ -142,31 +130,23 @@ private Set getDeclaredThrownExceptions(J.Try aTry) { * @return the original catch block if it doesn't catch java.lang.Exception, * otherwise a new catch block with specific exception types */ - private J.Try.Catch updateCatchIfGeneric(J.Try.Catch aCatch, Set thrownExceptions) { - if (!isGenericCatch(aCatch)) { - return aCatch; - } - - // Preserve the existing variable name from the original generic catch block - String variableName = aCatch.getParameter().getTree().getVariables().get(0).getSimpleName(); - + private J.Try.Catch updateCatch(J.Try.Catch aCatch, Set thrownExceptions) { String throwableTypes = thrownExceptions.stream() .map(TypeUtils::asFullyQualified) .filter(Objects::nonNull) .sorted(Comparator.comparing(FullyQualified::getClassName)) .map(FullyQualified::getFullyQualifiedName) - .collect(Collectors.joining(MULTI_CATCH_SEPARATOR)); + .collect(joining("|")); J.Try aTry = getCursor().firstEnclosing(J.Try.class); assert aTry != null; - J.Try tempTry = JavaTemplate.builder(String.format(TRY_CATCH_TEMPLATE, throwableTypes, variableName)) + // Preserve the existing variable name from the original generic catch block + String variableName = aCatch.getParameter().getTree().getVariables().get(0).getSimpleName(); + J.Try tempTry = JavaTemplate.builder(String.format("try {} catch (%s %s) {}", throwableTypes, variableName)) .contextSensitive() .build() - .apply( - new Cursor(getCursor(), aTry), - aTry.getCoordinates().replace() - ); + .apply(new Cursor(getCursor(), aTry), aTry.getCoordinates().replace()); J.ControlParentheses cp = tempTry.getCatches().get(0).getParameter(); doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(cp.getTree())); From 287ba4659a4e2aaaa3ab4572bedaa4ec68c62bbf Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 12:14:32 +0200 Subject: [PATCH 14/17] Rename method to show intention --- .../OnlyCatchDeclaredExceptions.java | 26 ++----------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java index dee138f393..9285b97cc1 100644 --- a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java +++ b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java @@ -67,7 +67,7 @@ public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { Set declaredThrown = getDeclaredThrownExceptions(t); declaredThrown.removeAll(getCaughtExceptions(t)); if (!declaredThrown.isEmpty()) { - return updateCatch(c, declaredThrown); + return multiCatchWithDeclaredExceptions(c, declaredThrown); } } return c; @@ -108,29 +108,7 @@ private Set getDeclaredThrownExceptions(J.Try aTry) { }.reduce(aTry.getBody(), new HashSet<>()); } - /** - * Updates a generic catch block (catching java.lang.Exception) to catch only the specific - * exception types that are actually thrown within the try block. - * - *

This method transforms generic catch blocks like: - * 

{@code
-             * catch (Exception e) { ... }
-             * }
- * - * into specific single or multi-catch blocks like: - * 
{@code
-             * catch (IOException e) { ... }
-             * // or
-             * catch (IOException | SQLException e) { ... }
-             * }
- * - * @param aCatch the catch block to potentially update - * @param thrownExceptions the set of specific exception types thrown in the try block - * that are not already caught by other catch clauses - * @return the original catch block if it doesn't catch java.lang.Exception, - * otherwise a new catch block with specific exception types - */ - private J.Try.Catch updateCatch(J.Try.Catch aCatch, Set thrownExceptions) { + private J.Try.Catch multiCatchWithDeclaredExceptions(J.Try.Catch aCatch, Set thrownExceptions) { String throwableTypes = thrownExceptions.stream() .map(TypeUtils::asFullyQualified) .filter(Objects::nonNull) From 527cd14c5c5e1813388afacaf65e3664ae6d3554 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 12:17:00 +0200 Subject: [PATCH 15/17] Add a precondition to limit where recipe applies --- .../OnlyCatchDeclaredExceptions.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java index 9285b97cc1..18d2a0ba57 100644 --- a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java +++ b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java @@ -16,14 +16,12 @@ package org.openrewrite.staticanalysis; import org.jspecify.annotations.Nullable; -import org.openrewrite.Cursor; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Recipe; +import org.openrewrite.*; import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; -import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.ShortenFullyQualifiedTypeReferences; +import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.JavaType.FullyQualified; @@ -35,6 +33,8 @@ public class OnlyCatchDeclaredExceptions extends Recipe { + private static final String JAVA_LANG_EXCEPTION = "java.lang.Exception"; + @Override public String getDisplayName() { return "Replace `catch(Exception)` with specific declared exceptions thrown in the try block"; @@ -53,11 +53,10 @@ public Set getTags() { return new HashSet<>(Arrays.asList("CWE-396", "RSPEC-S2221")); } - @Override - public JavaVisitor getVisitor() { - return new JavaIsoVisitor() { - private static final String JAVA_LANG_EXCEPTION = "java.lang.Exception"; + @Override + public TreeVisitor getVisitor() { + JavaIsoVisitor visitor = new JavaIsoVisitor() { @Override public J.Try visitTry(J.Try aTry, ExecutionContext ctx) { J.Try t = super.visitTry(aTry, ctx); @@ -131,6 +130,7 @@ private J.Try.Catch multiCatchWithDeclaredExceptions(J.Try.Catch aCatch, Set(JAVA_LANG_EXCEPTION, false), visitor); } } From 8e94573d550eb1944bfdab87ba6f07f49f30fbba Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 12:47:04 +0200 Subject: [PATCH 16/17] Generate template with shortened types and imports --- .../OnlyCatchDeclaredExceptions.java | 28 +++++++++---------- .../OnlyCatchDeclaredExceptionsTest.java | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java index 18d2a0ba57..79f935bea3 100644 --- a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java +++ b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java @@ -20,7 +20,6 @@ import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; -import org.openrewrite.java.ShortenFullyQualifiedTypeReferences; import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; @@ -30,6 +29,7 @@ import java.util.*; import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toList; public class OnlyCatchDeclaredExceptions extends Recipe { @@ -53,7 +53,6 @@ public Set getTags() { return new HashSet<>(Arrays.asList("CWE-396", "RSPEC-S2221")); } - @Override public TreeVisitor getVisitor() { JavaIsoVisitor visitor = new JavaIsoVisitor() { @@ -108,29 +107,30 @@ private Set getDeclaredThrownExceptions(J.Try aTry) { } private J.Try.Catch multiCatchWithDeclaredExceptions(J.Try.Catch aCatch, Set thrownExceptions) { - String throwableTypes = thrownExceptions.stream() + List fqs = thrownExceptions.stream() .map(TypeUtils::asFullyQualified) .filter(Objects::nonNull) .sorted(Comparator.comparing(FullyQualified::getClassName)) - .map(FullyQualified::getFullyQualifiedName) + .collect(toList()); + String throwableTypes = fqs + .stream() + .map(FullyQualified::getClassName) .collect(joining("|")); + String[] imports = fqs.stream().map(FullyQualified::getFullyQualifiedName).toArray(String[]::new); + Arrays.stream(imports).forEach(this::maybeAddImport); - J.Try aTry = getCursor().firstEnclosing(J.Try.class); - assert aTry != null; + J.Try surroundingTry = getCursor().firstEnclosing(J.Try.class); + assert surroundingTry != null; // Preserve the existing variable name from the original generic catch block String variableName = aCatch.getParameter().getTree().getVariables().get(0).getSimpleName(); - J.Try tempTry = JavaTemplate.builder(String.format("try {} catch (%s %s) {}", throwableTypes, variableName)) - .contextSensitive() + J.Try generatedTry = JavaTemplate.builder(String.format("try {} catch (%s %s) {}", throwableTypes, variableName)) + .imports(imports) .build() - .apply(new Cursor(getCursor(), aTry), aTry.getCoordinates().replace()); - - J.ControlParentheses cp = tempTry.getCatches().get(0).getParameter(); - doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(cp.getTree())); - return aCatch.withParameter(cp); + .apply(new Cursor(getCursor(), surroundingTry), surroundingTry.getCoordinates().replace()); + return aCatch.withParameter(generatedTry.getCatches().get(0).getParameter()); } }; return Preconditions.check(new UsesType<>(JAVA_LANG_EXCEPTION, false), visitor); } - } diff --git a/src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java b/src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java index cb447cef28..71d764322c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptionsTest.java @@ -423,7 +423,7 @@ class MyService { void doSomething() { try { new MockThrowingClass().throwsDeclaredRuntimeException(); - } catch (java.lang.RuntimeException e) { + } catch (RuntimeException e) { // This is a generic catch block that should be replaced System.out.println("Caught exception: " + e.getMessage()); } From c8d13cc8ee7224f1ffce69ccfd0b41398316988a Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 19 Jul 2025 12:55:21 +0200 Subject: [PATCH 17/17] Always add imports in case of missing types --- .../staticanalysis/OnlyCatchDeclaredExceptions.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java index 79f935bea3..512ae8f2eb 100644 --- a/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java +++ b/src/main/java/org/openrewrite/staticanalysis/OnlyCatchDeclaredExceptions.java @@ -117,7 +117,9 @@ private J.Try.Catch multiCatchWithDeclaredExceptions(J.Try.Catch aCatch, Set