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

sjrd · sjrd · commit 407962a3fa8c · 2024-08-23T17:16:16.000+02:00
Essentially, `this` cannot be used before the super constructor
call, except to assign fields (but not reading).

These restrictions match those enforced by the JVM for bytecode.

While we never intended `this` values to be restricted in this way,
in practice we never produced IR that would violate those
restrictions.

In addition, we also enforce that we cannot perform calls to
constructors, except exactly once within another constructor, and
only with `this` as receiver. This also matches JVM restrictions.

In the future, we may leverage those restrictions to validate
strict field initialization. If a field *needs* to be initialized
before it is ever read (for example, because it has a non-nullable
reference type), then we can do so: we only have to check that it
is definitely initialized before the super constructor call. The
restrictions implemented in this commit ensure that nobody can
read the field before the super constructor call starts.
diff --git a/linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala b/linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala
@@ -231,7 +231,7 @@ private final class ClassDefChecker(classDef: ClassDef,
   }
 
   private def checkMethodDef(methodDef: MethodDef): Unit = withPerMethodState {
-    val MethodDef(flags, MethodIdent(name), _, params, _, body) = methodDef
+    val MethodDef(flags, MethodIdent(name), _, params, _, optBody) = methodDef
     implicit val ctx = ErrorContext(methodDef)
 
     val namespace = flags.namespace
@@ -246,7 +246,7 @@ private final class ClassDefChecker(classDef: ClassDef,
     if (!methods(namespace.ordinal).add(name))
       reportError(i"duplicate ${namespace.prefixString}method '$name'")
 
-    if (body.isEmpty && namespace != MemberNamespace.Public)
+    if (optBody.isEmpty && namespace != MemberNamespace.Public)
       reportError("Abstract methods may only be in the public namespace")
 
     // ClassInitializer
@@ -294,11 +294,20 @@ private final class ClassDefChecker(classDef: ClassDef,
     }
 
     // Body
-    val thisType = if (static) NoType else instanceThisType
-    val bodyEnv = Env.fromParams(params)
-      .withThisType(thisType)
-      .withInConstructor(isConstructor)
-    body.foreach(checkTree(_, bodyEnv))
+    for (body <- optBody) {
+      val thisType = if (static) NoType else instanceThisType
+      val bodyEnv = Env.fromParams(params)
+        .withThisType(thisType)
+        .withInConstructor(isConstructor)
+
+      if (isConstructor && !postOptimizer) {
+        // Strict checking of `this` values and super/delegate constructor calls
+        checkConstructorBodyStrict(body, bodyEnv)
+      } else {
+        // Regular checking of `this` values
+        checkTree(body, bodyEnv)
+      }
+    }
   }
 
   private def checkJSConstructorDef(ctorDef: JSConstructorDef): Unit = withPerMethodState {
@@ -319,14 +328,10 @@ private final class ClassDefChecker(classDef: ClassDef,
       .withHasNewTarget(true)
       .withInConstructor(true)
 
-    val envJustBeforeSuper = body.beforeSuper.foldLeft(startEnv) { (prevEnv, stat) =>
-      checkTree(stat, prevEnv)
-    }
+    val envJustBeforeSuper = checkBlockStats(body.beforeSuper, startEnv)
     checkTreeOrSpreads(body.superCall.args, envJustBeforeSuper)
     val envJustAfterSuper = envJustBeforeSuper.withThisType(instanceThisType)
-    body.afterSuper.foldLeft(envJustAfterSuper) { (prevEnv, stat) =>
-      checkTree(stat, prevEnv)
-    }
+    checkBlockStats(body.afterSuper, envJustAfterSuper)
   }
 
   private def checkJSMethodDef(methodDef: JSMethodDef): Unit = withPerMethodState {
@@ -505,6 +510,81 @@ private final class ClassDefChecker(classDef: ClassDef,
     }
   }
 
+  private def checkConstructorBodyStrict(body: Tree, bodyEnv: Env): Unit = {
+    /* In a strictly checked constructor body, usages of the `this` value are
+     * restricted according to the following rules.
+     *
+     * If the enclosing class is `jl.Object`, the full `body` is unrestricted.
+     * Otherwise:
+     *
+     * - The body must be a possible Block with statements `stats`.
+     * - There exists a unique `stat` in the `stats` list that is an
+     *   `ApplyStatically(_, This(), cls, someConstructor, args)`, called the
+     *   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.
+     */
+
+    implicit val ctx = ErrorContext(body)
+
+    val bodyStats = body match {
+      case Block(stats) => stats
+      case Skip()       => Nil
+      case _            => body :: Nil
+    }
+
+    if (isJLObject) {
+      checkBlockStats(bodyStats, bodyEnv)
+    } else {
+      val (beforeDelegateCtor, rest) = bodyStats.span {
+        case ApplyStatically(_, This(), _, MethodIdent(ctor), _) =>
+          !ctor.isConstructor
+        case _ =>
+          true
+      }
+
+      if (rest.isEmpty) {
+        reportError(i"Constructor must contain a delegate constructor call")
+        checkBlockStats(beforeDelegateCtor, bodyEnv) // to report further errors
+      } else {
+        val (delegateCtorCall: ApplyStatically) :: afterDelegateCtor = rest
+        val ApplyStatically(_, receiver, cls, MethodIdent(ctor), args) = delegateCtorCall
+
+        val initEnv = bodyEnv.withIsThisRestricted(true)
+        val envJustBeforeSuper = checkBlockStats(beforeDelegateCtor, initEnv)
+
+        checkApplyArgs(ctor, args, envJustBeforeSuper)
+
+        val unrestrictedEnv = envJustBeforeSuper.withIsThisRestricted(false)
+
+        checkTree(receiver, unrestrictedEnv) // check that the This itself is valid
+
+        if (!(cls == classDef.className || classDef.superClass.exists(_.name == cls))) {
+          implicit val ctx = ErrorContext(delegateCtorCall)
+          reportError(
+              i"Invalid target class $cls for delegate constructor call; " +
+              i"expected ${classDef.className}" +
+              classDef.superClass.fold("")(s => i" or ${s.name}"))
+        }
+
+        checkBlockStats(afterDelegateCtor, unrestrictedEnv)
+      }
+    }
+  }
+
+  private def checkBlockStats(stats: List[Tree], env: Env): Env = {
+    stats.foldLeft(env) { (prevEnv, stat) =>
+      checkTree(stat, prevEnv)
+    }
+  }
+
   private def checkTreeOrSpreads(trees: List[TreeOrJSSpread], env: Env): Unit = {
     trees.foreach {
       case JSSpread(items) => checkTree(items, env)
@@ -515,15 +595,19 @@ private final class ClassDefChecker(classDef: ClassDef,
   private def checkTrees(trees: List[Tree], env: Env): Unit =
     trees.foreach(checkTree(_, env))
 
+  private def checkApplyArgs(methodName: MethodName, args: List[Tree], env: Env)(
+      implicit ctx: ErrorContext): Unit = {
+    val paramRefsCount = methodName.paramTypeRefs.size
+    if (args.size != paramRefsCount)
+      reportError(i"Arity mismatch: $paramRefsCount expected but ${args.size} found")
+    checkTrees(args, env)
+  }
+
   private def checkTree(tree: Tree, env: Env): Env = {
     implicit val ctx = ErrorContext(tree)
 
-    def checkApplyGeneric(methodName: MethodName, args: List[Tree]): Unit = {
-      val paramRefsCount = methodName.paramTypeRefs.size
-      if (args.size != paramRefsCount)
-        reportError(i"Arity mismatch: $paramRefsCount expected but ${args.size} found")
-      checkTrees(args, env)
-    }
+    def checkApplyGeneric(methodName: MethodName, args: List[Tree]): Unit =
+      checkApplyArgs(methodName, args, env)
 
     val newEnv = tree match {
       case VarDef(ident, _, vtpe, mutable, _) =>
@@ -540,16 +624,19 @@ private final class ClassDefChecker(classDef: ClassDef,
       case Skip() =>
 
       case Block(stats) =>
-        stats.foldLeft(env) { (prevEnv, stat) =>
-          checkTree(stat, prevEnv)
-        }
+        checkBlockStats(stats, env)
 
       case Labeled(label, _, body) =>
         checkDeclareLabel(label)
         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 {
@@ -618,10 +705,12 @@ private final class ClassDefChecker(classDef: ClassDef,
       case StoreModule() =>
         if (!classDef.kind.hasModuleAccessor)
           reportError(i"Illegal StoreModule inside class of kind ${classDef.kind}")
-        if (!env.inConstructor)
+        else if (!env.inConstructor)
           reportError(i"Illegal StoreModule outside of constructor")
-        if (env.thisType == NoType) // can happen before JSSuperConstructorCall in JSModuleClass
+        else if (env.thisType == NoType) // can happen before JSSuperConstructorCall in JSModuleClass
           reportError(i"Cannot find `this` in scope for StoreModule()")
+        else if (env.isThisRestricted)
+          reportError(i"Restricted use of `this` for StoreModule() before super constructor call")
 
       case Select(qualifier, _) =>
         checkTree(qualifier, env)
@@ -637,6 +726,8 @@ private final class ClassDefChecker(classDef: ClassDef,
         checkApplyGeneric(method, args)
 
       case ApplyStatically(_, receiver, _, MethodIdent(method), args) =>
+        if (!postOptimizer && method.isConstructor)
+          reportError(i"Illegal constructor call")
         checkTree(receiver, env)
         checkApplyGeneric(method, args)
 
@@ -816,6 +907,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}")
+        else 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
@@ -960,7 +1053,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._
 
@@ -979,20 +1074,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 =
@@ -1004,7 +1112,8 @@ object ClassDefChecker {
         thisType = NoType,
         paramLocalDefs.toMap,
         Set.empty,
-        inConstructor = false
+        inConstructor = false,
+        isThisRestricted = false
       )
     }
   }
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/AnalyzerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/AnalyzerTest.scala
@@ -192,7 +192,7 @@ class AnalyzerTest {
         val classDefs = Seq(
             classDef("A", kind = kindSub,
                 superClass = Some("B"),
-                methods = requiredMethods("A", kindSub),
+                methods = requiredMethods("A", kindSub, "B"),
                 jsConstructor = requiredJSConstructor(kindSub)),
             classDef("B", kind = kindBase,
                 superClass = validParentForKind(kindBase),
@@ -321,7 +321,7 @@ class AnalyzerTest {
                 )))(EOH, UNV)
             )),
         classDef("B", superClass = Some("A"),
-            methods = List(trivialCtor("B")))
+            methods = List(trivialCtor("B", "A")))
     )
 
     val analysis = computeAnalysis(classDefs,
@@ -350,7 +350,7 @@ class AnalyzerTest {
             methods = List(trivialCtor("A"))),
         classDef("B", superClass = Some("A"),
             methods = List(
-                trivialCtor("B"),
+                trivialCtor("B", "A"),
                 MethodDef(EMF, fooMethodName, NON, Nil, IntType, Some(int(5)))(EOH, UNV)
             ))
     )
@@ -771,12 +771,12 @@ class AnalyzerTest {
             )),
         classDef("B", superClass = Some("A"), interfaces = List("I2"),
             methods = List(
-                trivialCtor("B"),
+                trivialCtor("B", "A"),
                 MethodDef(EMF, fooMethodName, NON, Nil, IntType, Some(int(5)))(EOH, UNV)
             )),
         classDef("C", superClass = Some("B"),
             methods = List(
-                trivialCtor("C"),
+                trivialCtor("C", "B"),
                 MethodDef(EMF, barMethodName, NON, Nil, IntType, Some(int(5)))(EOH, UNV)
             ))
     )
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/BaseLinkerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/BaseLinkerTest.scala
@@ -55,7 +55,7 @@ class BaseLinkerTest {
           "Sub",
           kind = ClassKind.Class,
           superClass = Some("Base"),
-          methods = List(trivialCtor("Sub"))
+          methods = List(trivialCtor("Sub", "Base"))
       ),
       mainTestClassDef(
         consoleLog(Apply(EAF, New("Sub", NoArgConstructorName, Nil), fooName, Nil)(IntType))
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/IRCheckerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/IRCheckerTest.scala
@@ -132,7 +132,7 @@ class IRCheckerTest {
           superClass = Some("C"),
           interfaces = List("B"),
           methods = List(
-            trivialCtor("D"),
+            trivialCtor("D", "C"),
             MethodDef(
               EMF.withNamespace(MemberNamespace.PublicStatic),
               testMethodName,
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/OptimizerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/OptimizerTest.scala
@@ -500,10 +500,12 @@ class OptimizerTest {
             // def this() = {
             //   this.x = null
             //   this.y = 5
+            //   this.jl.Object::<init>()
             // }
             MethodDef(EMF.withNamespace(Constructor), NoArgConstructorName, NON, Nil, NoType, Some(Block(
               Assign(Select(This()(ClassType("Foo")), FieldName("Foo", "x"))(witnessType), Null()),
-              Assign(Select(This()(ClassType("Foo")), FieldName("Foo", "y"))(IntType), int(5))
+              Assign(Select(This()(ClassType("Foo")), FieldName("Foo", "y"))(IntType), int(5)),
+              trivialSuperCtorCall("Foo")
             )))(EOH, UNV),
 
             // def method(): Int = this.y
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/checker/ClassDefCheckerTest.scala b/linker/shared/src/test/scala/org/scalajs/linker/checker/ClassDefCheckerTest.scala
diff --git a/linker/shared/src/test/scala/org/scalajs/linker/testutils/TestIRBuilder.scala b/linker/shared/src/test/scala/org/scalajs/linker/testutils/TestIRBuilder.scala
