Refactoring: Generic notion of CheckingPhase for the IR checkers.

We previously had two boolean options for deciding what IR is allowed or not between phases. Of the four possible combinations of values, one was actually invalid. With the introduction of the desugarer, there would have been *three* such boolean options. Only 4 out of their 8 combinations would have been valid. We now introduce a generic concept of `CheckingPhase`. Some IR features are allowed as output of each `CheckingPhase`. Since the `ClassDefChecker` and `IRChecker` must agree on what features are valid when, we factor that logic into `FeatureSet`. It represents a set of logical features, and can compute which set is valid for each phase.
scala-js · sjrd · Feb 3, 2025 · Jan 22, 2025 · Feb 2, 2025 · 2a77c9cea28dfa4086802f230035366e55c20fdd
commit 2a77c9cea28dfa4086802f230035366e55c20fdd
diff --git a/linker/shared/src/main/scala/org/scalajs/linker/analyzer/Analyzer.scala b/linker/shared/src/main/scala/org/scalajs/linker/analyzer/Analyzer.scala
@@ -29,6 +29,7 @@ import org.scalajs.ir.Trees.{MemberNamespace, JSNativeLoadSpec}
 import org.scalajs.ir.Types.ClassRef
 
 import org.scalajs.linker._
+import org.scalajs.linker.checker.CheckingPhase
 import org.scalajs.linker.frontend.IRLoader
 import org.scalajs.linker.interface._
 import org.scalajs.linker.interface.unstable.ModuleInitializerImpl
@@ -43,15 +44,10 @@ import Analysis._
 import Infos.{NamespacedMethodName, ReachabilityInfo, ReachabilityInfoInClass}
 
 final class Analyzer(config: CommonPhaseConfig, initial: Boolean,
-    checkIR: Boolean, failOnError: Boolean, irLoader: IRLoader) {
-
-  private val infoLoader: InfoLoader = {
-    new InfoLoader(irLoader,
-        if (!checkIR) InfoLoader.NoIRCheck
-        else if (initial) InfoLoader.InitialIRCheck
-        else InfoLoader.InternalIRCheck
-    )
-  }
+    checkIRFor: Option[CheckingPhase], failOnError: Boolean, irLoader: IRLoader) {
+
+  private val infoLoader: InfoLoader =
+    new InfoLoader(irLoader, checkIRFor)
 
   def computeReachability(moduleInitializers: Seq[ModuleInitializer],
       symbolRequirements: SymbolRequirement, logger: Logger)(implicit ec: ExecutionContext): Future[Analysis] = {

diff --git a/linker/shared/src/main/scala/org/scalajs/linker/analyzer/InfoLoader.scala b/linker/shared/src/main/scala/org/scalajs/linker/analyzer/InfoLoader.scala
@@ -22,14 +22,14 @@ import org.scalajs.ir.Trees._
 
 import org.scalajs.logging._
 
-import org.scalajs.linker.checker.ClassDefChecker
+import org.scalajs.linker.checker._
 import org.scalajs.linker.frontend.IRLoader
 import org.scalajs.linker.interface.LinkingException
 import org.scalajs.linker.CollectionsCompat.MutableMapCompatOps
 
 import Platform.emptyThreadSafeMap
 
-private[analyzer] final class InfoLoader(irLoader: IRLoader, irCheckMode: InfoLoader.IRCheckMode) {
+private[analyzer] final class InfoLoader(irLoader: IRLoader, checkIRFor: Option[CheckingPhase]) {
   private var logger: Logger = _
   private val cache = emptyThreadSafeMap[ClassName, InfoLoader.ClassInfoCache]
 
@@ -44,7 +44,7 @@ private[analyzer] final class InfoLoader(irLoader: IRLoader, irCheckMode: InfoLo
       implicit ec: ExecutionContext): Option[Future[Infos.ClassInfo]] = {
     if (irLoader.classExists(className)) {
       val infoCache = cache.getOrElseUpdate(className,
-          new InfoLoader.ClassInfoCache(className, irLoader, irCheckMode))
+          new InfoLoader.ClassInfoCache(className, irLoader, checkIRFor))
       Some(infoCache.loadInfo(logger))
     } else {
       None
@@ -58,15 +58,9 @@ private[analyzer] final class InfoLoader(irLoader: IRLoader, irCheckMode: InfoLo
 }
 
 private[analyzer] object InfoLoader {
-  sealed trait IRCheckMode
-
-  case object NoIRCheck extends IRCheckMode
-  case object InitialIRCheck extends IRCheckMode
-  case object InternalIRCheck extends IRCheckMode
-
   private type MethodInfos = Array[Map[MethodName, Infos.MethodInfo]]
 
-  private class ClassInfoCache(className: ClassName, irLoader: IRLoader, irCheckMode: InfoLoader.IRCheckMode) {
+  private class ClassInfoCache(className: ClassName, irLoader: IRLoader, checkIRFor: Option[CheckingPhase]) {
     private var cacheUsed: Boolean = false
     private var version: Version = Version.Unversioned
     private var info: Future[Infos.ClassInfo] = _
@@ -86,26 +80,18 @@ private[analyzer] object InfoLoader {
         if (!version.sameVersion(newVersion)) {
           version = newVersion
           info = irLoader.loadClassDef(className).map { tree =>
-            irCheckMode match {
-              case InfoLoader.NoIRCheck =>
-                // no check
-
-              case InfoLoader.InitialIRCheck =>
-                val errorCount = ClassDefChecker.check(tree,
-                    postBaseLinker = false, postOptimizer = false, logger)
-                if (errorCount != 0) {
+            for (previousPhase <- checkIRFor) {
+              val errorCount = ClassDefChecker.check(tree, previousPhase, logger)
+              if (errorCount != 0) {
+                if (previousPhase == CheckingPhase.Compiler) {
                   throw new LinkingException(
                       s"There were $errorCount ClassDef checking errors.")
-                }
-
-              case InfoLoader.InternalIRCheck =>
-                val errorCount = ClassDefChecker.check(tree,
-                    postBaseLinker = true, postOptimizer = true, logger)
-                if (errorCount != 0) {
+                } else {
                   throw new LinkingException(
-                      s"There were $errorCount ClassDef checking errors after optimizing. " +
+                      s"There were $errorCount ClassDef checking errors after transformations. " +
                       "Please report this as a bug.")
                 }
+              }
             }
 
             generateInfos(tree)

diff --git a/linker/shared/src/main/scala/org/scalajs/linker/checker/CheckingPhase.scala b/linker/shared/src/main/scala/org/scalajs/linker/checker/CheckingPhase.scala
@@ -0,0 +1,28 @@
+/*
+ * Scala.js (https://www.scala-js.org/)
+ *
+ * Copyright EPFL.
+ *
+ * Licensed under Apache License 2.0
+ * (https://www.apache.org/licenses/LICENSE-2.0).
+ *
+ * See the NOTICE file distributed with this work for
+ * additional information regarding copyright ownership.
+ */
+
+package org.scalajs.linker.checker
+
+/** A phase *after which* we are checking IR.
+ *
+ *  When checking IR (with `ClassDefChecker` or `IRChecker`), different nodes
+ *  and transients are allowed between different phases. The `CheckingPhase`
+ *  records the *previous* phase to run before the check. We are therefore
+ *  checking that the IR is a valid *output* of the target phase.
+ */
+sealed abstract class CheckingPhase
+
+object CheckingPhase {
+  case object Compiler extends CheckingPhase
+  case object BaseLinker extends CheckingPhase
+  case object Optimizer extends CheckingPhase
+}
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
@@ -28,11 +28,13 @@ import org.scalajs.linker.standard.LinkedClass
 
 /** Checker for the validity of the IR. */
 private final class ClassDefChecker(classDef: ClassDef,
-    postBaseLinker: Boolean, postOptimizer: Boolean, reporter: ErrorReporter) {
+    previousPhase: CheckingPhase, reporter: ErrorReporter) {
   import ClassDefChecker._
 
   import reporter.reportError
 
+  private val featureSet = FeatureSet.allowedAfter(previousPhase)
+
   private[this] val isJLObject = classDef.name.name == ObjectClass
 
   private[this] val instanceThisType: Type = {
@@ -107,7 +109,7 @@ private final class ClassDefChecker(classDef: ClassDef,
      * module classes and JS classes that are never instantiated. The classes
      * may still exist because their class data are accessed.
      */
-    if (!postBaseLinker) {
+    if (!featureSet.supports(FeatureSet.OptionalConstructors)) {
       /* Check that we have exactly 1 constructor in a module class. This goes
        * together with `checkMethodDef`, which checks that a constructor in a
        * module class must be 0-arg.
@@ -489,7 +491,7 @@ private final class ClassDefChecker(classDef: ClassDef,
   private def checkMethodNameNamespace(name: MethodName, namespace: MemberNamespace)(
       implicit ctx: ErrorContext): Unit = {
     if (name.isReflectiveProxy) {
-      if (postBaseLinker) {
+      if (featureSet.supports(FeatureSet.ReflectiveProxies)) {
         if (namespace != MemberNamespace.Public)
           reportError("reflective profixes are only allowed in the public namespace")
       } else {
@@ -557,7 +559,7 @@ private final class ClassDefChecker(classDef: ClassDef,
       }
 
       if (rest.isEmpty) {
-        if (!postOptimizer)
+        if (!featureSet.supports(FeatureSet.RelaxedCtorBodies))
           reportError(i"Constructor must contain a delegate constructor call")
 
         val bodyStatsStoreModulesHandled =
@@ -576,7 +578,7 @@ private final class ClassDefChecker(classDef: ClassDef,
 
         checkTree(receiver, unrestrictedEnv) // check that the This itself is valid
 
-        if (!postOptimizer) {
+        if (!featureSet.supports(FeatureSet.RelaxedCtorBodies)) {
           if (!(cls == classDef.className || classDef.superClass.exists(_.name == cls))) {
             implicit val ctx = ErrorContext(delegateCtorCall)
             reportError(
@@ -597,7 +599,7 @@ private final class ClassDefChecker(classDef: ClassDef,
       implicit ctx: ErrorContext): List[Tree] = {
 
     if (classDef.kind.hasModuleAccessor) {
-      if (postOptimizer) {
+      if (featureSet.supports(FeatureSet.RelaxedCtorBodies)) {
         /* If the super constructor call was inlined, the StoreModule can be anywhere.
          * Moreover, the optimizer can remove StoreModules altogether in many cases.
          */
@@ -673,7 +675,7 @@ private final class ClassDefChecker(classDef: ClassDef,
       case Assign(lhs, rhs) =>
         lhs match {
           case Select(This(), field) if env.isThisRestricted =>
-            if (postOptimizer || field.name.className == classDef.className)
+            if (featureSet.supports(FeatureSet.RelaxedCtorBodies) || field.name.className == classDef.className)
               checkTree(lhs, env.withIsThisRestricted(false))
             else
               checkTree(lhs, env)
@@ -819,11 +821,13 @@ private final class ClassDefChecker(classDef: ClassDef,
         checkTree(index, env)
 
       case RecordSelect(record, _) =>
-        checkAllowTransients()
+        if (!featureSet.supports(FeatureSet.Records))
+          reportError("invalid use of records")
         checkTree(record, env)
 
       case RecordValue(_, elems) =>
-        checkAllowTransients()
+        if (!featureSet.supports(FeatureSet.Records))
+          reportError("invalid use of records")
         checkTrees(elems, env)
 
       case IsInstanceOf(expr, testType) =>
@@ -987,7 +991,9 @@ private final class ClassDefChecker(classDef: ClassDef,
         checkTrees(captureValues, env)
 
       case Transient(transient) =>
-        checkAllowTransients()
+        if (!featureSet.supports(FeatureSet.OptimizedTransients))
+          reportError(i"invalid transient tree of class ${transient.getClass().getName()}")
+
         transient.traverse(new Traversers.Traverser {
           override def traverse(tree: Tree): Unit = checkTree(tree, env)
         })
@@ -996,11 +1002,6 @@ private final class ClassDefChecker(classDef: ClassDef,
     newEnv
   }
 
-  private def checkAllowTransients()(implicit ctx: ErrorContext): Unit = {
-    if (!postOptimizer)
-      reportError("invalid transient tree")
-  }
-
   private def checkArrayType(tpe: ArrayType)(
       implicit ctx: ErrorContext): Unit = {
     checkArrayTypeRef(tpe.arrayTypeRef)
@@ -1036,13 +1037,13 @@ object ClassDefChecker {
    *
    *  @return Count of IR checking errors (0 in case of success)
    */
-  def check(classDef: ClassDef, postBaseLinker: Boolean, postOptimizer: Boolean, logger: Logger): Int = {
+  def check(classDef: ClassDef, previousPhase: CheckingPhase, logger: Logger): Int = {
     val reporter = new LoggerErrorReporter(logger)
-    new ClassDefChecker(classDef, postBaseLinker, postOptimizer, reporter).checkClassDef()
+    new ClassDefChecker(classDef, previousPhase, reporter).checkClassDef()
     reporter.errorCount
   }
 
-  def check(linkedClass: LinkedClass, postOptimizer: Boolean, logger: Logger): Int = {
+  def check(linkedClass: LinkedClass, previousPhase: CheckingPhase, logger: Logger): Int = {
     // Rebuild a ClassDef out of the LinkedClass
     import linkedClass._
     implicit val pos = linkedClass.pos
@@ -1063,7 +1064,7 @@ object ClassDefChecker {
       topLevelExportDefs = Nil
     )(optimizerHints)
 
-    check(classDef, postBaseLinker = true, postOptimizer, logger)
+    check(classDef, previousPhase, logger)
   }
 
   private class Env(

diff --git a/linker/shared/src/main/scala/org/scalajs/linker/checker/FeatureSet.scala b/linker/shared/src/main/scala/org/scalajs/linker/checker/FeatureSet.scala
@@ -0,0 +1,76 @@
+/*
+ * Scala.js (https://www.scala-js.org/)
+ *
+ * Copyright EPFL.
+ *
+ * Licensed under Apache License 2.0
+ * (https://www.apache.org/licenses/LICENSE-2.0).
+ *
+ * See the NOTICE file distributed with this work for
+ * additional information regarding copyright ownership.
+ */
+
+package org.scalajs.linker.checker
+
+import org.scalajs.linker.checker.CheckingPhase._
+
+/** A set of conditional IR features that the checkers can accept.
+ *
+ *  At any given phase, the `ClassDefChecker` and the `IRChecker` must agree on
+ *  the set of IR features that are valid. A `FeatureSet` factors out the
+ *  knowledge of what feature is acceptable when.
+ */
+private[checker] final class FeatureSet private (private val flags: Int) extends AnyVal {
+  /** Does this feature set support (all of) the given feature set. */
+  def supports(features: FeatureSet): Boolean =
+    (features.flags & flags) == features.flags
+
+  /** The union of this feature set and `that` feature set. */
+  def |(that: FeatureSet): FeatureSet =
+    new FeatureSet(this.flags | that.flags)
+}
+
+private[checker] object FeatureSet {
+  /** Empty feature set. */
+  val Empty = new FeatureSet(0)
+
+  // Individual features
+
+  /** Optional constructors in module classes and JS classes. */
+  val OptionalConstructors = new FeatureSet(1 << 0)
+
+  /** Explicit reflective proxy definitions. */
+  val ReflectiveProxies = new FeatureSet(1 << 1)
+
+  /** Transients that are the result of optimizations. */
+  val OptimizedTransients = new FeatureSet(1 << 2)
+
+  /** Records and record types. */
+  val Records = new FeatureSet(1 << 3)
+
+  /** Relaxed constructor discipline.
+   *
+   *  - Optional super/delegate constructor call.
+   *  - Delegate constructor calls can target any super class.
+   *  - `this.x = ...` assignments before the delegate call can assign super class fields.
+   *  - `StoreModule` can be anywhere, or not be there at all.
+   */
+  val RelaxedCtorBodies = new FeatureSet(1 << 4)
+
+  // Common sets
+
+  /** Features introduced by the base linker. */
+  private val Linked =
+    OptionalConstructors | ReflectiveProxies
+
+  /** IR that is only the result of optimizations. */
+  private val Optimized =
+    OptimizedTransients | Records | RelaxedCtorBodies
+
+  /** The set of features allowed as output of the given phase. */
+  def allowedAfter(phase: CheckingPhase): FeatureSet = phase match {
+    case Compiler   => Empty
+    case BaseLinker => Linked
+    case Optimizer  => Linked | Optimized
+  }
+}