Skip to content

Commit 2a77c9c

Browse files
sjrdsjrd
committed
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.
1 parent fb051e5 commit 2a77c9c

File tree

11 files changed

+173
-74
lines changed

11 files changed

+173
-74
lines changed

‎linker/shared/src/main/scala/org/scalajs/linker/analyzer/Analyzer.scala

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import org.scalajs.ir.Trees.{MemberNamespace, JSNativeLoadSpec}
2929
import org.scalajs.ir.Types.ClassRef
3030

3131
import org.scalajs.linker._
32+
import org.scalajs.linker.checker.CheckingPhase
3233
import org.scalajs.linker.frontend.IRLoader
3334
import org.scalajs.linker.interface._
3435
import org.scalajs.linker.interface.unstable.ModuleInitializerImpl
@@ -43,15 +44,10 @@ import Analysis._
4344
import Infos.{NamespacedMethodName, ReachabilityInfo, ReachabilityInfoInClass}
4445

4546
final class Analyzer(config: CommonPhaseConfig, initial: Boolean,
46-
checkIR: Boolean, failOnError: Boolean, irLoader: IRLoader) {
47-
48-
private val infoLoader: InfoLoader = {
49-
new InfoLoader(irLoader,
50-
if (!checkIR) InfoLoader.NoIRCheck
51-
else if (initial) InfoLoader.InitialIRCheck
52-
else InfoLoader.InternalIRCheck
53-
)
54-
}
47+
checkIRFor: Option[CheckingPhase], failOnError: Boolean, irLoader: IRLoader) {
48+
49+
private val infoLoader: InfoLoader =
50+
new InfoLoader(irLoader, checkIRFor)
5551

5652
def computeReachability(moduleInitializers: Seq[ModuleInitializer],
5753
symbolRequirements: SymbolRequirement, logger: Logger)(implicit ec: ExecutionContext): Future[Analysis] = {

‎linker/shared/src/main/scala/org/scalajs/linker/analyzer/InfoLoader.scala

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,14 @@ import org.scalajs.ir.Trees._
2222

2323
import org.scalajs.logging._
2424

25-
import org.scalajs.linker.checker.ClassDefChecker
25+
import org.scalajs.linker.checker._
2626
import org.scalajs.linker.frontend.IRLoader
2727
import org.scalajs.linker.interface.LinkingException
2828
import org.scalajs.linker.CollectionsCompat.MutableMapCompatOps
2929

3030
import Platform.emptyThreadSafeMap
3131

32-
private[analyzer] final class InfoLoader(irLoader: IRLoader, irCheckMode: InfoLoader.IRCheckMode) {
32+
private[analyzer] final class InfoLoader(irLoader: IRLoader, checkIRFor: Option[CheckingPhase]) {
3333
private var logger: Logger = _
3434
private val cache = emptyThreadSafeMap[ClassName, InfoLoader.ClassInfoCache]
3535

@@ -44,7 +44,7 @@ private[analyzer] final class InfoLoader(irLoader: IRLoader, irCheckMode: InfoLo
4444
implicit ec: ExecutionContext): Option[Future[Infos.ClassInfo]] = {
4545
if (irLoader.classExists(className)) {
4646
val infoCache = cache.getOrElseUpdate(className,
47-
new InfoLoader.ClassInfoCache(className, irLoader, irCheckMode))
47+
new InfoLoader.ClassInfoCache(className, irLoader, checkIRFor))
4848
Some(infoCache.loadInfo(logger))
4949
} else {
5050
None
@@ -58,15 +58,9 @@ private[analyzer] final class InfoLoader(irLoader: IRLoader, irCheckMode: InfoLo
5858
}
5959

6060
private[analyzer] object InfoLoader {
61-
sealed trait IRCheckMode
62-
63-
case object NoIRCheck extends IRCheckMode
64-
case object InitialIRCheck extends IRCheckMode
65-
case object InternalIRCheck extends IRCheckMode
66-
6761
private type MethodInfos = Array[Map[MethodName, Infos.MethodInfo]]
6862

69-
private class ClassInfoCache(className: ClassName, irLoader: IRLoader, irCheckMode: InfoLoader.IRCheckMode) {
63+
private class ClassInfoCache(className: ClassName, irLoader: IRLoader, checkIRFor: Option[CheckingPhase]) {
7064
private var cacheUsed: Boolean = false
7165
private var version: Version = Version.Unversioned
7266
private var info: Future[Infos.ClassInfo] = _
@@ -86,26 +80,18 @@ private[analyzer] object InfoLoader {
8680
if (!version.sameVersion(newVersion)) {
8781
version = newVersion
8882
info = irLoader.loadClassDef(className).map { tree =>
89-
irCheckMode match {
90-
case InfoLoader.NoIRCheck =>
91-
// no check
92-
93-
case InfoLoader.InitialIRCheck =>
94-
val errorCount = ClassDefChecker.check(tree,
95-
postBaseLinker = false, postOptimizer = false, logger)
96-
if (errorCount != 0) {
83+
for (previousPhase <- checkIRFor) {
84+
val errorCount = ClassDefChecker.check(tree, previousPhase, logger)
85+
if (errorCount != 0) {
86+
if (previousPhase == CheckingPhase.Compiler) {
9787
throw new LinkingException(
9888
s"There were $errorCount ClassDef checking errors.")
99-
}
100-
101-
case InfoLoader.InternalIRCheck =>
102-
val errorCount = ClassDefChecker.check(tree,
103-
postBaseLinker = true, postOptimizer = true, logger)
104-
if (errorCount != 0) {
89+
} else {
10590
throw new LinkingException(
106-
s"There were $errorCount ClassDef checking errors after optimizing. " +
91+
s"There were $errorCount ClassDef checking errors after transformations. " +
10792
"Please report this as a bug.")
10893
}
94+
}
10995
}
11096

11197
generateInfos(tree)

‎linker/shared/src/main/scala/org/scalajs/linker/checker/CheckingPhase.scala

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/*
2+
* Scala.js (https://www.scala-js.org/)
3+
*
4+
* Copyright EPFL.
5+
*
6+
* Licensed under Apache License 2.0
7+
* (https://www.apache.org/licenses/LICENSE-2.0).
8+
*
9+
* See the NOTICE file distributed with this work for
10+
* additional information regarding copyright ownership.
11+
*/
12+
13+
package org.scalajs.linker.checker
14+
15+
/** A phase *after which* we are checking IR.
16+
*
17+
* When checking IR (with `ClassDefChecker` or `IRChecker`), different nodes
18+
* and transients are allowed between different phases. The `CheckingPhase`
19+
* records the *previous* phase to run before the check. We are therefore
20+
* checking that the IR is a valid *output* of the target phase.
21+
*/
22+
sealed abstract class CheckingPhase
23+
24+
object CheckingPhase {
25+
case object Compiler extends CheckingPhase
26+
case object BaseLinker extends CheckingPhase
27+
case object Optimizer extends CheckingPhase
28+
}

‎linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,13 @@ import org.scalajs.linker.standard.LinkedClass
2828

2929
/** Checker for the validity of the IR. */
3030
private final class ClassDefChecker(classDef: ClassDef,
31-
postBaseLinker: Boolean, postOptimizer: Boolean, reporter: ErrorReporter) {
31+
previousPhase: CheckingPhase, reporter: ErrorReporter) {
3232
import ClassDefChecker._
3333

3434
import reporter.reportError
3535

36+
private val featureSet = FeatureSet.allowedAfter(previousPhase)
37+
3638
private[this] val isJLObject = classDef.name.name == ObjectClass
3739

3840
private[this] val instanceThisType: Type = {
@@ -107,7 +109,7 @@ private final class ClassDefChecker(classDef: ClassDef,
107109
* module classes and JS classes that are never instantiated. The classes
108110
* may still exist because their class data are accessed.
109111
*/
110-
if (!postBaseLinker) {
112+
if (!featureSet.supports(FeatureSet.OptionalConstructors)) {
111113
/* Check that we have exactly 1 constructor in a module class. This goes
112114
* together with `checkMethodDef`, which checks that a constructor in a
113115
* module class must be 0-arg.
@@ -489,7 +491,7 @@ private final class ClassDefChecker(classDef: ClassDef,
489491
private def checkMethodNameNamespace(name: MethodName, namespace: MemberNamespace)(
490492
implicit ctx: ErrorContext): Unit = {
491493
if (name.isReflectiveProxy) {
492-
if (postBaseLinker) {
494+
if (featureSet.supports(FeatureSet.ReflectiveProxies)) {
493495
if (namespace != MemberNamespace.Public)
494496
reportError("reflective profixes are only allowed in the public namespace")
495497
} else {
@@ -557,7 +559,7 @@ private final class ClassDefChecker(classDef: ClassDef,
557559
}
558560

559561
if (rest.isEmpty) {
560-
if (!postOptimizer)
562+
if (!featureSet.supports(FeatureSet.RelaxedCtorBodies))
561563
reportError(i"Constructor must contain a delegate constructor call")
562564

563565
val bodyStatsStoreModulesHandled =
@@ -576,7 +578,7 @@ private final class ClassDefChecker(classDef: ClassDef,
576578

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

579-
if (!postOptimizer) {
581+
if (!featureSet.supports(FeatureSet.RelaxedCtorBodies)) {
580582
if (!(cls == classDef.className || classDef.superClass.exists(_.name == cls))) {
581583
implicit val ctx = ErrorContext(delegateCtorCall)
582584
reportError(
@@ -597,7 +599,7 @@ private final class ClassDefChecker(classDef: ClassDef,
597599
implicit ctx: ErrorContext): List[Tree] = {
598600

599601
if (classDef.kind.hasModuleAccessor) {
600-
if (postOptimizer) {
602+
if (featureSet.supports(FeatureSet.RelaxedCtorBodies)) {
601603
/* If the super constructor call was inlined, the StoreModule can be anywhere.
602604
* Moreover, the optimizer can remove StoreModules altogether in many cases.
603605
*/
@@ -673,7 +675,7 @@ private final class ClassDefChecker(classDef: ClassDef,
673675
case Assign(lhs, rhs) =>
674676
lhs match {
675677
case Select(This(), field) if env.isThisRestricted =>
676-
if (postOptimizer || field.name.className == classDef.className)
678+
if (featureSet.supports(FeatureSet.RelaxedCtorBodies) || field.name.className == classDef.className)
677679
checkTree(lhs, env.withIsThisRestricted(false))
678680
else
679681
checkTree(lhs, env)
@@ -819,11 +821,13 @@ private final class ClassDefChecker(classDef: ClassDef,
819821
checkTree(index, env)
820822

821823
case RecordSelect(record, _) =>
822-
checkAllowTransients()
824+
if (!featureSet.supports(FeatureSet.Records))
825+
reportError("invalid use of records")
823826
checkTree(record, env)
824827

825828
case RecordValue(_, elems) =>
826-
checkAllowTransients()
829+
if (!featureSet.supports(FeatureSet.Records))
830+
reportError("invalid use of records")
827831
checkTrees(elems, env)
828832

829833
case IsInstanceOf(expr, testType) =>
@@ -987,7 +991,9 @@ private final class ClassDefChecker(classDef: ClassDef,
987991
checkTrees(captureValues, env)
988992

989993
case Transient(transient) =>
990-
checkAllowTransients()
994+
if (!featureSet.supports(FeatureSet.OptimizedTransients))
995+
reportError(i"invalid transient tree of class ${transient.getClass().getName()}")
996+
991997
transient.traverse(new Traversers.Traverser {
992998
override def traverse(tree: Tree): Unit = checkTree(tree, env)
993999
})
@@ -996,11 +1002,6 @@ private final class ClassDefChecker(classDef: ClassDef,
9961002
newEnv
9971003
}
9981004

999-
private def checkAllowTransients()(implicit ctx: ErrorContext): Unit = {
1000-
if (!postOptimizer)
1001-
reportError("invalid transient tree")
1002-
}
1003-
10041005
private def checkArrayType(tpe: ArrayType)(
10051006
implicit ctx: ErrorContext): Unit = {
10061007
checkArrayTypeRef(tpe.arrayTypeRef)
@@ -1036,13 +1037,13 @@ object ClassDefChecker {
10361037
*
10371038
* @return Count of IR checking errors (0 in case of success)
10381039
*/
1039-
def check(classDef: ClassDef, postBaseLinker: Boolean, postOptimizer: Boolean, logger: Logger): Int = {
1040+
def check(classDef: ClassDef, previousPhase: CheckingPhase, logger: Logger): Int = {
10401041
val reporter = new LoggerErrorReporter(logger)
1041-
new ClassDefChecker(classDef, postBaseLinker, postOptimizer, reporter).checkClassDef()
1042+
new ClassDefChecker(classDef, previousPhase, reporter).checkClassDef()
10421043
reporter.errorCount
10431044
}
10441045

1045-
def check(linkedClass: LinkedClass, postOptimizer: Boolean, logger: Logger): Int = {
1046+
def check(linkedClass: LinkedClass, previousPhase: CheckingPhase, logger: Logger): Int = {
10461047
// Rebuild a ClassDef out of the LinkedClass
10471048
import linkedClass._
10481049
implicit val pos = linkedClass.pos
@@ -1063,7 +1064,7 @@ object ClassDefChecker {
10631064
topLevelExportDefs = Nil
10641065
)(optimizerHints)
10651066

1066-
check(classDef, postBaseLinker = true, postOptimizer, logger)
1067+
check(classDef, previousPhase, logger)
10671068
}
10681069

10691070
private class Env(

‎linker/shared/src/main/scala/org/scalajs/linker/checker/FeatureSet.scala

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
/*
2+
* Scala.js (https://www.scala-js.org/)
3+
*
4+
* Copyright EPFL.
5+
*
6+
* Licensed under Apache License 2.0
7+
* (https://www.apache.org/licenses/LICENSE-2.0).
8+
*
9+
* See the NOTICE file distributed with this work for
10+
* additional information regarding copyright ownership.
11+
*/
12+
13+
package org.scalajs.linker.checker
14+
15+
import org.scalajs.linker.checker.CheckingPhase._
16+
17+
/** A set of conditional IR features that the checkers can accept.
18+
*
19+
* At any given phase, the `ClassDefChecker` and the `IRChecker` must agree on
20+
* the set of IR features that are valid. A `FeatureSet` factors out the
21+
* knowledge of what feature is acceptable when.
22+
*/
23+
private[checker] final class FeatureSet private (private val flags: Int) extends AnyVal {
24+
/** Does this feature set support (all of) the given feature set. */
25+
def supports(features: FeatureSet): Boolean =
26+
(features.flags & flags) == features.flags
27+
28+
/** The union of this feature set and `that` feature set. */
29+
def |(that: FeatureSet): FeatureSet =
30+
new FeatureSet(this.flags | that.flags)
31+
}
32+
33+
private[checker] object FeatureSet {
34+
/** Empty feature set. */
35+
val Empty = new FeatureSet(0)
36+
37+
// Individual features
38+
39+
/** Optional constructors in module classes and JS classes. */
40+
val OptionalConstructors = new FeatureSet(1 << 0)
41+
42+
/** Explicit reflective proxy definitions. */
43+
val ReflectiveProxies = new FeatureSet(1 << 1)
44+
45+
/** Transients that are the result of optimizations. */
46+
val OptimizedTransients = new FeatureSet(1 << 2)
47+
48+
/** Records and record types. */
49+
val Records = new FeatureSet(1 << 3)
50+
51+
/** Relaxed constructor discipline.
52+
*
53+
* - Optional super/delegate constructor call.
54+
* - Delegate constructor calls can target any super class.
55+
* - `this.x = ...` assignments before the delegate call can assign super class fields.
56+
* - `StoreModule` can be anywhere, or not be there at all.
57+
*/
58+
val RelaxedCtorBodies = new FeatureSet(1 << 4)
59+
60+
// Common sets
61+
62+
/** Features introduced by the base linker. */
63+
private val Linked =
64+
OptionalConstructors | ReflectiveProxies
65+
66+
/** IR that is only the result of optimizations. */
67+
private val Optimized =
68+
OptimizedTransients | Records | RelaxedCtorBodies
69+
70+
/** The set of features allowed as output of the given phase. */
71+
def allowedAfter(phase: CheckingPhase): FeatureSet = phase match {
72+
case Compiler => Empty
73+
case BaseLinker => Linked
74+
case Optimizer => Linked | Optimized
75+
}
76+
}

0 commit comments

Comments
 (0)