Merge pull request #19497 from michaelnebel/csharp/gethashcode

C#: Improve the query `cs/gethashcode-is-not-defined`.

Author: michaelnebel

csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll

@@ -159,6 +159,15 @@ class SystemCollectionsGenericIDictionaryInterface extends SystemCollectionsGene
   }
 }
 
+/** The ``System.Collections.Generic.IReadOnlyDictionary`2`` interface. */
+class SystemCollectionsGenericIReadOnlyDictionaryInterface extends SystemCollectionsGenericUnboundGenericInterface
+{
+  SystemCollectionsGenericIReadOnlyDictionaryInterface() {
+    this.hasName("IReadOnlyDictionary`2") and
+    this.getNumberOfTypeParameters() = 2
+  }
+}
+
 /** The ``System.Collections.Generic.IReadOnlyCollection`1`` interface. */
 class SystemCollectionsGenericIReadOnlyCollectionTInterface extends SystemCollectionsGenericUnboundGenericInterface
 {
@@ -176,3 +185,11 @@ class SystemCollectionsGenericIReadOnlyListTInterface extends SystemCollectionsG
     this.getNumberOfTypeParameters() = 1
   }
 }
+
+/** The ``System.Collections.Generic.HashSet`1`` class. */
+class SystemCollectionsGenericHashSetClass extends SystemCollectionsGenericUnboundGenericClass {
+  SystemCollectionsGenericHashSetClass() {
+    this.hasName("HashSet`1") and
+    this.getNumberOfTypeParameters() = 1
+  }
+}

csharp/ql/src/Likely Bugs/HashedButNoHash.ql

@@ -11,24 +11,44 @@
 
 import csharp
 import semmle.code.csharp.frameworks.System
+import semmle.code.csharp.frameworks.system.Collections
+import semmle.code.csharp.frameworks.system.collections.Generic
 
-predicate dictionary(ConstructedType constructed) {
-  exists(UnboundGenericType dict |
-    dict.hasFullyQualifiedName("System.Collections.Generic", "Dictionary`2") and
-    constructed = dict.getAConstructedGeneric()
+/**
+ * Holds if `t` is a dictionary type.
+ */
+predicate dictionary(ValueOrRefType t) {
+  exists(Type base | base = t.getABaseType*().getUnboundDeclaration() |
+    base instanceof SystemCollectionsGenericIDictionaryInterface or
+    base instanceof SystemCollectionsGenericIReadOnlyDictionaryInterface or
+    base instanceof SystemCollectionsIDictionaryInterface
   )
 }
 
-predicate hashtable(Class c) { c.hasFullyQualifiedName("System.Collections", "Hashtable") }
+/**
+ * Holds if `c` is a hashset type.
+ */
+predicate hashSet(ValueOrRefType t) {
+  t.getABaseType*().getUnboundDeclaration() instanceof SystemCollectionsGenericHashSetClass
+}
 
-predicate hashstructure(Type t) { hashtable(t) or dictionary(t) }
+predicate hashStructure(Type t) { dictionary(t) or hashSet(t) }
 
-predicate hashAdd(Expr e) {
+/**
+ * Holds if the expression `e` relies on `GetHashCode()` implementation.
+ * That is, if the call assumes that `e1.Equals(e2)` implies `e1.GetHashCode() == e2.GetHashCode()`.
+ */
+predicate usesHashing(Expr e) {
   exists(MethodCall mc, string name |
-    (name = "Add" or name = "ContainsKey") and
+    name = ["Add", "Contains", "ContainsKey", "Remove", "TryAdd", "TryGetValue"] and
     mc.getArgument(0) = e and
     mc.getTarget().hasName(name) and
-    hashstructure(mc.getTarget().getDeclaringType())
+    hashStructure(mc.getTarget().getDeclaringType())
+  )
+  or
+  exists(IndexerCall ic |
+    ic.getArgument(0) = e and
+    dictionary(ic.getTarget().getDeclaringType())
   )
 }
 
@@ -46,7 +66,7 @@ predicate hashCall(Expr e) {
 
 from Expr e, Type t
 where
-  (hashAdd(e) or hashCall(e)) and
+  (usesHashing(e) or hashCall(e)) and
   e.getType() = t and
   eqWithoutHash(t)
 select e,

csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The precision of the query `cs/gethashcode-is-not-defined` has been improved (false negative reduction). Calls to more methods (and indexers) that rely on the invariant `e1.Equals(e2)` implies `e1.GetHashCode() == e2.GetHashCode()` are taken into account.

csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs

@@ -6,9 +6,27 @@ public class Test
     public void M()
     {
         var h = new Hashtable();
-        h.Add(this, null); // BAD
+        h.Add(this, null); // $ Alert
+        h.Contains(this); // $ Alert
+        h.ContainsKey(this); // $ Alert
+        h[this] = null; // $ Alert
+        h.Remove(this); // $ Alert
+
+        var l = new List<Test>();
+        l.Add(this); // Good
+
         var d = new Dictionary<Test, bool>();
-        d.Add(this, false); // BAD
+        d.Add(this, false); // $ Alert
+        d.ContainsKey(this); // $ Alert
+        d[this] = false; // $ Alert
+        d.Remove(this); // $ Alert
+        d.TryAdd(this, false); // $ Alert
+        d.TryGetValue(this, out bool _); // $ Alert
+
+        var hs = new HashSet<Test>();
+        hs.Add(this); // $ Alert
+        hs.Contains(this); // $ Alert
+        hs.Remove(this); // $ Alert
     }
 
     public override bool Equals(object other)

csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected

@@ -1,2 +1,14 @@
 | HashedButNoHash.cs:9:15:9:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
-| HashedButNoHash.cs:11:15:11:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:10:20:10:23 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:11:23:11:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:12:11:12:14 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:13:18:13:21 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:19:15:19:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:20:23:20:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:21:11:21:14 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:22:18:22:21 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:23:18:23:21 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:24:23:24:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:27:16:27:19 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:28:21:28:24 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |
+| HashedButNoHash.cs:29:19:29:22 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). |

csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.qlref

@@ -1 +1,3 @@
-Likely Bugs/HashedButNoHash.ql
+query: Likely Bugs/HashedButNoHash.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql
+