Skip to content

Commit

Permalink
Improve hashCode() implementation for Tuple2 (#2803)
Browse files Browse the repository at this point in the history
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: #2733
  • Loading branch information
pivovarit authored Aug 28, 2024
1 parent 5ba70a2 commit b1ce608
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 6 deletions.
4 changes: 2 additions & 2 deletions generator/Generator.sc
Original file line number Diff line number Diff line change
Expand Up @@ -2236,7 +2236,7 @@ def generateMainClasses(): Unit = {

@Override
public int hashCode() {
return ${if (i == 0) "1" else if (i == 1) "Objects.hashCode(_1)" else s"""Objects.hash(${(1 to i).gen(j => s"_$j")(", ")})"""};
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")(", ")})"""};
}

@Override
Expand Down Expand Up @@ -3715,7 +3715,7 @@ def generateTestClasses(): Unit = {
@$test
public void shouldComputeCorrectHashCode() {
final int actual = createTuple().hashCode();
final int expected = ${im.getType("java.util.Objects")}.${if (i == 1) "hashCode" else "hash"}($nullArgs);
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)"};
$assertThat(actual).isEqualTo(expected);
}

Expand Down
2 changes: 1 addition & 1 deletion src-gen/main/java/io/vavr/Tuple2.java
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ public boolean equals(Object o) {

@Override
public int hashCode() {
return Objects.hash(_1, _2);
return Objects.hashCode(_1) ^ Objects.hashCode(_2);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions src-gen/test/java/io/vavr/Tuple2Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
import io.vavr.collection.Seq;
import io.vavr.collection.Stream;
import java.util.AbstractMap;
import java.util.AbstractMap.SimpleEntry;
import java.util.Comparator;
import java.util.Map;
import java.util.Objects;
import org.junit.Test;

public class Tuple2Test {
Expand Down Expand Up @@ -237,7 +237,7 @@ public void shouldRecognizeNonEqualityPerComponent() {
@Test
public void shouldComputeCorrectHashCode() {
final int actual = createTuple().hashCode();
final int expected = Objects.hash(null, null);
final int expected = new SimpleEntry<>(null, null).hashCode();
assertThat(actual).isEqualTo(expected);
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/io/vavr/TupleTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public void shouldCreateTuple2FromEntry() {
@Test
public void shouldHashTuple2() {
final Tuple2<?, ?> t = tuple2();
assertThat(t.hashCode()).isEqualTo(Objects.hash(t._1, t._2));
assertThat(t.hashCode()).isEqualTo(new AbstractMap.SimpleEntry<>(t._1, t._2).hashCode());
}

@Test
Expand Down
7 changes: 7 additions & 0 deletions src/test/java/io/vavr/collection/AbstractMapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -1441,4 +1441,11 @@ public void shouldNonNilGroupByIdentity() {
assertThat(actual).isEqualTo(expected);
}

// -- hashCode

@Override
@Test
public void shouldCalculateDifferentHashCodesForDifferentTraversables() {
assertThat(mapOf('a', 2, 'b', 1).hashCode()).isNotEqualTo(mapOf('a', 1, 'b', 2).hashCode());
}
}
9 changes: 9 additions & 0 deletions src/test/java/io/vavr/collection/AbstractMultimapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ protected <T1 extends Comparable<T1>, T2> Multimap<T1, T2> emptyMap() {

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

abstract protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2);

abstract protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K key, V value);

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

// -- hashCode

@Override
@Test
public void shouldCalculateDifferentHashCodesForDifferentTraversables() {
assertThat(mapOf("a", 2, "b", 1).hashCode()).isNotEqualTo(mapOf("a", 1, "b", 2).hashCode());
}
}
14 changes: 14 additions & 0 deletions src/test/java/io/vavr/collection/HashMultimapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,20 @@ protected <K extends Comparable<K>, V> HashMultimap<K, V> mapOfPairs(K k1, V v1,
}
}

@Override
protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2) {
switch (containerType) {
case SEQ:
return HashMultimap.withSeq().of(k1, v1, k2, v2);
case SET:
return HashMultimap.withSet().of(k1, v1, k2, v2);
case SORTED_SET:
return HashMultimap.withSortedSet(Comparators.naturalComparator()).of(k1, v1, k2, v2);
default:
throw new RuntimeException();
}
}

@Override
protected <K extends Comparable<K>, V> HashMultimap<K, V> mapOf(K key, V value) {
switch (containerType) {
Expand Down
14 changes: 14 additions & 0 deletions src/test/java/io/vavr/collection/LinkedHashMultimapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,20 @@ protected <K extends Comparable<K>, V> LinkedHashMultimap<K, V> mapOfPairs(K k1,
}
}

@Override
protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2) {
switch (containerType) {
case SEQ:
return LinkedHashMultimap.withSeq().of(k1, v1, k2, v2);
case SET:
return LinkedHashMultimap.withSet().of(k1, v1, k2, v2);
case SORTED_SET:
return LinkedHashMultimap.withSortedSet(Comparators.naturalComparator()).of(k1, v1, k2, v2);
default:
throw new RuntimeException();
}
}

@Override
protected <K extends Comparable<K>, V> LinkedHashMultimap<K, V> mapOf(K key, V value) {
switch (containerType) {
Expand Down
14 changes: 14 additions & 0 deletions src/test/java/io/vavr/collection/TreeMultimapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,20 @@ protected <K extends Comparable<K>, V> TreeMultimap<K, V> mapOfPairs(K k1, V v1,
}
}

@Override
protected <K extends Comparable<K>, V> Multimap<K, V> mapOf(K k1, V v1, K k2, V v2) {
switch (containerType) {
case SEQ:
return TreeMultimap.withSeq().of(k1, v1, k2, v2);
case SET:
return TreeMultimap.withSet().of(k1, v1, k2, v2);
case SORTED_SET:
return TreeMultimap.withSortedSet(Comparators.naturalComparator()).of(k1, v1, k2, v2);
default:
throw new RuntimeException();
}
}

@Override
protected <K extends Comparable<K>, V> TreeMultimap<K, V> mapOf(K key, V value) {
switch (containerType) {
Expand Down

0 comments on commit b1ce608

Please sign in to comment.