Merge pull request scala#10337 from som-snytt/issue/12745-map-from

Map.from and Set.from are more sound [ci: last-only]

Author: lrytz

src/library/scala/collection/immutable/HashMap.scala

@@ -57,7 +57,7 @@ final class HashMap[K, +V] private[immutable] (private[immutable] val rootNode:
 
   override def keySet: Set[K] = if (size == 0) Set.empty else new HashKeySet
 
-  private final class HashKeySet extends ImmutableKeySet {
+  private[immutable] final class HashKeySet extends ImmutableKeySet {
 
     private[this] def newKeySetOrThis(newHashMap: HashMap[K, _]): Set[K] =
       if (newHashMap eq HashMap.this) this else newHashMap.keySet

src/library/scala/collection/immutable/Map.scala

@@ -18,6 +18,7 @@ import scala.annotation.unchecked.uncheckedVariance
 import scala.collection.generic.DefaultSerializable
 import scala.collection.immutable.Map.Map4
 import scala.collection.mutable.{Builder, ReusableBuilder}
+import SeqMap.{SeqMap1, SeqMap2, SeqMap3, SeqMap4}
 
 /** Base type of immutable Maps */
 trait Map[K, +V]
@@ -28,7 +29,7 @@ trait Map[K, +V]
 
   override def mapFactory: scala.collection.MapFactory[Map] = Map
 
-  override final def toMap[K2, V2](implicit ev: (K, V) <:< (K2, V2)): Map[K2, V2] = this.asInstanceOf[Map[K2, V2]]
+  override final def toMap[K2, V2](implicit ev: (K, V) <:< (K2, V2)): Map[K2, V2] = Map.from(this.asInstanceOf[Map[K2, V2]])
 
   /** The same map with a given default function.
     *  Note: The default is only used for `apply`. Other methods like `get`, `contains`, `iterator`, `keys`, etc.
@@ -141,7 +142,7 @@ trait MapOps[K, +V, +CC[X, +Y] <: MapOps[X, Y, CC, _], +C <: MapOps[K, V, CC, C]
   override def keySet: Set[K] = new ImmutableKeySet
 
   /** The implementation class of the set returned by `keySet` */
-  protected class ImmutableKeySet extends AbstractSet[K] with GenKeySet with DefaultSerializable {
+  protected[immutable] class ImmutableKeySet extends AbstractSet[K] with GenKeySet with DefaultSerializable {
     def incl(elem: K): Set[K] = if (this(elem)) this else empty ++ this + elem
     def excl(elem: K): Set[K] = if (this(elem)) empty ++ this - elem else this
   }
@@ -206,11 +207,30 @@ object Map extends MapFactory[Map] {
 
   def empty[K, V]: Map[K, V] = EmptyMap.asInstanceOf[Map[K, V]]
 
-  def from[K, V](it: collection.IterableOnce[(K, V)]): Map[K, V] =
+  def from[K, V](it: IterableOnce[(K, V)]): Map[K, V] =
     it match {
       case it: Iterable[_] if it.isEmpty => empty[K, V]
-      case m: Map[K, V] => m
-      case _ => (newBuilder[K, V] ++= it).result()
+      // Since IterableOnce[(K, V)] launders the variance of K,
+      // identify only our implementations which can be soundly substituted.
+      // For example, the ordering used by sorted maps would fail on widened key type. (scala/bug#12745)
+      // The following type test is not sufficient: case m: Map[K, V] => m
+      case m: HashMap[K, V]    => m
+      case m: Map1[K, V]       => m
+      case m: Map2[K, V]       => m
+      case m: Map3[K, V]       => m
+      case m: Map4[K, V]       => m
+      //case m: WithDefault[K, V] => m    // cf SortedMap.WithDefault
+      //case m: SeqMap[K, V]     => SeqMap.from(it) // inlined here to avoid hard dependency
+      case m: ListMap[K, V]    => m
+      case m: TreeSeqMap[K, V] => m
+      case m: VectorMap[K, V]  => m
+      case m: SeqMap1[K, V]    => m
+      case m: SeqMap2[K, V]    => m
+      case m: SeqMap3[K, V]    => m
+      case m: SeqMap4[K, V]    => m
+
+      // Maps with a reified key type must be rebuilt, such as `SortedMap` and `IntMap`.
+      case _ => newBuilder[K, V].addAll(it).result()
     }
 
   def newBuilder[K, V]: Builder[(K, V), Map[K, V]] = new MapBuilderImpl

src/library/scala/collection/immutable/SeqMap.scala

@@ -46,7 +46,15 @@ object SeqMap extends MapFactory[SeqMap] {
 
   def from[K, V](it: collection.IterableOnce[(K, V)]): SeqMap[K, V] =
     it match {
-      case sm: SeqMap[K, V] => sm
+      //case sm: SeqMap[K, V] => sm
+      case m: ListMap[K, V]    => m
+      case m: TreeSeqMap[K, V] => m
+      case m: VectorMap[K, V]  => m
+      case m: SeqMap1[K, V]    => m
+      case m: SeqMap2[K, V]    => m
+      case m: SeqMap3[K, V]    => m
+      case m: SeqMap4[K, V]    => m
+      case it: Iterable[_] if it.isEmpty => empty[K, V]
       case _ => (newBuilder[K, V] ++= it).result()
     }
 
@@ -66,7 +74,7 @@ object SeqMap extends MapFactory[SeqMap] {
   }
 
   @SerialVersionUID(3L)
-  private final class SeqMap1[K, +V](key1: K, value1: V) extends SeqMap[K,V] with Serializable {
+  private[immutable] final class SeqMap1[K, +V](key1: K, value1: V) extends SeqMap[K,V] with Serializable {
     override def size: Int = 1
     override def knownSize: Int = 1
     override def apply(key: K) = if (key == key1) value1 else throw new NoSuchElementException("key not found: " + key)
@@ -90,7 +98,7 @@ object SeqMap extends MapFactory[SeqMap] {
   }
 
   @SerialVersionUID(3L)
-  private final class SeqMap2[K, +V](key1: K, value1: V, key2: K, value2: V) extends SeqMap[K,V] with Serializable {
+  private[immutable] final class SeqMap2[K, +V](key1: K, value1: V, key2: K, value2: V) extends SeqMap[K,V] with Serializable {
     override def size: Int = 2
     override def knownSize: Int = 2
     override def apply(key: K) =
@@ -125,7 +133,7 @@ object SeqMap extends MapFactory[SeqMap] {
   }
 
   @SerialVersionUID(3L)
-  private class SeqMap3[K, +V](key1: K, value1: V, key2: K, value2: V, key3: K, value3: V) extends SeqMap[K,V] with Serializable {
+  private[immutable] class SeqMap3[K, +V](key1: K, value1: V, key2: K, value2: V, key3: K, value3: V) extends SeqMap[K,V] with Serializable {
     override def size: Int = 3
     override def knownSize: Int = 3
     override def apply(key: K) =
@@ -166,7 +174,7 @@ object SeqMap extends MapFactory[SeqMap] {
   }
 
   @SerialVersionUID(3L)
-  private final class SeqMap4[K, +V](key1: K, value1: V, key2: K, value2: V, key3: K, value3: V, key4: K, value4: V) extends SeqMap[K,V] with Serializable {
+  private[immutable] final class SeqMap4[K, +V](key1: K, value1: V, key2: K, value2: V, key3: K, value3: V, key4: K, value4: V) extends SeqMap[K,V] with Serializable {
     override def size: Int = 4
     override def knownSize: Int = 4
     override def apply(key: K) =

src/library/scala/collection/immutable/Set.scala

@@ -96,12 +96,21 @@ object Set extends IterableFactory[Set] {
 
   def from[E](it: collection.IterableOnce[E]): Set[E] =
     it match {
-      // We want `SortedSet` (and subclasses, such as `BitSet`) to
-      // rebuild themselves to avoid element type widening issues
-      case _: SortedSet[E]         => (newBuilder[E] ++= it).result()
-      case _ if it.knownSize == 0  => empty[E]
-      case s: Set[E]               => s
-      case _                       => (newBuilder[E] ++= it).result()
+      case _ if it.knownSize == 0 => empty[E]
+      // Since IterableOnce[E] launders the variance of E,
+      // identify only our implementations which can be soundly substituted.
+      // It's not sufficient to match `SortedSet[E]` to rebuild and `Set[E]` to retain.
+      case s: HashSet[E] => s
+      case s: ListSet[E] => s
+      case s: Set1[E]    => s
+      case s: Set2[E]    => s
+      case s: Set3[E]    => s
+      case s: Set4[E]    => s
+      case s: HashMap[E @unchecked, _]#HashKeySet => s
+      case s: MapOps[E, Any, Map, Map[E, Any]]#ImmutableKeySet @unchecked => s
+      // We also want `SortedSet` (and subclasses, such as `BitSet`)
+      // to rebuild themselves, to avoid element type widening issues.
+      case _ => newBuilder[E].addAll(it).result()
     }
 
   def newBuilder[A]: Builder[A, Set[A]] = new SetBuilderImpl[A]

src/testkit/scala/tools/testkit/ReflectUtil.scala

@@ -42,6 +42,14 @@ object ReflectUtil {
           f.setAccessible(true)
       }
 
+  def getFinalFieldAccessible[T: ClassTag](n: String): Field =
+    classTag[T]
+      .runtimeClass.getDeclaredField(n)
+      .tap { f =>
+        if ((f.getModifiers & Modifier.PUBLIC) == 0)
+          f.setAccessible(true)
+      }
+
   // finds method with exact name or name$suffix but not name$default$suffix
   def getMethodAccessible[A: ClassTag](name: String): Method =
     implicitly[ClassTag[A]]

test/junit/scala/collection/FactoriesTest.scala

@@ -1,9 +1,9 @@
 package scala.collection
 
-import org.junit.Assert.{assertEquals, assertSame, assertTrue}
+import org.junit.Assert.{assertEquals, assertFalse, assertSame, assertTrue}
+import org.junit.Test
 
 import scala.collection.mutable.ArrayBuffer
-import org.junit.{Assert, Test}
 
 import scala.collection.{immutable => im}
 
@@ -16,15 +16,15 @@ class FactoriesTest {
     def cloneCollection[A, C](xs: Iterable[A])(implicit bf: BuildFrom[xs.type, A, C]): C =
       bf.fromSpecific(xs)(xs)
 
-    Assert.assertEquals("ArrayBuffer", cloneCollection(seq).collectionClassName)
+    assertEquals("ArrayBuffer", cloneCollection(seq).collectionClassName)
   }
 
   @Test def factoryIgnoresSourceCollectionFactory(): Unit = {
 
     def cloneElements[A, C](xs: Iterable[A])(cb: Factory[A, C]): C =
       cb.fromSpecific(xs)
 
-    Assert.assertEquals("List", cloneElements(seq)(Seq).collectionClassName)
+    assertEquals("List", cloneElements(seq)(Seq).collectionClassName)
   }
 
   def apply(factory: IterableFactory[Iterable]): Unit = {
@@ -203,8 +203,8 @@ class FactoriesTest {
       im.Set(1),
       im.HashSet("a", "b", "c"),
       im.ListSet('c', 'd'),
-      im.Map("a" -> 1, "b" -> 1, "c" -> 1).keySet,
-      im.HashMap("a" -> 1, "b" -> 1, "c" -> 1).keySet,
+      im.Map("a" -> 1, "b" -> 1, "c" -> 1).keySet,      // MapOps$ImmutableKeySet
+      im.HashMap("a" -> 1, "b" -> 1, "c" -> 1).keySet,  // HashKeySet
     )
 
     sortedFactoryFromIterableOnceReturnsSameReference(SortedSet, im.SortedSet)(
@@ -225,8 +225,9 @@ class FactoriesTest {
 
     mapFactoryFromIterableOnceReturnsSameReference(Map, im.Map)(im.Map(1 -> 2), im.HashMap(1 -> 2))
     mapFactoryFromIterableOnceReturnsSameReference(im.HashMap)(im.HashMap(1 -> 2))
-    mapFactoryFromIterableOnceReturnsSameReference(Map, im.Map)(im.IntMap(1 -> 2))
-    mapFactoryFromIterableOnceReturnsSameReference(Map, im.Map)(im.LongMap(1L -> 2))
+    // unsound due to widening, scala/bug#12745
+    //mapFactoryFromIterableOnceReturnsSameReference(Map, im.Map)(im.IntMap(1 -> 2))
+    //mapFactoryFromIterableOnceReturnsSameReference(Map, im.Map)(im.LongMap(1L -> 2))
 
     mapFactoryFromIterableOnceReturnsSameReference(im.SeqMap, Map, im.Map)(
       im.ListMap(1 -> 2),
@@ -291,6 +292,74 @@ class FactoriesTest {
 
   }
 
+  @Test def `custom set requires rebuild`: Unit = {
+    import scala.collection.immutable.{Set, SortedSet}
+    def testSame(xs: Set[Int], ys: Set[Any]): Boolean = {
+      assertFalse(ys("oops"))
+      assertFalse(ys.contains("oops"))
+      xs.eq(ys)
+    }
+    val s1 = Set(42)
+    assertTrue(testSame(s1, Set.from(s1)))
+    val ss = SortedSet(42)
+    assertFalse(testSame(ss, Set.from(ss)))
+
+    class Custom extends Set[Int] {
+      // Members declared in scala.collection.IterableOnce
+      def iterator: Iterator[Int] = Iterator.empty // implements `def iterator: Iterator[A]`
+
+      // Members declared in scala.collection.SetOps
+      def contains(elem: Int): Boolean = ??? // implements `def contains(elem: A): Boolean`
+
+      // Members declared in scala.collection.immutable.SetOps
+      def excl(elem: Int): scala.collection.immutable.Set[Int] = ??? // implements `def excl(elem: A): C`
+      def incl(elem: Int): scala.collection.immutable.Set[Int] = ??? // implements `def incl(elem: A): C`
+    }
+    val custom = new Custom
+    assertFalse(testSame(custom, Set.from(custom)))
+  }
+
+  @Test def `select maps do not require rebuild`: Unit = {
+    import scala.collection.immutable.{IntMap, ListMap, SeqMap, SortedMap, TreeSeqMap, VectorMap}
+
+    object X {
+      val iter: Iterable[(Any, String)] = List(1, 2, 3, 4, 5).map(i => i -> i.toString).to(SortedMap.sortedMapFactory)
+      val set: Map[Any, String] = Map.from(iter)
+    }
+
+    // where ys is constructed from(xs), verify no CEE, return true if same
+    def testSame(xs: Map[Int, String], ys: Map[Any, String]): Boolean = {
+      assertTrue(ys.get("oops").isEmpty)
+      assertFalse(ys.contains("oops"))
+      xs.eq(ys)
+    }
+    assertFalse(X.set.contains("oops")) // was CCE
+    // exercise small Maps
+    1.to(5).foreach { n =>
+      val m = Map(1.to(n).map(i => i -> i.toString): _*)
+      assertTrue(testSame(m, Map.from(m)))
+    }
+    // other Maps that don't require rebuilding
+    val listMap = ListMap(42 -> "four two")
+    assertTrue(testSame(listMap, Map.from(listMap)))
+    val treeSeqMap = TreeSeqMap(42 -> "four two")
+    assertTrue(testSame(treeSeqMap, Map.from(treeSeqMap)))
+    val vectorMap = VectorMap(42 -> "four two")
+    assertTrue(testSame(vectorMap, Map.from(vectorMap)))
+    val seqMap = SeqMap.empty[Int, String] + (42 -> "four two")
+    assertTrue(testSame(seqMap, Map.from(seqMap)))
+    // require rebuilding
+    val sordid = SortedMap(42 -> "four two")
+    assertFalse(testSame(sordid, Map.from(sordid)))
+    val defaulted = listMap.withDefault(_.toString * 2)
+    assertFalse(testSame(defaulted, Map.from(defaulted)))   // deoptimized, see desorted
+    val desorted = sordid.withDefault(_.toString * 2)
+    assertFalse(testSame(desorted, Map.from(desorted)))
+
+    assertTrue(Map.from(IntMap(42 -> "once", 27 -> "upon"): Iterable[(Any, String)]).get("a time").isEmpty)
+  }
+
+// java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Integer
 
   implicitly[Factory[Char, String]]
   implicitly[Factory[Char, Array[Char]]]

test/junit/scala/collection/immutable/MapTest.scala

@@ -1,10 +1,8 @@
 package scala.collection.immutable
 
-import org.junit.Assert.assertEquals
+import org.junit.Assert.{assertEquals, assertSame, assertTrue}
 import org.junit.Test
 
-import scala.annotation.nowarn
-
 class MapTest {
 
   @Test def builderCompare1(): Unit = {
@@ -144,10 +142,17 @@ class MapTest {
     }
   }
 
-  @Test @nowarn("cat=deprecation")
+  @Test @deprecated("Tests deprecated API", since="2.13.11")
   def t12699(): Unit = {
     val m1: HashMap[Int, Int] = HashMap(1 -> 1)
-    assertEquals(7, m1.+(elem1 = 2 -> 2, elem2 = 3 -> 3, elems = List( 4 -> 4, 5 -> 5, 6 -> 6, 7 -> 7): _*).size)
+    assertEquals(7, m1.+(elem1 = 2 -> 2, elem2 = 3 -> 3, elems = List(4 -> 4, 5 -> 5, 6 -> 6, 7 -> 7): _*).size)
     assertEquals(7, m1.+(2 -> 2, 3 -> 3, 4 -> 4, 5 -> 5, 6 -> 6, 7 -> 7).size)
   }
+
+  @Test def `t10496 unsound toMap`: Unit = {
+    val t = Map(42 -> 27)
+    assertSame(t, t.toMap[Any, Any])
+    assertTrue(t.toMap[Any, Any].get("hi").isEmpty)
+    assertTrue(TreeMap((1, 2)).toMap[Any, Any].get("hi").isEmpty) // was: CCE
+  }
 }