Fix variance loophole for private vars #18693
Conversation
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.
| /** 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 |
There was a problem hiding this comment.
Now that it's available outside of isAccessibleFrom, the name isCorrectThisType is not so clear anymore. Consider renaming to isAccessPrivilegedThisType or something like that?
| /** Update privateVarsSetNonLocally is symbol is a private variable | ||
| * that is selected from something other than `this` when assigned |
There was a problem hiding this comment.
Typo:
| /** Update privateVarsSetNonLocally is symbol is a private variable | |
| * that is selected from something other than `this` when assigned | |
| /** Update privateVarsSetNonLocally if symbol is a private variable | |
| * that is selected from something other than `this` when assigned |
| case Assign(lhs @ Select(qual, _), _) => | ||
| markVarAccess(lhs, qual) | ||
| super.transform(tree) |
There was a problem hiding this comment.
Do I understand correctly that private vars don't get a setter then? When mutated, they are directly Assigned instead of having an apply(setter, ...)?
There was a problem hiding this comment.
The setter gets added in a later phase, Getters.
| if sym.isClass then | ||
| VarianceChecker.check(tree, privateVarsSetNonLocally) |
There was a problem hiding this comment.
Does this work if we find an illegal access from the companion object, defined after the class? IIUC the flow of this phase, we won't explore the object before we get here.
A test case would be
class BoxWithCompanion[+A](value: A) {
private var cached: A = value // error
def get: A = cached
}
object BoxWithCompanion {
def put[A](box: BoxWithCompanion[A], value: A): Unit = {
box.cached = value
}
}There was a problem hiding this comment.
Good catch; it does not.
We need to mark such assigns in the phase before PostTyper since they can appear in companion objects that come after the variable declaration.
|
@sjrd Since the revision undoes all changes in PostTyper, it might be easiest to just review the total change set over the two commits directly. |
|
I think it's probably fine to keep the history. |
| /** 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 |
There was a problem hiding this comment.
do these annotation classes have any consequences for library compatibility?
There was a problem hiding this comment.
no, it's internal compiler stuff only.
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.