Skip to content

Commit c2f35b8

Browse files
committed
Fix #9970: Reconcile how Scala 3 and 2 decide whether a trait has $init$.
Scala 2 uses a very simple mechanism to detect traits that have or don't have a `$init$` method: * At parsing time, a `$init$` method is created if and only if there at least one member in the body that "requires" it. * `$init$` are otherwise never created nor removed. * A call to `T.$init$` in a subclass `C` is generated if and only if `T.$init$` exists. Scala 3 has a flags-based approach to the above: * At parsing time, a `<init>` method is always created for traits. * The `NoInits` flag is added if the body does not contain any member that "requires" an initialization. * In addition, the constructor receives the `StableRealizable` flag if the trait *and all its base classes/traits* have the `NoInits` flag. This is then used for purity analysis in the inliner. * The `Constructors` phase creates a `$init$` method for traits that do not have the `NoInits` flag, and generates calls based on the same criteria. Now, one might think that this is all fine, except it isn't when we mix Scala 2 and 3, and in particular when a Scala 2 class extend a Scala 3 trait. Indeed, since Scala 3 *always* defines a `<init>` method in traits, which Scala 2 translates as `$init$`, Scala 2 would think that it always needs to emit a call to `$init$` (even for traits where Scala 3 does not, in fact, emit a `$init$` method in the end). This was mitigated in the TASTy reader of Scala 2 by removing `$init$` if it has the `STABLE` flag (coming from `StableRealizable`). This would have been fine if `StableRealizable` was present if and only if the owner trait has `NoInits`. But until this commit, that was not the case: a trait without initialization in itself, but extending a trait with initialization code, would have the flag `NoInits` but its constructor would not have the `StableRealizable` flag. Therefore, this commit basically reconciles the `NoInits` and `StableRealizable` flags, so that Scala 2 understands whether or not a Scala 3 trait has a `$init$` method. We also align those flags when reading from Scala 2 traits, so that Scala 3 also understands whether or not a Scala 2 trait has a `$init$` trait. This solves the compatibility issue between Scala 2 and Scala 3. One detail remains. The attentive reader may have noticed the quotes in 'an element of the body that "requires" initialization'. Scala 2 and Scala 3 do not agree on what requires initialization: notably, Scala 2 thinks that a concrete `def` requires initialization, whereas Scala 3 knows that this is not the case. This is not an issue for synchronous interoperability between Scala 2 and 3, since each decides on its own which of its traits has a `$init$` method, and communicates that fact to the other version. However, this still poses an issue for "diachronic" compatibility: if a library defines a trait with a concrete `def` and is compiled by Scala 2 in version v1, that trait will have a `$init$`. When the library upgrades to Scala 3 in version v2, the trait will *lose* the `$init$`, which can break third-party subclasses. This commit does not address this issue. There are two ways we could do so: * Precisely align the detection of whether a trait should have a `$init$` method with Scala 2 (notably, adding one when there is a concrete `def`), or * *Always* emit an empty static `$init$` method, even for traits that have the `NoInits` flag (but do not generate calls to them, nor have that affect whether or not subclasses are considered pure).
1 parent b2a09d3 commit c2f35b8

File tree

8 files changed

+128
-23
lines changed

8 files changed

+128
-23
lines changed

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

+3-4
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ trait UntypedTreeInfo extends TreeInfo[Untyped] { self: Trees.Instance[Untyped]
316316
/** The largest subset of {NoInits, PureInterface} that a
317317
* trait or class enclosing this statement can have as flags.
318318
*/
319-
def defKind(tree: Tree)(using Context): FlagSet = unsplice(tree) match {
319+
private def defKind(tree: Tree)(using Context): FlagSet = unsplice(tree) match {
320320
case EmptyTree | _: Import => NoInitsInterface
321321
case tree: TypeDef => if (tree.isClassDef) NoInits else NoInitsInterface
322322
case tree: DefDef =>
@@ -402,8 +402,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
402402
|| sym.owner == defn.StringClass
403403
|| defn.pureMethods.contains(sym)
404404
if (tree.tpe.isInstanceOf[ConstantType] && isKnownPureOp(tree.symbol) // A constant expression with pure arguments is pure.
405-
|| (fn.symbol.isStableMember && !fn.symbol.is(Lazy))
406-
|| fn.symbol.isPrimaryConstructor && fn.symbol.owner.isNoInitsClass) // TODO: include in isStable?
405+
|| (fn.symbol.isStableMember && !fn.symbol.is(Lazy))) // constructors of no-inits classes are stable
407406
minOf(exprPurity(fn), args.map(exprPurity)) `min` Pure
408407
else if (fn.symbol.is(Erased)) Pure
409408
else if (fn.symbol.isStableMember) /* && fn.symbol.is(Lazy) */
@@ -459,7 +458,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
459458
else if tree.tpe.isInstanceOf[ConstantType] then PurePath
460459
else if (!sym.isStableMember) Impure
461460
else if (sym.is(Module))
462-
if (sym.moduleClass.isNoInitsClass) PurePath else IdempotentPath
461+
if (sym.moduleClass.isNoInitsRealClass) PurePath else IdempotentPath
463462
else if (sym.is(Lazy)) IdempotentPath
464463
else if sym.isAllOf(Inline | Param) then Impure
465464
else PurePath

compiler/src/dotty/tools/dotc/core/Definitions.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ class Definitions {
181181
}
182182

183183
private def completeClass(cls: ClassSymbol, ensureCtor: Boolean = true): ClassSymbol = {
184-
if (ensureCtor) ensureConstructor(cls, EmptyScope)
184+
if (ensureCtor) ensureConstructor(cls, cls.denot.asClass, EmptyScope)
185185
if (cls.linkedClass.exists) cls.linkedClass.markAbsent()
186186
cls
187187
}

compiler/src/dotty/tools/dotc/core/Flags.scala

+11-3
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,14 @@ object Flags {
293293
*/
294294
val (Abstract @ _, _, _) = newFlags(23, "abstract")
295295

296-
/** Lazy val or method is known or assumed to be stable and realizable */
296+
/** Lazy val or method is known or assumed to be stable and realizable.
297+
*
298+
* For a trait constructor, this is set if and only if owner.is(NoInits),
299+
* including for Java interfaces and for Scala 2 traits. It will be used by
300+
*
301+
* - the purity analysis used by the inliner, and
302+
* - the TASTy reader of Scala 2.13, to determine whether there is a $init$ method.
303+
*/
297304
val (_, StableRealizable @ _, _) = newFlags(24, "<stable>")
298305

299306
/** A case parameter accessor */
@@ -319,7 +326,9 @@ object Flags {
319326

320327
/** Variable is accessed from nested function
321328
* /
322-
* Trait does not have fields or initialization code.
329+
* Trait does not have own fields or initialization code or class does not
330+
* have own or inherited initialization code.
331+
*
323332
* Warning: NoInits is set during regular typer pass, should be tested only after typer.
324333
*/
325334
val (_, Captured @ _, NoInits @ _) = newFlags(32, "<captured>", "<noinits>")
@@ -536,7 +545,6 @@ object Flags {
536545
val DeferredOrTermParamOrAccessor: FlagSet = Deferred | ParamAccessor | TermParam // term symbols without right-hand sides
537546
val DeferredOrTypeParam: FlagSet = Deferred | TypeParam // type symbols without right-hand sides
538547
val EnumValue: FlagSet = Enum | StableRealizable // A Scala enum value
539-
val StableOrErased: FlagSet = Erased | StableRealizable // Assumed to be pure
540548
val FinalOrInline: FlagSet = Final | Inline
541549
val FinalOrModuleClass: FlagSet = Final | ModuleClass // A module class or a final class
542550
val EffectivelyFinalFlags: FlagSet = Final | Private

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

+3-3
Original file line numberDiff line numberDiff line change
@@ -725,11 +725,11 @@ object SymDenotations {
725725
isType || is(StableRealizable) || exists && !isUnstableValue
726726
}
727727

728-
/** Is this a denotation of a class that does not have - either direct or inherited -
728+
/** Is this a denotation of a real class that does not have - either direct or inherited -
729729
* initialization code?
730730
*/
731-
def isNoInitsClass(using Context): Boolean =
732-
isClass &&
731+
def isNoInitsRealClass(using Context): Boolean =
732+
isRealClass &&
733733
(asClass.baseClasses.forall(_.is(NoInits)) || defn.isAssuredNoInits(symbol))
734734

735735
/** Is this a "real" method? A real method is a method which is:

compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala

+14-7
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,14 @@ object Scala2Unpickler {
5959
denot.info = PolyType.fromParams(denot.owner.typeParams, denot.info)
6060
}
6161

62-
def ensureConstructor(cls: ClassSymbol, scope: Scope)(using Context): Unit = {
62+
def ensureConstructor(cls: ClassSymbol, clsDenot: ClassDenotation, scope: Scope)(using Context): Unit = {
6363
if (scope.lookup(nme.CONSTRUCTOR) == NoSymbol) {
6464
val constr = newDefaultConstructor(cls)
65+
// Scala 2 traits have a constructor iff they have initialization code
66+
// In dotc we represent that as !StableRealizable, which is also owner.is(NoInits)
67+
if clsDenot.flagsUNSAFE.is(Trait) then
68+
constr.setFlag(StableRealizable)
69+
clsDenot.setFlag(NoInits)
6570
addConstructorTypeParams(constr)
6671
cls.enter(constr, scope)
6772
}
@@ -95,7 +100,7 @@ object Scala2Unpickler {
95100
if (tsym.exists) tsym.setFlag(TypeParam)
96101
else denot.enter(tparam, decls)
97102
}
98-
if (!denot.flagsUNSAFE.isAllOf(JavaModule)) ensureConstructor(denot.symbol.asClass, decls)
103+
if (!denot.flagsUNSAFE.isAllOf(JavaModule)) ensureConstructor(cls, denot, decls)
99104

100105
val scalacCompanion = denot.classSymbol.scalacLinkedClass
101106

@@ -465,11 +470,13 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
465470
denot.setFlag(flags)
466471
denot.resetFlag(Touched) // allow one more completion
467472

468-
// Temporary measure, as long as we do not read these classes from Tasty.
469-
// Scala-2 classes don't have NoInits set even if they are pure. We override this
470-
// for Product and Serializable so that case classes can be pure. A full solution
471-
// requires that we read all Scala code from Tasty.
472-
if (owner == defn.ScalaPackageClass && ((name eq tpnme.Serializable) || (name eq tpnme.Product)))
473+
// Temporary measure, as long as we do not recompile these traits with Scala 3.
474+
// Scala 2 is more aggressive when it comes to defining a $init$ method than Scala 3.
475+
// Any concrete definition, even a `def`, causes a trait to receive a $init$ method
476+
// to be created, even if it does not do anything, and hence causes not have the NoInits flag.
477+
// We override this for Product so that cases classes can be pure.
478+
// A full solution requires that we compile Product with Scala 3 in the future.
479+
if (owner == defn.ScalaPackageClass && (name eq tpnme.Product))
473480
denot.setFlag(NoInits)
474481

475482
denot.setPrivateWithin(privateWithin)

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+3-4
Original file line numberDiff line numberDiff line change
@@ -555,11 +555,10 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {
555555
meth.name == nme.apply &&
556556
meth.flags.is(Synthetic) &&
557557
meth.owner.linkedClass.is(Case) &&
558-
cls.isNoInitsClass
558+
cls.isNoInitsRealClass
559559
}
560560
if (tree.tpe.isInstanceOf[ConstantType] && isKnownPureOp(tree.symbol) // A constant expression with pure arguments is pure.
561-
|| (fn.symbol.isStableMember && !fn.symbol.is(Lazy))
562-
|| fn.symbol.isPrimaryConstructor && fn.symbol.owner.isNoInitsClass) // TODO: include in isStable?
561+
|| (fn.symbol.isStableMember && !fn.symbol.is(Lazy))) // constructors of no-inits classes are stable
563562
apply(fn) && args.forall(apply)
564563
else if (isCaseClassApply)
565564
args.forall(apply)
@@ -864,7 +863,7 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {
864863
def reduceProjection(tree: Tree)(using Context): Tree = {
865864
if (ctx.debug) inlining.println(i"try reduce projection $tree")
866865
tree match {
867-
case Select(NewInstance(cls, args, prefix, precomputed), field) if cls.isNoInitsClass =>
866+
case Select(NewInstance(cls, args, prefix, precomputed), field) if cls.isNoInitsRealClass =>
868867
def matches(param: Symbol, selection: Symbol): Boolean =
869868
param == selection || {
870869
selection.name match {

compiler/src/dotty/tools/dotc/typer/Namer.scala

+4-1
Original file line numberDiff line numberDiff line change
@@ -1197,7 +1197,10 @@ class Namer { typer: Typer =>
11971197
cls.info = avoidPrivateLeaks(cls)
11981198
cls.baseClasses.foreach(_.invalidateBaseTypeCache()) // we might have looked before and found nothing
11991199
cls.setNoInitsFlags(parentsKind(parents), untpd.bodyKind(rest))
1200-
if (cls.isNoInitsClass) cls.primaryConstructor.setFlag(StableRealizable)
1200+
val ctorStable =
1201+
if cls.is(Trait) then cls.is(NoInits)
1202+
else cls.isNoInitsRealClass
1203+
if ctorStable then cls.primaryConstructor.setFlag(StableRealizable)
12011204
processExports(using localCtx)
12021205
defn.patchStdLibClass(denot.asClass)
12031206
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
import scala.quoted._
2+
import scala.tasty.inspector._
3+
4+
/* Test that the constructor of a trait has the StableRealizable trait if and
5+
* only if the trait has NoInits. This is used by the TASTy reader in Scala 2
6+
* to determine whether the constructor should be kept or discarded, and
7+
* consequently whether calls to $init$ should be emitted for the trait or not.
8+
*/
9+
10+
// Definitions to be inspected
11+
12+
trait I9970IOApp {
13+
protected val foo = 23
14+
15+
def run(args: List[String]): Int
16+
17+
final def main(args: Array[String]): Unit = {
18+
sys.exit(run(args.toList))
19+
}
20+
}
21+
22+
object I9970IOApp {
23+
trait Simple extends I9970IOApp {
24+
def run: Unit
25+
26+
final def run(args: List[String]): Int = {
27+
run
28+
0
29+
}
30+
}
31+
}
32+
33+
// TASTy Inspector test boilerplate
34+
35+
object Test {
36+
def main(args: Array[String]): Unit = {
37+
// Artefact of the current test infrastructure
38+
// TODO improve infrastructure to avoid needing this code on each test
39+
val classpath = dotty.tools.dotc.util.ClasspathFromClassloader(this.getClass.getClassLoader).split(java.io.File.pathSeparator).find(_.contains("runWithCompiler")).get
40+
val allTastyFiles = dotty.tools.io.Path(classpath).walkFilter(_.extension == "tasty").map(_.toString).toList
41+
val tastyFiles = allTastyFiles.filter(_.contains("I9970"))
42+
43+
new TestInspector().inspectTastyFiles(tastyFiles)
44+
}
45+
}
46+
47+
// Inspector that performs the actual tests
48+
49+
class TestInspector() extends TastyInspector:
50+
51+
private var foundIOApp: Boolean = false
52+
private var foundSimple: Boolean = false
53+
54+
protected def processCompilationUnit(using Quotes)(root: quotes.reflect.Tree): Unit =
55+
foundIOApp = false
56+
foundSimple = false
57+
inspectClass(root)
58+
// Sanity check to make sure that our pattern matches are not simply glossing over the things we want to test
59+
assert(foundIOApp, "the inspector did not encounter IOApp")
60+
assert(foundSimple, "the inspect did not encounter IOApp.Simple")
61+
62+
private def inspectClass(using Quotes)(tree: quotes.reflect.Tree): Unit =
63+
import quotes.reflect._
64+
tree match
65+
case t: PackageClause =>
66+
t.stats.foreach(inspectClass(_))
67+
68+
case t: ClassDef if t.name.endsWith("$") =>
69+
t.body.foreach(inspectClass(_))
70+
71+
case t: ClassDef =>
72+
t.name match
73+
case "I9970IOApp" =>
74+
foundIOApp = true
75+
// Cannot test the following because NoInits is not part of the quotes API
76+
//assert(!t.symbol.flags.is(Flags.NoInits))
77+
assert(!t.constructor.symbol.flags.is(Flags.StableRealizable))
78+
79+
case "Simple" =>
80+
foundSimple = true
81+
// Cannot test the following because NoInits is not part of the quotes API
82+
//assert(t.symbol.flags.is(Flags.NoInits))
83+
assert(t.constructor.symbol.flags.is(Flags.StableRealizable))
84+
85+
case _ =>
86+
assert(false, s"unexpected ClassDef '${t.name}'")
87+
88+
case _ =>
89+
end inspectClass

0 commit comments

Comments
 (0)