Revised scheme:Mark non-local assigns at Typer

We need to mark such assigns in the phase before PostTyper since they can
appear in companion objects that come after the variable declaration.

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

@@ -867,7 +867,7 @@ object SymDenotations {
     /** 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
+    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 =>
@@ -932,7 +932,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/transform/PostTyper.scala

@@ -109,13 +109,6 @@ 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
@@ -156,8 +149,6 @@ 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
@@ -266,14 +257,6 @@ 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)
@@ -412,6 +395,7 @@ 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
@@ -488,9 +472,6 @@ 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/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

@@ -19,8 +19,8 @@ import printing.Formatting.hl
  */
 object VarianceChecker {
   case class VarianceError(tvar: Symbol, required: Variance)
-  def check(tree: tpd.Tree, privateVarsSetNonLocally: collection.Set[Symbol])(using Context): Unit =
-    VarianceChecker(privateVarsSetNonLocally).Traverser.traverse(tree)
+  def check(tree: tpd.Tree)(using Context): Unit =
+    VarianceChecker().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(privateVarsSetNonLocally: collection.Set[Symbol])(using Context) {
+class VarianceChecker(using Context) {
   import VarianceChecker._
   import tpd._
 
@@ -154,8 +154,9 @@ class VarianceChecker(privateVarsSetNonLocally: collection.Set[Symbol])(using Co
       val savedVariance = variance
       def isLocal =
         base.isAllOf(PrivateLocal)
-        || base.is(Private) && !privateVarsSetNonLocally.contains(base)
+        || 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

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

@@ -2,3 +2,7 @@
 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

@@ -13,10 +13,30 @@ class Box[+A](value: A) {
   }
 }
 
+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
+  }
+}