MapWrapper.hashCode breaks expected contract of java.util.Map

[isnotinvain]

MapWrapper's hashCode does not behave the same as other java.util.Maps, which can cause issues when trying to check equality of two collections where one collection is (or contains) a MapWrapper and the other is or contains a regular java.util.Map

Some background info:

java.util.Map has the following contract for its hashCode:

Returns the hash code value for this map. The hash code of a map is defined to be the sum of the hash codes of each entry in the map's entrySet() view. This ensures that m1.equals(m2) implies that m1.hashCode()==m2.hashCode() for any two maps m1 and m2, as required by the general contract of Object.hashCode()

MapWrapper inherits its implementation of hashCode from java.util.AbstractMap and so its implementation is correct. However, as defined, the hash code produced is the sum of the Map.Entry objects' hashes, so this is also very much dependent on the hashCode implementation of the Map.Entry objects as well. This is the part that MapWrapper is doing incorrectly / differently from the java stdlib.

The docs for Map.Entry.hashCode state:

Returns the hash code value for this map entry. The hash code of a map entry e is defined to be:
(e.getKey()==null ? 0 : e.getKey().hashCode()) ^
(e.getValue()==null ? 0 : e.getValue().hashCode())

This ensures that e1.equals(e2) implies that e1.hashCode()==e2.hashCode() for any two Entries e1 and e2, as required by the general contract of Object.hashCode.

A quick check of some of the Entry implementations in the java stdlib confirms that this is how they are implemented.

However, in MapWrapper, we have:

new ju.Map.Entry[A, B] {
  import scala.util.hashing.byteswap32
  def getKey = k
  def getValue = v
  def setValue(v1 : B) = self.put(k, v1)
  override def hashCode = byteswap32(k.##) + (byteswap32(v.##) << 16)
  override def equals(other: Any) = other match {
  case e: ju.Map.Entry[_, _] => k == e.getKey && v == e.getValue
    case _ => false
  }
}

The important part being:

override def hashCode = byteswap32(k.##) + (byteswap32(v.##) << 16)

The end result can also be confirmed with a repl session like this:

$ scala
Welcome to Scala 2.11.11 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_111).
Type in expressions for evaluation. Or try :help.

scala> import java.util.HashMap
import java.util.HashMap

scala> import scala.collection.JavaConverters._
import scala.collection.JavaConverters._

scala> val jmap = new HashMap[Int, Int]
jmap: java.util.HashMap[Int,Int] = {}

scala> jmap.put(100, 7)
res0: Int = 0

scala> jmap.hashCode()
res1: Int = 99

scala> 100 ^ 7
res2: Int = 99

scala> val wrapped = Map(100 -> 7).asJava
wrapped: java.util.Map[Int,Int] = {100=7}

scala> wrapped.getClass
res3: Class[_ <: java.util.Map[Int,Int]] = class scala.collection.convert.Wrappers$MapWrapper

scala> wrapped.hashCode
res4: Int = 1196324649

scala> wrapped == jmap
res14: Boolean = true

scala> wrapped.hashCode == jmap.hashCode
res15: Boolean = false

I will try to open a PR to fix this, but wanted to open this issue first.

Thanks!
Alex

[isnotinvain]

I think this may be all that is needed -- I think it's actually correct to call hashCode here and not ## since this collection should behave the way java does, including for boxed objects, but I'd appreciate some feedback on whether that's right or not:
https://github.com/isnotinvain/scala/pull/1

I need to add a regression test and see if this breaks any existing tests, I haven't finished getting a local scala build up and running on my laptop yet. Does this project have travis CI or anything like that?

[SethTisue]

Does this project have travis CI or anything like that

yes, when you submit a PR, our Jenkins instance will build it and run the tests. it's fine to submit a half-baked PR just for that purpose, just please clearly label it as such

[isnotinvain]

Great, thanks! I'll do that.

[isnotinvain]

This has been merged in both scala/scala#6233 and scala/collection-strawman#487

[SethTisue]

thanks for your patience in seeing this through