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.

4 files changed, 58 insertions, 38 deletions

diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala
index 19141a873..474958b86 100644
--- a/src/dotty/tools/dotc/core/Types.scala
+++ b/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
diff --git a/src/dotty/tools/dotc/typer/Typer.scala b/src/dotty/tools/dotc/typer/Typer.scala
index 729536308..0ce02d198 100644
--- a/src/dotty/tools/dotc/typer/Typer.scala
+++ b/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
diff --git a/src/dotty/tools/dotc/typer/CheckVariances.scala b/src/dotty/tools/dotc/typer/VarianceChecker.scala
index f21a99566..36310767a 100644
--- a/src/dotty/tools/dotc/typer/CheckVariances.scala
+++ b/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}")
diff --git a/src/dotty/tools/dotc/typer/Variances.scala b/src/dotty/tools/dotc/typer/Variances.scala
index d793f7942..fadf9f952 100644
--- a/src/dotty/tools/dotc/typer/Variances.scala
+++ b/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 ""
 }