diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EmptyCatch.java b/core/src/main/java/com/google/errorprone/bugpatterns/EmptyCatch.java index a254dda89758..4f05bb15d9dd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EmptyCatch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/EmptyCatch.java @@ -24,7 +24,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CatchTreeMatcher; import com.google.errorprone.matchers.Description; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.BlockTree; import com.sun.source.tree.CatchTree; @@ -47,12 +46,6 @@ public Description matchCatch(CatchTree tree, VisitorState state) { if (state.getTokensForNode(block).stream().anyMatch(t -> !t.comments().isEmpty())) { return NO_MATCH; } - if (ASTHelpers.isJUnitTestCode(state)) { - return NO_MATCH; - } - if (ASTHelpers.isTestNgTestCode(state)) { - return NO_MATCH; - } return describeMatch(tree); } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EmptyCatchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EmptyCatchTest.java index a35e16f652d3..96be7eb2fb28 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/EmptyCatchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/EmptyCatchTest.java @@ -65,6 +65,7 @@ public class SomeTest { public void testNG() { try { System.err.println(); + // BUG: Diagnostic contains: } catch (Exception doNotCare) { } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchNegativeCases.java index b1a733bfea6e..30b99b6b7250 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchNegativeCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchNegativeCases.java @@ -16,10 +16,8 @@ package com.google.errorprone.bugpatterns; -import static org.junit.Assert.fail; import java.io.FileNotFoundException; -import org.junit.Test; /** * @author yuan@ece.toronto.edu (Ding Yuan) @@ -126,13 +124,4 @@ public void catchIsLoggedOnly() { System.out.println("Caught an exception: " + t); } } - - @Test - public void expectedException() { - try { - System.err.println(); - fail(); - } catch (Exception expected) { - } - } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchPositiveCases.java index 7cf5252ace4d..3ca9445a16ed 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchPositiveCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/EmptyCatchPositiveCases.java @@ -16,6 +16,10 @@ package com.google.errorprone.bugpatterns; +import static org.junit.Assert.fail; + +import org.junit.Test; + /** * @author yuan@ece.toronto.edu (Ding Yuan) */ @@ -32,4 +36,14 @@ public void catchIsCompleteEmpty() { } } + + @Test + public void expectedException() { + try { + System.err.println(); + fail(); + // BUG: Diagnostic contains: + } catch (Exception expected) { + } + } } diff --git a/docs/bugpattern/EmptyCatch.md b/docs/bugpattern/EmptyCatch.md new file mode 100644 index 000000000000..0422ed168bc2 --- /dev/null +++ b/docs/bugpattern/EmptyCatch.md @@ -0,0 +1,30 @@ +The [Google Java Style Guide ยง6.2][style] states: + +> It is very rarely correct to do nothing in response to a caught exception. +> (Typical responses are to log it, or if it is considered "impossible", rethrow +> it as an AssertionError.) +> +> When it truly is appropriate to take no action whatsoever in a catch block, +> the reason this is justified is explained in a comment. + +When writing tests that expect an exception to be thrown, prefer using +[`Assert.assertThrows`][assertthrows] instead of writing a try-catch. That is, +prefer this: + +```java +assertThrows(NoSuchElementException.class, () -> emptyStack.pop()); +``` + +instead of this: + +```java +try { + emptyStack.pop(); + fail(); +} catch (NoSuchElementException expected) { +} +``` + +[style]: https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions + +[assertthrows]: https://junit.org/junit4/javadoc/latest/org/junit/Assert.html#assertThrows(java.lang.Class,%20org.junit.function.ThrowingRunnable)