Fix for Issue 2040

CHANGELOG.md

@@ -39,6 +39,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Fixed UI freezes in Eclipse on bug count decorations update ([#285](https://github.com/spotbugs/spotbugs/issues/285))
 - Bumped log4j from 2.17.1 to 2.17.2 ([#1960](https://github.com/spotbugs/spotbugs/pull/1960))
 - Bumped gson from 2.8.9 to 2.9.0 ([#1960](https://github.com/spotbugs/spotbugs/pull/1966))
+- Fixed detector `ThrowingExcpetions` by omitting cases where throwing `java.lang.Exception` or `java.lang.Throwable` is unavoidable ([#2040](https://github.com/spotbugs/spotbugs/pull/2040))
 
 ### Added
 * New detector `FindInstanceLockOnSharedStaticData` for new bug type `SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA`. This detector reports a bug if an instance level lock is used to modify a shared static data. (See [SEI CERT rule LCK06-J](https://wiki.sei.cmu.edu/confluence/display/java/LCK06-J.+Do+not+use+an+instance+lock+to+protect+shared+static+data))

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2040Test.java

@@ -0,0 +1,29 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class Issue2040Test extends AbstractIntegrationTest {
+    @Test
+    public void test() {
+        performAnalysis("ghIssues/issue2040/Base.class",
+                "ghIssues/issue2040/Derived.class",
+                "ghIssues/issue2040/Interface.class",
+                "ghIssues/issue2040/Generic.class",
+                "ghIssues/issue2040/Caller.class");
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("THROWS_METHOD_THROWS_CLAUSE_THROWABLE")
+                .build();
+        assertThat(getBugCollection(), containsExactly(1, bugTypeMatcher));
+
+        bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION")
+                .build();
+        assertThat(getBugCollection(), containsExactly(1, bugTypeMatcher));
+    }
+}

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java

@@ -3,12 +3,23 @@
 import edu.umd.cs.findbugs.BugInstance;
 import edu.umd.cs.findbugs.BugReporter;
 import edu.umd.cs.findbugs.OpcodeStack;
+import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.ba.AnalysisContext;
+import edu.umd.cs.findbugs.ba.SignatureParser;
 import edu.umd.cs.findbugs.ba.XMethod;
 import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
+import edu.umd.cs.findbugs.internalAnnotations.DottedClassName;
+
+import java.util.Arrays;
+import java.util.Optional;
+import java.util.stream.Stream;
 
 import org.apache.bcel.Const;
+import org.apache.bcel.classfile.Code;
 import org.apache.bcel.classfile.ExceptionTable;
+import org.apache.bcel.classfile.JavaClass;
 import org.apache.bcel.classfile.Method;
+import org.apache.commons.lang3.StringUtils;
 
 
 public class ThrowingExceptions extends OpcodeStackDetector {
@@ -18,21 +29,70 @@ public ThrowingExceptions(BugReporter bugReporter) {
         this.bugReporter = bugReporter;
     }
 
+    private JavaClass klass;
+    private String exceptionThrown = null;
+
+    @Override
+    public void visit(JavaClass obj) {
+        exceptionThrown = null;
+        klass = obj;
+    }
+
     @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());
-                }
+        exceptionThrown = null;
+
+        if (obj.isSynthetic()) {
+            return;
+        }
+
+        Stream<String> exceptionStream = null;
+
+        // If the method is generic or is a method of a generic class, then check the generic signature to avoid detection
+        // of generic descendants of Exception or Throwable as Exception or Throwable itself.
+        String signature = obj.getGenericSignature();
+        if (signature != null) {
+            String[] exceptions = StringUtils.substringsBetween(signature, "^", ";");
+            if (exceptions != null) {
+                exceptionStream = Arrays.stream(exceptions)
+                        .filter(s -> s.charAt(0) == 'L')
+                        .map(s -> s.substring(1).replace('/', '.'));
+            }
+        } else {
+            ExceptionTable exceptionTable = obj.getExceptionTable();
+            if (exceptionTable != null) {
+                exceptionStream = Arrays.stream(exceptionTable.getExceptionNames());
+            }
+        }
+
+        // If the method throws Throwable or Exception because its ancestor throws the same exception then ignore it.
+        if (exceptionStream != null) {
+            Optional<String> exception = exceptionStream
+                    .filter(s -> "java.lang.Exception".equals(s) || "java.lang.Throwable".equals(s))
+                    .findAny();
+            if (exception.isPresent() && !parentThrows(obj, exception.get())) {
+                exceptionThrown = exception.get();
+            }
+        }
+
+        // If the method's body is empty then we report the bug immediately.
+        if (obj.getCode() == null) {
+            if (exceptionThrown != null) {
+                reportBug("java.lang.Exception".equals(exceptionThrown) ? "THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION"
+                        : "THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
             }
         }
     }
 
+    @Override
+    public void visitAfter(Code obj) {
+        // For methods with bodies we report the exception after checking their body.
+        if (exceptionThrown != null) {
+            reportBug("java.lang.Exception".equals(exceptionThrown) ? "THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION"
+                    : "THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
+        }
+    }
+
     @Override
     public void sawOpcode(int seen) {
         if (seen == Const.ATHROW) {
@@ -42,10 +102,104 @@ public void sawOpcode(int seen) {
                     reportBug("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", getXMethod());
                 }
             }
+        } else if (exceptionThrown != null &&
+                (seen == Const.INVOKEVIRTUAL ||
+                        seen == Const.INVOKEINTERFACE ||
+                        seen == Const.INVOKESTATIC)) {
+
+            // If the method throws Throwable or Exception because it invokes another method throwing such
+            // exceptions then ignore this bug by resetting exceptionThrown to null.
+            XMethod calledMethod = getXMethodOperand();
+            if (calledMethod == null) {
+                return;
+            }
+
+            String[] thrownExceptions = calledMethod.getThrownExceptions();
+            if (thrownExceptions != null && Arrays.stream(thrownExceptions)
+                    .map(s -> s.replace('/', '.'))
+                    .anyMatch(exceptionThrown::equals)) {
+                exceptionThrown = null;
+            }
+
         }
     }
 
     private void reportBug(String bugName, XMethod method) {
-        bugReporter.reportBug(new BugInstance(this, bugName, NORMAL_PRIORITY).addClass(this).addMethod(method));
+        bugReporter.reportBug(new BugInstance(this, bugName, LOW_PRIORITY).addClass(this).addMethod(method));
+    }
+
+    private boolean parentThrows(@NonNull Method method, @DottedClassName String exception) {
+        return parentThrows(klass, method, exception);
+    }
+
+    private boolean parentThrows(@NonNull JavaClass clazz, @NonNull Method method, @DottedClassName String exception) {
+        JavaClass ancestor;
+        boolean throwsEx = false;
+        try {
+            ancestor = clazz.getSuperClass();
+            if (ancestor != null) {
+                Optional<Method> superMethod = Arrays.stream(ancestor.getMethods())
+                        .filter(m -> method.getName().equals(m.getName()) && sameSignature(method, m))
+                        .findAny();
+                if (superMethod.isPresent()) {
+                    throwsEx = Arrays.stream(superMethod.get().getExceptionTable().getExceptionNames())
+                            .anyMatch(exception::equals);
+                } else {
+                    throwsEx = parentThrows(ancestor, method, exception);
+                }
+            }
+
+            for (JavaClass intf : clazz.getInterfaces()) {
+                Optional<Method> superMethod = Arrays.stream(intf.getMethods())
+                        .filter(m -> method.getName().equals(m.getName()) && sameSignature(method, m))
+                        .findAny();
+                if (superMethod.isPresent()) {
+                    throwsEx |= Arrays.stream(superMethod.get().getExceptionTable().getExceptionNames())
+                            .anyMatch(exception::equals);
+                } else {
+                    throwsEx |= parentThrows(intf, method, exception);
+                }
+            }
+        } catch (ClassNotFoundException e) {
+            AnalysisContext.reportMissingClass(e);
+        }
+        return throwsEx;
+    }
+
+    private boolean sameSignature(Method child, Method parent) {
+        String genSig = parent.getGenericSignature();
+        if (genSig == null) {
+            return child.getSignature().equals(parent.getSignature());
+        }
+
+        String sig = child.getSignature();
+
+        SignatureParser genSP = new SignatureParser(genSig);
+        SignatureParser sp = new SignatureParser(sig);
+
+        if (genSP.getNumParameters() != sp.getNumParameters()) {
+            return false;
+        }
+
+        String[] gArgs = genSP.getArguments();
+        String[] args = sp.getArguments();
+
+        for (int i = 0; i < sp.getNumParameters(); ++i) {
+            if (gArgs[i].charAt(i) == 'T') {
+                if (args[i].charAt(i) != 'L') {
+                    return false;
+                }
+            } else {
+                if (!gArgs[i].equals(args[i])) {
+                    return false;
+                }
+            }
+        }
+
+        String gRet = genSP.getReturnTypeSignature();
+        String ret = sp.getReturnTypeSignature();
+
+        return (gRet.charAt(0) == 'T' && ret.charAt(0) == 'L') ||
+                gRet.equals(ret);
     }
 }

spotbugsTestCases/src/java/ghIssues/issue2040/Base.java

@@ -0,0 +1,7 @@
+package ghIssues.issue2040;
+
+public class Base {
+    public void method() throws Throwable {
+        throw new Throwable();
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue2040/Caller.java

@@ -0,0 +1,8 @@
+package ghIssues.issue2040;
+
+public class Caller {
+    public void method() throws Throwable {
+        Base b = new Base();
+        b.method();
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue2040/Derived.java

@@ -0,0 +1,30 @@
+package ghIssues.issue2040;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class Derived extends Base implements Interface, Callable<Integer> {
+    @Override
+    public void method() throws Throwable {
+        super.method();
+    }
+
+    @Override
+    public void iMethod() throws Exception {
+        return;
+    }
+
+    public <T extends Throwable> void genericMethod() throws T {
+
+    }
+
+    public Integer call() throws Exception {
+        return 0;
+    }
+
+    public void lambda() {
+        ExecutorService es = Executors.newCachedThreadPool();
+        es.submit(() -> { return null; });
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue2040/Generic.java

@@ -0,0 +1,6 @@
+package ghIssues.issue2040;
+
+public class Generic<E extends Exception> {
+    void method() throws E {
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue2040/Interface.java

@@ -0,0 +1,5 @@
+package ghIssues.issue2040;
+
+public interface Interface {
+    public void iMethod() throws Exception;
+}