Added new detector for rule ERR07 (#1956)

* Added detector and tests for ThrowingExceptions * Issue893 - removed throws clause * Issue1338 - specialized throws clause * Ran gradlew spotlessApply * docs: move CHANGELOG entry under the "Unreleased" Co-authored-by: Bálint Dominik Orosz <bck909@INF.ELTE.HU> Co-authored-by: Kengo TODA <skypencil@gmail.com> Co-authored-by: Kengo TODA <skypencil+github@gmail.com>
spotbugs · Mar 26, 2022 · 818b14c · 818b14c
1 parent 3155f38
commit 818b14c
Show file tree

Hide file tree

Showing 8 changed files with 216 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,14 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Bumped Saxon-HE from 10.6 to 11.2 ([#1955](https://github.com/spotbugs/spotbugs/pull/1955))
 - Fixed traversal of nested archives governed by `-nested:true` ([#1930](https://github.com/spotbugs/spotbugs/pull/1930))
 
+### Added
+* New detector `ThrowingExceptions` and introduced new bug types:
+  * `THROWS_METHOD_THROWS_RUNTIMEEXCEPTION` is reported in case of a method throwing RuntimeException, 
+  * `THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION` is reported when a method has Exception in its throws clause and 
+  * `THROWS_METHOD_THROWS_CLAUSE_THROWABLE` is reported when a method has Throwable in its throws clause
+
+(See [SEI CERT ERR07-J](https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable))
+
 ## 4.6.0 - 2022-03-08
 ### Fixed
 - Fixed spotbugs build with ecj compiler ([#1903](https://github.com/spotbugs/spotbugs/issues/1903))

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/CheckThrowingExceptions.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/CheckThrowingExceptions.java
@@ -0,0 +1,49 @@
+package edu.umd.cs.findbugs.detect;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+public class CheckThrowingExceptions extends AbstractIntegrationTest {
+    @Test
+    public void throwingExceptionsTests() {
+        performAnalysis("MethodsThrowingExceptions.class");
+
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 1);
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 1);
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", 1);
+
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 1, "MethodsThrowingExceptions", "isCapitalizedThrowingRuntimeException");
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 0, "MethodsThrowingExceptions", "isCapitalizedThrowingSpecializedException");
+
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 1, "MethodsThrowingExceptions", "methodThrowingBasicException");
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 0, "MethodsThrowingExceptions", "methodThrowingIOException");
+
+        assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", 1, "MethodsThrowingExceptions", "methodThrowingThrowable");
+    }
+
+    private void assertNumOfTHROWSBugs(String bugType, int num) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugType)
+                .build();
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+
+    private void assertNumOfTHROWSBugs(String bugType, int num, String className, String method) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugType)
+                .inClass(className)
+                .inMethod(method)
+                .build();
+        if (num > 0) {
+            assertThat(getBugCollection(), hasItem(bugTypeMatcher));
+        }
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+}
diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml
@@ -659,6 +659,8 @@
                     reports="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR,MC_OVERRIDABLE_METHOD_CALL_IN_CLONE"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindInstanceLockOnSharedStaticData" speed="fast"
                     reports="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA"/>
+          <Detector class="edu.umd.cs.findbugs.detect.ThrowingExceptions" speed="fast"
+                    reports="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION,THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION,THROWS_METHOD_THROWS_CLAUSE_THROWABLE" />
           <Detector class="edu.umd.cs.findbugs.detect.PermissionsSuper"
                     reports="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS"/>
             <!-- Bug Categories -->
@@ -1271,5 +1273,8 @@
           <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR" category="MALICIOUS_CODE" />
           <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_CLONE" category="MALICIOUS_CODE" />
           <BugPattern abbrev="SSD" type="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA" category="CORRECTNESS" />
+          <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION" category="BAD_PRACTICE" />
+          <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION" category="BAD_PRACTICE" />
+          <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_CLAUSE_THROWABLE" category="BAD_PRACTICE" />
           <BugPattern abbrev="PERM" type="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS" category="MALICIOUS_CODE" />
 </FindbugsPlugin>
diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml
@@ -710,6 +710,14 @@ boolean operators (<code>|</code> and <code>&amp;</code> instead of
 <![CDATA[
 <p> This detector looks for violations of the idioms for writing
 cloneable classes. </p>
+]]>
+    </Details>
+  </Detector>
+  <Detector class="edu.umd.cs.findbugs.detect.ThrowingExceptions">
+    <Details>
+<![CDATA[
+<p> This detector looks for methods throwing RuntimeException and methods 
+that have Exception or Throwable in their throws clause. </p>
 ]]>
     </Details>
   </Detector>
@@ -8653,7 +8661,58 @@ after the call to initLogging, the logger configuration is lost
       </p>]]>
     </Details>
   </BugPattern>
+  <BugPattern type="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION">
+    <ShortDescription>Method intentionally throws RuntimeException.</ShortDescription>
+    <LongDescription>Method intentionally throws RuntimeException.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>
+        Method intentionally throws RuntimeException.<br>
+        
+        According to the <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable">SEI CERT ERR07-J rule</a>, 
+        throwing a RuntimeException may cause errors, like the caller not being able to examine the exception and therefore cannot properly recover from it.<br>
+
+        Moreover, throwing a RuntimeException would force the caller to catch RuntimeException and therefore violate the 
+        <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors">SEI CERT ERR08-J rule</a>.<br>
+        
+        Please note that you can derive from Exception or RuntimeException and may throw a new instance of that exception.
+        </p>]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION">
+    <ShortDescription>Method lists Exception in its throws clause.</ShortDescription>
+    <LongDescription>Method lists Exception in its throws clause.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>
+        Method lists Exception in its throws clause.<br>
 
+        When declaring a method, the types of exceptions in the throws clause should be the most specific.
+        Therefore, using Exception in the throws clause would force the caller to either use it in its own throws clause, or use it in a try-catch block (when it does not necessarily 
+        contain any meaningful information about the thrown exception).<br><br>
+        
+        For more information, see the <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable">SEI CERT ERR07-J rule</a>.
+        </p>]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="THROWS_METHOD_THROWS_CLAUSE_THROWABLE">
+    <ShortDescription>Method lists Throwable in its throws clause.</ShortDescription>
+    <LongDescription>Method lists Throwable in its throws clause.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>
+        Method lists Throwable in its throws clause.<br>
+
+        When declaring a method, the types of exceptions in the throws clause should be the most specific.
+        Therefore, using Throwable in the throws clause would force the caller to either use it in its own throws clause, or use it in a try-catch block (when it does not necessarily 
+        contain any meaningful information about the thrown exception).<br>
+        
+        Furthermore, using Throwable like that is semantically a bad practice, considered that Throwables include Errors as well, but by definition they occur in unrecoverable scenarios.<br><br>
+        
+        For more information, see the <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable">SEI CERT ERR07-J rule</a>.
+        </p>]]>
+    </Details>
+  </BugPattern>
   <BugPattern type="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS">
     <ShortDescription>Custom class loader does not call its superclass's getPermissions()</ShortDescription>
     <LongDescription>Custom class loader {1} does not call its superclass's getPermissions()</LongDescription>
@@ -8818,5 +8877,6 @@ after the call to initLogging, the logger configuration is lost
   <BugCode abbrev="REFLF">Reflection increasing accessibility of fields</BugCode>
   <BugCode abbrev="MC">Dangerous call to overridable method</BugCode>
   <BugCode abbrev="SSD">Do not use an instance lock to protect shared static data</BugCode>
+  <BugCode abbrev="THROWS">Exception throwing related problems</BugCode>
   <BugCode abbrev="PERM">Custom class loader does not call its superclass's getPermissions()</BugCode>
 </MessageCollection>
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java
@@ -0,0 +1,51 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.OpcodeStack;
+import edu.umd.cs.findbugs.ba.XMethod;
+import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
+
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.ExceptionTable;
+import org.apache.bcel.classfile.Method;
+
+
+public class ThrowingExceptions extends OpcodeStackDetector {
+    private final BugReporter bugReporter;
+
+    public ThrowingExceptions(BugReporter bugReporter) {
+        this.bugReporter = bugReporter;
+    }
+
+    @Override
+    public void visit(Method obj) {
+        ExceptionTable exceptionTable = obj.getExceptionTable();
+        if (exceptionTable != null) {
+            String[] exceptionNames = exceptionTable.getExceptionNames();
+            for (String exception : exceptionNames) {
+                if ("java.lang.Exception".equals(exception)) {
+                    reportBug("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", getXMethod());
+                } else if ("java.lang.Throwable".equals(exception)) {
+                    reportBug("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
+                }
+            }
+        }
+    }
+
+    @Override
+    public void sawOpcode(int seen) {
+        if (seen == Const.ATHROW) {
+            OpcodeStack.Item item = stack.getStackItem(0);
+            if (item != null) {
+                if ("Ljava/lang/RuntimeException;".equals(item.getSignature())) {
+                    reportBug("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", getXMethod());
+                }
+            }
+        }
+    }
+
+    private void reportBug(String bugName, XMethod method) {
+        bugReporter.reportBug(new BugInstance(this, bugName, NORMAL_PRIORITY).addClass(this).addMethod(method));
+    }
+}
diff --git a/spotbugsTestCases/src/java/MethodsThrowingExceptions.java b/spotbugsTestCases/src/java/MethodsThrowingExceptions.java
@@ -0,0 +1,39 @@
+import java.io.IOException;
+
+public class MethodsThrowingExceptions {
+    boolean isCapitalizedThrowingRuntimeException(String s) {
+        if (s == null) {
+          throw new RuntimeException("Null String");
+        }
+        if (s.equals("")) {
+          return true;
+        }
+        String first = s.substring(0, 1);
+        String rest = s.substring(1);
+        return first.equals(first.toUpperCase()) && rest.equals(rest.toLowerCase());
+    }
+
+    boolean isCapitalizedThrowingSpecializedException(String s) {
+        if (s == null) {
+          throw new NullPointerException();
+        }
+        if (s.equals("")) {
+          return true;
+        }
+        String first = s.substring(0, 1);
+        String rest = s.substring(1);
+        return first.equals(first.toUpperCase()) && rest.equals(rest.toLowerCase());
+    }
+
+    private void methodThrowingBasicException() throws Exception {
+        System.out.println("Method throwing Exception");
+    }
+
+    private void methodThrowingIOException() throws IOException {
+        System.out.println("Method throwing IOException");
+    }
+
+    private void methodThrowingThrowable() throws Throwable {
+        System.out.println("Method throwing Throwable");
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/Issue1338.java b/spotbugsTestCases/src/java/ghIssues/Issue1338.java
@@ -1,12 +1,14 @@
 package ghIssues;
 
 import java.io.InputStream;
+import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.KeyStore;
+import java.security.KeyStoreException;
 
 public class Issue1338 {
-    public static KeyStore loadKeyStore(final Path path) throws Exception {
+    public static KeyStore loadKeyStore(final Path path) throws IOException, KeyStoreException {
         try (InputStream inputStream = Files.newInputStream(path)) {
             // final KeyStore keyStore = KeyStore.getInstance("JKS");
             // keyStore.load(inputStream, password.toCharArray());

diff --git a/spotbugsTestCases/src/java/ghIssues/Issue893.java b/spotbugsTestCases/src/java/ghIssues/Issue893.java
@@ -1,7 +1,7 @@
 package ghIssues;
 
 public class Issue893 {
-    public boolean test() throws Exception {
+    public boolean test() {
         Boolean b = Boolean.valueOf(true);
         // Line below should not throw IllegalArgumentException during analysis
         b &= Boolean.valueOf(false);