Close the protected[local] escape hatch

Author: odersky

compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala

@@ -32,7 +32,7 @@ class VarianceChecker()(implicit ctx: Context) {
     def ignoreVarianceIn(base: Symbol): Boolean = (
          base.isTerm
       || base.is(Package)
-      || base.is(Local)
+      || base.is(PrivateLocal)
     )
 
     /** The variance of a symbol occurrence of `tvar` seen at the level of the definition of `base`.
@@ -112,9 +112,13 @@ class VarianceChecker()(implicit ctx: Context) {
     def checkVariance(sym: Symbol, pos: Position) = Validator.validateDefinition(sym) match {
       case Some(VarianceError(tvar, required)) =>
         def msg = i"${varianceString(tvar.flags)} $tvar occurs in ${varianceString(required)} position in type ${sym.info} of $sym"
-        if (ctx.scala2Mode && sym.owner.isConstructor) {
+        if (ctx.scala2Mode &&
+            (sym.owner.isConstructor || sym.ownersIterator.exists(_.is(ProtectedLocal)))) {
           ctx.migrationWarning(s"According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:\n$msg", pos)
-          patch(Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") // TODO use an import or shorten if possible
+            // patch(Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance")
+            // Patch is disabled until two TODOs are solved:
+            // TODO use an import or shorten if possible
+            // TODO need to use a `:' if annotation is on term
         }
         else ctx.error(msg, pos)
       case None =>
@@ -125,7 +129,7 @@ class VarianceChecker()(implicit ctx: Context) {
       // No variance check for private/protected[this] methods/values.
       def skip =
         !sym.exists ||
-        sym.is(Local) || // !!! watch out for protected local!
+        sym.is(PrivateLocal) ||
         sym.is(TypeParam) && sym.owner.isClass // already taken care of in primary constructor of class
       tree match {
         case defn: MemberDef if skip =>

tests/neg/i3989d.scala

@@ -0,0 +1,18 @@
+trait A[+_X] {
+  protected[this] type X = _X // error: variance
+  def f: X
+}
+
+trait B extends A[B] {
+  def f: X = new B {}
+}
+
+class C extends B with A[C] {
+  // should be required because the inherited f is of type B, not C
+  // override def f: X = new C
+}
+
+object Test extends App {
+  val c1 = new C
+  val c2: C = c1.f
+}

tests/neg/t3272.scala

@@ -0,0 +1,8 @@
+trait A {
+  trait C[+T] {
+    protected[this] def f(t: T): Unit = {} // error: covariant type T occurs in contravariant position in type T of value t
+  }
+  trait D[T] extends C[T] {
+    def g(t: T): Unit = { f(t) }
+  }
+}

tests/pos/t7093.scala renamed to tests/neg/t7093.scala

@@ -1,12 +1,10 @@
-object Test {
-
-  trait A[+X] {
-    protected[this] def f(x: X): X = x
-  }
+trait A[+X] {
+  protected[this] def f(x: X): X = x // error: variance
+}
 
-  trait B extends A[B] {
-    def kaboom = f(new B {})
-  }
+trait B extends A[B] {
+  def kaboom = f(new B {})
+}
 
 // protected[this] disables variance checking
 // of the signature of `f`.
@@ -16,12 +14,12 @@ object Test {
 // The protected[this] loophole is widely used
 // in the collections, every newBuilder method
 // would fail variance checking otherwise.
-  class C extends B with A[C] {
-    override protected[this] def f(c: C) = c
-  }
+class C extends B with A[C] {
+  override protected[this] def f(c: C) = c
+}
 
 // java.lang.ClassCastException: B$$anon$1 cannot be cast to C
 //  at C.f(<console>:15)
+object Test extends App {
   new C().kaboom
 }
-

tests/pos-special/strawman-collections/CollectionStrawMan5.scala

@@ -56,7 +56,7 @@ object CollectionStrawMan5 {
 
   /** Base trait for strict collections */
   trait Buildable[+A, +To <: Iterable[A]] extends Iterable[A] {
-    protected[this] def newBuilder: Builder[A, To]
+    protected[this] def newBuilder: Builder[A @uncheckedVariance, To]
     override def partition(p: A => Boolean): (To, To) = {
       val l, r = newBuilder
       iterator.foreach(x => (if (p(x)) l else r) += x)
@@ -95,7 +95,7 @@ object CollectionStrawMan5 {
        with IterableOps[A]
        with IterableMonoTransforms[A, C[A @uncheckedVariance]] // sound bcs of VarianceNote
        with IterablePolyTransforms[A, C] {
-    protected[this] def fromLikeIterable(coll: Iterable[A]): C[A] = fromIterable(coll)
+    protected[this] def fromLikeIterable(coll: Iterable[A @uncheckedVariance]): C[A @uncheckedVariance] = fromIterable(coll)
   }
 
   /** Base trait for Seq operations */
@@ -115,7 +115,7 @@ object CollectionStrawMan5 {
 
   trait IterableMonoTransforms[+A, +Repr] extends Any {
     protected def coll: Iterable[A]
-    protected[this] def fromLikeIterable(coll: Iterable[A]): Repr
+    protected[this] def fromLikeIterable(coll: Iterable[A] @scala.annotation.unchecked.uncheckedVariance): Repr
     def filter(p: A => Boolean): Repr = fromLikeIterable(View.Filter(coll, p))
     def partition(p: A => Boolean): (Repr, Repr) = {
       val pn = View.Partition(coll, p)
@@ -166,7 +166,7 @@ object CollectionStrawMan5 {
     }
     def length: Int =
       if (isEmpty) 0 else 1 + tail.length
-    protected[this] def newBuilder = new ListBuffer[A]
+    protected[this] def newBuilder = new ListBuffer[A @uncheckedVariance]
     def ++:[B >: A](prefix: List[B]): List[B] =
       if (prefix.isEmpty) this
       else Cons(prefix.head, prefix.tail ++: this)

tests/pos-special/strawman-collections/CollectionStrawMan6.scala

@@ -168,7 +168,7 @@ object CollectionStrawMan6 extends LowPriority {
   trait Buildable[+A, +Repr] extends Any with IterableMonoTransforms[A, Repr]  {
 
     /** Creates a new builder. */
-    protected[this] def newBuilder: Builder[A, Repr]
+    protected[this] def newBuilder: Builder[A, Repr] @uncheckedVariance
 
     /** Optimized, push-based version of `partition`. */
     override def partition(p: A => Boolean): (Repr, Repr) = {
@@ -226,7 +226,7 @@ object CollectionStrawMan6 extends LowPriority {
     /** Create a collection of type `C[A]` from the elements of `coll`, which has
      *  the same element type as this collection. Overridden in StringOps and ArrayOps.
      */
-    protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): C[A] = fromIterable(coll)
+    protected[this] def fromIterableWithSameElemType(coll: Iterable[A] @uncheckedVariance): C[A]@uncheckedVariance  = fromIterable(coll)
   }
 
   /** Base trait for Seq operations */
@@ -349,7 +349,7 @@ object CollectionStrawMan6 extends LowPriority {
    */
   trait IterableMonoTransforms[+A, +Repr] extends Any {
     protected def coll: Iterable[A]
-    protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): Repr
+    protected[this] def fromIterableWithSameElemType(coll: Iterable[A] @uncheckedVariance): Repr
 
     /** All elements satisfying predicate `p` */
     def filter(p: A => Boolean): Repr = fromIterableWithSameElemType(View.Filter(coll, p))
@@ -420,7 +420,7 @@ object CollectionStrawMan6 extends LowPriority {
 
     def fromIterable[B](c: Iterable[B]): List[B] = List.fromIterable(c)
 
-    protected[this] def newBuilder = new ListBuffer[A].mapResult(_.toList)
+    protected[this] def newBuilder = new ListBuffer[A].mapResult(_.toList): @uncheckedVariance
 
     /** Prepend element */
     def :: [B >: A](elem: B): List[B] =  new ::(elem, this)

tests/pos/GenericTraversableTemplate.scala

@@ -62,7 +62,7 @@ trait GenericTraversableTemplate[+A, +CC[X] <: GenTraversable[X]] extends HasNew
 
   /** The builder that builds instances of type $Coll[A]
    */
-  protected[this] def newBuilder: Builder[A, CC[A]] = companion.newBuilder[A]
+  protected[this] def newBuilder: Builder[A, CC[A]] @uncheckedVariance = companion.newBuilder[A]
 
   /** The generic builder that builds instances of $Coll
    *  at arbitrary element types.