Fix variance loophole for private vars

In Scala 2 a setter was created at Typer for private, non-local vars. Variance
 checking then worked on the setter. But in Scala 3, the setter is only created
later, which caused a loophole for variance checking.

This PR does actually better than Scala 2 in the following sense: A private variable
counts as an invariant occurrence only if it is assigned with a selector different
from `this`. Or conversely, a variable containing a covariant type parameter in its
type can be read from different objects, but all assignments must be via this. The
motivation is that such variables effectively behave like vals for the purposes
of variance checking.

Author: odersky

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -864,6 +864,17 @@ object SymDenotations {
     final def isNullableClassAfterErasure(using Context): Boolean =
       isClass && !isValueClass && !is(ModuleClass) && symbol != defn.NothingClass
 
+    /** Is `pre` the same as C.this, where C is exactly the owner of this symbol,
+     *  or, if this symbol is protected, a subclass of the owner?
+     */
+    def isCorrectThisType(pre: Type)(using Context): Boolean = pre match
+      case pre: ThisType =>
+        (pre.cls eq owner) || this.is(Protected) && pre.cls.derivesFrom(owner)
+      case pre: TermRef =>
+        pre.symbol.moduleClass == owner
+      case _ =>
+        false
+
     /** Is this definition accessible as a member of tree with type `pre`?
      *  @param pre          The type of the tree from which the selection is made
      *  @param superAccess  Access is via super
@@ -888,18 +899,6 @@ object SymDenotations {
         (linked ne NoSymbol) && accessWithin(linked)
       }
 
-      /** Is `pre` the same as C.thisThis, where C is exactly the owner of this symbol,
-       *  or, if this symbol is protected, a subclass of the owner?
-       */
-      def isCorrectThisType(pre: Type): Boolean = pre match {
-        case pre: ThisType =>
-          (pre.cls eq owner) || this.is(Protected) && pre.cls.derivesFrom(owner)
-        case pre: TermRef =>
-          pre.symbol.moduleClass == owner
-        case _ =>
-          false
-      }
-
       /** Is protected access to target symbol permitted? */
       def isProtectedAccessOK: Boolean =
         inline def fail(str: String): false =

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

@@ -109,6 +109,13 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
       try op finally noCheckNews = saved
     }
 
+    /** The set of all private class variables that are assigned
+     *  when selected with a qualifier other than the `this` of the owning class.
+     *  Such variables can contain only invariant type parameters in
+     *  their types.
+     */
+    private var privateVarsSetNonLocally: Set[Symbol] = Set()
+
     def isCheckable(t: New): Boolean = !inJavaAnnot && !noCheckNews.contains(t)
 
     /** Mark parameter accessors that are aliases of like-named parameters
@@ -149,6 +156,8 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
     private def processMemberDef(tree: Tree)(using Context): tree.type = {
       val sym = tree.symbol
       Checking.checkValidOperator(sym)
+      if sym.isClass then
+        VarianceChecker.check(tree, privateVarsSetNonLocally)
       sym.transformAnnotations(transformAnnot)
       sym.defTree = tree
       tree
@@ -257,6 +266,14 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
       }
     }
 
+    /** Update privateVarsSetNonLocally is symbol is a private variable
+     *  that is selected from something other than `this` when assigned
+     */
+    private def markVarAccess(tree: Tree, qual: Tree)(using Context): Unit =
+      val sym = tree.symbol
+      if sym.is(Private, butNot = Local) && !sym.isCorrectThisType(qual.tpe) then
+        privateVarsSetNonLocally += sym
+
     def checkNoConstructorProxy(tree: Tree)(using Context): Unit =
       if tree.symbol.is(ConstructorProxy) then
         report.error(em"constructor proxy ${tree.symbol} cannot be used as a value", tree.srcPos)
@@ -395,7 +412,6 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
           registerIfHasMacroAnnotations(tree)
           val sym = tree.symbol
           if (sym.isClass)
-            VarianceChecker.check(tree)
             annotateExperimental(sym)
             checkMacroAnnotation(sym)
             if sym.isOneOf(GivenOrImplicit) then
@@ -472,6 +488,9 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
               case tpe => tpe
             }
           )
+        case Assign(lhs @ Select(qual, _), _) =>
+          markVarAccess(lhs, qual)
+          super.transform(tree)
         case Typed(Ident(nme.WILDCARD), _) =>
           withMode(Mode.Pattern)(super.transform(tree))
             // The added mode signals that bounds in a pattern need not

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

@@ -19,8 +19,8 @@ import printing.Formatting.hl
  */
 object VarianceChecker {
   case class VarianceError(tvar: Symbol, required: Variance)
-  def check(tree: tpd.Tree)(using Context): Unit =
-    VarianceChecker().Traverser.traverse(tree)
+  def check(tree: tpd.Tree, privateVarsSetNonLocally: collection.Set[Symbol])(using Context): Unit =
+    VarianceChecker(privateVarsSetNonLocally).Traverser.traverse(tree)
 
   /** Check that variances of type lambda correspond to their occurrences in its body.
    *  Note: this is achieved by a mechanism separate from checking class type parameters.
@@ -62,7 +62,7 @@ object VarianceChecker {
   end checkLambda
 }
 
-class VarianceChecker(using Context) {
+class VarianceChecker(privateVarsSetNonLocally: collection.Set[Symbol])(using Context) {
   import VarianceChecker._
   import tpd._
 
@@ -148,12 +148,19 @@ class VarianceChecker(using Context) {
       case _ =>
         apply(None, info)
 
-    def validateDefinition(base: Symbol): Option[VarianceError] = {
-      val saved = this.base
+    def validateDefinition(base: Symbol): Option[VarianceError] =
+      val savedBase = this.base
       this.base = base
+      val savedVariance = variance
+      def isLocal =
+        base.isAllOf(PrivateLocal)
+        || base.is(Private) && !privateVarsSetNonLocally.contains(base)
+      if base.is(Mutable, butNot = Method) && !isLocal then
+        variance = 0
       try checkInfo(base.info)
-      finally this.base = saved
-    }
+      finally
+        this.base = savedBase
+        this.variance = savedVariance
   }
 
   private object Traverser extends TreeTraverser {

tests/neg/i18588.check

@@ -0,0 +1,4 @@
+-- Error: tests/neg/i18588.scala:7:14 ----------------------------------------------------------------------------------
+7 |  private var cached: A = value // error
+  |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  |  covariant type A occurs in invariant position in type A of variable cached

tests/neg/i18588.scala

@@ -0,0 +1,22 @@
+class ROBox[+A](value: A) {
+  private var cached: A = value
+  def get: A = ROBox[A](value).cached
+}
+
+class Box[+A](value: A) {
+  private var cached: A = value // error
+  def get: A = cached
+
+  def put[AA >: A](value: AA): Unit = {
+    val box: Box[AA] = this
+    box.cached = value
+  }
+}
+
+trait Animal
+object Dog extends Animal
+object Cat extends Animal
+
+val dogBox: Box[Dog.type] = new Box(Dog)
+val _ = dogBox.put(Cat)
+val dog: Dog.type = dogBox.get