Fix CT_CONSTRUCTOR_THROW FP when Supertype has final finalize (#2666)

* Fix constructorthrow FP when superclass has final finalize * Add changelog entry * spotlessApply
spotbugs · Oct 26, 2023 · 674a7d0 · 674a7d0
1 parent a834b53
commit 674a7d0
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Fix exception escapes when calling functions of JUnit Assert or Assertions ([[#2640](https://github.com/spotbugs/spotbugs/issues/2640)])
 - Fixed an error in the SARIF export when a bug annotation is missing ([[#2632](https://github.com/spotbugs/spotbugs/issues/2632)])
 - Fixed false positive RV_EXCEPTION_NOT_THROWN when asserting to exception throws ([[#2628](https://github.com/spotbugs/spotbugs/issues/2628)])
+- Fix false positive CT_CONSTRUCTOR_THROW when supertype has final finalize ([[#2665](https://github.com/spotbugs/spotbugs/issues/2665)])
 
 ### Build
 - Fix deprecated GHA on '::set-output' by using GITHUB_OUTPUT ([[#2651](https://github.com/spotbugs/spotbugs/pull/2651)])

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/ConstructorThrowTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/ConstructorThrowTest.java
@@ -190,6 +190,19 @@ void testGoodConstructorThrowCheck11() {
         assertNumOfCTBugs(0);
     }
 
+    @Test
+    void testGoodConstructorThrowCheck12() {
+        performAnalysis("constructorthrow/ConstructorThrowNegativeTest12.class");
+        assertNumOfCTBugs(0);
+    }
+
+    @Test
+    void testGoodConstructorThrowCheck13() {
+        performAnalysis("constructorthrow/ConstructorThrowNegativeTest12.class",
+                "constructorthrow/ConstructorThrowNegativeTest13.class");
+        assertNumOfCTBugs(0);
+    }
+
     private void assertNumOfCTBugs(int num) {
         final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
                 .bugType("CT_CONSTRUCTOR_THROW").build();

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ConstructorThrow.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ConstructorThrow.java
@@ -78,17 +78,29 @@ public void visit(JavaClass obj) {
             isFinalClass = true;
             return;
         }
+
+        isFinalFinalizer = hasFinalFinalizer(obj);
+        try {
+            for (JavaClass cl : obj.getSuperClasses()) {
+                isFinalFinalizer |= hasFinalFinalizer(cl);
+            }
+        } catch (ClassNotFoundException e) {
+            AnalysisContext.reportMissingClass(e);
+        }
+
         for (Method m : obj.getMethods()) {
             doVisitMethod(m);
-            // Check for final finalizer.
-            // Signature of the finalizer is also needed to be checked
-            if ("finalize".equals(m.getName()) && "()V".equals(m.getSignature()) && m.isFinal()) {
-                isFinalFinalizer = true;
-            }
         }
         isFirstPass = false;
     }
 
+    private static boolean hasFinalFinalizer(JavaClass jc) {
+        // Check for final finalizer.
+        // Signature of the finalizer is also needed to be checked
+        return Arrays.stream(jc.getMethods())
+                .anyMatch(m -> "finalize".equals(m.getName()) && "()V".equals(m.getSignature()) && m.isFinal());
+    }
+
     @Override
     public void visit(Method obj) {
         if (isFinalClass || isFinalFinalizer) {
@@ -135,7 +147,6 @@ public void sawOpcode(int seen) {
     /**
      * Reports ContructorThrow bug if there is an unhandled unchecked exception thrown directly or indirectly
      * from the currently visited method.
-     *
      * If the exception is thrown directly, the bug is reported at the throw.
      * If the exception is thrown indirectly (through a method call), the bug is reported at the call of the method
      * which throws the exception.

diff --git a/spotbugsTestCases/src/java/constructorthrow/ConstructorThrowNegativeTest12.java b/spotbugsTestCases/src/java/constructorthrow/ConstructorThrowNegativeTest12.java
@@ -0,0 +1,13 @@
+package constructorthrow;
+
+/**
+ * However the constructor throws an unchecked exception, there is an empty finalize in this class.
+ */
+public class ConstructorThrowNegativeTest12 {
+    public ConstructorThrowNegativeTest12() {
+        throw new RuntimeException(); // No error, final finalize.
+    }
+
+    @Override
+    protected final void finalize(){}
+}
diff --git a/spotbugsTestCases/src/java/constructorthrow/ConstructorThrowNegativeTest13.java b/spotbugsTestCases/src/java/constructorthrow/ConstructorThrowNegativeTest13.java
@@ -0,0 +1,11 @@
+package constructorthrow;
+
+/**
+ * However there is an unchecked exception thrown from the constructor, the superclass contains a final finalize.
+ * @see <a href="https://github.com/spotbugs/spotbugs/issues/2665">GitHub issue</a>
+ */
+public class ConstructorThrowNegativeTest13 extends ConstructorThrowNegativeTest12 {
+    public ConstructorThrowNegativeTest13() {
+        throw new RuntimeException(); // No error, final finalize.
+    }
+}