From e657d300562ab8b2182bb364beea39f5717612c9 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 29 Oct 2021 14:45:03 +0200 Subject: [PATCH 1/3] run scalafmt --- build.sbt | 61 +++++++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/build.sbt b/build.sbt index 2bd0c785..238bffcf 100644 --- a/build.sbt +++ b/build.sbt @@ -87,29 +87,28 @@ lazy val compat = new MultiScalaCrossProject( ) .disablePlugins(ScalafixPlugin), _.jsSettings( - scalacOptions ++= { - val x = (LocalRootProject / baseDirectory).value.toURI.toString - val y = "https://raw.githubusercontent.com/scala/scala-collection-compat/" + sys.process - .Process("git rev-parse HEAD") - .lineStream_! - .head - val opt = CrossVersion.partialVersion(scalaVersion.value) match { - case Some((3, _)) => "-scalajs-mapSourceURI" - case _ => "-P:scalajs:mapSourceURI" - } - Seq(s"$opt:$x->$y/") - }, - Test / fork := false // Scala.js cannot run forked tests - ) - .jsEnablePlugins(ScalaJSJUnitPlugin), + scalacOptions ++= { + val x = (LocalRootProject / baseDirectory).value.toURI.toString + val y = "https://raw.githubusercontent.com/scala/scala-collection-compat/" + sys.process + .Process("git rev-parse HEAD") + .lineStream_! + .head + val opt = CrossVersion.partialVersion(scalaVersion.value) match { + case Some((3, _)) => "-scalajs-mapSourceURI" + case _ => "-P:scalajs:mapSourceURI" + } + Seq(s"$opt:$x->$y/") + }, + Test / fork := false // Scala.js cannot run forked tests + ).jsEnablePlugins(ScalaJSJUnitPlugin), _.nativeSettings( - nativeLinkStubs := true, - addCompilerPlugin( - "org.scala-native" % "junit-plugin" % nativeVersion cross CrossVersion.full - ), - libraryDependencies += "org.scala-native" %%% "junit-runtime" % nativeVersion, - Test / fork := false // Scala Native cannot run forked tests - ) + nativeLinkStubs := true, + addCompilerPlugin( + "org.scala-native" % "junit-plugin" % nativeVersion cross CrossVersion.full + ), + libraryDependencies += "org.scala-native" %%% "junit-runtime" % nativeVersion, + Test / fork := false // Scala Native cannot run forked tests + ) ) val compat211 = compat(Seq(JSPlatform, JVMPlatform, NativePlatform), scala211) @@ -171,7 +170,7 @@ lazy val scalafixRules = project .settings( scalaModuleAutomaticModuleName := None, versionPolicyIntention := Compatibility.None, - versionCheck := {}, // I don't understand why this fails otherwise?! oh well + versionCheck := {}, // I don't understand why this fails otherwise?! oh well name := "scala-collection-migrations", scalaVersion := scalafixScala212, libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % scalafixVersion @@ -289,14 +288,14 @@ lazy val scalafixTests = project .dependsOn(scalafixInput, scalafixRules) .enablePlugins(BuildInfoPlugin, ScalafixTestkitPlugin) -val ciScalaVersion = sys.env.get("CI_SCALA_VERSION").flatMap(Version.parse) -val isScalaJs = sys.env.get("CI_PLATFORM") == Some("js") -val isScalaNative = sys.env.get("CI_PLATFORM") == Some("native") -val isScalafix = sys.env.get("CI_MODE") == Some("testScalafix") -val isScalafmt = sys.env.get("CI_MODE") == Some("testScalafmt") -val isBinaryCompat = sys.env.get("CI_MODE") == Some("testBinaryCompat") -val isHeaderCheck = sys.env.get("CI_MODE") == Some("headerCheck") -val jdkVersion = sys.env.get("CI_JDK").map(_.toInt) +val ciScalaVersion = sys.env.get("CI_SCALA_VERSION").flatMap(Version.parse) +val isScalaJs = sys.env.get("CI_PLATFORM") == Some("js") +val isScalaNative = sys.env.get("CI_PLATFORM") == Some("native") +val isScalafix = sys.env.get("CI_MODE") == Some("testScalafix") +val isScalafmt = sys.env.get("CI_MODE") == Some("testScalafmt") +val isBinaryCompat = sys.env.get("CI_MODE") == Some("testBinaryCompat") +val isHeaderCheck = sys.env.get("CI_MODE") == Some("headerCheck") +val jdkVersion = sys.env.get("CI_JDK").map(_.toInt) // required by sbt-scala-module inThisBuild { From 9a9f0526cb8623ce3125667d7902a608309a026f Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 29 Oct 2021 14:47:02 +0200 Subject: [PATCH 2/3] preserve immutable collections in to(Target) conversions --- build.sbt | 18 ++- .../scala/collection/compat/CompatImpl.scala | 38 +++-- .../collection/compat/PackageShared.scala | 116 ++++++++++++-- .../scala/collection/ToConserveTest.scala | 141 ++++++++++++++++++ 4 files changed, 284 insertions(+), 29 deletions(-) create mode 100644 compat/src/test/scala-2.11_2.12/test/scala/collection/ToConserveTest.scala diff --git a/build.sbt b/build.sbt index 238bffcf..c33b07a5 100644 --- a/build.sbt +++ b/build.sbt @@ -72,19 +72,29 @@ lazy val compat = new MultiScalaCrossProject( sharedSourceDir / "scala-2.11_2.12" } }, + Test / unmanagedSourceDirectories += { + val sharedSourceDir = (ThisBuild / baseDirectory).value / "compat/src/test" + CrossVersion.partialVersion(scalaVersion.value) match { + case Some((3, _) | (2, 13)) => + sharedSourceDir / "scala-2.13" + case _ => + sharedSourceDir / "scala-2.11_2.12" + } + }, versionPolicyIntention := Compatibility.BinaryCompatible, - ) - .jvmSettings( - Test / unmanagedSourceDirectories += (ThisBuild / baseDirectory).value / "compat/src/test/scala-jvm", - junit, mimaBinaryIssueFilters ++= { import com.typesafe.tools.mima.core._ import com.typesafe.tools.mima.core.ProblemFilters._ Seq( exclude[ReversedMissingMethodProblem]("scala.collection.compat.PackageShared.*"), // it's package-private + exclude[Problem]("scala.collection.compat.*PreservingBuilder*") ) }, ) + .jvmSettings( + Test / unmanagedSourceDirectories += (ThisBuild / baseDirectory).value / "compat/src/test/scala-jvm", + junit, + ) .disablePlugins(ScalafixPlugin), _.jsSettings( scalacOptions ++= { diff --git a/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala b/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala index 8c320eda..5f3580f6 100644 --- a/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala +++ b/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala @@ -14,36 +14,35 @@ package scala.collection.compat import scala.reflect.ClassTag import scala.collection.generic.CanBuildFrom +import scala.{collection => c} import scala.collection.{immutable => i, mutable => m} /* builder optimized for a single ++= call, which returns identity on result if possible * and defers to the underlying builder if not. */ -private final class IdentityPreservingBuilder[A, CC[X] <: TraversableOnce[X]]( - that: m.Builder[A, CC[A]])(implicit ct: ClassTag[CC[A]]) - extends m.Builder[A, CC[A]] { +private abstract class PreservingBuilder[A, C <: TraversableOnce[A]] extends m.Builder[A, C] { + val that: m.Builder[A, C] + val ct: ClassTag[C] //invariant: ruined => (collection == null) - var collection: CC[A] = null.asInstanceOf[CC[A]] - var ruined = false + var collection: C = null.asInstanceOf[C] + var ruined = false private[this] def ruin(): Unit = { if (collection != null) that ++= collection - collection = null.asInstanceOf[CC[A]] + collection = null.asInstanceOf[C] ruined = true } override def ++=(elems: TraversableOnce[A]): this.type = elems match { - case ct(ca) if collection == null && !ruined => { + case ct(ca) if collection == null && !ruined => collection = ca this - } - case _ => { + case _ => ruin() that ++= elems this - } } def +=(elem: A): this.type = { @@ -53,14 +52,29 @@ private final class IdentityPreservingBuilder[A, CC[X] <: TraversableOnce[X]]( } def clear(): Unit = { - collection = null.asInstanceOf[CC[A]] + collection = null.asInstanceOf[C] if (ruined) that.clear() ruined = false } - def result(): CC[A] = if (collection == null) that.result() else collection + def result(): C = if (collection == null) that.result() else collection } +private final class IdentityPreservingBuilder[A, CC[X] <: TraversableOnce[X]]( + val that: m.Builder[A, CC[A]])(implicit val ct: ClassTag[CC[A]]) + extends PreservingBuilder[A, CC[A]] + +private final class IdentityPreservingBitSetBuilder[C <: c.BitSet](val that: m.Builder[Int, C])( + implicit val ct: ClassTag[C]) + extends PreservingBuilder[Int, C] + +private final class IdentityPreservingMapBuilder[ + K, + V, + CC[X, Y] <: c.Map[X, Y] with c.MapLike[X, Y, CC[X, Y]]](val that: m.Builder[(K, V), CC[K, V]])( + implicit val ct: ClassTag[CC[K, V]]) + extends PreservingBuilder[(K, V), CC[K, V]] + private[compat] object CompatImpl { def simpleCBF[A, C](f: => m.Builder[A, C]): CanBuildFrom[Any, A, C] = new CanBuildFrom[Any, A, C] { diff --git a/compat/src/main/scala-2.11_2.12/scala/collection/compat/PackageShared.scala b/compat/src/main/scala-2.11_2.12/scala/collection/compat/PackageShared.scala index 2bd2eff6..f8541e10 100644 --- a/compat/src/main/scala-2.11_2.12/scala/collection/compat/PackageShared.scala +++ b/compat/src/main/scala-2.11_2.12/scala/collection/compat/PackageShared.scala @@ -12,10 +12,10 @@ package scala.collection.compat +import scala.annotation.nowarn import scala.collection.generic._ import scala.reflect.ClassTag import scala.collection.{ - BitSet, GenTraversable, IterableLike, IterableView, @@ -61,9 +61,39 @@ private[compat] trait PackageShared { must be non-strict */ def builder: m.Builder[A, CC[A]] = fact match { - case c.Seq | i.Seq => new IdentityPreservingBuilder[A, i.Seq](i.Seq.newBuilder[A]) + case c.Seq | i.Seq => + new IdentityPreservingBuilder[A, i.Seq](i.Seq.newBuilder[A]) + case c.LinearSeq | i.LinearSeq => new IdentityPreservingBuilder[A, i.LinearSeq](i.LinearSeq.newBuilder[A]) + case i.Queue => + new IdentityPreservingBuilder[A, i.Queue](i.Queue.newBuilder[A]) + case i.Stream => + new IdentityPreservingBuilder[A, i.Stream](i.Stream.newBuilder[A]) + case i.Stack => + new IdentityPreservingBuilder[A, i.Stack](i.Stack.newBuilder[A]): @nowarn("cat=deprecation") + case i.List => + new IdentityPreservingBuilder[A, i.List](i.List.newBuilder[A]) + + case c.IndexedSeq | i.IndexedSeq => + new IdentityPreservingBuilder[A, i.IndexedSeq](i.IndexedSeq.newBuilder[A]) + case i.Vector => + new IdentityPreservingBuilder[A, i.Vector](i.Vector.newBuilder[A]) + + case c.Set | i.Set => + new IdentityPreservingBuilder[A, i.Set](i.Set.newBuilder[A]) + + case i.HashSet => + new IdentityPreservingBuilder[A, i.HashSet](i.HashSet.newBuilder[A]) + case i.ListSet => + new IdentityPreservingBuilder[A, i.ListSet](i.ListSet.newBuilder[A]) + + case c.Iterable | i.Iterable => + new IdentityPreservingBuilder[A, i.Iterable](i.Iterable.newBuilder[A]) + + case c.Traversable | i.Traversable => + new IdentityPreservingBuilder[A, i.Traversable](i.Traversable.newBuilder[A]) + case _ => fact.newBuilder[A] } simpleCBF(builder) @@ -71,29 +101,89 @@ private[compat] trait PackageShared { implicit def sortedSetCompanionToCBF[A: Ordering, CC[X] <: c.SortedSet[X] with c.SortedSetLike[X, CC[X]]]( - fact: SortedSetFactory[CC]): CanBuildFrom[Any, A, CC[A]] = - simpleCBF(fact.newBuilder[A]) + fact: SortedSetFactory[CC]): CanBuildFrom[Any, A, CC[A]] = { + def builder: m.Builder[A, CC[A]] = { + val b = fact match { + case c.SortedSet | i.SortedSet => + new IdentityPreservingBuilder[A, i.SortedSet](i.SortedSet.newBuilder[A]) + case i.TreeSet => + new IdentityPreservingBuilder[A, i.TreeSet](i.TreeSet.newBuilder[A]) + case _ => + fact.newBuilder[A] + } + // Cast needed because GADT inference doesn't unify CC (didn't dig down why). Example: + // def t: CC[A] = fact match { case i.SortedSet => null: i.SortedSet[A] } + b.asInstanceOf[m.Builder[A, CC[A]]] + } + simpleCBF(builder) + } implicit def arrayCompanionToCBF[A: ClassTag](fact: Array.type): CanBuildFrom[Any, A, Array[A]] = simpleCBF(Array.newBuilder[A]) - implicit def mapFactoryToCBF[K, V, CC[A, B] <: Map[A, B] with MapLike[A, B, CC[A, B]]]( - fact: MapFactory[CC]): CanBuildFrom[Any, (K, V), CC[K, V]] = - simpleCBF(fact.newBuilder[K, V]) + // bounds should be `c.` but binary compatibility + implicit def mapFactoryToCBF[K, + V, + CC[A, B] <: /*c.*/ Map[A, B] with /*c.*/ MapLike[A, B, CC[A, B]]]( + fact: MapFactory[CC]): CanBuildFrom[Any, (K, V), CC[K, V]] = { + def builder: m.Builder[(K, V), CC[K, V]] = { + val b = fact match { + case c.Map | i.Map => + new IdentityPreservingMapBuilder[K, V, i.Map](i.Map.newBuilder[K, V]) + case i.HashMap => + new IdentityPreservingMapBuilder[K, V, i.HashMap](i.HashMap.newBuilder[K, V]) + case i.ListMap => + new IdentityPreservingMapBuilder[K, V, i.ListMap](i.ListMap.newBuilder[K, V]) + case _ => + fact.newBuilder[K, V] + } + // Cast needed because GADT inference doesn't unify CC (didn't dig down why). Example: + // def t: CC[K, V] = fact match { case i.Map => null: i.Map[K, V] } + b.asInstanceOf[m.Builder[(K, V), CC[K, V]]] + } + simpleCBF(builder) + } implicit def sortedMapFactoryToCBF[ K: Ordering, V, CC[A, B] <: c.SortedMap[A, B] with c.SortedMapLike[A, B, CC[A, B]]]( - fact: SortedMapFactory[CC]): CanBuildFrom[Any, (K, V), CC[K, V]] = - simpleCBF(fact.newBuilder[K, V]) + fact: SortedMapFactory[CC]): CanBuildFrom[Any, (K, V), CC[K, V]] = { + def builder: m.Builder[(K, V), CC[K, V]] = { + val b = fact match { + case c.SortedMap | i.SortedMap => + new IdentityPreservingMapBuilder[K, V, i.SortedMap](i.SortedMap.newBuilder[K, V]) + case i.TreeMap => + new IdentityPreservingMapBuilder[K, V, i.TreeMap](i.TreeMap.newBuilder[K, V]) + case _ => + fact.newBuilder[K, V] + } + b.asInstanceOf[m.Builder[(K, V), CC[K, V]]] + } + simpleCBF(builder) + } - implicit def bitSetFactoryToCBF(fact: BitSetFactory[BitSet]): CanBuildFrom[Any, Int, BitSet] = - simpleCBF(fact.newBuilder) + implicit def bitSetFactoryToCBF( + fact: BitSetFactory[c.BitSet]): CanBuildFrom[Any, Int, c.BitSet] = { + def builder: m.Builder[Int, c.BitSet] = fact match { + case c.BitSet => + new IdentityPreservingBitSetBuilder[i.BitSet](i.BitSet.newBuilder) + case _ => + fact.newBuilder + } + simpleCBF(builder) + } implicit def immutableBitSetFactoryToCBF( - fact: BitSetFactory[i.BitSet]): CanBuildFrom[Any, Int, ImmutableBitSetCC[Int]] = - simpleCBF(fact.newBuilder) + fact: BitSetFactory[i.BitSet]): CanBuildFrom[Any, Int, ImmutableBitSetCC[Int]] = { + def builder: m.Builder[Int, i.BitSet] = fact match { + case i.BitSet => + new IdentityPreservingBitSetBuilder[i.BitSet](i.BitSet.newBuilder) + case _ => + fact.newBuilder + } + simpleCBF(builder) + } implicit def mutableBitSetFactoryToCBF( fact: BitSetFactory[m.BitSet]): CanBuildFrom[Any, Int, MutableBitSetCC[Int]] = diff --git a/compat/src/test/scala-2.11_2.12/test/scala/collection/ToConserveTest.scala b/compat/src/test/scala-2.11_2.12/test/scala/collection/ToConserveTest.scala new file mode 100644 index 00000000..2e8bdb95 --- /dev/null +++ b/compat/src/test/scala-2.11_2.12/test/scala/collection/ToConserveTest.scala @@ -0,0 +1,141 @@ +package test.scala.collection + +import org.junit.Assert.{assertSame, assertNotSame} +import org.junit.Test + +import scala.collection.compat._ +import scala.collection.compat.immutable.LazyList +import scala.{collection => c} +import scala.collection.{immutable => i, mutable => m} + +class ToConserveTest { + @Test def toConserveList: Unit = { + val l: c.Iterable[Int] = (1 to 3).toList + + assertSame(l, l.toList) + assertSame(l, l.toSeq) + assertSame(l, l.toIterable) + assertSame(l, l.toTraversable) + + assertSame(l, l.to(c.Traversable)) + assertSame(l, l.to(i.Traversable)) + + assertSame(l, l.to(c.Iterable)) + assertSame(l, l.to(i.Iterable)) + + assertSame(l, l.to(c.Seq)) + assertSame(l, l.to(i.Seq)) + + assertSame(l, l.to(c.LinearSeq)) + assertSame(l, l.to(i.LinearSeq)) + + assertSame(l, l.to(List)) + } + + @Test def toConserveImmutableHashSet: Unit = { + val s: c.Iterable[Int] = (1 to 10).to(i.HashSet) + assertSame(s, s.toSet) + assertSame(s, s.toIterable) + + assertSame(s, s.to(c.Iterable)) + assertSame(s, s.to(i.Iterable)) + + assertSame(s, s.to(c.Set)) + assertSame(s, s.to(i.Set)) + + assertSame(s, s.to(i.HashSet)) + + val ts: c.Iterable[Int] = s.to(i.TreeSet) + assertSame(ts, ts.to(c.SortedSet)) + assertSame(ts, ts.to(i.SortedSet)) + assertSame(ts, ts.to(i.TreeSet)) + + val bs: c.Iterable[Int] = s.to(i.BitSet) + assertSame(bs, bs.to(c.SortedSet)) + assertSame(bs, bs.to(i.SortedSet)) + assertSame(bs, bs.to(c.BitSet)) + assertSame(bs, bs.to(i.BitSet)) + } + + @Test def toConserveImmutableHashMap: Unit = { + val m: c.Iterable[(Int, Int)] = i.HashMap((1 to 10).map(x => (x, x)): _*) + + assertSame(m, m.toMap) + assertSame(m, m.toIterable) + + assertSame(m, m.to(c.Iterable)) + assertSame(m, m.to(i.Iterable)) + + // doesn't compile. found: Map.type, required: CanBuildFrom + // that's because mapFactoryToCBF's signature should accept c.Map, not i.Map, but bin compat + // assertSame(m, m.to(c.Map)) + assertSame(m, m.to(i.Map)) + + assertSame(m, m.to(i.HashMap)) + + val lm = m.to(i.ListMap) + assertSame(lm, lm.to(i.ListMap)) + + val tm = m.to(i.TreeMap) + assertSame(tm, tm.to(c.SortedMap)) + assertSame(tm, tm.to(i.SortedMap)) + assertSame(tm, tm.to(i.TreeMap)) + } + + @Test def toConserveLazyList: Unit = { + val l: c.Iterable[Int] = LazyList.from(1 to 10) + + assertSame(l, l.toSeq) + assertSame(l, l.toIterable) + + assertSame(l, l.to(c.Iterable)) + assertSame(l, l.to(i.Iterable)) + + assertSame(l, l.to(c.Seq)) + assertSame(l, l.to(i.Seq)) + + assertSame(l, l.to(c.LinearSeq)) + assertSame(l, l.to(i.LinearSeq)) + + assertSame(l, l.to(LazyList)) + } + + @Test def toConserveStream: Unit = { + val l: c.Iterable[Int] = Stream.from(1 to 10) + + assertSame(l, l.toStream) + assertSame(l, l.toSeq) + assertSame(l, l.toIterable) + + assertSame(l, l.to(c.Iterable)) + assertSame(l, l.to(i.Iterable)) + + assertSame(l, l.to(c.Seq)) + assertSame(l, l.to(i.Seq)) + + assertSame(l, l.to(c.LinearSeq)) + assertSame(l, l.to(i.LinearSeq)) + + assertSame(l, l.to(Stream)) + } + + @Test def toRebuildMutable: Unit = { + val s: c.Iterable[Int] = (1 to 3).to(m.HashSet) + assertSame(s, s.toIterable) // slightly inconsistent... + assertNotSame(s, s.to(c.Iterable)) + assertNotSame(s, s.to(m.Iterable)) + assertNotSame(s, s.to(c.Set)) + assertNotSame(s, s.to(m.Set)) + assertNotSame(s, s.to(m.HashSet)) + + val b: c.Iterable[Int] = (1 to 6).to(m.ArrayBuffer) + assertSame(b, b.toIterable) // slightly inconsistent... + assertNotSame(b, b.toBuffer) + assertNotSame(b, b.to(c.Iterable)) + assertNotSame(b, b.to(m.Iterable)) + assertNotSame(b, b.to(c.Seq)) + assertNotSame(b, b.to(m.Seq)) + assertNotSame(b, b.to(m.Buffer)) + assertNotSame(b, b.to(m.ArrayBuffer)) + } +} From 2133b3fd4cf8bbe96ab1491f5a8a37afdf2ae07b Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 1 Nov 2021 10:34:11 +0100 Subject: [PATCH 3/3] review feedback --- .../scala/collection/compat/CompatImpl.scala | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala b/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala index 5f3580f6..d6be2c2e 100644 --- a/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala +++ b/compat/src/main/scala-2.11_2.12/scala/collection/compat/CompatImpl.scala @@ -34,16 +34,15 @@ private abstract class PreservingBuilder[A, C <: TraversableOnce[A]] extends m.B ruined = true } - override def ++=(elems: TraversableOnce[A]): this.type = - elems match { - case ct(ca) if collection == null && !ruined => - collection = ca - this + override def ++=(elems: TraversableOnce[A]): this.type = { + (if (collection == null && !ruined) ct.unapply(elems) else None) match { + case Some(c) => collection = c case _ => ruin() that ++= elems - this } + this + } def +=(elem: A): this.type = { ruin() @@ -53,8 +52,10 @@ private abstract class PreservingBuilder[A, C <: TraversableOnce[A]] extends m.B def clear(): Unit = { collection = null.asInstanceOf[C] - if (ruined) that.clear() - ruined = false + if (ruined) { + that.clear() + ruined = false + } } def result(): C = if (collection == null) that.result() else collection