Assorted InfiniteRecursion perfectionism:

- Complain about recursive calls even if they come after `catch (SomeException e) { return; }`, and attempt to clarify how this fits into how we otherwise treat exceptions. (This follows through on my post-submit comment from google@d70a18c.)
- Test that handling of switch expressions falls naturally out of the existing code, since it looks for `CaseTree` and not `SwitchTree` (and thus doesn't need an update for `SwitchExpressionTree`).
- Leave a comment about how we _could_ get _even better_ handling of both kinds of `switch`, plus `if` and `?:` and maybe more, if we wanted to check whether all branches make recursive calls. (Such an approach likely _would_ lead us to look for `SwitchTree` and `SwitchExpressionTree`, rather than just `CaseTree`.)

PiperOrigin-RevId: 699467832

core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java

@@ -85,6 +85,10 @@ public Void visitReturn(ReturnTree tree, Boolean underConditional) {
        * Since we're scanning such code only for `return` statements, there's no need to scan any
        * code that contains only expressions (e.g., for a ternary, getTrueExpression()
        * and getFalseExpression()).
+       *
+       * TODO(cpovirk): Enhance the check to look at conditionally executed code (including
+       * expression-only code) to identify conditionals for which *every* branch makes a recursive
+       * call.
        */
 
       @Override
@@ -101,18 +105,6 @@ public Void visitCase(CaseTree tree, Boolean underConditional) {
         return super.visitCase(tree, /*underConditional*/ true);
       }
 
-      @Override
-      public Void visitCatch(CatchTree tree, Boolean underConditional) {
-        /*
-         * We mostly ignore exceptions. Notably, if a loop would be infinite *except* that it throws
-         * an exception, we still report it as an infinite loop. But we do want to consider `catch`
-         * blocks to be conditionally executed, since it would be reasonable for a method to
-         * delegate to itself (on another object or with a different argument) in case of an
-         * exception. For example, `toString(COMPLEX)` might fall back to `toString(SIMPLE)`.
-         */
-        return super.visitCatch(tree, /*underConditional*/ true);
-      }
-
       @Override
       public Void visitConditionalExpression(
           ConditionalExpressionTree tree, Boolean underConditional) {
@@ -152,6 +144,40 @@ public Void visitWhileLoop(WhileLoopTree tree, Boolean underConditional) {
         return null;
       }
 
+      /*
+       * We ignore the possibility of exceptions. After all, if we were to say "The recursion in
+       * this method might not be infinite because the method might throw an exception," then we'd
+       * report infinite recursion almost nowhere, given that almost any code can throw an exception
+       * (especially a RuntimeException or Error). So:
+       *
+       * For a `try` block: We treat the whole thing as executed unconditionally. Yes, some of its
+       * code could throw an exception, but that's equally true for code outside a `try` block, as
+       * discussed above.
+       *
+       * For a `catch` block:
+       *
+       * - Clearly it's not executed _unconditionally_, so we don't want to perform our normal
+       *   scanning, which would report errors for any recursive calls. (For example, maybe a
+       *   `toString(Style)` method has a `catch` block that calls `toString(Style.SIMPLE)`, but it
+       *   can't create infinite recursion because the `catch` block can entered only for
+       *   `Style.DETAILED`. Such code would be more than a little scary (given that some very
+       *   _similar_ code _could_ produce infinite recursion!), but it would be wrong to flag it as
+       *   infinite recursion.)
+       *
+       * - While it would make conceptual sense to say that it's executed _conditionally_, that
+       *   doesn't fit with the policy above: Recall that the whole reason that we scan
+       *   conditionally executed code is to see if it might `return`, since a conditional `return`
+       *   can preventing subsequent code from making recursive calls. But also recall that we are
+       *   ignoring the possibility of exceptions. Thus, we don't want to say "The recursion in this
+       *   method might not be infinite because the method might throw an exception _and then catch
+       *   it and return_."
+       */
+
+      @Override
+      public Void visitCatch(CatchTree tree, Boolean underConditional) {
+        return null;
+      }
+
       /*
        * Don't descend into classes and lambdas at all, but resume checking afterward.
        *

core/src/test/java/com/google/errorprone/bugpatterns/InfiniteRecursionTest.java

@@ -260,7 +260,7 @@ void f(boolean callAgain) {
   }
 
   @Test
-  public void negativeConditional() {
+  public void negativeIfConditional() {
     compilationHelper
         .addSourceLines(
             "Test.java",
@@ -276,6 +276,52 @@ void f(boolean callAgain) {
         .doTest();
   }
 
+  @Test
+  public void negativeSwitchStatementConditional() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            final class Test {
+              void f(CallAgain callAgain) {
+                switch (callAgain) {
+                  case TRUE:
+                    f(CallAgain.FALSE);
+                }
+              }
+
+              enum CallAgain {
+                TRUE,
+                FALSE,
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void negativeSwitchExpressionConditional() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            final class Test {
+              int f(CallAgain callAgain) {
+                return switch (callAgain) {
+                  case TRUE -> f(CallAgain.FALSE) + 1;
+                  case FALSE -> 0;
+                };
+              }
+
+              enum CallAgain {
+                TRUE,
+                FALSE,
+              }
+            }
+            """)
+        .doTest();
+  }
+
   @Test
   public void negativeNestedClass() {
     compilationHelper
@@ -470,4 +516,25 @@ String asString() {
             """)
         .doTest();
   }
+
+  @Test
+  public void positiveCatchAndReturnDoesNotMakeItSafe() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            final class Test {
+              void f() {
+                try {
+                  System.out.println("hi");
+                } catch (Exception e) {
+                  return;
+                }
+                // BUG: Diagnostic contains:
+                f();
+              }
+            }
+            """)
+        .doTest();
+  }
 }