Fix HashMap/Set remove operation

Author: odersky

compiler/src/dotty/tools/dotc/util/GenericHashMap.scala

@@ -23,7 +23,7 @@ object GenericHashMap:
  *                           once the number of elements reaches the table's size.
  */
 abstract class GenericHashMap[Key <: AnyRef, Value >: Null <: AnyRef]
-    (initialCapacity: Int = 8, capacityMultiple: Int = 3) extends MutableMap[Key, Value]:
+    (initialCapacity: Int, capacityMultiple: Int) extends MutableMap[Key, Value]:
   import GenericHashMap.DenseLimit
 
   protected var used: Int = _
@@ -102,11 +102,15 @@ abstract class GenericHashMap[Key <: AnyRef, Value >: Null <: AnyRef]
         while
           idx = nextIndex(idx)
           k = keyAt(idx)
-          k != null && (isDense || index(hash(k)) != idx)
+          k != null
         do
-          table(hole) = k
-          table(hole + 1) = valueAt(idx)
-          hole = idx
+          if isDense
+            || index(hole - index(hash(k))) < limit * 2
+               // hash(k) is then logically at or after hole; can be moved forward to fill hole
+          then
+            table(hole) = k
+            table(hole + 1) = valueAt(idx)
+            hole = idx
         table(hole) = null
         used -= 1
         return

compiler/src/dotty/tools/dotc/util/HashSet.scala

@@ -108,8 +108,12 @@ class HashSet[T >: Null <: AnyRef](initialCapacity: Int = 8, capacityMultiple: I
           e = entryAt(idx)
           e != null && (isDense || index(hash(e)) != idx)
         do
-          table(hole) = e
-          hole = idx
+          if isDense
+            || index(hole - index(hash(k))) < limit
+               // hash(k) is then logically at or after hole; can be moved forward to fill hole
+          then
+            table(hole) = e
+            hole = idx
         table(hole) = null
         used -= 1
         return

tests/run-with-compiler/maptest.scala

@@ -0,0 +1,74 @@
+trait Generator[+T]:
+  self =>
+  def generate: T
+  def map[S](f: T => S) = new Generator[S]:
+    def generate: S = f(self.generate)
+  def flatMap[S](f: T => Generator[S]) = new Generator[S]:
+    def generate: S = f(self.generate).generate
+
+object Generator:
+  val NumLimit = 300
+  val Iterations = 10000
+
+  given integers as Generator[Int]:
+    val rand = new java.util.Random
+    def generate = rand.nextInt()
+
+  given booleans as Generator[Boolean] =
+    integers.map(x => x > 0)
+
+  def range(end: Int): Generator[Int] =
+    integers.map(x => (x % end).abs)
+
+  enum Op:
+    case Lookup, Update, Remove
+  export Op._
+
+  given ops as Generator[Op] =
+    range(10).map {
+      case 0 | 1 | 2 | 3 => Lookup
+      case 4 | 5 | 6 | 7 => Update
+      case 8 | 9         => Remove
+    }
+
+  val nums: Generator[Integer] = range(NumLimit).map(Integer(_))
+
+@main def Test =
+  import Generator._
+
+  val map1 = dotty.tools.dotc.util.HashMap[Integer, Integer]()
+  val map2 = scala.collection.mutable.HashMap[Integer, Integer]()
+
+  def checkSame() =
+    assert(map1.size == map2.size)
+    for (k, v) <- map1.iterator do
+      assert(map2.get(k) == Some(v))
+    for (k, v) <- map2.iterator do
+      assert(Option(map1.lookup(k)) == Some(v))
+
+  def lookupTest(num: Integer) =
+    //println(s"test lookup $num")
+    val res1 = Option(map1.lookup(num))
+    val res2 = map2.get(num)
+    assert(res1 == res2)
+
+  def updateTest(num: Integer) =
+    //println(s"test update $num")
+    lookupTest(num)
+    map1(num) = num
+    map2(num) = num
+    checkSame()
+
+  def removeTest(num: Integer) =
+    //println(s"test remove $num")
+    map1.remove(num)
+    map2.remove(num)
+    checkSame()
+
+  for i <- 0 until Iterations do
+    //if i % 1000 == 0 then println(map1.size)
+    val num = nums.generate
+    Generator.ops.generate match
+      case Lookup => lookupTest(num)
+      case Update => updateTest(num)
+      case Remove => removeTest(num)