Replace unsound protected[this] members with explicit @uncheckedVariance annotations.

This commit is a port of scala/collection-strawman#521

Let’s assume that our code is unsound by not relying on the
`protected[this]` escape hatch that disables variance checking.

Instead, properly insert `@uncheckedVariance` annotations to see
where we are doing bad things.

On a positive note, this also encourages us to better document the
restrictions of such unsound members.

Add tests showing that `reverse` behaves correctly on ArrayDeque subclasses.

Fixes scala/collection-strawman#505
Fixes scala/collection-strawman#547

Author: julienrf

src/library/scala/Enumeration.scala

@@ -286,8 +286,8 @@ abstract class Enumeration (initial: Int) extends Serializable {
      *  new array of longs */
     def toBitMask: Array[Long] = nnIds.toBitMask
 
-    override protected[this] def fromSpecificIterable(coll: Iterable[Value]) = ValueSet.fromSpecific(coll)
-    override protected[this] def newSpecificBuilder() = ValueSet.newBuilder
+    override protected def fromSpecificIterable(coll: Iterable[Value]) = ValueSet.fromSpecific(coll)
+    override protected def newSpecificBuilder() = ValueSet.newBuilder
 
     def map(f: Value => Value): ValueSet = fromSpecificIterable(new View.Map(toIterable, f))
     def flatMap(f: Value => IterableOnce[Value]): ValueSet = fromSpecificIterable(new View.FlatMap(toIterable, f))

src/library/scala/collection/BitSet.scala

@@ -1,6 +1,7 @@
 package scala
 package collection
 
+import scala.annotation.unchecked.uncheckedVariance
 import scala.collection.mutable.Builder
 
 
@@ -17,8 +18,8 @@ import scala.collection.mutable.Builder
   * @define Coll `BitSet`
   */
 trait BitSet extends SortedSet[Int] with BitSetOps[BitSet] {
-  override protected[this] def fromSpecificIterable(coll: Iterable[Int]): BitSetC = bitSetFactory.fromSpecific(coll)
-  override protected[this] def newSpecificBuilder(): Builder[Int, BitSetC] = bitSetFactory.newBuilder()
+  override protected def fromSpecificIterable(coll: Iterable[Int]): BitSetC = bitSetFactory.fromSpecific(coll)
+  override protected def newSpecificBuilder(): Builder[Int, BitSetC] = bitSetFactory.newBuilder()
   override def empty: BitSetC = bitSetFactory.empty
 }
 
@@ -35,7 +36,14 @@ trait BitSetOps[+C <: BitSet with BitSetOps[C]]
 
   def bitSetFactory: SpecificIterableFactory[Int, BitSetC]
 
-  protected[this] type BitSetC = C
+  /**
+    * Type alias to `C`. It is used to provide a default implementation of the `fromSpecificIterable`
+    * and `newSpecificBuilder` operations.
+    *
+    * Due to the `@uncheckedVariance` annotation, usage of this type member can be unsound and is
+    * therefore not recommended.
+    */
+  protected type BitSetC = C @uncheckedVariance
 
   final def ordering: Ordering[Int] = Ordering.Int
 

src/library/scala/collection/IndexedSeq.scala

@@ -27,7 +27,7 @@ trait IndexedSeqOps[+A, +CC[_], +C] extends Any with SeqOps[A, CC, C] { self =>
 
   override def view: IndexedView[A] = new IndexedView.Id[A](this)
 
-  override protected[this] def reversed: Iterable[A] = new IndexedView.Reverse(this)
+  override protected def reversed: Iterable[A] = new IndexedView.Reverse(this)
 
   // Override transformation operations to use more efficient views than the default ones
   override def prepended[B >: A](elem: B): CC[B] = iterableFactory.from(new IndexedView.Prepended(elem, this))

src/library/scala/collection/Iterable.scala

@@ -22,11 +22,19 @@ trait Iterable[+A] extends IterableOnce[A] with IterableOps[A, Iterable, Iterabl
   final def toIterable: this.type = this
 
   //TODO scalac generates an override for this in AbstractMap; Making it final leads to a VerifyError
-  protected[this] def coll: this.type = this
+  protected def coll: this.type = this
 
-  protected[this] def fromSpecificIterable(coll: Iterable[A]): IterableCC[A] = iterableFactory.from(coll)
-  protected[this] def newSpecificBuilder(): Builder[A, IterableCC[A]] = iterableFactory.newBuilder[A]()
+  protected def fromSpecificIterable(coll: Iterable[A @uncheckedVariance]): IterableCC[A] @uncheckedVariance = iterableFactory.from(coll)
+  protected def newSpecificBuilder(): Builder[A, IterableCC[A]] @uncheckedVariance = iterableFactory.newBuilder[A]()
 
+  /**
+    * @note This operation '''has''' to be overridden by concrete collection classes to effectively
+    *       return an `IterableFactory[IterableCC]`. The implementation in `Iterable` only returns
+    *       an `IterableFactory[Iterable]`, but the compiler will '''not''' throw an error if the
+    *       effective `IterableCC` type constructor is more specific than `Iterable`.
+    *
+    * @return The factory of this collection.
+    */
   def iterableFactory: IterableFactory[IterableCC] = Iterable
 }
 
@@ -66,7 +74,14 @@ trait Iterable[+A] extends IterableOnce[A] with IterableOps[A, Iterable, Iterabl
   */
 trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with IterableOnceOps[A, CC, C] {
 
-  protected[this] type IterableCC[X] = CC[X]
+  /**
+    * Type alias to `CC`. It is used to provide a default implementation of the `fromSpecificIterable`
+    * and `newSpecificBuilder` operations.
+    *
+    * Due to the `@uncheckedVariance` annotation, usage of this type member can be unsound and is
+    * therefore not recommended.
+    */
+  protected type IterableCC[X] = CC[X] @uncheckedVariance
 
   /**
     * @return This collection as an `Iterable[A]`. No new collection will be built if `this` is already an `Iterable[A]`.
@@ -76,21 +91,26 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable
   /**
     * @return This collection as a `C`.
     */
-  protected[this] def coll: C
+  protected def coll: C
 
   /**
     * Defines how to turn a given `Iterable[A]` into a collection of type `C`.
     *
     * This process can be done in a strict way or a non-strict way (ie. without evaluating
     * the elements of the resulting collections). In other words, this methods defines
     * the evaluation model of the collection.
+    *
+    * @note As witnessed by the `@uncheckedVariance` annotation, using this method
+    *       might be unsound. However, as long as it is called with an
+    *       `Iterable[A]` obtained from `this` collection (as it is the case in the
+    *       implementations of operations where we use a `View[A]`), it is safe.
     */
-  protected[this] def fromSpecificIterable(coll: Iterable[A]): C
+  protected def fromSpecificIterable(coll: Iterable[A @uncheckedVariance]): C
 
   /** Similar to `fromSpecificIterable`, but for a (possibly) different type of element.
     * Note that the return type is now `CC[E]`.
     */
-  @`inline` final protected[this] def fromIterable[E](it: Iterable[E]): CC[E] = iterableFactory.from(it)
+  @`inline` final protected def fromIterable[E](it: Iterable[E]): CC[E] = iterableFactory.from(it)
 
   /**
     * @return The companion object of this ${coll}, providing various factory methods.
@@ -104,8 +124,12 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable
     * it is possible to implement this method but the resulting `Builder` will break laziness.
     * As a consequence, operations should preferably be implemented with `fromSpecificIterable`
     * instead of this method.
+    *
+    * @note As witnessed by the `@uncheckedVariance` annotation, using this method might
+    *       be unsound. However, as long as the returned builder is only fed
+    *       with `A` values taken from `this` instance, it is safe.
     */
-  protected[this] def newSpecificBuilder(): Builder[A, C]
+  protected def newSpecificBuilder(): Builder[A @uncheckedVariance, C]
 
   /** Selects the first element of this $coll.
     *  $orderDependent
@@ -315,26 +339,7 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable
     *             All these operations apply to those elements of this $coll
     *             which satisfy the predicate `p`.
     */
-  def withFilter(p: A => Boolean): collection.WithFilter[A, CC] = new WithFilter(p)
-
-  /** A template trait that contains just the `map`, `flatMap`, `foreach` and `withFilter` methods
-    * of trait `Iterable`.
-    *
-    * @define coll iterable collection
-    */
-  class WithFilter(p: A => Boolean) extends collection.WithFilter[A, CC] {
-
-    protected[this] def filtered = new View.Filter(IterableOps.this, p, isFlipped = false)
-
-    def map[B](f: A => B): CC[B] = iterableFactory.from(new View.Map(filtered, f))
-
-    def flatMap[B](f: A => IterableOnce[B]): CC[B] = iterableFactory.from(new View.FlatMap(filtered, f))
-
-    def foreach[U](f: A => U): Unit = filtered.foreach(f)
-
-    def withFilter(q: A => Boolean): WithFilter = new WithFilter(a => p(a) && q(a))
-
-  }
+  def withFilter(p: A => Boolean): collection.WithFilter[A, CC] = new IterableOps.WithFilter(this, p)
 
   /** A pair of, first, all elements that satisfy prediacte `p` and, second,
     *  all elements that do not. Interesting because it splits a collection in two.
@@ -710,6 +715,39 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable
   }
 }
 
+object IterableOps {
+
+  /** A trait that contains just the `map`, `flatMap`, `foreach` and `withFilter` methods
+    * of trait `Iterable`.
+    *
+    * @tparam A Element type (e.g. `Int`)
+    * @tparam CC Collection type constructor (e.g. `List`)
+    *
+    * @define coll collection
+    */
+  class WithFilter[+A, +CC[_]](
+    self: IterableOps[A, CC, _],
+    p: A => Boolean
+  ) extends collection.WithFilter[A, CC] {
+
+    protected def filtered: Iterable[A] =
+      new View.Filter(self, p, isFlipped = false)
+
+    def map[B](f: A => B): CC[B] =
+      self.iterableFactory.from(new View.Map(filtered, f))
+
+    def flatMap[B](f: A => IterableOnce[B]): CC[B] =
+      self.iterableFactory.from(new View.FlatMap(filtered, f))
+
+    def foreach[U](f: A => U): Unit = filtered.foreach(f)
+
+    def withFilter(q: A => Boolean): WithFilter[A, CC] =
+      new WithFilter(self, (a: A) => p(a) && q(a))
+
+  }
+
+}
+
 object Iterable extends IterableFactory.Delegate[Iterable](immutable.Iterable) {
   implicit def toLazyZipOps[A, CC[X] <: Iterable[X]](that: CC[A]): LazyZipOps[A, CC[A]] = new LazyZipOps(that)
 }

src/library/scala/collection/IterableOnce.scala

@@ -994,7 +994,7 @@ trait IterableOnceOps[+A, +CC[_], +C] extends Any { this: IterableOnce[A] =>
     else mutable.ArrayBuffer.from(this).toArray[B]
 
   // For internal use
-  protected[this] def reversed: Iterable[A] = {
+  protected def reversed: Iterable[A] = {
     var xs: immutable.List[A] = immutable.Nil
     val it = iterator()
     while (it.hasNext) xs = it.next() :: xs

src/library/scala/collection/Map.scala

@@ -13,12 +13,20 @@ trait Map[K, +V]
     with MapOps[K, V, Map, Map[K, V]]
     with Equals {
 
-  override protected[this] def fromSpecificIterable(coll: Iterable[(K, V)]): MapCC[K, V] = mapFactory.from(coll)
-  override protected[this] def newSpecificBuilder(): mutable.Builder[(K, V), MapCC[K, V]] = mapFactory.newBuilder[K, V]()
-
+  override protected def fromSpecificIterable(coll: Iterable[(K, V)] @uncheckedVariance): MapCC[K, V] @uncheckedVariance = mapFactory.from(coll)
+  override protected def newSpecificBuilder(): mutable.Builder[(K, V), MapCC[K, V]] @uncheckedVariance = mapFactory.newBuilder[K, V]()
+
+  /**
+    * @note This operation '''has''' to be overridden by concrete collection classes to effectively
+    *       return a `MapFactory[MapCC]`. The implementation in `Map` only returns
+    *       a `MapFactory[Map]`, but the compiler will '''not''' throw an error if the
+    *       effective `MapCC` type constructor is more specific than `Map`.
+    *
+    * @return The factory of this collection.
+    */
   def mapFactory: scala.collection.MapFactory[MapCC] = Map
 
-  def empty: MapCC[K, V] = mapFactory.empty
+  def empty: MapCC[K, V] @uncheckedVariance = mapFactory.empty
 
   def canEqual(that: Any): Boolean = true
 
@@ -60,12 +68,19 @@ trait MapOps[K, +V, +CC[_, _] <: IterableOps[_, AnyConstr, _], +C]
 
   override def view: MapView[K, V] = new MapView.Id(this)
 
-  protected[this] type MapCC[K, V] = CC[K, V]
+  /**
+    * Type alias to `CC`. It is used to provide a default implementation of the `fromSpecificIterable`
+    * and `newSpecificBuilder` operations.
+    *
+    * Due to the `@uncheckedVariance` annotation, usage of this type member can be unsound and is
+    * therefore not recommended.
+    */
+  protected type MapCC[K, V] = CC[K, V] @uncheckedVariance
 
   /** Similar to `fromIterable`, but returns a Map collection type.
     * Note that the return type is now `CC[K2, V2]` aka `MapCC[K2, V2]` rather than `IterableCC[(K2, V2)]`.
     */
-  @`inline` protected[this] final def mapFromIterable[K2, V2](it: Iterable[(K2, V2)]): CC[K2, V2] = mapFactory.from(it)
+  @`inline` protected final def mapFromIterable[K2, V2](it: Iterable[(K2, V2)]): CC[K2, V2] = mapFactory.from(it)
 
   def mapFactory: MapFactory[MapCC]
 
@@ -209,21 +224,7 @@ trait MapOps[K, +V, +CC[_, _] <: IterableOps[_, AnyConstr, _], +C]
     */
   def empty: C
 
-  override def withFilter(p: ((K, V)) => Boolean): MapWithFilter = new MapWithFilter(p)
-
-  /** Specializes `WithFilter` for Map collection types
-    *
-    * @define coll map collection
-    */
-  class MapWithFilter(p: ((K, V)) => Boolean) extends WithFilter(p) {
-
-    def map[K2, V2](f: ((K, V)) => (K2, V2)): CC[K2, V2] = mapFactory.from(new View.Map(filtered, f))
-
-    def flatMap[K2, V2](f: ((K, V)) => IterableOnce[(K2, V2)]): CC[K2, V2] = mapFactory.from(new View.FlatMap(filtered, f))
-
-    override def withFilter(q: ((K, V)) => Boolean): MapWithFilter = new MapWithFilter(kv => p(kv) && q(kv))
-
-  }
+  override def withFilter(p: ((K, V)) => Boolean): MapOps.WithFilter[K, V, IterableCC, CC] = new MapOps.WithFilter(this, p)
 
   /** Builds a new map by applying a function to all elements of this $coll.
     *
@@ -280,6 +281,30 @@ trait MapOps[K, +V, +CC[_, _] <: IterableOps[_, AnyConstr, _], +C]
   def + [V1 >: V](kv: (K, V1)): CC[K, V1] = mapFactory.from(new View.Appended(toIterable, kv))
 }
 
+object MapOps {
+  /** Specializes `WithFilter` for Map collection types by adding overloads to transformation
+    * operations that can return a Map.
+    *
+    * @define coll map collection
+    */
+  class WithFilter[K, +V, +IterableCC[_], +CC[_, _] <: IterableOps[_, AnyConstr, _]](
+    self: MapOps[K, V, CC, _] with IterableOps[(K, V), IterableCC, _],
+    p: ((K, V)) => Boolean
+  ) extends IterableOps.WithFilter[(K, V), IterableCC](self, p) {
+
+    def map[K2, V2](f: ((K, V)) => (K2, V2)): CC[K2, V2] =
+      self.mapFactory.from(new View.Map(filtered, f))
+
+    def flatMap[K2, V2](f: ((K, V)) => IterableOnce[(K2, V2)]): CC[K2, V2] =
+      self.mapFactory.from(new View.FlatMap(filtered, f))
+
+    override def withFilter(q: ((K, V)) => Boolean): WithFilter[K, V, IterableCC, CC] =
+      new WithFilter[K, V, IterableCC, CC](self, (kv: (K, V)) => p(kv) && q(kv))
+
+  }
+
+}
+
 /**
   * $factoryInfo
   * @define coll map