From 3cc86de1074d73251ff65b69bea5224594bdaa08 Mon Sep 17 00:00:00 2001 From: odersky Date: Fri, 13 Oct 2023 18:47:05 +0200 Subject: [PATCH 1/2] 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. --- .../tools/dotc/core/SymDenotations.scala | 23 +++++++++---------- .../tools/dotc/transform/PostTyper.scala | 21 ++++++++++++++++- .../tools/dotc/typer/VarianceChecker.scala | 21 +++++++++++------ tests/neg/i18588.check | 4 ++++ tests/neg/i18588.scala | 22 ++++++++++++++++++ 5 files changed, 71 insertions(+), 20 deletions(-) create mode 100644 tests/neg/i18588.check create mode 100644 tests/neg/i18588.scala diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index 3dfa5225df5b..519f4778349d 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/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 = diff --git a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala b/compiler/src/dotty/tools/dotc/transform/PostTyper.scala index 964486632979..299bcdea044e 100644 --- a/compiler/src/dotty/tools/dotc/transform/PostTyper.scala +++ b/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 diff --git a/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala b/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala index a5e0b62094ef..30e426c55da1 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)(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 { diff --git a/tests/neg/i18588.check b/tests/neg/i18588.check new file mode 100644 index 000000000000..f08aae89c44c --- /dev/null +++ b/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 diff --git a/tests/neg/i18588.scala b/tests/neg/i18588.scala new file mode 100644 index 000000000000..d57de4f695b8 --- /dev/null +++ b/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 From c2f0db4ad669218a88cacbd9a039d5437903447a Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 16 Oct 2023 20:20:34 +0200 Subject: [PATCH 2/2] 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