Fix variance loophole for private vars (#18693)

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/Definitions.scala

@@ -993,6 +993,7 @@ class Definitions {
   // Annotation classes
   @tu lazy val AllowConversionsAnnot: ClassSymbol = requiredClass("scala.annotation.allowConversions")
   @tu lazy val AnnotationDefaultAnnot: ClassSymbol = requiredClass("scala.annotation.internal.AnnotationDefault")
+  @tu lazy val AssignedNonLocallyAnnot: ClassSymbol = requiredClass("scala.annotation.internal.AssignedNonLocally")
   @tu lazy val BeanPropertyAnnot: ClassSymbol = requiredClass("scala.beans.BeanProperty")
   @tu lazy val BooleanBeanPropertyAnnot: ClassSymbol = requiredClass("scala.beans.BooleanBeanProperty")
   @tu lazy val BodyAnnot: ClassSymbol = requiredClass("scala.annotation.internal.Body")

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

@@ -868,6 +868,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 isAccessPrivilegedThisType(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
@@ -892,18 +903,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 =
@@ -937,7 +936,7 @@ object SymDenotations {
         || boundary.isRoot
         || (accessWithin(boundary) || accessWithinLinked(boundary)) &&
              (  !this.is(Local)
-             || isCorrectThisType(pre)
+             || isAccessPrivilegedThisType(pre)
              || canBeLocal(name, flags)
                 && {
                   resetFlag(Local)

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

@@ -1103,6 +1103,18 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
             // allow assignments from the primary constructor to class fields
           ctx.owner.name.is(TraitSetterName) || ctx.owner.isStaticConstructor
 
+        /** Mark private variables that are assigned with a prefix other than
+         *  the `this` type of their owner with a `annotation.internal.AssignedNonLocally`
+         *  annotation. The annotation influences the variance check for these
+         *  variables, which is done at PostTyper. It will be removed after the
+         *  variance check.
+         */
+        def rememberNonLocalAssignToPrivate(sym: Symbol) = lhs1 match
+          case Select(qual, _)
+          if sym.is(Private, butNot = Local) && !sym.isAccessPrivilegedThisType(qual.tpe) =>
+            sym.addAnnotation(Annotation(defn.AssignedNonLocallyAnnot, lhs1.span))
+          case _ =>
+
         lhsCore match
           case Apply(fn, _) if fn.symbol.is(ExtensionMethod) =>
             def toSetter(fn: Tree): untpd.Tree = fn match
@@ -1136,15 +1148,16 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
           case _ => lhsCore.tpe match {
             case ref: TermRef =>
               val lhsVal = lhsCore.denot.suchThat(!_.is(Method))
-              if (canAssign(lhsVal.symbol)) {
-                // lhsBounds: (T .. Any) as seen from lhs prefix, where T is the type of lhsVal.symbol
+              val lhsSym = lhsVal.symbol
+              if canAssign(lhsSym) then
+                rememberNonLocalAssignToPrivate(lhsSym)
+                // lhsBounds: (T .. Any) as seen from lhs prefix, where T is the type of lhsSym
                 // This ensures we do the as-seen-from on T with variance -1. Test case neg/i2928.scala
                 val lhsBounds =
-                  TypeBounds.lower(lhsVal.symbol.info).asSeenFrom(ref.prefix, lhsVal.symbol.owner)
+                  TypeBounds.lower(lhsSym.info).asSeenFrom(ref.prefix, lhsSym.owner)
                 assignType(cpy.Assign(tree)(lhs1, typed(tree.rhs, lhsBounds.loBound)))
                   .computeAssignNullable()
-              }
-              else {
+              else
                 val pre = ref.prefix
                 val setterName = ref.name.setterName
                 val setter = pre.member(setterName)
@@ -1157,7 +1170,6 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
                   case _ =>
                     reassignmentToVal
                 }
-              }
             case TryDynamicCallType =>
               typedDynamicAssign(tree, pt)
             case tpe =>

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

@@ -148,12 +148,20 @@ 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) && !base.hasAnnotation(defn.AssignedNonLocallyAnnot)
+      if base.is(Mutable, butNot = Method) && !isLocal then
+        base.removeAnnotation(defn.AssignedNonLocallyAnnot)
+        variance = 0
       try checkInfo(base.info)
-      finally this.base = saved
-    }
+      finally
+        this.base = savedBase
+        this.variance = savedVariance
   }
 
   private object Traverser extends TreeTraverser {

library/src/scala/annotation/internal/AssignedNonLocally.scala

@@ -0,0 +1,7 @@
+package scala.annotation
+package internal
+
+/** An annotation to indicate that a private `var` was assigned with a prefix
+ *  other than the `this` type of its owner.
+ */
+class AssignedNonLocally() extends Annotation

tests/neg/i18588.check

@@ -0,0 +1,8 @@
+-- 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
+-- Error: tests/neg/i18588.scala:17:14 ---------------------------------------------------------------------------------
+17 |  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,42 @@
+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
+  }
+}
+
+class BoxWithCompanion[+A](value: A) {
+  private var cached: A = value // error
+  def get: A = cached
+}
+
+class BoxValid[+A](value: A, orig: A) {
+  private var cached: A = value // ok
+  def get: A = cached
+
+  def reset(): Unit =
+    cached = orig // ok: mutated through this prefix
+}
+
+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
+
+
+object BoxWithCompanion {
+  def put[A](box: BoxWithCompanion[A], value: A): Unit = {
+    box.cached = value
+  }
+}