From 2a28a8531027eb53fe8ae69aace60cd499e71958 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 17 Jun 2020 13:05:12 +1000 Subject: [PATCH 1/8] [backport] Bulk ops on TreeSet, TreeMap prefer keys from the left The test suite that aimed to bring all of our sets and maps into line did not exercise code paths optimized for: collection.$op(collectionOfSameType) This commit updates the test to: - add test variants that generate operands of the same type as the input collection - add "preserves identity" tests for set intersection - fix the resulting failures by flipping the operands in `RedBlackTree.{union,intersection}` I've also added concrete test cases to `Tree{Map,Set}Test` that are easier to review and that also exercise builders. --- .../collection/immutable/RedBlackTree.scala | 4 +-- .../collection/immutable/TreeMapTest.scala | 26 +++++++++++++++++ .../collection/immutable/TreeSetTest.scala | 29 +++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/library/scala/collection/immutable/RedBlackTree.scala b/src/library/scala/collection/immutable/RedBlackTree.scala index 660f16944371..8dee032f681d 100644 --- a/src/library/scala/collection/immutable/RedBlackTree.scala +++ b/src/library/scala/collection/immutable/RedBlackTree.scala @@ -760,9 +760,9 @@ private[collection] object RedBlackTree { // of child nodes from it. Where possible the black height is used directly instead of deriving the rank from it. // Our trees are supposed to have a black root so we always blacken as the last step of union/intersect/difference. - def union[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_union(t1, t2)) + def union[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_union(t2, t1)) - def intersect[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_intersect(t1, t2)) + def intersect[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_intersect(t2, t1)) def difference[A, B](t1: Tree[A, B], t2: Tree[A, _])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_difference(t1, t2.asInstanceOf[Tree[A, B]])) diff --git a/test/junit/scala/collection/immutable/TreeMapTest.scala b/test/junit/scala/collection/immutable/TreeMapTest.scala index 96bdb3d534a0..0ae740a3145b 100644 --- a/test/junit/scala/collection/immutable/TreeMapTest.scala +++ b/test/junit/scala/collection/immutable/TreeMapTest.scala @@ -172,4 +172,30 @@ class TreeMapTest extends AllocationTest { val map2 = map.updated(3, 3) assertEquals(List(3 -> 3), map2.toList) } + + @Test + def unionAndIntersectRetainLeft(): Unit = { + case class C(a: Int)(override val toString: String) + implicit val ordering: Ordering[C] = Ordering.by(_.a) + val c0l = C(0)("l") + val c0r = C(0)("r") + def assertIdenticalKeys(expected: Map[C, Unit], actual: Map[C, Unit]): Unit = { + val expected1, actual1 = new java.util.IdentityHashMap[C, Unit]() + expected.keys.foreach(expected1.put(_, ())) + actual.keys.foreach(actual1.put(_, ())) + assertEquals(expected1, actual1) + } + + // This holds in 2.13.x only + //assertIdenticalKeys(Map((c0l, ())), HashMap((c0l, ())).++(HashMap((c0r, ())))) + + assertIdenticalKeys(Map((c0l, ())), TreeMap((c0l, ())).++(HashMap((c0r, ())))) + assertIdenticalKeys(Map((c0l, ())), TreeMap((c0l, ())).++(TreeMap((c0r, ())))) + + // This holds in 2.13.x only + //assertIdenticalKeys(Map((c0l, ())), HashMap.newBuilder[C, Unit].++=(HashMap((c0l, ()))).++=(HashMap((c0r, ()))).result()) + + assertIdenticalKeys(Map((c0l, ())), TreeMap.newBuilder[C, Unit].++=(TreeMap((c0l, ()))).++=(HashMap((c0r, ()))).result()) + assertIdenticalKeys(Map((c0l, ())), TreeMap.newBuilder[C, Unit].++=(TreeMap((c0l, ()))).++=(TreeMap((c0r, ()))).result()) + } } diff --git a/test/junit/scala/collection/immutable/TreeSetTest.scala b/test/junit/scala/collection/immutable/TreeSetTest.scala index e7718d6b85dc..3d78f42030d1 100644 --- a/test/junit/scala/collection/immutable/TreeSetTest.scala +++ b/test/junit/scala/collection/immutable/TreeSetTest.scala @@ -213,4 +213,33 @@ class TreeSetTest extends AllocationTest{ assertEquals(s"$rText $lText", rhs, lhs) } } + + @Test + def unionAndIntersectRetainLeft(): Unit = { + case class C(a: Int)(override val toString: String) + implicit val ordering: Ordering[C] = Ordering.by(_.a) + val c0l = C(0)("l") + val c0r = C(0)("r") + def assertIdenticalElements(expected: Set[C], actual: Set[C]): Unit = { + val expected1, actual1 = new java.util.IdentityHashMap[C, Unit]() + expected.foreach(expected1.put(_, ())) + actual.foreach(actual1.put(_, ())) + assertEquals(expected1, actual1) + } + assertIdenticalElements(Set(c0l), HashSet(c0l).union(HashSet(c0r))) + assertIdenticalElements(Set(c0l), TreeSet(c0l).union(HashSet(c0r))) + assertIdenticalElements(Set(c0l), TreeSet(c0l).union(TreeSet(c0r))) + + assertIdenticalElements(Set(c0l), HashSet(c0l).++(HashSet(c0r))) + assertIdenticalElements(Set(c0l), TreeSet(c0l).++(HashSet(c0r))) + assertIdenticalElements(Set(c0l), TreeSet(c0l).++(TreeSet(c0r))) + + assertIdenticalElements(Set(c0l), HashSet.newBuilder[C].++=(HashSet(c0l)).++=(HashSet(c0r)).result()) + assertIdenticalElements(Set(c0l), TreeSet.newBuilder[C].++=(TreeSet(c0l)).++=(HashSet(c0r)).result()) + assertIdenticalElements(Set(c0l), TreeSet.newBuilder[C].++=(TreeSet(c0l)).++=(TreeSet(c0r)).result()) + + assertIdenticalElements(Set(c0l), HashSet(c0l).intersect(HashSet(c0r))) + assertIdenticalElements(Set(c0l), TreeSet(c0l).intersect(HashSet(c0r))) + assertIdenticalElements(Set(c0l), TreeSet(c0l).intersect(TreeSet(c0r))) + } } From 51f96c9bcd486dcbe89295f51aeffc3f9678ba1f Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 19 Jun 2020 15:11:37 +0200 Subject: [PATCH 2/8] remove unused value --- src/reflect/scala/reflect/internal/StdNames.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/src/reflect/scala/reflect/internal/StdNames.scala b/src/reflect/scala/reflect/internal/StdNames.scala index d1ecc376ea1f..ea7f4428f13b 100644 --- a/src/reflect/scala/reflect/internal/StdNames.scala +++ b/src/reflect/scala/reflect/internal/StdNames.scala @@ -1128,7 +1128,6 @@ trait StdNames { val reflParamsCacheName: NameType = "reflParams$Cache" val reflMethodName: NameType = "reflMethod$Method" val argument: NameType = "" - val liftedTree: String = "liftedTree" } From a359fd4c2ac2149656b99eacbba378b7c9bcac4e Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 19 Jun 2020 15:11:46 +0200 Subject: [PATCH 3/8] fix test flag --- test/files/run/pure-warning-post-macro.flags | 1 + test/files/run/pure-warning-post-macro/test_2.scala | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 test/files/run/pure-warning-post-macro.flags diff --git a/test/files/run/pure-warning-post-macro.flags b/test/files/run/pure-warning-post-macro.flags new file mode 100644 index 000000000000..85d8eb2ba295 --- /dev/null +++ b/test/files/run/pure-warning-post-macro.flags @@ -0,0 +1 @@ +-Xfatal-warnings diff --git a/test/files/run/pure-warning-post-macro/test_2.scala b/test/files/run/pure-warning-post-macro/test_2.scala index 74094d958647..05aa3d620d9b 100644 --- a/test/files/run/pure-warning-post-macro/test_2.scala +++ b/test/files/run/pure-warning-post-macro/test_2.scala @@ -1,4 +1,3 @@ -// scalac: -Xfatal-errors object Test { def main(args: Array[String]): Unit = { // We don't want a "pure expression discarded" warning here as the macro will From 762afa29e2001285169d1a3d3724fc56f4114fed Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 17 Jun 2020 13:48:54 +1000 Subject: [PATCH 4/8] [backport] Refactor fix from prior commit --- .../collection/immutable/RedBlackTree.scala | 22 +++++++++---------- .../collection/immutable/TreeMapTest.scala | 8 ++++--- .../collection/immutable/TreeSetTest.scala | 6 ++--- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/library/scala/collection/immutable/RedBlackTree.scala b/src/library/scala/collection/immutable/RedBlackTree.scala index 8dee032f681d..ae579a2b14bb 100644 --- a/src/library/scala/collection/immutable/RedBlackTree.scala +++ b/src/library/scala/collection/immutable/RedBlackTree.scala @@ -760,9 +760,9 @@ private[collection] object RedBlackTree { // of child nodes from it. Where possible the black height is used directly instead of deriving the rank from it. // Our trees are supposed to have a black root so we always blacken as the last step of union/intersect/difference. - def union[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_union(t2, t1)) + def union[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_union(t1, t2)) - def intersect[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_intersect(t2, t1)) + def intersect[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_intersect(t1, t2)) def difference[A, B](t1: Tree[A, B], t2: Tree[A, _])(implicit ordering: Ordering[A]): Tree[A, B] = blacken(_difference(t1, t2.asInstanceOf[Tree[A, B]])) @@ -851,22 +851,22 @@ private[collection] object RedBlackTree { private[this] def _union[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = if((t1 eq null) || (t1 eq t2)) t2 - else if(t2 eq null) t1 + else if(t1 eq null) t2 else { - val (l1, _, r1) = split(t1, t2.key) - val tl = _union(l1, t2.left) - val tr = _union(r1, t2.right) - join(tl, t2.key, t2.value, tr) + val (l2, _, r2) = split(t2, t1.key) + val tl = _union(t1.left, l2) + val tr = _union(t1.right, r2) + join(tl, t1.key, t1.value, tr) } private[this] def _intersect[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = if((t1 eq null) || (t2 eq null)) null else if (t1 eq t2) t1 else { - val (l1, b, r1) = split(t1, t2.key) - val tl = _intersect(l1, t2.left) - val tr = _intersect(r1, t2.right) - if(b ne null) join(tl, t2.key, t2.value, tr) + val (l2, b, r2) = split(t2, t1.key) + val tl = _intersect(t1.left, l2) + val tr = _intersect(t1.right, r2) + if(b ne null) join(tl, t1.key, t1.value, tr) else join2(tl, tr) } diff --git a/test/junit/scala/collection/immutable/TreeMapTest.scala b/test/junit/scala/collection/immutable/TreeMapTest.scala index 0ae740a3145b..3f6a3952f622 100644 --- a/test/junit/scala/collection/immutable/TreeMapTest.scala +++ b/test/junit/scala/collection/immutable/TreeMapTest.scala @@ -1,5 +1,7 @@ package scala.collection.immutable +import java.util.Collections + import org.junit.Assert._ import org.junit.Test import org.junit.runner.RunWith @@ -180,9 +182,9 @@ class TreeMapTest extends AllocationTest { val c0l = C(0)("l") val c0r = C(0)("r") def assertIdenticalKeys(expected: Map[C, Unit], actual: Map[C, Unit]): Unit = { - val expected1, actual1 = new java.util.IdentityHashMap[C, Unit]() - expected.keys.foreach(expected1.put(_, ())) - actual.keys.foreach(actual1.put(_, ())) + val expected1, actual1 = Collections.newSetFromMap[C](new java.util.IdentityHashMap()) + expected.keys.foreach(expected1.add) + actual.keys.foreach(actual1.add) assertEquals(expected1, actual1) } diff --git a/test/junit/scala/collection/immutable/TreeSetTest.scala b/test/junit/scala/collection/immutable/TreeSetTest.scala index 3d78f42030d1..55f0c882fb0b 100644 --- a/test/junit/scala/collection/immutable/TreeSetTest.scala +++ b/test/junit/scala/collection/immutable/TreeSetTest.scala @@ -221,9 +221,9 @@ class TreeSetTest extends AllocationTest{ val c0l = C(0)("l") val c0r = C(0)("r") def assertIdenticalElements(expected: Set[C], actual: Set[C]): Unit = { - val expected1, actual1 = new java.util.IdentityHashMap[C, Unit]() - expected.foreach(expected1.put(_, ())) - actual.foreach(actual1.put(_, ())) + val expected1, actual1 = Collections.newSetFromMap[C](new java.util.IdentityHashMap()) + expected.foreach(expected1.add) + actual.foreach(actual1.add) assertEquals(expected1, actual1) } assertIdenticalElements(Set(c0l), HashSet(c0l).union(HashSet(c0r))) From 368ec741cc5b607db80fbff71f257e3343b80c79 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 22 Jun 2020 10:21:09 +0200 Subject: [PATCH 5/8] fix an oversight --- src/library/scala/collection/immutable/RedBlackTree.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/scala/collection/immutable/RedBlackTree.scala b/src/library/scala/collection/immutable/RedBlackTree.scala index ae579a2b14bb..92ade8ee1d92 100644 --- a/src/library/scala/collection/immutable/RedBlackTree.scala +++ b/src/library/scala/collection/immutable/RedBlackTree.scala @@ -851,7 +851,7 @@ private[collection] object RedBlackTree { private[this] def _union[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = if((t1 eq null) || (t1 eq t2)) t2 - else if(t1 eq null) t2 + else if(t2 eq null) t1 else { val (l2, _, r2) = split(t2, t1.key) val tl = _union(t1.left, l2) From 141efea564478c8891eabb3a0a7db56826ce5027 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Mon, 22 Jun 2020 09:50:20 +1000 Subject: [PATCH 6/8] Add an additional test left-bias in for Set.+ --- test/junit/scala/collection/immutable/TreeSetTest.scala | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/junit/scala/collection/immutable/TreeSetTest.scala b/test/junit/scala/collection/immutable/TreeSetTest.scala index 55f0c882fb0b..8446c7aab2c0 100644 --- a/test/junit/scala/collection/immutable/TreeSetTest.scala +++ b/test/junit/scala/collection/immutable/TreeSetTest.scala @@ -230,6 +230,10 @@ class TreeSetTest extends AllocationTest{ assertIdenticalElements(Set(c0l), TreeSet(c0l).union(HashSet(c0r))) assertIdenticalElements(Set(c0l), TreeSet(c0l).union(TreeSet(c0r))) + assertIdenticalElements(Set(c0l), HashSet(c0l).+(c0r)) + assertIdenticalElements(Set(c0l), TreeSet(c0l).+(c0r)) + assertIdenticalElements(Set(c0l), TreeSet(c0l).+(c0r)) + assertIdenticalElements(Set(c0l), HashSet(c0l).++(HashSet(c0r))) assertIdenticalElements(Set(c0l), TreeSet(c0l).++(HashSet(c0r))) assertIdenticalElements(Set(c0l), TreeSet(c0l).++(TreeSet(c0r))) From 4fe011f006e2890be7897d7145cb163782715840 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Mon, 22 Jun 2020 12:29:01 +1000 Subject: [PATCH 7/8] HashMap bulk operations should retain existing keys ... when the operand overwrites mapping with a key that is == but ne. This is consistent with the super class implementation, which was overridden in 2.12.11 for efficiency. I also found a pair of ClassCastExceptions in the new implementations of `HashMap.++:`, for instance: ``` case class C(a: Int)(override val toString: String); implicit val Ordering_C: Ordering[C] = Ordering.by(_.a); val c0l = C(0)("l"); val c0r = C(0)("r"); import collection.immutable._; println(HashMap((c0l, ())).++:(TreeMap((c0r, ()))))'; done v2.12.10 Map(r -> ()) v2.12.11 java.lang.ClassCastException: scala.collection.immutable.HashMap$adder$1$ cannot be cast to scala.collection.immutable.HashMap at scala.collection.immutable.HashMap$adder$1$.(HashMap.scala:215) ``` --- .../scala/collection/immutable/HashMap.scala | 120 +++++++++++------- .../collection/immutable/HashMapTest.scala | 31 ++++- .../collection/immutable/TreeMapTest.scala | 8 +- 3 files changed, 104 insertions(+), 55 deletions(-) diff --git a/src/library/scala/collection/immutable/HashMap.scala b/src/library/scala/collection/immutable/HashMap.scala index 1b08dd0a9aa4..c2cabf13102e 100644 --- a/src/library/scala/collection/immutable/HashMap.scala +++ b/src/library/scala/collection/immutable/HashMap.scala @@ -46,7 +46,7 @@ sealed class HashMap[A, +B] extends AbstractMap[A, B] with CustomParallelizable[(A, B), ParHashMap[A, B]] with HasForeachEntry[A, B] { - import HashMap.{nullToEmpty, bufferSize} + import HashMap.{bufferSize, concatMerger, nullToEmpty} override def size: Int = 0 @@ -159,7 +159,7 @@ sealed class HashMap[A, +B] extends AbstractMap[A, B] override def values: scala.collection.Iterable[B] = new HashMapValues override final def transform[W, That](f: (A, B) => W)(implicit bf: CanBuildFrom[HashMap[A, B], (A, W), That]): That = - if ((bf eq Map.canBuildFrom) || (bf eq HashMap.canBuildFrom)) transformImpl(f).asInstanceOf[That] + if ((bf eq Map.canBuildFrom) || (bf eq HashMap.canBuildFrom)) castToThat(transformImpl(f)) else super.transform(f)(bf) /* `transform` specialized to return a HashMap */ @@ -179,17 +179,18 @@ sealed class HashMap[A, +B] extends AbstractMap[A, B] override def ++[C >: (A, B), That](that: GenTraversableOnce[C])(implicit bf: CanBuildFrom[HashMap[A, B], C, That]): That = { if (isCompatibleCBF(bf)) { //here we know that That =:= HashMap[_, _], or compatible with it - if (this eq that.asInstanceOf[AnyRef]) that.asInstanceOf[That] - else if (that.isEmpty) this.asInstanceOf[That] - else that match { - case thatHash: HashMap[A, B] => - //default Merge prefers to keep than replace - //so we merge from thatHash - (thatHash.merged(this) (null) ).asInstanceOf[That] - case that => - var result: HashMap[Any, _] = this.asInstanceOf[HashMap[Any, _]] - that foreach { case kv: (_, _) => result = result + kv } - result.asInstanceOf[That] + if (this eq that.asInstanceOf[AnyRef]) castToThat(that) + else if (that.isEmpty) castToThat(this) + else { + val result: HashMap[A, B] = that match { + case thatHash: HashMap[A, B] => + this.merge0(thatHash, 0, concatMerger[A, B]) + case that => + var result: HashMap[A, B] = this + that.asInstanceOf[GenTraversableOnce[(A, B)]] foreach { case kv: (_, _) => result = result + kv } + result + } + castToThat(result) } } else super.++(that)(bf) } @@ -203,42 +204,45 @@ sealed class HashMap[A, +B] extends AbstractMap[A, B] if (isCompatibleCBF(bf)) addSimple(that) else super.++:(that) } - private def addSimple[C >: (A, B), That](that: TraversableOnce[C]): That = { + private def addSimple[C >: (A, B), That](that: TraversableOnce[C])(implicit bf: CanBuildFrom[HashMap[A, B], C, That]): That = { //here we know that That =:= HashMap[_, _], or compatible with it - if (this eq that.asInstanceOf[AnyRef]) that.asInstanceOf[That] - else if (that.isEmpty) this.asInstanceOf[That] - else that match { - case thatHash: HashMap[A, B] => - val merger: Merger[A, B] = HashMap.liftMerger[A, B](null) - // merger prefers to keep than replace - // so we invert - (this.merge0(thatHash, 0, merger.invert)).asInstanceOf[That] - - case that:HasForeachEntry[A, B] => - object adder extends Function2[A, B, Unit] { - var result: HashMap[A, B] = this.asInstanceOf[HashMap[A, B]] - val merger = HashMap.liftMerger[A, B](null) - - override def apply(key: A, value: B): Unit = { - result = result.updated0(key, computeHash(key), 0, value, null, merger) + if (this eq that.asInstanceOf[AnyRef]) castToThat(that) + else if (that.isEmpty) castToThat(this) + else { + val merger = HashMap.concatMerger[A, B].invert + val result: HashMap[A, B] = that match { + case thatHash: HashMap[A, B] => + this.merge0(thatHash, 0, HashMap.concatMerger[A, B].invert) + + case that:HasForeachEntry[A, B] => + object adder extends Function2[A, B, Unit] { + var result: HashMap[A, B] = HashMap.this + override def apply(key: A, value: B): Unit = { + result = result.updated0(key, computeHash(key), 0, value, null, merger) + } } - } - that foreachEntry adder - adder.result.asInstanceOf[That] - case that => - object adder extends Function1[(A,B), Unit] { - var result: HashMap[A, B] = this.asInstanceOf[HashMap[A, B]] - val merger = HashMap.liftMerger[A, B](null) - - override def apply(kv: (A, B)): Unit = { - val key = kv._1 - result = result.updated0(key, computeHash(key), 0, kv._2, kv, merger) + that foreachEntry adder + adder.result + case that => + object adder extends Function1[(A,B), Unit] { + var result: HashMap[A, B] = HashMap.this + override def apply(kv: (A, B)): Unit = { + val key = kv._1 + result = result.updated0(key, computeHash(key), 0, kv._2, kv, merger) + } } - } - that.asInstanceOf[scala.Traversable[(A,B)]] foreach adder - adder.result.asInstanceOf[That] + that.asInstanceOf[scala.Traversable[(A,B)]] foreach adder + adder.result + } + castToThat(result) } } + private[this] def castToThat[C, That](m: HashMap[A, B])(implicit bf: CanBuildFrom[HashMap[A, B], C, That]): That = { + m.asInstanceOf[That] + } + private[this] def castToThat[C, That](m: GenTraversableOnce[C])(implicit bf: CanBuildFrom[HashMap[A, B], C, That]): That = { + m.asInstanceOf[That] + } } /** $factoryInfo @@ -261,9 +265,10 @@ object HashMap extends ImmutableMapFactory[HashMap] with BitOperations.Int { private type MergeFunction[A1, B1] = ((A1, B1), (A1, B1)) => (A1, B1) private def liftMerger[A1, B1](mergef: MergeFunction[A1, B1]): Merger[A1, B1] = - if (mergef == null) defaultMerger.asInstanceOf[Merger[A1, B1]] else liftMerger0(mergef) + if (mergef == null) defaultMerger[A1, B1] else liftMerger0(mergef) - private val defaultMerger : Merger[Any, Any] = new Merger[Any, Any] { + private def defaultMerger[A, B]: Merger[A, B] = _defaultMerger.asInstanceOf[Merger[A, B]] + private[this] val _defaultMerger : Merger[Any, Any] = new Merger[Any, Any] { override def apply(a: (Any, Any), b: (Any, Any)): (Any, Any) = a override def retainIdentical: Boolean = true override val invert: Merger[Any, Any] = new Merger[Any, Any] { @@ -273,6 +278,23 @@ object HashMap extends ImmutableMapFactory[HashMap] with BitOperations.Int { } } + private def concatMerger[A, B]: Merger[A, B] = _concatMerger.asInstanceOf[Merger[A, B]] + private[this] val _concatMerger : Merger[Any, Any] = new Merger[Any, Any] { + override def apply(a: (Any, Any), b: (Any, Any)): (Any, Any) = { + if (a._1.asInstanceOf[AnyRef] eq b._1.asInstanceOf[AnyRef]) b + else (a._1, b._2) + } + override def retainIdentical: Boolean = true + override val invert: Merger[Any, Any] = new Merger[Any, Any] { + override def apply(a: (Any, Any), b: (Any, Any)): (Any, Any) = { + if (b._1.asInstanceOf[AnyRef] eq a._1.asInstanceOf[AnyRef]) a + else (b._1, a._2) + } + override def retainIdentical: Boolean = true + override def invert = concatMerger + } + } + private[this] def liftMerger0[A1, B1](mergef: MergeFunction[A1, B1]): Merger[A1, B1] = new Merger[A1, B1] { self => def apply(kv1: (A1, B1), kv2: (A1, B1)): (A1, B1) = mergef(kv1, kv2) @@ -1049,7 +1071,7 @@ object HashMap extends ImmutableMapFactory[HashMap] with BitOperations.Int { /** The root node of the partially build hashmap */ private var rootNode: HashMap[A, B] = HashMap.empty - private def plusPlusMerger = liftMerger[A, B](null).invert + private def isMutable(hs: HashMap[A, B]) = { hs.isInstanceOf[HashTrieMap[A, B]] && hs.size == -1 } @@ -1249,11 +1271,11 @@ object HashMap extends ImmutableMapFactory[HashMap] with BitOperations.Int { if (toNode eq toBeAdded) toNode else toBeAdded match { case bLeaf: HashMap1[A, B] => - if (toNodeHash == bLeaf.hash) toNode.merge0(bLeaf, level, plusPlusMerger) + if (toNodeHash == bLeaf.hash) toNode.merge0(bLeaf, level, concatMerger[A, B]) else makeMutableTrie(toNode, bLeaf, level) case bLeaf: HashMapCollision1[A, B] => - if (toNodeHash == bLeaf.hash) toNode.merge0(bLeaf, level, plusPlusMerger) + if (toNodeHash == bLeaf.hash) toNode.merge0(bLeaf, level, concatMerger[A, B]) else makeMutableTrie(toNode, bLeaf, level) case bTrie: HashTrieMap[A, B] => diff --git a/test/junit/scala/collection/immutable/HashMapTest.scala b/test/junit/scala/collection/immutable/HashMapTest.scala index c6119fa4dae6..c62db75b4f7a 100644 --- a/test/junit/scala/collection/immutable/HashMapTest.scala +++ b/test/junit/scala/collection/immutable/HashMapTest.scala @@ -1,5 +1,7 @@ package scala.collection.immutable +import java.util.Collections + import org.junit.Assert._ import org.junit.Test import org.junit.runner.RunWith @@ -203,7 +205,7 @@ class HashMapTest extends AllocationTest { "i" -> 9, "j" -> 10 ) - assertSame(nonEmpty2, nonAllocating(nonEmpty1 ++ nonEmpty2)) + assertSame(nonEmpty1, nonAllocating(nonEmpty1 ++ nonEmpty2)) } @Test @@ -269,4 +271,31 @@ class HashMapTest extends AllocationTest { //calls GenTraversableOnce.++ assertEquals(1, (m2 ++ m1).apply(2)) } + + @Test + def retainLeft(): Unit = { + case class C(a: Int)(override val toString: String) + implicit val ordering: Ordering[C] = Ordering.by(_.a) + val c0l = C(0)("l") + val c0r = C(0)("r") + def assertIdenticalKeys(expected: Map[C, Unit], actual: Map[C, Unit]): Unit = { + val expected1, actual1 = Collections.newSetFromMap[C](new java.util.IdentityHashMap()) + expected.keys.foreach(expected1.add) + actual.keys.foreach(actual1.add) + assertEquals(expected1, actual1) + } + assertIdenticalKeys(Map((c0l, ())), HashMap((c0l, ())).updated(c0r, ())) + + def check(factory: Seq[(C, Unit)] => Map[C, Unit]): Unit = { + val c0LMap = factory(Seq((c0l, ()))) + val c0RMap = factory(Seq((c0r, ()))) + assertIdenticalKeys(Map((c0l, ())), HashMap((c0l, ())).++(c0RMap)) + assertIdenticalKeys(Map((c0l, ())), HashMap.newBuilder[C, Unit].++=(HashMap((c0l, ()))).++=(c0RMap).result()) + assertIdenticalKeys(Map((c0l, ())), HashMap((c0l, ())).++(c0RMap)) + assertIdenticalKeys(Map((c0l, ())), c0LMap ++: HashMap((c0r, ()))) + } + check(cs => HashMap(cs: _*)) // exercise special case for HashMap/HashMap + check(cs => TreeMap(cs: _*)) // exercise special case for HashMap/HasForEachEntry + check(cs => HashMap(cs: _*).withDefault(_ => ???)) // default cases + } } diff --git a/test/junit/scala/collection/immutable/TreeMapTest.scala b/test/junit/scala/collection/immutable/TreeMapTest.scala index 3f6a3952f622..82ec8130d2ef 100644 --- a/test/junit/scala/collection/immutable/TreeMapTest.scala +++ b/test/junit/scala/collection/immutable/TreeMapTest.scala @@ -176,7 +176,7 @@ class TreeMapTest extends AllocationTest { } @Test - def unionAndIntersectRetainLeft(): Unit = { + def retainLeft(): Unit = { case class C(a: Int)(override val toString: String) implicit val ordering: Ordering[C] = Ordering.by(_.a) val c0l = C(0)("l") @@ -188,14 +188,12 @@ class TreeMapTest extends AllocationTest { assertEquals(expected1, actual1) } - // This holds in 2.13.x only - //assertIdenticalKeys(Map((c0l, ())), HashMap((c0l, ())).++(HashMap((c0r, ())))) + assertIdenticalKeys(Map((c0l, ())), HashMap((c0l, ())).++(HashMap((c0r, ())))) assertIdenticalKeys(Map((c0l, ())), TreeMap((c0l, ())).++(HashMap((c0r, ())))) assertIdenticalKeys(Map((c0l, ())), TreeMap((c0l, ())).++(TreeMap((c0r, ())))) - // This holds in 2.13.x only - //assertIdenticalKeys(Map((c0l, ())), HashMap.newBuilder[C, Unit].++=(HashMap((c0l, ()))).++=(HashMap((c0r, ()))).result()) + assertIdenticalKeys(Map((c0l, ())), HashMap.newBuilder[C, Unit].++=(HashMap((c0l, ()))).++=(HashMap((c0r, ()))).result()) assertIdenticalKeys(Map((c0l, ())), TreeMap.newBuilder[C, Unit].++=(TreeMap((c0l, ()))).++=(HashMap((c0r, ()))).result()) assertIdenticalKeys(Map((c0l, ())), TreeMap.newBuilder[C, Unit].++=(TreeMap((c0l, ()))).++=(TreeMap((c0r, ()))).result()) From 493168baed4fde40ea3efd53a935338f9a28878e Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 23 Jun 2020 18:55:30 +1000 Subject: [PATCH 8/8] Rework recent, buggy change to immutable.TreeMap We need to favour keys from the left that equivalent (but ne), while favouring values from the right. (cherry picked from commit abd82a2f120f66c0303791768e806b7fb82db7c2) --- .../collection/immutable/RedBlackTree.scala | 34 +++++++++---------- .../collection/immutable/TreeMapTest.scala | 9 +++++ 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/library/scala/collection/immutable/RedBlackTree.scala b/src/library/scala/collection/immutable/RedBlackTree.scala index 92ade8ee1d92..86e4ec9079a1 100644 --- a/src/library/scala/collection/immutable/RedBlackTree.scala +++ b/src/library/scala/collection/immutable/RedBlackTree.scala @@ -820,17 +820,17 @@ private[collection] object RedBlackTree { } else mkTree(isRedTree(tl) || isRedTree(tr), k, v, tl, tr) } - private[this] def split[A, B](t: Tree[A, B], k: A)(implicit ordering: Ordering[A]): (Tree[A, B], Tree[A, B], Tree[A, B]) = - if(t eq null) (null, null, null) + private[this] def split[A, B](t: Tree[A, B], k2: A)(implicit ordering: Ordering[A]): (Tree[A, B], Tree[A, B], Tree[A, B], A) = + if(t eq null) (null, null, null, k2) else { - val cmp = ordering.compare(k, t.key) - if(cmp == 0) (t.left, t, t.right) + val cmp = ordering.compare(k2, t.key) + if(cmp == 0) (t.left, t, t.right, t.key) else if(cmp < 0) { - val (ll, b, lr) = split(t.left, k) - (ll, b, join(lr, t.key, t.value, t.right)) + val (ll, b, lr, k1) = split(t.left, k2) + (ll, b, join(lr, t.key, t.value, t.right), k1) } else { - val (rl, b, rr) = split(t.right, k) - (join(t.left, t.key, t.value, rl), b, rr) + val (rl, b, rr, k1) = split(t.right, k2) + (join(t.left, t.key, t.value, rl), b, rr, k1) } } @@ -853,20 +853,20 @@ private[collection] object RedBlackTree { if((t1 eq null) || (t1 eq t2)) t2 else if(t2 eq null) t1 else { - val (l2, _, r2) = split(t2, t1.key) - val tl = _union(t1.left, l2) - val tr = _union(t1.right, r2) - join(tl, t1.key, t1.value, tr) + val (l1, _, r1, k1) = split(t1, t2.key) + val tl = _union(l1, t2.left) + val tr = _union(r1, t2.right) + join(tl, k1, t2.value, tr) } private[this] def _intersect[A, B](t1: Tree[A, B], t2: Tree[A, B])(implicit ordering: Ordering[A]): Tree[A, B] = if((t1 eq null) || (t2 eq null)) null else if (t1 eq t2) t1 else { - val (l2, b, r2) = split(t2, t1.key) - val tl = _intersect(t1.left, l2) - val tr = _intersect(t1.right, r2) - if(b ne null) join(tl, t1.key, t1.value, tr) + val (l1, b, r1, k1) = split(t1, t2.key) + val tl = _intersect(l1, t2.left) + val tr = _intersect(r1, t2.right) + if(b ne null) join(tl, k1, t2.value, tr) else join2(tl, tr) } @@ -874,7 +874,7 @@ private[collection] object RedBlackTree { if((t1 eq null) || (t2 eq null)) t1 else if (t1 eq t2) null else { - val (l1, _, r1) = split(t1, t2.key) + val (l1, _, r1, k1) = split(t1, t2.key) val tl = _difference(l1, t2.left) val tr = _difference(r1, t2.right) join2(tl, tr) diff --git a/test/junit/scala/collection/immutable/TreeMapTest.scala b/test/junit/scala/collection/immutable/TreeMapTest.scala index 3f6a3952f622..f89e061dd42b 100644 --- a/test/junit/scala/collection/immutable/TreeMapTest.scala +++ b/test/junit/scala/collection/immutable/TreeMapTest.scala @@ -200,4 +200,13 @@ class TreeMapTest extends AllocationTest { assertIdenticalKeys(Map((c0l, ())), TreeMap.newBuilder[C, Unit].++=(TreeMap((c0l, ()))).++=(HashMap((c0r, ()))).result()) assertIdenticalKeys(Map((c0l, ())), TreeMap.newBuilder[C, Unit].++=(TreeMap((c0l, ()))).++=(TreeMap((c0r, ()))).result()) } + + @Test + def overwriteEntryRegression(): Unit = { + val x = TreeMap(1 -> "herring", 2 -> "cod", 3 -> "salmon") + val y = TreeMap(3 -> "wish") + val r1 = x ++ y + val r2 = (x.toSeq ++ y.toSeq).toMap + assertEquals(r1, r2) + } }