Skip to content

Check restrictions on how this values can be used in constructors. #5019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 19, 2024
Merged

Check restrictions on how this values can be used in constructors. #5019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bacf85
Emit lifted methods and lambda methods as static if possible.
sjrd Oct 7, 2024
274713e
Check restrictions on how `this` values can be used in constructors.
sjrd Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 134 additions & 13 deletions compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,133 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
)(withNewLocalNameScope(body))
}

// Global state for tracking methods that reference `this` -----------------

/* scalac generates private instance methods for
* a) local defs and
* b) anonymous functions.
*
* In many cases, these methods do not need access to the enclosing `this`
* value. For every other local variable, `lambdalift` only adds them as
* arguments when they are needed; but `this` is always implicitly added
* because all such methods are instance methods.
*
* We run a separate analysis to figure out which of those methods actually
* need their `this` value. We compile those that do not as static methods,
* as if they were `isStaticMember`.
*
* The `delambdafy` phase of scalac performs a similar analysis, although
* as it runs after our backend (for other reasons), we do not benefit from
* it. Our analysis is much simpler because it deals with local defs and
* anonymous functions in a unified way.
*
* Performing this analysis is particularly important for lifted methods
* that appear within arguments to a super constructor call. The lexical
* scope of Scala guarantees that they cannot use `this`, but if they are
* compiled as instance methods, they force the constructor to leak the
* `this` value before the super constructor call, which is invalid.
* While most of the time this analysis is only an optimization, in those
* (rare) situations it is required for correctness. See for example
* the partest `run/t8733.scala`.
*/

private var statifyCandidateMethodsThatReferenceThis: scala.collection.Set[Symbol] = null

/** Is that given method a statify-candidate?
*
* If a method is a statify-candidate, we will analyze whether it actually
* needs its `this` value. If it does not, we will compile it as a static
* method in the IR.
*
* A method is a statify-candidate if it is a lifted method (a local def)
* or the target of an anonymous function.
*
* TODO Currently we also require that the method owner not be a JS type.
* We should relax that restriction in the future.
*/
private def isStatifyCandidate(sym: Symbol): Boolean =
(sym.isLiftedMethod || sym.isDelambdafyTarget) && !isJSType(sym.owner)

/** Do we compile the given method as a static method in the IR?
*
* This is true if one of the following is true:
*
* - It is `isStaticMember`, or
* - It is a statify-candidate method and it does not reference `this`.
*/
private def compileAsStaticMethod(sym: Symbol): Boolean = {
sym.isStaticMember || {
isStatifyCandidate(sym) &&
!statifyCandidateMethodsThatReferenceThis.contains(sym)
}
}

/** Finds all statify-candidate methods that reference `this`, inspired by Delambdafy. */
private final class ThisReferringMethodsTraverser extends Traverser {
// the set of statify-candidate methods that directly refer to `this`
private val roots = mutable.Set.empty[Symbol]

// for each statify-candidate method `m`, the set of statify-candidate methods that call `m`
private val methodReferences = mutable.Map.empty[Symbol, mutable.Set[Symbol]]

def methodReferencesThisIn(tree: Tree): collection.Set[Symbol] = {
traverse(tree)
propagateReferences()
}

private def addRoot(symbol: Symbol): Unit =
roots += symbol

private def addReference(from: Symbol, to: Symbol): Unit =
methodReferences.getOrElseUpdate(to, mutable.Set.empty) += from

private def propagateReferences(): collection.Set[Symbol] = {
val result = mutable.Set.empty[Symbol]

def rec(symbol: Symbol): Unit = {
// mark `symbol` as referring to `this`
if (result.add(symbol)) {
// `symbol` was not yet in the set; propagate further
methodReferences.getOrElse(symbol, Nil).foreach(rec(_))
}
}

roots.foreach(rec(_))

result
}

private var currentMethod: Symbol = NoSymbol

override def traverse(tree: Tree): Unit = tree match {
case _: DefDef =>
if (isStatifyCandidate(tree.symbol)) {
currentMethod = tree.symbol
super.traverse(tree)
currentMethod = NoSymbol
} else {
// No need to traverse other methods; we always assume they refer to `this`.
}
case Function(_, Apply(target, _)) =>
/* We don't drill into functions because at this phase they will always refer to `this`.
* They will be of the form {(args...) => this.anonfun(args...)}
* but we do need to make note of the lifted body method in case it refers to `this`.
*/
if (currentMethod.exists)
addReference(from = currentMethod, to = target.symbol)
case Apply(sel @ Select(This(_), _), args) if isStatifyCandidate(sel.symbol) =>
if (currentMethod.exists)
addReference(from = currentMethod, to = sel.symbol)
super.traverseTrees(args)
case This(_) =>
// Note: tree.symbol != enclClass is possible if it is a module loaded with genLoadModule
if (currentMethod.exists && tree.symbol == currentMethod.enclClass)
addRoot(currentMethod)
case _ =>
super.traverse(tree)
}
}

// Global class generation state -------------------------------------------

private val lazilyGeneratedAnonClasses = mutable.Map.empty[Symbol, ClassDef]
Expand Down Expand Up @@ -306,6 +433,9 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
*/
override def apply(cunit: CompilationUnit): Unit = {
try {
statifyCandidateMethodsThatReferenceThis =
new ThisReferringMethodsTraverser().methodReferencesThisIn(cunit.body)

def collectClassDefs(tree: Tree): List[ClassDef] = {
tree match {
case EmptyTree => Nil
Expand Down Expand Up @@ -455,6 +585,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
lazilyGeneratedAnonClasses.clear()
generatedStaticForwarderClasses.clear()
generatedClasses.clear()
statifyCandidateMethodsThatReferenceThis = null
pos2irPosCache.clear()
}
}
Expand Down Expand Up @@ -1144,16 +1275,6 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)

assert(moduleClass.isModuleClass, moduleClass)

val hasAnyExistingPublicStaticMethod =
existingMethods.exists(_.flags.namespace == js.MemberNamespace.PublicStatic)
if (hasAnyExistingPublicStaticMethod) {
reporter.error(pos,
"Unexpected situation: found existing public static methods in " +
s"the class ${moduleClass.fullName} while trying to generate " +
"static forwarders for its companion object. " +
"Please report this as a bug in Scala.js.")
}

def listMembersBasedOnFlags = {
// Copy-pasted from BCodeHelpers.
val ExcludedForwarderFlags: Long = {
Expand Down Expand Up @@ -1964,7 +2085,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
} else {
val resultIRType = toIRType(sym.tpe.resultType)
val namespace = {
if (sym.isStaticMember) {
if (compileAsStaticMethod(sym)) {
if (sym.isPrivate) js.MemberNamespace.PrivateStatic
else js.MemberNamespace.PublicStatic
} else {
Expand Down Expand Up @@ -3453,7 +3574,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
genApplyJSClassMethod(genExpr(receiver), sym, genActualArgs(sym, args))
} else if (sym.hasAnnotation(JSNativeAnnotation)) {
genJSNativeMemberCall(tree, isStat)
} else if (sym.isStaticMember) {
} else if (compileAsStaticMethod(sym)) {
if (sym.isMixinConstructor) {
/* Do not emit a call to the $init$ method of JS traits.
* This exception is necessary because optional JS fields cause the
Expand Down Expand Up @@ -6333,7 +6454,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)

val formalArgs = params.map(genParamDef(_))

val (allFormalCaptures, body, allActualCaptures) = if (!target.isStaticMember) {
val (allFormalCaptures, body, allActualCaptures) = if (!compileAsStaticMethod(target)) {
val thisActualCapture = genExpr(receiver)
val thisFormalCapture = js.ParamDef(
freshLocalIdent("this")(receiver.pos), thisOriginalName,
Expand Down
55 changes: 45 additions & 10 deletions linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,13 @@ private final class ClassDefChecker(classDef: ClassDef,
* delegate constructor call.
* - There is no such `ApplyStatically` anywhere else in the body.
* - `cls` must be the enclosing class or its direct superclass.
* - In the statements before the delegate constructor call, and within
* `args`:
* - `This()` cannot be used except in `Assign(Select(This(), _), _)`,
* i.e., to assign to a field (but not read from one).
* - `StoreModule()` cannot be used.
* - After the delegate constructor call, `This` can be used without
* restriction.
*
* After the optimizer, there may be no delegate constructor call at all.
* This frequently happens as the optimizer inlines super constructor
Expand Down Expand Up @@ -559,11 +566,14 @@ private final class ClassDefChecker(classDef: ClassDef,
val (delegateCtorCall: ApplyStatically) :: afterDelegateCtor = rest
val ApplyStatically(_, receiver, cls, MethodIdent(ctor), args) = delegateCtorCall

val envJustBeforeDelegate = checkBlockStats(beforeDelegateCtor, bodyEnv)
val initEnv = bodyEnv.withIsThisRestricted(true)
val envJustBeforeDelegate = checkBlockStats(beforeDelegateCtor, initEnv)

checkApplyArgs(ctor, args, envJustBeforeDelegate)

checkTree(receiver, envJustBeforeDelegate) // check that the This itself is valid
val unrestrictedEnv = envJustBeforeDelegate.withIsThisRestricted(false)

checkTree(receiver, unrestrictedEnv) // check that the This itself is valid

if (!postOptimizer) {
if (!(cls == classDef.className || classDef.superClass.exists(_.name == cls))) {
Expand All @@ -575,7 +585,7 @@ private final class ClassDefChecker(classDef: ClassDef,
}
}

checkBlockStats(afterDelegateCtor, envJustBeforeDelegate)
checkBlockStats(afterDelegateCtor, unrestrictedEnv)
}
}
}
Expand Down Expand Up @@ -632,7 +642,12 @@ private final class ClassDefChecker(classDef: ClassDef,
checkTree(body, env.withLabel(label.name))

case Assign(lhs, rhs) =>
checkTree(lhs, env)
lhs match {
case Select(This(), _) if env.isThisRestricted =>
checkTree(lhs, env.withIsThisRestricted(false))
case _ =>
checkTree(lhs, env)
}
checkTree(rhs, env)

lhs match {
Expand Down Expand Up @@ -708,6 +723,8 @@ private final class ClassDefChecker(classDef: ClassDef,
reportError(i"Illegal StoreModule outside of constructor")
if (env.thisType == NoType) // can happen before JSSuperConstructorCall in JSModuleClass
reportError(i"Cannot find `this` in scope for StoreModule()")
if (env.isThisRestricted)
reportError(i"Restricted use of `this` for StoreModule() before super constructor call")

case Select(qualifier, _) =>
checkTree(qualifier, env)
Expand Down Expand Up @@ -915,6 +932,8 @@ private final class ClassDefChecker(classDef: ClassDef,
reportError(i"Cannot find `this` in scope")
else if (tree.tpe != env.thisType)
reportError(i"`this` of type ${env.thisType} typed as ${tree.tpe}")
if (env.isThisRestricted)
reportError(i"Restricted use of `this` before the super constructor call")

case Closure(arrow, captureParams, params, restParam, body, captureValues) =>
/* Check compliance of captureValues wrt. captureParams in the current
Expand Down Expand Up @@ -1045,7 +1064,9 @@ object ClassDefChecker {
/** Return types by label. */
val returnLabels: Set[LabelName],
/** Whether we are in a constructor of the class. */
val inConstructor: Boolean
val inConstructor: Boolean,
/** Whether usages of `this` are restricted in this scope. */
val isThisRestricted: Boolean
) {
import Env._

Expand All @@ -1064,20 +1085,33 @@ object ClassDefChecker {
def withInConstructor(inConstructor: Boolean): Env =
copy(inConstructor = inConstructor)

def withIsThisRestricted(isThisRestricted: Boolean): Env =
copy(isThisRestricted = isThisRestricted)

private def copy(
hasNewTarget: Boolean = hasNewTarget,
thisType: Type = thisType,
locals: Map[LocalName, LocalDef] = locals,
returnLabels: Set[LabelName] = returnLabels,
inConstructor: Boolean = inConstructor
inConstructor: Boolean = inConstructor,
isThisRestricted: Boolean = isThisRestricted
): Env = {
new Env(hasNewTarget, thisType, locals, returnLabels, inConstructor)
new Env(hasNewTarget, thisType, locals, returnLabels, inConstructor,
isThisRestricted)
}
}

private object Env {
val empty: Env =
new Env(hasNewTarget = false, thisType = NoType, Map.empty, Set.empty, inConstructor = false)
val empty: Env = {
new Env(
hasNewTarget = false,
thisType = NoType,
locals = Map.empty,
returnLabels = Set.empty,
inConstructor = false,
isThisRestricted = false
)
}

def fromParams(params: List[ParamDef]): Env = {
val paramLocalDefs =
Expand All @@ -1089,7 +1123,8 @@ object ClassDefChecker {
thisType = NoType,
paramLocalDefs.toMap,
Set.empty,
inConstructor = false
inConstructor = false,
isThisRestricted = false
)
}
}
Expand Down
Loading