Skip to content

Commit b1ce608

Browse files
pivovaritpivovarit
authored
Improve hashCode() implementation for Tuple2 (vavr-io#2803)
The current implementation of the hashCode() method for `Tuple2` has a significant flaw that can easily lead to hash collisions, particularly in data structures like `LinkedHashMap`. ```java LinkedHashMap<String, Integer> m1 = LinkedHashMap.of("a", 1, "b", 2); LinkedHashMap<String, Integer> m2 = LinkedHashMap.of("a", 2, "b", 1); System.out.println(m1.hashCode() == m2.hashCode()); // true ``` In this case, _m1_ and _m2_ should ideally produce different hash codes, but they don't. This is because the current implementation of `Tuple#hashCode()` simply sums the hash codes of all elements, which can result in identical hash codes for different tuples. A potential solution is to apply the XOR (^) operator to the hash codes of all elements. XOR is order-sensitive, so it would resolve the issue in the example above, where the order of elements differs between tuples. However, XOR has its limitations and is only a suitable solution for tuples with up to two elements. This is because XOR is a commutative and associative operation, meaning that the XOR of multiple elements can still result in rather bad collisions: ```java Tuple3<Integer, Integer, Integer> t1 = Tuple.of(1, 2, 3); Tuple3<Integer, Integer, Integer> t2 = Tuple.of(1, 3, 2); System.out.println(t1.hashCode() == t2.hashCode()); // true ``` The new implementation is not perfect as well: ```java HashMap<Integer, Integer> hm1 = HashMap.of(0, 1, 1, 2); HashMap<Integer, Integer> hm2 = HashMap.of(0, 2, 1, 3); System.out.println(hm1.hashCode() == hm2.hashCode()); // true ``` But it is imperfect in the same way as standard library: ```java java.util.HashMap<Integer, Integer> jhm1 = new java.util.HashMap<>(); jhm1.put(0, 1); jhm1.put(1, 2); java.util.HashMap<Integer, Integer> jhm2 = new java.util.HashMap<>(); jhm2.put(0, 2); jhm2.put(1, 3); System.out.println(jhm1.hashCode() == jhm2.hashCode()); // true ``` ---- Related: vavr-io#2733
1 parent 5ba70a2 commit b1ce608

File tree

9 files changed

+64
-6
lines changed

9 files changed

+64
-6
lines changed

‎generator/Generator.sc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2236,7 +2236,7 @@ def generateMainClasses(): Unit = {
22362236

22372237
@Override
22382238
public int hashCode() {
2239-
return ${if (i == 0) "1" else if (i == 1) "Objects.hashCode(_1)" else s"""Objects.hash(${(1 to i).gen(j => s"_$j")(", ")})"""};
2239+
return ${if (i == 0) "1" else if (i == 1) "Objects.hashCode(_1)" else if (i == 2) "Objects.hashCode(_1) ^ Objects.hashCode(_2)" else s"""Objects.hash(${(1 to i).gen(j => s"_$j")(", ")})"""};
22402240
}
22412241

22422242
@Override
@@ -3715,7 +3715,7 @@ def generateTestClasses(): Unit = {
37153715
@$test
37163716
public void shouldComputeCorrectHashCode() {
37173717
final int actual = createTuple().hashCode();
3718-
final int expected = ${im.getType("java.util.Objects")}.${if (i == 1) "hashCode" else "hash"}($nullArgs);
3718+
final int expected = ${if (i == 2) s"new ${im.getType("java.util.AbstractMap.SimpleEntry")}<>($nullArgs)" else im.getType("java.util.Objects")}.${if (i == 2) "hashCode()" else if (i == 1) s"hashCode($nullArgs)" else s"hash($nullArgs)"};
37193719
$assertThat(actual).isEqualTo(expected);
37203720
}
37213721

‎src-gen/main/java/io/vavr/Tuple2.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,7 @@ public boolean equals(Object o) {
394394

395395
@Override
396396
public int hashCode() {
397-
return Objects.hash(_1, _2);
397+
return Objects.hashCode(_1) ^ Objects.hashCode(_2);
398398
}
399399

400400
@Override

‎src-gen/test/java/io/vavr/Tuple2Test.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@
3636
import io.vavr.collection.Seq;
3737
import io.vavr.collection.Stream;
3838
import java.util.AbstractMap;
39+
import java.util.AbstractMap.SimpleEntry;
3940
import java.util.Comparator;
4041
import java.util.Map;
41-
import java.util.Objects;
4242
import org.junit.Test;
4343

4444
public class Tuple2Test {
@@ -237,7 +237,7 @@ public void shouldRecognizeNonEqualityPerComponent() {
237237
@Test
238238
public void shouldComputeCorrectHashCode() {
239239
final int actual = createTuple().hashCode();
240-
final int expected = Objects.hash(null, null);
240+
final int expected = new SimpleEntry<>(null, null).hashCode();
241241
assertThat(actual).isEqualTo(expected);
242242
}
243243

‎src/test/java/io/vavr/TupleTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ public void shouldCreateTuple2FromEntry() {
164164
@Test
165165
public void shouldHashTuple2() {
166166
final Tuple2<?, ?> t = tuple2();
167-
assertThat(t.hashCode()).isEqualTo(Objects.hash(t._1, t._2));
167+
assertThat(t.hashCode()).isEqualTo(new AbstractMap.SimpleEntry<>(t._1, t._2).hashCode());
168168
}
169169

170170
@Test

‎src/test/java/io/vavr/collection/AbstractMapTest.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1441,4 +1441,11 @@ public void shouldNonNilGroupByIdentity() {
14411441
assertThat(actual).isEqualTo(expected);
14421442
}
14431443

1444+
// -- hashCode
1445+
1446+
@Override
1447+
@Test
1448+
public void shouldCalculateDifferentHashCodesForDifferentTraversables() {
1449+
assertThat(mapOf('a', 2, 'b', 1).hashCode()).isNotEqualTo(mapOf('a', 1, 'b', 2).hashCode());
1450+
}
14441451
}

‎src/test/java/io/vavr/collection/AbstractMultimapTest.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,8 @@ protected <T1 extends Comparable<T1>, T2> Multimap<T1, T2> emptyMap() {
183183

184184
abstract protected <K extends Comparable<K>, V> Multimap<K, V> mapOfPairs(K k1, V v1, K k, V v2, K k3, V v3);
185185

186+
abstract protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2);
187+
186188
abstract protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K key, V value);
187189

188190
abstract protected <K extends Comparable<? super K>, V> Multimap<K, V> mapOf(java.util.Map<? extends K, ? extends V> map);
@@ -1198,4 +1200,11 @@ public void shouldCollectUsingMultimap() {
11981200
// java.lang.ClassCastException: io.vavr.collection.List$Cons cannot be cast to java.lang.Comparable
11991201
}
12001202

1203+
// -- hashCode
1204+
1205+
@Override
1206+
@Test
1207+
public void shouldCalculateDifferentHashCodesForDifferentTraversables() {
1208+
assertThat(mapOf("a", 2, "b", 1).hashCode()).isNotEqualTo(mapOf("a", 1, "b", 2).hashCode());
1209+
}
12011210
}

‎src/test/java/io/vavr/collection/HashMultimapTest.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,20 @@ protected <K extends Comparable<K>, V> HashMultimap<K, V> mapOfPairs(K k1, V v1,
9898
}
9999
}
100100

101+
@Override
102+
protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2) {
103+
switch (containerType) {
104+
case SEQ:
105+
return HashMultimap.withSeq().of(k1, v1, k2, v2);
106+
case SET:
107+
return HashMultimap.withSet().of(k1, v1, k2, v2);
108+
case SORTED_SET:
109+
return HashMultimap.withSortedSet(Comparators.naturalComparator()).of(k1, v1, k2, v2);
110+
default:
111+
throw new RuntimeException();
112+
}
113+
}
114+
101115
@Override
102116
protected <K extends Comparable<K>, V> HashMultimap<K, V> mapOf(K key, V value) {
103117
switch (containerType) {

‎src/test/java/io/vavr/collection/LinkedHashMultimapTest.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,20 @@ protected <K extends Comparable<K>, V> LinkedHashMultimap<K, V> mapOfPairs(K k1,
9898
}
9999
}
100100

101+
@Override
102+
protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2) {
103+
switch (containerType) {
104+
case SEQ:
105+
return LinkedHashMultimap.withSeq().of(k1, v1, k2, v2);
106+
case SET:
107+
return LinkedHashMultimap.withSet().of(k1, v1, k2, v2);
108+
case SORTED_SET:
109+
return LinkedHashMultimap.withSortedSet(Comparators.naturalComparator()).of(k1, v1, k2, v2);
110+
default:
111+
throw new RuntimeException();
112+
}
113+
}
114+
101115
@Override
102116
protected <K extends Comparable<K>, V> LinkedHashMultimap<K, V> mapOf(K key, V value) {
103117
switch (containerType) {

‎src/test/java/io/vavr/collection/TreeMultimapTest.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,20 @@ protected <K extends Comparable<K>, V> TreeMultimap<K, V> mapOfPairs(K k1, V v1,
9898
}
9999
}
100100

101+
@Override
102+
protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2) {
103+
switch (containerType) {
104+
case SEQ:
105+
return TreeMultimap.withSeq().of(k1, v1, k2, v2);
106+
case SET:
107+
return TreeMultimap.withSet().of(k1, v1, k2, v2);
108+
case SORTED_SET:
109+
return TreeMultimap.withSortedSet(Comparators.naturalComparator()).of(k1, v1, k2, v2);
110+
default:
111+
throw new RuntimeException();
112+
}
113+
}
114+
101115
@Override
102116
protected <K extends Comparable<K>, V> TreeMultimap<K, V> mapOf(K key, V value) {
103117
switch (containerType) {

0 commit comments

Comments
 (0)