Fix for Issue 2040 (spotbugs#2072)

* 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. --------- Co-authored-by: Ádám Balogh <adam.balogh@ericsson.com> Co-authored-by: Kengo TODA <skypencil@gmail.com> Co-authored-by: gtoison <guillaume.toison@tobam.fr> Co-authored-by: Judit Knoll <123470644+JuditKnoll@users.noreply.github.com> Co-authored-by: Judit Knoll <judit.knoll@sigmatechnology.com> Co-authored-by: Judit Knoll <knoll.judit94@gmail.com> Co-authored-by: Guillaume Toison <86775455+gtoison@users.noreply.github.com>
JuditKnoll · Dec 4, 2024 · bc30b44 · bc30b44
1 parent 8d986ba
commit bc30b44
Show file tree

Hide file tree

Showing 13 changed files with 384 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Fix SARIF report's message property in Exception to meet the standard ([#3197](https://github.com/spotbugs/spotbugs/issues/3197))
 - Fixed `FI_FINALIZER_NULLS_FIELDS` FPs for functions called finalize() but not with the correct signature. ([#3207](https://github.com/spotbugs/spotbugs/issues/3207))
 - Fixed an error in the detection of bridge methods causing analysis crashes ([#3208](https://github.com/spotbugs/spotbugs/issues/3208))
+- 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))
 
 ### Cleanup
 - Cleanup thread issue and regex issue in test-harness ([#3130](https://github.com/spotbugs/spotbugs/issues/3130))

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,35 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import org.junit.jupiter.api.Test;
+
+class Issue2040Test extends AbstractIntegrationTest {
+    @Test
+    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");
+
+        assertBugTypeCount("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 3);
+        assertBugInMethodAtLine("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", "Derived", "lambda$lambda2$1", 36);
+        assertBugInMethodAtLine("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", "Derived", "runtimeExThrownFromCatch", 97);
+        assertBugInMethodAtLine("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", "Derived", "runtimeExceptionThrowingMethod", 45);
+
+        assertBugTypeCount("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", 4);
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", "Base", "method");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", "Derived", "throwableThrowingMethod");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", "GenericBase", "method1");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", "GenericBase", "method2");
+
+        assertBugTypeCount("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 5);
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", "Derived", "exceptionThrowingMethod");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", "Derived", "exThrownFromCatch");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", "GenericBase", "method");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", "GenericBase", "method2");
+        assertBugInMethod("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", "Interface", "iMethod");
+    }
+}
diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml
@@ -669,7 +669,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"/>

diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml
@@ -9002,8 +9002,8 @@ Using floating-point variables should not be used as loop counters, as they are
     </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>
+    <ShortDescription>Method lists Exception in its throws clause, but it could be more specific.</ShortDescription>
+    <LongDescription>Method lists Exception in its throws clause, but it could be more specific.</LongDescription>
     <Details>
       <![CDATA[
         <p>
@@ -9019,8 +9019,8 @@ Using floating-point variables should not be used as loop counters, as they are
     </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>
+    <ShortDescription>Method lists Throwable in its throws clause, but it could be more specific.</ShortDescription>
+    <LongDescription>Method lists Throwable in its throws clause, but it could be more specific.</LongDescription>
     <Details>
       <![CDATA[
         <p>

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/SignatureParser.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/SignatureParser.java
@@ -112,6 +112,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);

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,25 @@
 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 edu.umd.cs.findbugs.util.ClassName;
+import edu.umd.cs.findbugs.util.Values;
 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,40 +31,164 @@ public ThrowingExceptions(BugReporter bugReporter) {
         this.bugReporter = bugReporter;
     }
 
+    private String exceptionThrown = null;
+
     @Override
     public void visit(Method obj) {
+        exceptionThrown = null;
+
         if (obj.isSynthetic()) {
             return;
         }
 
-        ExceptionTable exceptionTable = obj.getExceptionTable();
-        if (exceptionTable == null) {
-            return;
+        // 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.
+        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 -> ClassName.toDottedClassName(s.substring(1)));
+            }
         }
 
-        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 is not generic, or it does not throw a generic exception then it has no exception specification in its generic signature.
+        if (signature == null || exceptionStream == null) {
+            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 -> Values.DOTTED_JAVA_LANG_EXCEPTION.equals(s) || Values.DOTTED_JAVA_LANG_THROWABLE.equals(s))
+                    .findAny();
+            if (exception.isPresent() && !parentThrows(getThisClass(), 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(Values.DOTTED_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(Values.DOTTED_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) {
             OpcodeStack.Item item = stack.getStackItem(0);
-            if (item != null) {
-                if ("Ljava/lang/RuntimeException;".equals(item.getSignature())) {
-                    reportBug("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", getXMethod());
-                }
+            if (item != null && "Ljava/lang/RuntimeException;".equals(item.getSignature())) {
+                bugReporter.reportBug(new BugInstance(this, "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", LOW_PRIORITY)
+                        .addClass(this)
+                        .addMethod(getXMethod())
+                        .addSourceLine(this));
             }
+
+        } 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(ClassName::toDottedClassName)
+                    .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 JavaClass clazz, @NonNull Method method, @DottedClassName String exception) {
+        boolean throwsEx = false;
+        try {
+            JavaClass 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.asList(superMethod.get().getExceptionTable().getExceptionNames()).contains(exception);
+                } 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.asList(superMethod.get().getExceptionTable().getExceptionNames()).contains(exception);
+                } 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);
     }
 }
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();
+    }
+}