Only allow StoreModule at the top-level of constructor bodies.

sjrd · sjrd · commit 7840ce17318a · 2024-10-21T12:16:45.000+02:00
This allows to get rid of `Env.inConstructor` in the
`ClassDefChecker`.

In theory, this is a breaking change. However, we never emitted IR
with `StoreModule`s that would violate the new rule.

In fact, all the IR we emit is even stricter: there is always a
`StoreModule` *immediately after* the super constructor call of
(JS) module class constructors, and never anywhere else.
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
@@ -303,7 +303,6 @@ private final class ClassDefChecker(classDef: ClassDef,
       val thisType = if (static) NoType else instanceThisType
       val bodyEnv = Env.fromParams(params)
         .withThisType(thisType)
-        .withInConstructor(isConstructor)
 
       if (isConstructor)
         checkConstructorBody(body, bodyEnv)
@@ -328,12 +327,13 @@ private final class ClassDefChecker(classDef: ClassDef,
 
     val startEnv = Env.fromParams(classDef.jsClassCaptures.getOrElse(Nil) ++ params ++ restParam)
       .withHasNewTarget(true)
-      .withInConstructor(true)
 
     val envJustBeforeSuper = checkBlockStats(body.beforeSuper, startEnv)
     checkTreeOrSpreads(body.superCall.args, envJustBeforeSuper)
     val envJustAfterSuper = envJustBeforeSuper.withThisType(instanceThisType)
-    checkBlockStats(body.afterSuper, envJustAfterSuper)
+    val afterSuperStoreModulesHandled =
+      handleStoreModulesAfterSuperCtorCall(body.afterSuper)
+    checkBlockStats(afterSuperStoreModulesHandled, envJustAfterSuper)
   }
 
   private def checkJSMethodDef(methodDef: JSMethodDef): Unit = withPerMethodState {
@@ -525,12 +525,10 @@ private final class ClassDefChecker(classDef: ClassDef,
      * - 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.
+     *   `args`, `This()` cannot be used except in `Assign(Select(This(), _), _)`,
+     *   i.e., to assign to a field (but not read from one).
      * - After the delegate constructor call, `This` can be used without
-     *   restriction.
+     *   restriction. Moreover, we can have `StoreModule`s at the top-level.
      *
      * After the optimizer, there may be no delegate constructor call at all.
      * This frequently happens as the optimizer inlines super constructor
@@ -561,7 +559,9 @@ private final class ClassDefChecker(classDef: ClassDef,
         if (!postOptimizer)
           reportError(i"Constructor must contain a delegate constructor call")
 
-        checkBlockStats(bodyStats, bodyEnv)
+        val bodyStatsStoreModulesHandled =
+          handleStoreModulesAfterSuperCtorCall(bodyStats)
+        checkBlockStats(bodyStatsStoreModulesHandled, bodyEnv)
       } else {
         val (delegateCtorCall: ApplyStatically) :: afterDelegateCtor = rest
         val ApplyStatically(_, receiver, cls, MethodIdent(ctor), args) = delegateCtorCall
@@ -585,11 +585,23 @@ private final class ClassDefChecker(classDef: ClassDef,
           }
         }
 
-        checkBlockStats(afterDelegateCtor, unrestrictedEnv)
+        val afterDelegateCtorStoreModulesHandled =
+          handleStoreModulesAfterSuperCtorCall(afterDelegateCtor)
+        checkBlockStats(afterDelegateCtorStoreModulesHandled, unrestrictedEnv)
       }
     }
   }
 
+  private def handleStoreModulesAfterSuperCtorCall(trees: List[Tree]): List[Tree] = {
+    /* If StoreModule's are allowed, there is nothing left to check about them,
+     * so we filter them out.
+     */
+    if (classDef.kind.hasModuleAccessor)
+      trees.filter(!_.isInstanceOf[StoreModule])
+    else
+      trees
+  }
+
   private def checkBlockStats(stats: List[Tree], env: Env): Env = {
     stats.foldLeft(env) { (prevEnv, stat) =>
       checkTree(stat, prevEnv)
@@ -717,14 +729,7 @@ private final class ClassDefChecker(classDef: ClassDef,
       case _: LoadModule =>
 
       case StoreModule() =>
-        if (!classDef.kind.hasModuleAccessor)
-          reportError(i"Illegal StoreModule inside class of kind ${classDef.kind}")
-        if (!env.inConstructor)
-          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")
+        reportError(i"Illegal StoreModule")
 
       case Select(qualifier, _) =>
         checkTree(qualifier, env)
@@ -1063,8 +1068,6 @@ object ClassDefChecker {
       val locals: Map[LocalName, LocalDef],
       /** Return types by label. */
       val returnLabels: Set[LabelName],
-      /** Whether we are in a constructor of the class. */
-      val inConstructor: Boolean,
       /** Whether usages of `this` are restricted in this scope. */
       val isThisRestricted: Boolean
   ) {
@@ -1082,9 +1085,6 @@ object ClassDefChecker {
     def withLabel(label: LabelName): Env =
       copy(returnLabels = returnLabels + label)
 
-    def withInConstructor(inConstructor: Boolean): Env =
-      copy(inConstructor = inConstructor)
-
     def withIsThisRestricted(isThisRestricted: Boolean): Env =
       copy(isThisRestricted = isThisRestricted)
 
@@ -1093,11 +1093,9 @@ object ClassDefChecker {
       thisType: Type = thisType,
       locals: Map[LocalName, LocalDef] = locals,
       returnLabels: Set[LabelName] = returnLabels,
-      inConstructor: Boolean = inConstructor,
       isThisRestricted: Boolean = isThisRestricted
     ): Env = {
-      new Env(hasNewTarget, thisType, locals, returnLabels, inConstructor,
-          isThisRestricted)
+      new Env(hasNewTarget, thisType, locals, returnLabels, isThisRestricted)
     }
   }
 
@@ -1108,7 +1106,6 @@ object ClassDefChecker {
         thisType = NoType,
         locals = Map.empty,
         returnLabels = Set.empty,
-        inConstructor = false,
         isThisRestricted = false
       )
     }
@@ -1123,7 +1120,6 @@ object ClassDefChecker {
         thisType = NoType,
         paramLocalDefs.toMap,
         Set.empty,
-        inConstructor = false,
         isThisRestricted = false
       )
     }
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
@@ -516,7 +516,7 @@ class ClassDefCheckerTest {
           })(EOH, UNV)
         )
       ),
-      "Illegal StoreModule inside class of kind Class"
+      "Illegal StoreModule"
     )
 
     assertError(
@@ -533,7 +533,24 @@ class ClassDefCheckerTest {
           })(EOH, UNV)
         )
       ),
-      "Restricted use of `this` for StoreModule() before super constructor call"
+      "Illegal StoreModule"
+    )
+
+    assertError(
+      classDef(
+        "Foo",
+        kind = ClassKind.ModuleClass,
+        superClass = Some(ObjectClass),
+        methods = List(
+          MethodDef(ctorFlags, NoArgConstructorName, NON, Nil, NoType, Some {
+            Block(
+              superCtorCall,
+              If(BooleanLiteral(true), StoreModule(), Skip())(NoType)
+            )
+          })(EOH, UNV)
+        )
+      ),
+      "Illegal StoreModule"
     )
 
     assertError(
@@ -550,7 +567,7 @@ class ClassDefCheckerTest {
           })(EOH, UNV)
         )
       ),
-      "Illegal StoreModule outside of constructor"
+      "Illegal StoreModule"
     )
 
     assertError(
@@ -564,7 +581,22 @@ class ClassDefCheckerTest {
               EOH, UNV)
         )
       ),
-      "Cannot find `this` in scope for StoreModule()"
+      "Illegal StoreModule"
+    )
+
+    assertError(
+      classDef(
+        "Foo",
+        kind = ClassKind.JSModuleClass,
+        superClass = Some("scala.scalajs.js.Object"),
+        jsConstructor = Some(
+          JSConstructorDef(JSCtorFlags, Nil, None,
+              JSConstructorBody(Nil, JSSuperConstructorCall(Nil),
+                  If(BooleanLiteral(true), StoreModule(), Skip())(NoType) :: Undefined() :: Nil))(
+              EOH, UNV)
+        )
+      ),
+      "Illegal StoreModule"
     )
   }
 
