From d273cbdb1460a61f434685833af13fd92cbab30b Mon Sep 17 00:00:00 2001 From: PratikDhanave Date: Sat, 18 Jul 2026 08:27:20 +0530 Subject: [PATCH 1/2] Disambiguate hash collisions in internal/hashmap via Hasher.Equal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Hasher interface requires an Equal method and Map stores each entry's key, but Load, Delete and Set keyed purely on the hash value and never called Equal. When two distinct keys hashed to the same value: - Set overwrote the earlier key's entry (silent data loss), - Load returned another key's value — even for a key never stored, - Delete removed a different key's entry. Store entries in per-hash buckets and use Hasher.Equal to select the matching entry in Load, Delete and Set, honoring the interface contract the stored keys and Equal method were already there to support. No exported method signature changes. Adds the package's first test, covering distinct keys that share a hash. --- internal/hashmap/hashmap.go | 82 ++++++++++++++++++++++---------- internal/hashmap/hashmap_test.go | 48 +++++++++++++++++++ 2 files changed, 105 insertions(+), 25 deletions(-) create mode 100644 internal/hashmap/hashmap_test.go diff --git a/internal/hashmap/hashmap.go b/internal/hashmap/hashmap.go index 2332ffc1..d7c66c62 100644 --- a/internal/hashmap/hashmap.go +++ b/internal/hashmap/hashmap.go @@ -20,14 +20,16 @@ type entry[K, V any] struct { // Map is a mapping from keys of type K to values of type V, // using key-equivalence relation H. type Map[K, V any] struct { - entries map[uint64]entry[K, V] + // entries maps each key hash to the bucket of entries sharing that hash. + // Buckets disambiguate hash collisions via the Hasher's Equal method. + entries map[uint64][]entry[K, V] h Hasher[K] } // NewMap returns a new mapping. func NewMap[K, V any](h Hasher[K]) *Map[K, V] { return &Map[K, V]{ - entries: make(map[uint64]entry[K, V]), + entries: make(map[uint64][]entry[K, V]), h: h, } } @@ -35,9 +37,11 @@ func NewMap[K, V any](h Hasher[K]) *Map[K, V] { // All returns an iterator over the key/value entries of the map in undefined order. func (m *Map[K, V]) All() iter.Seq2[K, V] { return func(yield func(K, V) bool) { - for _, entry := range m.entries { - if !yield(entry.key, entry.value) { - return + for _, bucket := range m.entries { + for _, entry := range bucket { + if !yield(entry.key, entry.value) { + return + } } } } @@ -45,15 +49,28 @@ func (m *Map[K, V]) All() iter.Seq2[K, V] { // Load returns the map entry for the given key. func (m *Map[K, V]) Load(key K) (V, bool) { - entry, ok := m.entries[m.h.Hash(key)] - return entry.value, ok + for _, entry := range m.entries[m.h.Hash(key)] { + if m.h.Equal(entry.key, key) { + return entry.value, true + } + } + var zero V + return zero, false } -// Delete removes th//e entry with the given key, if any. It returns true if the entry was found. +// Delete removes the entry with the given key, if any. It returns true if the entry was found. func (m *Map[K, V]) Delete(key K) bool { hash := m.h.Hash(key) - if _, exists := m.entries[hash]; exists { - delete(m.entries, hash) + bucket := m.entries[hash] + for i, entry := range bucket { + if !m.h.Equal(entry.key, key) { + continue + } + if len(bucket) == 1 { + delete(m.entries, hash) + } else { + m.entries[hash] = append(bucket[:i], bucket[i+1:]...) + } return true } return false @@ -62,9 +79,11 @@ func (m *Map[K, V]) Delete(key K) bool { // Keys returns an iterator over the map keys in unspecified order. func (m *Map[K, V]) Keys() iter.Seq[K] { return func(yield func(K) bool) { - for _, entry := range m.entries { - if !yield(entry.key) { - return + for _, bucket := range m.entries { + for _, entry := range bucket { + if !yield(entry.key) { + return + } } } } @@ -73,9 +92,11 @@ func (m *Map[K, V]) Keys() iter.Seq[K] { // Values returns an iterator over the map values in unspecified order. func (m *Map[K, V]) Values() iter.Seq[V] { return func(yield func(V) bool) { - for _, entry := range m.entries { - if !yield(entry.value) { - return + for _, bucket := range m.entries { + for _, entry := range bucket { + if !yield(entry.value) { + return + } } } } @@ -83,16 +104,25 @@ func (m *Map[K, V]) Values() iter.Seq[V] { // Len returns the number of map entries. func (m *Map[K, V]) Len() int { - return len(m.entries) + n := 0 + for _, bucket := range m.entries { + n += len(bucket) + } + return n } // Set updates the map entry for key to value, and returns the previous entry, if any. func (m *Map[K, V]) Set(key K, value V) (prev V) { hash := m.h.Hash(key) - if entry, exists := m.entries[hash]; exists { - prev = entry.value + bucket := m.entries[hash] + for i, entry := range bucket { + if m.h.Equal(entry.key, key) { + prev = entry.value + bucket[i].value = value + return prev + } } - m.entries[hash] = entry[K, V]{key, value} + m.entries[hash] = append(bucket, entry[K, V]{key, value}) return prev } @@ -105,12 +135,14 @@ func (m *Map[K, V]) Clear() { func (m *Map[K, V]) String() string { s := "{" first := true - for _, entry := range m.entries { - if !first { - s += " " + for _, bucket := range m.entries { + for _, entry := range bucket { + if !first { + s += " " + } + s += fmt.Sprintf("%v: %v", entry.key, entry.value) + first = false } - s += fmt.Sprintf("%v: %v", entry.key, entry.value) - first = false } s += "}" return s diff --git a/internal/hashmap/hashmap_test.go b/internal/hashmap/hashmap_test.go new file mode 100644 index 00000000..130929b4 --- /dev/null +++ b/internal/hashmap/hashmap_test.go @@ -0,0 +1,48 @@ +// Copyright (c) Microsoft. All rights reserved. + +package hashmap_test + +import ( + "testing" + + "github.com/microsoft/agent-framework-go/internal/hashmap" +) + +// collidingHasher forces every key to the same hash so distinct keys land in the +// same bucket, exercising the hash-collision path. Equal still distinguishes keys. +type collidingHasher struct{} + +func (collidingHasher) Hash(string) uint64 { return 1 } +func (collidingHasher) Equal(a, b string) bool { return a == b } + +func TestMap_DistinctKeysWithCollidingHashCoexist(t *testing.T) { + m := hashmap.NewMap[string, string](collidingHasher{}) + m.Set("alice", "a") + m.Set("bob", "b") + + if got, ok := m.Load("alice"); !ok || got != "a" { + t.Errorf(`Load("alice") = (%q, %v), want ("a", true)`, got, ok) + } + if got, ok := m.Load("bob"); !ok || got != "b" { + t.Errorf(`Load("bob") = (%q, %v), want ("b", true)`, got, ok) + } + if got := m.Len(); got != 2 { + t.Errorf("Len() = %d, want 2", got) + } + + // A missing key that collides with stored keys must not resolve to a stored value. + if got, ok := m.Load("carol"); ok { + t.Errorf(`Load("carol") = (%q, %v), want ("", false)`, got, ok) + } + + // Deleting one colliding key must not remove the other. + if !m.Delete("alice") { + t.Error(`Delete("alice") = false, want true`) + } + if _, ok := m.Load("alice"); ok { + t.Error(`Load("alice") after delete still found, want not found`) + } + if got, ok := m.Load("bob"); !ok || got != "b" { + t.Errorf(`Load("bob") after deleting alice = (%q, %v), want ("b", true)`, got, ok) + } +} From 998a9d84ac64cb7aad126f4c30515acbf477c87a Mon Sep 17 00:00:00 2001 From: PratikDhanave Date: Sat, 18 Jul 2026 09:57:17 +0530 Subject: [PATCH 2/2] Track entry count for O(1) Len and clear vacated slot on delete Address review feedback: - Len() is O(1) again via an entry count updated in Set/Delete/Clear, instead of summing bucket lengths. - Delete shifts the tail down and zeroes the vacated slot so the removed key/value are not retained by the bucket's backing array. Adds a test covering Len across insert/update/delete/clear. --- internal/hashmap/hashmap.go | 24 +++++++++++++++--------- internal/hashmap/hashmap_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/internal/hashmap/hashmap.go b/internal/hashmap/hashmap.go index d7c66c62..779d92cd 100644 --- a/internal/hashmap/hashmap.go +++ b/internal/hashmap/hashmap.go @@ -23,7 +23,10 @@ type Map[K, V any] struct { // entries maps each key hash to the bucket of entries sharing that hash. // Buckets disambiguate hash collisions via the Hasher's Equal method. entries map[uint64][]entry[K, V] - h Hasher[K] + // count is the total number of entries across all buckets, kept so Len is + // O(1) rather than summing bucket lengths. + count int + h Hasher[K] } // NewMap returns a new mapping. @@ -62,15 +65,20 @@ func (m *Map[K, V]) Load(key K) (V, bool) { func (m *Map[K, V]) Delete(key K) bool { hash := m.h.Hash(key) bucket := m.entries[hash] - for i, entry := range bucket { - if !m.h.Equal(entry.key, key) { + for i, e := range bucket { + if !m.h.Equal(e.key, key) { continue } if len(bucket) == 1 { delete(m.entries, hash) } else { - m.entries[hash] = append(bucket[:i], bucket[i+1:]...) + // Shift the tail down and clear the vacated slot so the removed + // entry's key/value are not retained by the backing array. + copy(bucket[i:], bucket[i+1:]) + bucket[len(bucket)-1] = entry[K, V]{} + m.entries[hash] = bucket[:len(bucket)-1] } + m.count-- return true } return false @@ -104,11 +112,7 @@ func (m *Map[K, V]) Values() iter.Seq[V] { // Len returns the number of map entries. func (m *Map[K, V]) Len() int { - n := 0 - for _, bucket := range m.entries { - n += len(bucket) - } - return n + return m.count } // Set updates the map entry for key to value, and returns the previous entry, if any. @@ -123,11 +127,13 @@ func (m *Map[K, V]) Set(key K, value V) (prev V) { } } m.entries[hash] = append(bucket, entry[K, V]{key, value}) + m.count++ return prev } func (m *Map[K, V]) Clear() { clear(m.entries) + m.count = 0 } // String returns a string representation of the map's entries in unspecified order. diff --git a/internal/hashmap/hashmap_test.go b/internal/hashmap/hashmap_test.go index 130929b4..872ea825 100644 --- a/internal/hashmap/hashmap_test.go +++ b/internal/hashmap/hashmap_test.go @@ -46,3 +46,35 @@ func TestMap_DistinctKeysWithCollidingHashCoexist(t *testing.T) { t.Errorf(`Load("bob") after deleting alice = (%q, %v), want ("b", true)`, got, ok) } } + +func TestMap_LenTracksSetUpdateDeleteClear(t *testing.T) { + m := hashmap.NewMap[string, int](collidingHasher{}) + if got := m.Len(); got != 0 { + t.Fatalf("Len() = %d, want 0", got) + } + m.Set("a", 1) + m.Set("b", 2) + if got := m.Len(); got != 2 { + t.Fatalf("after two inserts Len() = %d, want 2", got) + } + // Updating an existing key must not change the count. + m.Set("a", 10) + if got := m.Len(); got != 2 { + t.Fatalf("after updating an existing key Len() = %d, want 2", got) + } + if !m.Delete("a") { + t.Fatal(`Delete("a") = false, want true`) + } + if got := m.Len(); got != 1 { + t.Fatalf("after delete Len() = %d, want 1", got) + } + // Deleting a missing key must not change the count. + m.Delete("missing") + if got := m.Len(); got != 1 { + t.Fatalf("after deleting a missing key Len() = %d, want 1", got) + } + m.Clear() + if got := m.Len(); got != 0 { + t.Fatalf("after Clear Len() = %d, want 0", got) + } +}