Fix FPs with multiple initialization of Singletons (#2951)

* Fix Singleton identification eagerness * small refactor of detector * make singleton identification more accurate * add lazy init check to instance field init * modify testcase
spotbugs · Apr 27, 2024 · bebfdf8 · bebfdf8
1 parent e8a364a
commit bebfdf8
Show file tree

Hide file tree

Showing 12 changed files with 413 additions and 76 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,7 +7,8 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 
 ## Unreleased - 2024-??-??
 ### Fixed
-- Fix FP `SING_SINGLETON_GETTER_NOT_SYNCHRONIZED` with eager instances ([#2932]https://github.com/spotbugs/spotbugs/issues/2932)
+- Fix FP `SING_SINGLETON_GETTER_NOT_SYNCHRONIZED` with eager instances ([#2932](https://github.com/spotbugs/spotbugs/issues/2932))
+- Fix FPs when looking for multiple initialization of Singletons ([#2934](https://github.com/spotbugs/spotbugs/issues/2934))
 - Do not report DLS_DEAD_LOCAL_STORE for Java 21's type switches when switch instruction is TABLESWITCH([#2736](https://github.com/spotbugs/spotbugs/issues/2736))
 - Fix FP `SE_BAD_FIELD` for record fields ([#2935]https://github.com/spotbugs/spotbugs/issues/2935)
 

diff --git a/...ests/src/test/java/edu/umd/cs/findbugs/detect/MultipleInstantiationsOfSingletonsTest.java b/...ests/src/test/java/edu/umd/cs/findbugs/detect/MultipleInstantiationsOfSingletonsTest.java
@@ -11,6 +11,65 @@
 import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
 
 class MultipleInstantiationsOfSingletonsTest extends AbstractIntegrationTest {
+    @Test
+    void abstractClassTest() {
+        performAnalysis("singletons/AbstractClass.class",
+                "singletons/AbstractClass$Nested.class");
+        assertNoBugs();
+    }
+
+    @Test
+    void innerChildInstanceTest() {
+        performAnalysis("singletons/InnerChildInstance.class",
+                "singletons/InnerChildInstance$Unknown.class",
+                "singletons/InnerChildInstance$1.class");
+        assertNoBugs();
+    }
+
+    @Test
+    void innerChildAndMoreInstanceTest() {
+        performAnalysis("singletons/InnerChildAndMoreInstance.class",
+                "singletons/InnerChildAndMoreInstance$1.class",
+                "singletons/InnerChildAndMoreInstance$Unknown.class");
+        assertNumOfSINGBugs("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", 1);
+        assertSINGBug("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", "InnerChildAndMoreInstance");
+
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_INDIRECTLY_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONE_METHOD", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_SERIALIZABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_GETTER_NOT_SYNCHRONIZED", 0);
+    }
+
+    @Test
+    void InnerChildAndMoreInstanceReorderedTest() {
+        performAnalysis("singletons/InnerChildAndMoreInstanceReordered.class",
+                "singletons/InnerChildAndMoreInstanceReordered$1.class",
+                "singletons/InnerChildAndMoreInstanceReordered$Unknown.class");
+        assertNumOfSINGBugs("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", 1);
+        assertSINGBug("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", "InnerChildAndMoreInstanceReordered");
+
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_INDIRECTLY_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONE_METHOD", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_SERIALIZABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_GETTER_NOT_SYNCHRONIZED", 0);
+    }
+
+    @Test
+    void InnerChildAndMoreInstanceNoGetterTest() {
+        performAnalysis("singletons/InnerChildAndMoreInstanceNoGetter.class",
+                "singletons/InnerChildAndMoreInstanceNoGetter$1.class",
+                "singletons/InnerChildAndMoreInstanceNoGetter$Unknown.class");
+        assertNoBugs();
+    }
+
+    @Test
+    void notSingletonWithFactoryMethodTest() {
+        performAnalysis("singletons/NotSingletonWithFactoryMethod.class");
+        assertNoBugs();
+    }
+
     @Test
     void notLazyInitSingletonTest() {
         performAnalysis("singletons/NotLazyInitSingleton.class");
@@ -69,6 +128,32 @@ void protectedConstructorTest() {
         assertNumOfSINGBugs("SING_SINGLETON_GETTER_NOT_SYNCHRONIZED", 0);
     }
 
+    @Test
+    void protectedConstructorStaticInitTest() {
+        performAnalysis("singletons/ProtectedConstructorStaticInit.class");
+        assertNumOfSINGBugs("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", 1);
+        assertSINGBug("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", "ProtectedConstructorStaticInit");
+
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_INDIRECTLY_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONE_METHOD", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_SERIALIZABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_GETTER_NOT_SYNCHRONIZED", 0);
+    }
+
+    @Test
+    void protectedConstructorStaticInitReorderedTest() {
+        performAnalysis("singletons/ProtectedConstructorStaticInitReordered.class");
+        assertNumOfSINGBugs("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", 1);
+        assertSINGBug("SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", "ProtectedConstructorStaticInitReordered");
+
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_INDIRECTLY_IMPLEMENTS_CLONEABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_CLONE_METHOD", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_IMPLEMENTS_SERIALIZABLE", 0);
+        assertNumOfSINGBugs("SING_SINGLETON_GETTER_NOT_SYNCHRONIZED", 0);
+    }
+
     @Test
     void notCloneableButHasCloneMethodTest() {
         performAnalysis("singletons/NotCloneableButHasCloneMethod.class");
@@ -142,6 +227,12 @@ void enumSingletonTest() {
         assertNoBugs();
     }
 
+    @Test
+    void oneEnumSingletonTest() {
+        performAnalysis("singletons/OneEnumSingleton.class");
+        assertNoBugs();
+    }
+
     @Test
     void unconditionalThrowerCloneableTest() {
         performAnalysis("singletons/UnconditionalThrowerCloneable.class");