Fix for Issue 2040

CHANGELOG.md

@@ -38,6 +38,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Do not report SIC_INNER_SHOULD_BE_STATIC for classes annotated with JUnit Nested ([#560](https://github.com/spotbugs/spotbugs/issues/560))
 - Detect created, but not-thrown exceptions, which are created by not the constructor ([#2547](https://github.com/spotbugs/spotbugs/issues/2547))
 - Fixed eclipse plugin Effort.values pass to effortViewer as required cast to varargs ([#2579](https://github.com/spotbugs/spotbugs/pull/2579))
+- Fixed detector `ThrowingExceptions` by removing false positive reports, such as synthetic methods (lambdas), methods which inherited their exception specifications and methods which call throwing methods ([#2040](https://github.com/spotbugs/spotbugs/issues/2040))
 
 ### Added
 - New simple name-based AnnotationMatcher for exclude files (now bug annotations store the class java annotations in an attribute called `classAnnotationNames`). For example, use like <Match><Annotation name="org.immutables.value.Generated" /></Match> in an excludeFilter.xml to ignore classes generated by the Immutable framework. This ignores all class, method or field bugs in classes with that annotation.

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

@@ -0,0 +1,31 @@
+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/GenericBase.class",
+                "ghIssues/issue2040/GenericDerived.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(3, bugTypeMatcher));
+
+        bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION")
+                .build();
+        assertThat(getBugCollection(), containsExactly(3, bugTypeMatcher));
+    }
+}

spotbugs/etc/findbugs.xml

@@ -665,7 +665,7 @@
                     reports="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA"/>
           <Detector class="edu.umd.cs.findbugs.detect.DontUseFloatsAsLoopCounters" speed="fast"
                     reports="FL_FLOATS_AS_LOOP_COUNTERS"/>
-          <Detector class="edu.umd.cs.findbugs.detect.ThrowingExceptions" speed="fast" disabled="true"
+          <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"/>

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/SignatureParser.java

@@ -113,6 +113,7 @@ public String next() {
                     break;
 
                 case 'L':
+                case 'T':
                     int semi = signature.indexOf(';', index + 1);
                     if (semi < 0) {
                         throw new IllegalStateException("Invalid method signature: " + signature);

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,25 +29,71 @@ public ThrowingExceptions(BugReporter bugReporter) {
         this.bugReporter = bugReporter;
     }
 
+    private JavaClass clazz;
+    private String exceptionThrown = null;
+
+    @Override
+    public void visit(JavaClass obj) {
+        exceptionThrown = null;
+        clazz = obj;
+    }
+
     @Override
     public void visit(Method obj) {
+        exceptionThrown = null;
+
         if (obj.isSynthetic()) {
             return;
         }
 
-        ExceptionTable exceptionTable = obj.getExceptionTable();
-        if (exceptionTable == null) {
-            return;
+        Stream<String> exceptionStream = null;
+
+        // If the method is generic or is a method of a generic class, then first check the generic signature to avoid detection
+        // of generic descendants of Exception or Throwable as Exception or Throwable itself.
+        String signature = obj.getGenericSignature();
+        String[] exceptions = null;
+        if (signature != null) {
+            exceptions = StringUtils.substringsBetween(signature, "^", ";");
+            if (exceptions != null) {
+                exceptionStream = Arrays.stream(exceptions)
+                        .filter(s -> s.charAt(0) == 'L')
+                        .map(s -> s.substring(1).replace('/', '.'));
+            }
+        }
+
+        // If the method is not generic or it does not throw a generic exception then it has no exception specification in its generic
+        // signature.
+        if (signature == null || exceptions == null) {
+            ExceptionTable exceptionTable = obj.getExceptionTable();
+            if (exceptionTable != null) {
+                exceptionStream = Arrays.stream(exceptionTable.getExceptionNames());
+            }
         }
 
-        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());
+        // 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 && 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
@@ -48,10 +105,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(clazz, 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()) && signatureMatches(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()) && signatureMatches(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 signatureMatches(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(0) == 'T') {
+                if (args[i].charAt(0) != '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/GenericBase.java

@@ -0,0 +1,34 @@
+package ghIssues.issue2040;
+
+public class GenericBase<T> {
+    public T method() throws Exception {
+        throw new Exception();
+    }
+
+    public void method1(int n) {
+    }
+
+    public void method1(T t) throws Throwable {
+        throw new Throwable();
+    }
+
+    public void method2(String s, T t) {
+    }
+
+    public void method2(Number n, T t) {
+    }
+
+    public void method2(int n, T t) throws Throwable {
+        throw new Throwable();
+    }
+
+    public void method2(T t, int n) {
+    }
+
+    public void method2(T t, Number n) {
+    }
+
+    public void method2(T t, String s) throws Exception {
+        throw new Exception();
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue2040/GenericDerived.java

@@ -0,0 +1,23 @@
+package ghIssues.issue2040;
+
+public class GenericDerived extends GenericBase<Byte> {
+    @Override
+    public Byte method() throws Exception {
+        throw new Exception();
+    }
+
+    @Override
+    public void method1(Byte b) throws Throwable {
+        throw new Throwable();
+    }
+
+    @Override
+    public void method2(int n, Byte b) throws Throwable {
+        throw new Throwable();
+    }
+
+    @Override
+    public void method2(Byte b, String s) throws Exception {
+        throw new Exception();
+    }
+}

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

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