Fix the hash code of maps.

0.20.0-release-notes.md

@@ -25,6 +25,9 @@ Draft release notes for Elvish 0.20.0.
 -   `has-value $li $v` now works correctly when `$li` is a list and `$v` is a
     composite value, like a map or a list.
 
+-   A bug with how the hash code of a map was computed could lead to unexpected
+    results when using maps as map keys; it has now been fixed.
+
 # Breaking changes
 
 -   The `except` keyword in the `try` command was deprecated since 0.18.0 and is

pkg/eval/vals/hash.go

@@ -50,11 +50,20 @@ func Hash(v any) uint32 {
 		}
 		return h
 	case Map:
-		h := hash.DJBInit
+		// The iteration order of maps only depends on the hash of the keys. It
+		// is almost deterministic, with only one exception: when two keys have
+		// the same hash, they get produced in insertion order. As a result, it
+		// is possible for two maps that should be considered equal to produce
+		// entries in different orders.
+		//
+		// So instead of using hash.DJBCombine, combine the hash result from
+		// each key-value pair by summing, so that the order doesn't matter.
+		//
+		// TODO: This may not have very good hashing properties.
+		var h uint32
 		for it := v.Iterator(); it.HasElem(); it.Next() {
 			k, v := it.Elem()
-			h = hash.DJBCombine(h, Hash(k))
-			h = hash.DJBCombine(h, Hash(v))
+			h += hash.DJB(Hash(k), Hash(v))
 		}
 		return h
 	case StructMap:

pkg/eval/vals/hash_test.go

@@ -8,6 +8,7 @@ import (
 	"unsafe"
 
 	"src.elv.sh/pkg/persistent/hash"
+	"src.elv.sh/pkg/persistent/hashmap"
 	"src.elv.sh/pkg/tt"
 )
 
@@ -39,3 +40,18 @@ func TestHash(t *testing.T) {
 		Args(nonHasher{}).Rets(uint32(0)),
 	})
 }
+
+func TestHash_EqualMapsWithDifferentInternal(t *testing.T) {
+	// The internal representation of maps with the same value is not always the
+	// same: when some keys of the map have the same hash, their values are
+	// stored in the insertion order.
+	//
+	// To reliably test this case, we construct maps with a custom hashing
+	// function.
+	m0 := hashmap.New(Equal, func(v any) uint32 { return 0 })
+	m1 := m0.Assoc("k1", "v1").Assoc("k2", "v2")
+	m2 := m0.Assoc("k2", "v2").Assoc("k1", "v1")
+	if h1, h2 := Hash(m1), Hash(m2); h1 != h2 {
+		t.Errorf("%v != %v", h1, h2)
+	}
+}