From 7840ce17318a22ba8708de4f55b6df89b1091e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Mon, 21 Oct 2024 11:25:07 +0200 Subject: [PATCH] Only allow `StoreModule` at the top-level of constructor bodies. 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. --- .../linker/checker/ClassDefChecker.scala | 52 +++++++++---------- .../linker/checker/ClassDefCheckerTest.scala | 40 ++++++++++++-- 2 files changed, 60 insertions(+), 32 deletions(-) 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 index f2134214a7..418a18b05c 100644 --- 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 index d48a8f7c45..f1e17cc5f1 100644 --- 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" ) }