HE_SIGNATURE_DECLARES_HASHING_OF_UNHASHABLE_CLASS not reported for so…

…me classes (spotbugs#2538) * test: issue spotbugs#2402 reproducer HE_SIGNATURE_DECLARES_HASHING_OF_UNHASHABLE_CLASS is reported for TestA, TestC and TestD, not for TestB * fix: store potential bugs by class name and bug type Updated the key for potential bugs from only the class name to also the bug type. * removed commented out code * applied spotless
trancexpress · Aug 28, 2023 · ffa1248 · ffa1248
1 parent b035207
commit ffa1248
Show file tree

Hide file tree

Showing 7 changed files with 166 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,6 +31,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Fix XML Output incorrectly escaped in Eclipse Bug Info view ([#2520](https://github.com/spotbugs/spotbugs/pull/2520))
 - Updated the MS_EXPOSE_REP description to mention mutable objects, not just arrays ([#1669](https://github.com/spotbugs/spotbugs/issues/1669))
 - Described Configuration option frc.suspicious for bug RC_REF_COMPARISON in bug description ([#2297](https://github.com/spotbugs/spotbugs/issues/2297))
+- Fixed FindHEMismatch not reporting HE_SIGNATURE_DECLARES_HASHING_OF_UNHASHABLE_CLASS for some classes ([#2402](https://github.com/spotbugs/spotbugs/issues/2402))
 
 ### 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.

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue2402Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue2402Test.java
@@ -0,0 +1,32 @@
+package edu.umd.cs.findbugs.ba;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import edu.umd.cs.findbugs.SortedBugCollection;
+
+public class Issue2402Test extends AbstractIntegrationTest {
+
+    @Test
+    public void test() {
+        performAnalysis("ghIssues/issue2402/TestA.class",
+                "ghIssues/issue2402/TestA$UMap.class",
+                "ghIssues/issue2402/TestB.class",
+                "ghIssues/issue2402/TestB$UMap.class",
+                "ghIssues/issue2402/TestC.class",
+                "ghIssues/issue2402/TestC$UMap.class",
+                "ghIssues/issue2402/TestD.class",
+                "ghIssues/issue2402/TestD$UMap.class");
+
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder().bugType("HE_SIGNATURE_DECLARES_HASHING_OF_UNHASHABLE_CLASS").build();
+
+        SortedBugCollection bugCollection = (SortedBugCollection) getBugCollection();
+        assertThat(bugCollection, containsExactly(4, bugTypeMatcher));
+    }
+
+}
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindHEmismatch.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindHEmismatch.java
@@ -22,6 +22,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -110,7 +111,7 @@ public class FindHEmismatch extends OpcodeStackDetector implements StatelessDete
 
     final HashSet<String> nonHashableClasses;
 
-    final Map<String, BugInstance> potentialBugs;
+    final Map<PotentialBugKey, BugInstance> potentialBugs;
 
     private final BugReporter bugReporter;
 
@@ -563,11 +564,12 @@ private void check(int pos) {
         if (type.isAbstract() || type.isInterface()) {
             priority++;
         }
-        potentialBugs.put(
-                type.getClassName(),
-                new BugInstance(this, "HE_USE_OF_UNHASHABLE_CLASS", priority).addClassAndMethod(this)
-                        .addTypeOfNamedClass(type.getClassName()).describe(TypeAnnotation.UNHASHABLE_ROLE).addCalledMethod(this)
-                        .addSourceLine(this));
+
+        BugInstance bugInstance = new BugInstance(this, "HE_USE_OF_UNHASHABLE_CLASS", priority).addClassAndMethod(this)
+                .addTypeOfNamedClass(type.getClassName()).describe(TypeAnnotation.UNHASHABLE_ROLE).addCalledMethod(this)
+                .addSourceLine(this);
+        PotentialBugKey key = new PotentialBugKey(type.getClassName(), bugInstance);
+        potentialBugs.put(key, bugInstance);
     }
 
     @CheckForNull
@@ -638,18 +640,52 @@ public void visit(Signature obj) {
             bug = new BugInstance(this, "HE_SIGNATURE_DECLARES_HASHING_OF_UNHASHABLE_CLASS", priority).addClass(this)
                     .addClass(this).addTypeOfNamedClass(className).describe(TypeAnnotation.UNHASHABLE_ROLE);
         }
-        potentialBugs.put(className, bug);
+        PotentialBugKey key = new PotentialBugKey(className, bug);
+        potentialBugs.put(key, bug);
     }
 
     @Override
     public void report() {
-        for (Map.Entry<String, BugInstance> e : potentialBugs.entrySet()) {
-            if (!isHashableClassName(e.getKey())) {
+        for (Map.Entry<PotentialBugKey, BugInstance> e : potentialBugs.entrySet()) {
+            if (!isHashableClassName(e.getKey().className)) {
                 BugInstance bug = e.getValue();
 
                 bugReporter.reportBug(bug);
             }
         }
 
     }
+
+    private static class PotentialBugKey {
+        private final String className;
+        private final String bugType;
+
+        public PotentialBugKey(String className, BugInstance bugInstance) {
+            this.className = className;
+            this.bugType = bugInstance.getType();
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(bugType, className);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            }
+
+            if (obj == null) {
+                return false;
+            }
+
+            if (getClass() != obj.getClass()) {
+                return false;
+            }
+
+            PotentialBugKey other = (PotentialBugKey) obj;
+            return Objects.equals(bugType, other.bugType) && Objects.equals(className, other.className);
+        }
+    }
 }
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2402/TestA.java b/spotbugsTestCases/src/java/ghIssues/issue2402/TestA.java
@@ -0,0 +1,22 @@
+package ghIssues.issue2402;
+import java.util.HashMap;
+public class TestA {
+    static class UMap extends HashMap<TestA, String> {}
+    ;
+    static HashMap<TestA, String> m =
+            new HashMap<TestA, String>();
+
+    static int foo(HashMap<TestA, String> map) {
+        return map.size();
+    }
+    @Override
+    public boolean equals(Object o) {
+        return hashCode() == o.hashCode();
+    }
+    public static String add(TestA b, String s) {
+        return m.put(b, s);
+    }
+    public static String get(TestA b) {
+        return m.get(b);
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2402/TestB.java b/spotbugsTestCases/src/java/ghIssues/issue2402/TestB.java
@@ -0,0 +1,22 @@
+package ghIssues.issue2402;
+import java.util.HashMap;
+public class TestB {
+    static class UMap extends HashMap<TestB, String> {}
+    ;
+    static HashMap<TestB, String> m =
+            new HashMap<TestB, String>();
+
+    static int foo(HashMap<TestB, String> map) {
+        return map.size();
+    }
+    @Override
+    public boolean equals(Object o) {
+        return hashCode() == o.hashCode();
+    }
+    public static String add(TestB b, String s) {
+        return m.put(b, s);
+    }
+    public static String get(TestB b) {
+        return m.get(b);
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2402/TestC.java b/spotbugsTestCases/src/java/ghIssues/issue2402/TestC.java
@@ -0,0 +1,22 @@
+package ghIssues.issue2402;
+import java.util.HashMap;
+public class TestC {
+    static class UMap extends HashMap<TestC, String> {}
+    ;
+    static HashMap<TestC, String> m =
+            new HashMap<TestC, String>();
+
+    static int foo(HashMap<TestC, String> map) {
+        return map.size();
+    }
+    @Override
+    public boolean equals(Object o) {
+        return hashCode() == o.hashCode();
+    }
+    public static String add(TestC b, String s) {
+        return m.put(b, s);
+    }
+    public static String get(TestC b) {
+        return m.get(b);
+    }
+}
diff --git a/spotbugsTestCases/src/java/ghIssues/issue2402/TestD.java b/spotbugsTestCases/src/java/ghIssues/issue2402/TestD.java
@@ -0,0 +1,22 @@
+package ghIssues.issue2402;
+import java.util.HashMap;
+public class TestD {
+    static class UMap extends HashMap<TestD, String> {}
+    ;
+    static HashMap<TestD, String> m =
+            new HashMap<TestD, String>();
+
+    static int foo(HashMap<TestD, String> map) {
+        return map.size();
+    }
+    @Override
+    public boolean equals(Object o) {
+        return hashCode() == o.hashCode();
+    }
+    public static String add(TestD b, String s) {
+        return m.put(b, s);
+    }
+    public static String get(TestD b) {
+        return m.get(b);
+    }
+}