Add OnlyCatchDeclaredExceptions recipe to address RSPEC-S2221

[stefanodallapalma]

Overview

Replaces generic catch(Exception) blocks with specific exception types that are actually thrown within the try block. This improves exception handling precision and code maintainability by catching only the exceptions that can actually occur.

What's your motivation?

Generic exception catching with catch(Exception e) is considered a code smell (SonarQube rule S2221, CWE-396) because it masks specific error conditions and makes debugging harder. When developers write code that throws specific exceptions like IOException or SQLException, the catch blocks should reflect that specificity rather than using the overly broad Exception type.

What it detects

• Generic catch(Exception e) blocks in try-catch statements
• Analyzes method calls and constructor invocations within try blocks to identify thrown exception types
• Generates appropriate single-catch or multi-catch syntax for those thrown exception types if not already caught by more specific catch clauses

Before:

try {
    Files.readAllLines(path);
    connection.createStatement().executeQuery(sql);
} catch (Exception e) {
    log.error("Operation failed", e);
}

After:

try {
    Files.readAllLines(path);
    connection.createStatement().executeQuery(sql);
} catch (IOException | SQLException e) {
    log.error("Operation failed", e);
}

Anything in particular you'd like reviewers to focus on?

Implementation approach

• Exact matching: The recipe specifically targets java.lang.Exception catches (not subclasses like RuntimeException) to be conservative about what constitutes "generic" exception handling
• Multi-catch generation: Uses JavaTemplate with complete try-catch syntax to properly parse multi-catch expressions with | separators
• Recipe name: feel free to suggest more suitable names

Limitations

• Static analysis only: Relies on method/constructor signatures for thrown exceptions; doesn't perform data flow analysis for dynamically thrown exceptions
• Checked exceptions focus: Primarily improves handling of checked exceptions since those must be declared in method signatures

Anyone you would like to review specifically?

@timtebeek

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the [recipe conventions and best practices](https://docs.openrewrite.org/authoring-recipes/recipe-conventions-and-best-practices)
• I've used the IntelliJ IDEA auto-formatter on affected files

[greg-at-moderne]

Thanks for the great contribution.
I am looking into you test cases and am wondering what the behavior should be for:

                      try {
                          new MockThrowingClass().throwsBothIOAndSQL();
                      } catch (IOException | Exception e) {
                          System.out.println("Caught: " + e.getMessage());
                      }

[stefanodallapalma]

Hey @greg-at-moderne

That's an interesting case! My take is that Exception should ideally be replaced by SQLException for better specificity. However, directly using catch (IOException | Exception e) isn't possible because IOException and Exception are not disjoint types. The compiler will flag this as an error since IOException is a subclass of Exception.

[stefanodallapalma]

Hi @timtebeek and @greg-at-moderne, since there hasn't been any recent activity on this PR, do you have a timeline for merging and releasing this recipe? While I can use it as a standalone recipe in our organization's project, it would be great to have it available out-of-the-box from rewrite-static-analysis.

[timtebeek]

hi @stefanodallapalma ; thanks for the help! For a bit of context: Greg is out in Denmark enjoying his holiday, while I was out in Luxembourg enjoying mine, so it's taking us a little longer to get through everything. 😅

I'll try to fit this in when I can, but hard to say exactly when that'll be. Know that the work is appreciated though! Reviews are just taking a little longer over the summer.

[stefanodallapalma]

@timtebeek I see. I didn't mean to be pushy, just wanted to check in. Take your time! 😊

[timtebeek]

@timtebeek I see. I didn't mean to be pushy, just wanted to check in. Take your time! 😊

No worries at all; the context is just to let you know we strive to get things in quickly where we can, but life at times gets in the way. :)

[timtebeek]

Seeing somewhat debatable results for let's say:

diff --git a/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java b/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java
index 2c745ff..8558d2b 100644
--- a/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java
+++ b/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java
@@ -19,6 +19,7 @@ org.openrewrite.staticanalysis.OnlyCatchDeclaredExceptions
 package org.apache.maven.scm.provider.git.jgit.command.checkout;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
@@ -205,7 +206,7 @@
             logger.debug("current branch: " + git.getRepository().getBranch());
 
             return new CheckOutScmResult("checkout via JGit", listedFiles);
-        } catch (Exception e) {
+        } catch (AmbiguousObjectException | CanceledException | CheckoutConflictException | CorruptObjectException | GitAPIException | IOException | IncorrectObjectTypeException | InvalidConfigurationException | InvalidRefNameException | InvalidRemoteException | MissingObjectException | NoHeadException | RefAlreadyExistsException | RefNotAdvertisedException | RefNotFoundException | RevisionSyntaxException | ScmException | TransportException | WrongRepositoryStateException e) {
             throw new ScmException("JGit checkout failure!", e);
         } finally {
             JGitUtils.closeRepo(git);

But given that we're not including this recipe by default anywhere (yet) I think that's fine for now; folks can run this when they want these results.

[timtebeek]

Thanks a lot for the initiative here @stefanodallapalma ! Hope you agree with the changes I've made to mostly keep the logic the same (not removing catch blocks) and opting to focus on declared exceptions.

[stefanodallapalma]

Seeing somewhat debatable results for let's say:

diff --git a/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java b/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java
index 2c745ff..8558d2b 100644
--- a/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java
+++ b/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/checkout/JGitCheckOutCommand.java
@@ -19,6 +19,7 @@ org.openrewrite.staticanalysis.OnlyCatchDeclaredExceptions
 package org.apache.maven.scm.provider.git.jgit.command.checkout;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
@@ -205,7 +206,7 @@
             logger.debug("current branch: " + git.getRepository().getBranch());
 
             return new CheckOutScmResult("checkout via JGit", listedFiles);
-        } catch (Exception e) {
+        } catch (AmbiguousObjectException | CanceledException | CheckoutConflictException | CorruptObjectException | GitAPIException | IOException | IncorrectObjectTypeException | InvalidConfigurationException | InvalidRefNameException | InvalidRemoteException | MissingObjectException | NoHeadException | RefAlreadyExistsException | RefNotAdvertisedException | RefNotFoundException | RevisionSyntaxException | ScmException | TransportException | WrongRepositoryStateException e) {
             throw new ScmException("JGit checkout failure!", e);
         } finally {
             JGitUtils.closeRepo(git);

But given that we're not including this recipe by default anywhere (yet) I think that's fine for now; folks can run this when they want these results.

I see this is an interesting edge case, and I'm not surprised given that the try block in that method spans ~100 lines of code. I can see both sides here: on one hand, you could argue for ignoring the specific thrown types since a custom ScmException is being rethrown anyway. On the other hand, when working in a complex codebase, I'd see this as an opportunity to refactor that block instead.
I think it really depends on the specific use case, and I agree it's fine for now. This is the kind of recipe that people (myself included) would run selectively.

[stefanodallapalma]

Thanks a lot for the initiative here @stefanodallapalma ! Hope you agree with the changes I've made to mostly keep the logic the same (not removing catch blocks) and opting to focus on declared exceptions.

Thanks for all the enhancements, Tim! I really appreciate them as they continue to teach me how to best use the OpenRewrite AST constructs.