Fix for Issue 2040

This PR fixes issue spotbugs#2040 by limiting the errors reported by the detector `ThrowingExceptions` to cases where `java.lang.Exception` or `lava.lang.Throwable` appears in the exception specification of th method but it is neither inherited from an overridden method nor is is coming from another method that the method invokes. Syntathic methods are also omitted as well as methods which throw generic exceptions.
baloghadamsoftware · May 30, 2022 · 191f62b · 191f62b
1 parent 68e7dca
commit 191f62b
Show file tree

Hide file tree

Showing 8 changed files with 259 additions and 10 deletions.
diff --git a/CHANGELOG.md b/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))

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2040Test.java b/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));
+    }
+}
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
@@ -3,12 +3,20 @@
 import edu.umd.cs.findbugs.BugInstance;
 import edu.umd.cs.findbugs.BugReporter;
 import edu.umd.cs.findbugs.OpcodeStack;
+import edu.umd.cs.findbugs.ba.AnalysisContext;
 import edu.umd.cs.findbugs.ba.XMethod;
 import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
 
+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,19 +26,63 @@ 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;
+
+        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 (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 (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) {
+        if (exceptionThrown != null) {
+            reportBug("java.lang.Exception".equals(exceptionThrown) ? "THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION"
+                    : "THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod());
+        }
     }
 
     @Override
@@ -42,10 +94,121 @@ public void sawOpcode(int seen) {
                     reportBug("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", getXMethod());
                 }
             }
+        } else if (exceptionThrown != null &&
+                seen == Const.INVOKEVIRTUAL ||
+                seen == Const.INVOKEINTERFACE ||
+                seen == Const.INVOKESTATIC) {
+            XMethod calledMethod = getXMethodOperand();
+            if (calledMethod == null) {
+                return;
+            }
+
+            String[] thrownExceptions = calledMethod.getThrownExceptions();
+            if (thrownExceptions != null && Arrays.stream(thrownExceptions)
+                    .anyMatch(s -> s.replace('/', '.').equals(exceptionThrown))) {
+                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(Method method, String exception) {
+        return parentThrows(klass, method, exception);
+    }
+
+    private boolean parentThrows(JavaClass clazz, Method method, 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(s -> exception.equals(s));
+                } 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(s -> exception.equals(s));
+                } 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();
+        int genIndex = 1;
+        char genSigChar = genSig.charAt(genIndex);
+        int index = 1;
+        char sigChar = sig.charAt(index);
+        while (genSigChar != ')' && sigChar != ')') {
+            switch (genSigChar) {
+            default:
+                if (sigChar != genSigChar) {
+                    return false;
+                }
+                ++index;
+                ++genIndex;
+                break;
+            case 'L':
+                int genSigObjEnd = genSig.indexOf(';', genIndex);
+                int sigObjEnd = sig.indexOf(';', index);
+                if (!genSig.substring(genIndex, genSigObjEnd - genIndex)
+                        .equals(sig.substring(index, sigObjEnd - index))) {
+                    return false;
+                }
+                genIndex = genSigObjEnd + 1;
+                index = sigObjEnd + 1;
+                break;
+            case 'T':
+                if (sigChar != 'L') {
+                    return false;
+                }
+                genSigObjEnd = genSig.indexOf(';', genIndex);
+                sigObjEnd = sig.indexOf(';', index);
+                genIndex = genSigObjEnd + 1;
+                index = sigObjEnd + 1;
+                break;
+            }
+            genSigChar = genSig.charAt(genIndex);
+            sigChar = sig.charAt(index);
+        }
+
+        if (sigChar != genSigChar) {
+            return false;
+        }
+
+        genSigChar = genSig.charAt(++genIndex);
+        sigChar = sig.charAt(++index);
+
+        if (genSigChar == 'T') {
+            return sigChar == 'L';
+        } else {
+            return genSig.substring(genIndex).equals(sig.substring(index));
+        }
     }
 }
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2040/Base.java b/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();
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2040/Caller.java b/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();
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2040/Derived.java b/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; });
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2040/Generic.java b/spotbugsTestCases/src/java/ghIssues/issue2040/Generic.java
@@ -0,0 +1,6 @@
+package ghIssues.issue2040;
+
+public class Generic<E extends Exception> {
+    void method() throws E {
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2040/Interface.java b/spotbugsTestCases/src/java/ghIssues/issue2040/Interface.java
@@ -0,0 +1,5 @@
+package ghIssues.issue2040;
+
+public interface Interface {
+    public void iMethod() throws Exception;
+}