From 4e69562f88879b26fd992b849b3633a9cb6b4871 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 22 Nov 2024 11:36:58 -0800 Subject: [PATCH] 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 https://github.com/google/error-prone/commit/d70a18cce3fa5bd3daeb057c4b8e4ad2fe0e3d5c.) - 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: 699237373 --- .../bugpatterns/InfiniteRecursion.java | 50 ++++++++++---- .../bugpatterns/InfiniteRecursionTest.java | 69 ++++++++++++++++++- 2 files changed, 106 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java b/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java index 0d883303486..016b2e94beb 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/InfiniteRecursion.java +++ b/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. * diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/InfiniteRecursionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/InfiniteRecursionTest.java index 0606d01acf7..d24eec9e334 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/InfiniteRecursionTest.java +++ b/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(); + } }