From c2f0db4ad669218a88cacbd9a039d5437903447a Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 16 Oct 2023 20:20:34 +0200 Subject: [PATCH] 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. --- .../dotty/tools/dotc/core/Definitions.scala | 1 + .../tools/dotc/core/SymDenotations.scala | 4 ++-- .../tools/dotc/transform/PostTyper.scala | 21 +--------------- .../src/dotty/tools/dotc/typer/Typer.scala | 24 ++++++++++++++----- .../tools/dotc/typer/VarianceChecker.scala | 9 +++---- .../internal/AssignedNonLocally.scala | 7 ++++++ tests/neg/i18588.check | 4 ++++ tests/neg/i18588.scala | 20 ++++++++++++++++ 8 files changed, 58 insertions(+), 32 deletions(-) create mode 100644 library/src/scala/annotation/internal/AssignedNonLocally.scala diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 51182f52a382..c4fa382fa94c 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/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") diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index 519f4778349d..99ae0dd60905 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/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) diff --git a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala b/compiler/src/dotty/tools/dotc/transform/PostTyper.scala index 299bcdea044e..964486632979 100644 --- a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala +++ b/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 diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 431e863f85d2..a5225ad46b73 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/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 => diff --git a/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala b/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala index 30e426c55da1..21fa9eed0df4 100644 --- a/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala +++ b/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 diff --git a/library/src/scala/annotation/internal/AssignedNonLocally.scala b/library/src/scala/annotation/internal/AssignedNonLocally.scala new file mode 100644 index 000000000000..8095ba91cfee --- /dev/null +++ b/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 diff --git a/tests/neg/i18588.check b/tests/neg/i18588.check index f08aae89c44c..5f7d6181a93c 100644 --- a/tests/neg/i18588.check +++ b/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 diff --git a/tests/neg/i18588.scala b/tests/neg/i18588.scala index d57de4f695b8..e8c89c1b06f1 100644 --- a/tests/neg/i18588.scala +++ b/tests/neg/i18588.scala @@ -13,6 +13,19 @@ 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 @@ -20,3 +33,10 @@ 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 + } +} \ No newline at end of file