Enabled variance checking

Variance checking is now run as part of type-checking.

Fixed tests that exhibited variance errors. Added tests where some classes of
variance errors should be detected.

Author: odersky

src/dotty/tools/dotc/core/Types.scala

@@ -2203,6 +2203,7 @@ object Types {
     override def variance = 1
     override def toString = "Co" + super.toString
   }
+
   final class ContraTypeBounds(lo: Type, hi: Type, hc: Int) extends CachedTypeBounds(lo, hi, hc) {
     override def variance = -1
     override def toString = "Contra" + super.toString

src/dotty/tools/dotc/typer/Typer.scala

@@ -844,6 +844,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
     checkNoDoubleDefs(cls)
     val impl1 = cpy.Template(impl, constr1, parents1, self1, body1)
       .withType(dummy.termRef)
+    VarianceChecker.check(impl1)
     assignType(cpy.TypeDef(cdef, mods1, name, impl1), cls)
 
     // todo later: check that

src/dotty/tools/dotc/typer/CheckVariances.scala renamed to src/dotty/tools/dotc/typer/VarianceChecker.scala

@@ -1,63 +1,81 @@
 package dotty.tools.dotc
-package transform
+package typer
 
 import dotty.tools.dotc.ast.{ Trees, tpd }
 import core._
-import Types._, Contexts._, Flags._, Symbols._, Annotations._, Trees._
+import Types._, Contexts._, Flags._, Symbols._, Annotations._, Trees._, NameOps._
 import Decorators._
 import Variances._
 
 object VarianceChecker {
-
-  case class VarianceError(tvar: Symbol, required: Variance)
+  private case class VarianceError(tvar: Symbol, required: Variance)
+  def check(tree: tpd.Tree)(implicit ctx: Context) =
+    new VarianceChecker()(ctx).Traverser.traverse(tree)
 }
 
 /** See comments at scala.reflect.internal.Variance.
  */
-class VarianceChecker(implicit ctx: Context) {
+class VarianceChecker()(implicit ctx: Context) {
   import VarianceChecker._
   import tpd._
 
   private object Validator extends TypeAccumulator[Option[VarianceError]] {
     private var base: Symbol = _
 
+    /** Is no variance checking needed within definition of `base`? */
+    def ignoreVarianceIn(base: Symbol): Boolean = (
+         base.isTerm
+      || base.is(Package)
+      || base.is(Local)
+    )
+
     /** The variance of a symbol occurrence of `tvar` seen at the level of the definition of `base`.
      *  The search proceeds from `base` to the owner of `tvar`.
      *  Initially the state is covariant, but it might change along the search.
      */
-    def relativeVariance(tvar: Symbol, base: Symbol, v: Variance = Covariant): Variance = {
-      if (base.owner == tvar.owner) v
-      else if ((base is Param) && base.owner.isTerm) relativeVariance(tvar, base.owner.owner, flip(v))
-      else if (base.isTerm) Bivariant
+    def relativeVariance(tvar: Symbol, base: Symbol, v: Variance = Covariant): Variance = /*ctx.traceIndented(i"relative variance of $tvar wrt $base, so far: $v")*/ {
+      if (base == tvar.owner) v
+      else if ((base is Param) && base.owner.isTerm)
+        relativeVariance(tvar, paramOuter(base.owner), flip(v))
+      else if (ignoreVarianceIn(base.owner)) Bivariant
       else if (base.isAliasType) relativeVariance(tvar, base.owner, Invariant)
       else relativeVariance(tvar, base.owner, v)
     }
 
-    def isUncheckedVariance(tp: Type): Boolean = tp match {
-      case AnnotatedType(annot, tp1) =>
-        annot.symbol == defn.UncheckedVarianceAnnot || isUncheckedVariance(tp1)
-      case _ => false
-    }
+    /** The next level to take into account when determining the
+     *  relative variance with a method parameter as base. The method
+     *  is always skipped. If the method is a constructor, we also skip
+     *  its class owner, because constructors are not checked for variance
+     *  relative to the type parameters of their own class. On the other
+     *  hand constructors do count for checking the variance of type parameters
+     *  of enclosing classes. I believe the Scala 2 rules are too lenient in
+     *  that respect.
+     */
+    private def paramOuter(meth: Symbol) =
+      if (meth.isConstructor) meth.owner.owner else meth.owner
 
+    /** Check variance of abstract type `tvar` when referred from `base`. */
     private def checkVarianceOfSymbol(tvar: Symbol): Option[VarianceError] = {
       val relative = relativeVariance(tvar, base)
-      val required = Variances.compose(relative, this.variance)
       if (relative == Bivariant) None
       else {
-        def tvar_s = s"$tvar (${tvar.variance}${tvar.showLocated})"
+        val required = compose(relative, this.variance)
+        def tvar_s = s"$tvar (${varianceString(tvar.flags)} ${tvar.showLocated})"
         def base_s = s"$base in ${base.owner}" + (if (base.owner.isClass) "" else " in " + base.owner.enclosingClass)
-        ctx.log(s"verifying $tvar_s is $required at $base_s")
-        if (tvar.variance == required) None
+        ctx.log(s"verifying $tvar_s is ${varianceString(required)} at $base_s")
+        ctx.log(s"relative variance: ${varianceString(relative)}")
+        ctx.log(s"current variance: ${this.variance}")
+        ctx.log(s"owner chain: ${base.ownersIterator.toList}")
+        if (tvar is required) None
         else Some(VarianceError(tvar, required))
       }
     }
 
     /** For PolyTypes, type parameters are skipped because they are defined
      *  explicitly (their TypeDefs will be passed here.) For MethodTypes, the
-     *  same is true of the parameters (ValDefs) unless we are inside a
-     *  refinement, in which case they are checked from here.
+     *  same is true of the parameters (ValDefs).
      */
-    def apply(status: Option[VarianceError], tp: Type): Option[VarianceError] =
+    def apply(status: Option[VarianceError], tp: Type): Option[VarianceError] = ctx.traceIndented(s"variance checking $tp of $base at $variance") {
       if (status.isDefined) status
       else tp match {
         case tp: TypeRef =>
@@ -71,11 +89,12 @@ class VarianceChecker(implicit ctx: Context) {
           this(status, tp.resultType) // params will be checked in their ValDef nodes.
         case AnnotatedType(annot, _) if annot.symbol == defn.UncheckedVarianceAnnot =>
           status
-        case tp: ClassInfo =>
-          ???
+        //case tp: ClassInfo =>
+        //  ???  not clear what to do here yet. presumably, it's all checked at local typedefs
         case _ =>
           foldOver(status, tp)
       }
+    }
 
     def validateDefinition(base: Symbol): Option[VarianceError] = {
       val saved = this.base
@@ -85,12 +104,7 @@ class VarianceChecker(implicit ctx: Context) {
     }
   }
 
-  def varianceString(v: Variance) =
-    if (v is Covariant) "covariant"
-    else if (v is Contravariant) "contravariant"
-    else "invariant"
-
-  object Traverser extends TreeTraverser {
+  private object Traverser extends TreeTraverser {
     def checkVariance(sym: Symbol) = Validator.validateDefinition(sym) match {
       case Some(VarianceError(tvar, required)) =>
         ctx.error(
@@ -101,14 +115,8 @@ class VarianceChecker(implicit ctx: Context) {
 
     override def traverse(tree: Tree) = {
       def sym = tree.symbol
-      // No variance check for object-private/protected methods/values.
-      // Or constructors, or case class factory or extractor.
-      def skip = (
-           sym == NoSymbol
-        || sym.is(Local)
-        || sym.owner.isConstructor
-        //|| sym.owner.isCaseApplyOrUnapply // not clear why needed
-      )
+      // No variance check for private/protected[this] methods/values.
+      def skip = !sym.exists || sym.is(Local)
       tree match {
         case defn: MemberDef if skip =>
           ctx.debuglog(s"Skipping variance check of ${sym.showDcl}")

src/dotty/tools/dotc/typer/Variances.scala

@@ -1,5 +1,5 @@
 package dotty.tools.dotc
-package transform
+package typer
 
 import dotty.tools.dotc.ast.{Trees, tpd}
 import core._
@@ -90,4 +90,14 @@ object Variances {
     case _ =>
       Bivariant
   }
+
+  def varianceString(v: Variance) =
+    if (v is Covariant) "covariant"
+    else if (v is Contravariant) "contravariant"
+    else "invariant"
+
+  def varianceString(v: Int) =
+    if (v > 0) "+"
+    else if (v < 0) "-"
+    else ""
 }

test/dotc/tests.scala

@@ -85,7 +85,9 @@ class tests extends CompilerTest {
   @Test def neg_tailcall2 = compileFile(negDir, "tailcall/tailrec-2", xerrors = 2)
   @Test def neg_tailcall3 = compileFile(negDir, "tailcall/tailrec-3", xerrors = 2)
   @Test def neg_t1843 = compileFile(negDir, "t1843", xerrors = 1)
+  @Test def neg_t1843_variances = compileFile(negDir, "t1843-variances", xerrors = 1)
   @Test def neg_t2994 = compileFile(negDir, "t2994", xerrors = 2)
+  @Test def neg_variances = compileFile(negDir, "variances", xerrors = 2)
 
   @Test def dotc = compileDir(dotcDir + "tools/dotc", twice)
   @Test def dotc_ast = compileDir(dotcDir + "tools/dotc/ast", twice)

tests/neg/t1843-variances.scala

@@ -0,0 +1,25 @@
+/**
+* Scala Compiler Will Crash On this File
+* ... Or Will It?
+*
+*/
+
+object Crash {
+  trait UpdateType[A]
+  case class StateUpdate[+A](updateType : UpdateType[A], value : A)
+  case object IntegerUpdateType extends UpdateType[Integer]
+
+  //However this method will cause a crash
+  def crash(updates: List[StateUpdate[_]]): Unit = {
+    updates match {
+      case Nil =>
+      case u::us =>
+        u match {
+          //Line below seems to be the crashing line
+          case StateUpdate(key, newValue) if (key == IntegerUpdateType) =>
+            println("Requires a statement to induce the crash")
+          case _ =>
+        }
+    }
+  }
+}

tests/neg/variances.scala

@@ -0,0 +1,44 @@
+// Tests variance checking on default methods
+import reflect.ClassTag
+
+class Foo[+A: ClassTag](x: A) {
+
+  private[this] val elems: Array[A] = Array(x)
+
+  def f[B](x: Array[B] = elems): Array[B] = x // (1) should give a variance error here or ...
+
+}
+
+object Test extends App {
+
+  val foo: Foo[Object] = new Foo[String]("A")
+
+  val arr = foo.f[Object]()
+
+  arr(0) = new Integer(1) // (1) ... will give an ArrayStoreException here
+
+}
+
+class Outer[+A](x: A) {
+
+  private[this] var elem: A = x
+
+  def getElem: A = elem
+
+  class Inner(constrParam: A) {  // (2) should give a variance error here or ...
+    elem = constrParam
+  }
+
+}
+
+object Test2 extends App {
+  val o1: Outer[String] = new Outer[String]("A")
+  val o2: Outer[Object] = o1
+  new o2.Inner(new Integer(1))
+
+  val x: String = o1.getElem  // (2) ... will give a classcast exeption here
+
+}
+
+
+

tests/pos/t1843.scala

@@ -5,7 +5,7 @@
 */
 
 object Crash {
-  trait UpdateType[A]
+  trait UpdateType[+A]
   case class StateUpdate[+A](updateType : UpdateType[A], value : A)
   case object IntegerUpdateType extends UpdateType[Integer]
 

tests/pos/typers.scala

@@ -32,7 +32,7 @@ object typers {
   }
 
   class List[+T] {
-    def :: (x: T) = new :: (x, this)
+    def :: [U >: T](x: U): List[U] = new :: (x, this)
 
     def len: Int = this match {
       case x :: xs1 => 1 + xs1.len

tests/pos/variances.scala

@@ -0,0 +1,3 @@
+trait C[+T <: C[T, U], -U <: C[T, U]] {
+
+}