Skip to content

Commit d0237e0

Browse files
retronymretronym
committed
Desugar module var and accessor in refchecks/lazyvals
Rather than leaving it until mixin. The broader motivation is to simplify the mixin phase of the compiler before we get rid of implementatation classes in favour of using JDK8 default interface methods. The current code in mixin is used for both lazy val and modules, and puts the "slow path" code that uses the monitor into a dedicated method (`moduleName$lzyCompute`). I tracked this back to a3d4d17. I can't tell from that commit whether the performance sensititivity was related to modules or lazy vals, from the commit message I'd say the latter. As the initialization code for a module is just a constructor call, rather than an arbitraryly large chunk of code for a lazy initializer, this commit opts to inline the `lzycompute` method. During refchecks, mixin module accessors are added to classes, so that mixed in and defined modules are translated uniformly. Defer synthesis of the double checked locking idiom to the lazyvals phase, which gets us a step closer to a unified translation of modules and lazy vals. I had to change the `atOwner` methods to to avoid using the non-existent module class of a module accessor method as the current owner. This fixes a latent bug. Without this change, retypechecking of the module accessor method during erasure crashes with an accessibility error selecting the module var.
1 parent 5af7f23 commit d0237e0

File tree

14 files changed

+139
-76
lines changed

14 files changed

+139
-76
lines changed

‎src/compiler/scala/tools/nsc/transform/LazyVals.scala‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ abstract class LazyVals extends Transform with TypingTransformers with ast.TreeD
5454
private val lazyVals = perRunCaches.newMap[Symbol, Int]() withDefaultValue 0
5555

5656
import symtab.Flags._
57+
private def flattenThickets(stats: List[Tree]): List[Tree] = stats.flatMap(_ match {
58+
case b @ Block(List(d1@DefDef(_, n1, _, _, _, _)), d2@DefDef(_, n2, _, _, _, _)) if b.tpe == null && n1.endsWith(nme.LAZY_SLOW_SUFFIX) =>
59+
List(d1, d2)
60+
case stat =>
61+
List(stat)
62+
})
5763

5864
/** Perform the following transformations:
5965
* - for a lazy accessor inside a method, make it check the initialization bitmap
@@ -72,20 +78,14 @@ abstract class LazyVals extends Transform with TypingTransformers with ast.TreeD
7278
case Block(_, _) =>
7379
val block1 = super.transform(tree)
7480
val Block(stats, expr) = block1
75-
val stats1 = stats.flatMap(_ match {
76-
case Block(List(d1@DefDef(_, n1, _, _, _, _)), d2@DefDef(_, n2, _, _, _, _)) if (nme.newLazyValSlowComputeName(n2) == n1) =>
77-
List(d1, d2)
78-
case stat =>
79-
List(stat)
80-
})
81-
treeCopy.Block(block1, stats1, expr)
81+
treeCopy.Block(block1, flattenThickets(stats), expr)
8282

8383
case DefDef(_, _, _, _, _, rhs) => atOwner(tree.symbol) {
8484
val (res, slowPathDef) = if (!sym.owner.isClass && sym.isLazy) {
8585
val enclosingClassOrDummyOrMethod = {
8686
val enclMethod = sym.enclMethod
8787

88-
if (enclMethod != NoSymbol ) {
88+
if (enclMethod != NoSymbol) {
8989
val enclClass = sym.enclClass
9090
if (enclClass != NoSymbol && enclMethod == enclClass.enclMethod)
9191
enclClass
@@ -100,11 +100,26 @@ abstract class LazyVals extends Transform with TypingTransformers with ast.TreeD
100100
val (rhs1, sDef) = mkLazyDef(enclosingClassOrDummyOrMethod, transform(rhs), idx, sym)
101101
sym.resetFlag((if (lazyUnit(sym)) 0 else LAZY) | ACCESSOR)
102102
(rhs1, sDef)
103-
} else
103+
} else if (!sym.owner.isTrait && sym.isModule && sym.isMethod) {
104+
rhs match {
105+
case b @ Block((assign @ Assign(lhs, rhs)) :: Nil, expr) =>
106+
val cond = Apply(Select(lhs, Object_eq), List(Literal(Constant(null))))
107+
val moduleDefs = mkFastPathBody(sym.owner.enclClass, lhs.symbol, cond, transform(assign) :: Nil, Nil, transform(expr))
108+
(localTyper.typedPos(tree.pos)(moduleDefs._1), localTyper.typedPos(tree.pos)(moduleDefs._2))
109+
case rhs =>
110+
global.reporter.error(tree.pos, "Unexpected tree on the RHS of a module accessor: " + rhs)
111+
(rhs, EmptyTree)
112+
}
113+
} else {
104114
(transform(rhs), EmptyTree)
115+
}
105116

106117
val ddef1 = deriveDefDef(tree)(_ => if (LocalLazyValFinder.find(res)) typed(addBitmapDefs(sym, res)) else res)
107-
if (slowPathDef != EmptyTree) Block(slowPathDef, ddef1) else ddef1
118+
if (slowPathDef != EmptyTree) {
119+
// The contents of this block are flattened into the enclosing statement sequence, see flattenThickets
120+
// This is a poor man's version of dotty's Thicket: https://github.com/lampepfl/dotty/blob/d5280358d1/src/dotty/tools/dotc/ast/Trees.scala#L707
121+
Block(slowPathDef, ddef1)
122+
} else ddef1
108123
}
109124

110125
case Template(_, _, body) => atOwner(currentOwner) {
@@ -135,7 +150,7 @@ abstract class LazyVals extends Transform with TypingTransformers with ast.TreeD
135150
})
136151
toAdd0
137152
} else List()
138-
deriveTemplate(tree)(_ => innerClassBitmaps ++ stats)
153+
deriveTemplate(tree)(_ => innerClassBitmaps ++ flattenThickets(stats))
139154
}
140155

141156
case ValDef(_, _, _, _) if !sym.owner.isModule && !sym.owner.isClass =>

‎src/compiler/scala/tools/nsc/transform/Mixin.scala‎

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -861,16 +861,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
861861
typedPos(init.head.pos)(mkFastPathLazyBody(clazz, lzyVal, cond, syncBody, nulls, retVal))
862862
}
863863

864-
def mkInnerClassAccessorDoubleChecked(attrThis: Tree, rhs: Tree, moduleSym: Symbol, args: List[Tree]): Tree =
865-
rhs match {
866-
case Block(List(assign), returnTree) =>
867-
val Assign(moduleVarRef, _) = assign
868-
val cond = Apply(Select(moduleVarRef, Object_eq), List(NULL))
869-
mkFastPathBody(clazz, moduleSym, cond, List(assign), List(NULL), returnTree, attrThis, args)
870-
case _ =>
871-
abort(s"Invalid getter $rhs for module in $clazz")
872-
}
873-
874864
def mkCheckedAccessor(clazz: Symbol, retVal: Tree, offset: Int, pos: Position, fieldSym: Symbol): Tree = {
875865
val sym = fieldSym.getterIn(fieldSym.owner)
876866
val bitmapSym = bitmapFor(clazz, offset, sym)
@@ -926,18 +916,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
926916
deriveDefDef(stat)(rhs => Block(List(rhs, localTyper.typed(mkSetFlag(clazz, fieldOffset(getter), getter, bitmapKind(getter)))), UNIT))
927917
else stat
928918
}
929-
else if (sym.isModule && (!clazz.isTrait || clazz.isImplClass) && !sym.isBridge) {
930-
deriveDefDef(stat)(rhs =>
931-
typedPos(stat.pos)(
932-
mkInnerClassAccessorDoubleChecked(
933-
// Martin to Hubert: I think this can be replaced by selfRef(tree.pos)
934-
// @PP: It does not seem so, it crashes for me trying to bootstrap.
935-
if (clazz.isImplClass) gen.mkAttributedIdent(stat.vparamss.head.head.symbol) else gen.mkAttributedThis(clazz),
936-
rhs, sym, stat.vparamss.head
937-
)
938-
)
939-
)
940-
}
941919
else stat
942920
}
943921
stats map {
@@ -1082,18 +1060,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
10821060
})
10831061
}
10841062
else if (sym.isModule && !(sym hasFlag LIFTED | BRIDGE)) {
1085-
// add modules
1086-
val vsym = sym.owner.newModuleVarSymbol(sym)
1087-
addDef(position(sym), ValDef(vsym))
1088-
1089-
// !!! TODO - unravel the enormous duplication between this code and
1090-
// eliminateModuleDefs in RefChecks.
1091-
val rhs = gen.newModule(sym, vsym.tpe)
1092-
val assignAndRet = gen.mkAssignAndReturn(vsym, rhs)
1093-
val attrThis = gen.mkAttributedThis(clazz)
1094-
val rhs1 = mkInnerClassAccessorDoubleChecked(attrThis, assignAndRet, sym, List())
1095-
1096-
addDefDef(sym, rhs1)
1063+
// Moved to Refchecks
10971064
}
10981065
else if (!sym.isMethod) {
10991066
// add fields

‎src/compiler/scala/tools/nsc/transform/TypingTransformers.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ trait TypingTransformers {
2626

2727
def atOwner[A](tree: Tree, owner: Symbol)(trans: => A): A = {
2828
val savedLocalTyper = localTyper
29-
localTyper = localTyper.atOwner(tree, if (owner.isModule) owner.moduleClass else owner)
29+
localTyper = localTyper.atOwner(tree, if (owner.isModuleNotMethod) owner.moduleClass else owner)
3030
val result = super.atOwner(owner)(trans)
3131
localTyper = savedLocalTyper
3232
result

‎src/compiler/scala/tools/nsc/typechecker/RefChecks.scala‎

Lines changed: 38 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,26 +1188,6 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
11881188
// set NoType so it will be ignored.
11891189
val cdef = ClassDef(module.moduleClass, impl) setType NoType
11901190

1191-
// Create the module var unless the immediate owner is a class and
1192-
// the module var already exists there. See SI-5012, SI-6712.
1193-
def findOrCreateModuleVar() = {
1194-
val vsym = (
1195-
if (site.isTerm) NoSymbol
1196-
else site.info decl nme.moduleVarName(moduleName)
1197-
)
1198-
vsym orElse (site newModuleVarSymbol module)
1199-
}
1200-
def newInnerObject() = {
1201-
// Create the module var unless it is already in the module owner's scope.
1202-
// The lookup is on module.enclClass and not module.owner lest there be a
1203-
// nullary method between us and the class; see SI-5012.
1204-
val moduleVar = findOrCreateModuleVar()
1205-
val rhs = gen.newModule(module, moduleVar.tpe)
1206-
val body = if (site.isTrait) rhs else gen.mkAssignAndReturn(moduleVar, rhs)
1207-
val accessor = DefDef(module, body.changeOwner(moduleVar -> module))
1208-
1209-
ValDef(moduleVar) :: accessor :: Nil
1210-
}
12111191
def matchingInnerObject() = {
12121192
val newFlags = (module.flags | STABLE) & ~MODULE
12131193
val newInfo = NullaryMethodType(module.moduleClass.tpe)
@@ -1219,10 +1199,45 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
12191199
if (module.isStatic)
12201200
if (module.isOverridingSymbol) matchingInnerObject() else Nil
12211201
else
1222-
newInnerObject()
1202+
newInnerObject(site, module)
12231203
)
12241204
transformTrees(newTrees map localTyper.typedPos(moduleDef.pos))
12251205
}
1206+
def newInnerObject(site: Symbol, module: Symbol): List[Tree] = {
1207+
// Create the module var unless the immediate owner is a class and
1208+
// the module var already exists there. See SI-5012, SI-6712.
1209+
def findOrCreateModuleVar() = {
1210+
val vsym = (
1211+
if (site.isTerm) NoSymbol
1212+
else site.info decl nme.moduleVarName(module.name.toTermName)
1213+
)
1214+
vsym orElse (site newModuleVarSymbol module)
1215+
}
1216+
// Create the module var unless it is already in the module owner's scope.
1217+
// The lookup is on module.enclClass and not module.owner lest there be a
1218+
// nullary method between us and the class; see SI-5012.
1219+
val moduleVar = findOrCreateModuleVar()
1220+
val rhs = gen.newModule(module, moduleVar.tpe)
1221+
val body = if (site.isTrait) rhs else {
1222+
if (moduleVar.enclClass == NoSymbol) gen.mkAssignAndReturn(moduleVar, rhs) // e.g. neg/t6666c.scala, will be an error later on.
1223+
else gen.mkAssignAndReturn(moduleVar, rhs) // will be wrapped in double checked lock idiom in lazyvals
1224+
}
1225+
val accessor = if (module.owner == site) module else {
1226+
site.newMethod(module.name.toTermName, site.pos, STABLE | MODULE | MIXEDIN).setInfo(moduleVar.tpe).andAlso(self => if (module.isPrivate) self.expandName(module.owner))
1227+
}
1228+
val accessorDef = DefDef(accessor, body.changeOwner(moduleVar -> accessor))
1229+
1230+
ValDef(moduleVar) :: accessorDef :: Nil
1231+
}
1232+
1233+
def mixinModuleDefs(clazz: Symbol): List[Tree] = {
1234+
val res = for {
1235+
mixinClass <- clazz.mixinClasses.iterator
1236+
module <- mixinClass.info.decls.iterator.filter(_.isModule)
1237+
newMember <- newInnerObject(clazz, module)
1238+
} yield transform(localTyper.typedPos(clazz.pos)(newMember))
1239+
res.toList
1240+
}
12261241

12271242
def transformStat(tree: Tree, index: Int): List[Tree] = tree match {
12281243
case t if treeInfo.isSelfConstrCall(t) =>
@@ -1651,11 +1666,12 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
16511666
// SI-7870 default getters for constructors live in the companion module
16521667
checkOverloadedRestrictions(currentOwner, currentOwner.companionModule)
16531668
val bridges = addVarargBridges(currentOwner)
1669+
val moduleDesugared = if (currentOwner.isTrait) Nil else mixinModuleDefs(currentOwner)
16541670
checkAllOverrides(currentOwner)
16551671
checkAnyValSubclass(currentOwner)
16561672
if (currentOwner.isDerivedValueClass)
16571673
currentOwner.primaryConstructor makeNotPrivate NoSymbol // SI-6601, must be done *after* pickler!
1658-
if (bridges.nonEmpty) deriveTemplate(tree)(_ ::: bridges) else tree
1674+
if (bridges.nonEmpty || moduleDesugared.nonEmpty) deriveTemplate(tree)(_ ::: bridges ::: moduleDesugared) else tree
16591675

16601676
case dc@TypeTreeWithDeferredRefCheck() => abort("adapt should have turned dc: TypeTreeWithDeferredRefCheck into tpt: TypeTree, with tpt.original == dc")
16611677
case tpt@TypeTree() =>

‎src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
387387
val savedValid = validCurrentOwner
388388
if (owner.isClass) validCurrentOwner = true
389389
val savedLocalTyper = localTyper
390-
localTyper = localTyper.atOwner(tree, if (owner.isModule) owner.moduleClass else owner)
390+
localTyper = localTyper.atOwner(tree, if (owner.isModuleNotMethod) owner.moduleClass else owner)
391391
typers = typers updated (owner, localTyper)
392392
val result = super.atOwner(tree, owner)(trans)
393393
localTyper = savedLocalTyper

‎src/reflect/scala/reflect/internal/StdNames.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -876,7 +876,7 @@ trait StdNames {
876876
val toCharacter: NameType = "toCharacter"
877877
val toInteger: NameType = "toInteger"
878878

879-
def newLazyValSlowComputeName(lzyValName: Name) = lzyValName append LAZY_SLOW_SUFFIX
879+
def newLazyValSlowComputeName(lzyValName: Name) = (lzyValName stripSuffix MODULE_VAR_SUFFIX append LAZY_SLOW_SUFFIX).toTermName
880880

881881
// ASCII names for operators
882882
val ADD = encode("+")

‎src/reflect/scala/reflect/internal/Symbols.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
323323
def newModuleVarSymbol(accessor: Symbol): TermSymbol = {
324324
val newName = nme.moduleVarName(accessor.name.toTermName)
325325
val newFlags = MODULEVAR | ( if (this.isClass) PrivateLocal | SYNTHETIC else 0 )
326-
val newInfo = accessor.tpe.finalResultType
326+
val newInfo = thisType.memberType(accessor).finalResultType
327327
val mval = newVariable(newName, accessor.pos.focus, newFlags.toLong) addAnnotation VolatileAttr
328328

329329
if (this.isClass)

‎src/reflect/scala/reflect/internal/transform/Erasure.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ trait Erasure {
123123
case tref @ TypeRef(pre, sym, args) =>
124124
if (sym == ArrayClass)
125125
if (unboundedGenericArrayLevel(tp) == 1) ObjectTpe
126-
else if (args.head.typeSymbol.isBottomClass) arrayType(ObjectTpe)
126+
else if (args.head.typeSymbol.isBottomClass) arrayType(ObjectTpe)
127127
else typeRef(apply(pre), sym, args map applyInArray)
128128
else if (sym == AnyClass || sym == AnyValClass || sym == SingletonClass) ObjectTpe
129129
else if (sym == UnitClass) BoxedUnitTpe

‎test/files/run/delambdafy_t6028.check‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,24 @@ package <empty> {
3838
<synthetic> <stable> <artifact> def $outer(): T = MethodLocalObject$2.this.$outer;
3939
<synthetic> <stable> <artifact> def $outer(): T = MethodLocalObject$2.this.$outer
4040
};
41-
final <stable> private[this] def MethodLocalObject$1(barParam$1: String, MethodLocalObject$module$1: runtime.VolatileObjectRef): T#MethodLocalObject$2.type = {
42-
MethodLocalObject$module$1.elem = new T#MethodLocalObject$2.type(T.this, barParam$1);
41+
final <stable> private[this] def MethodLocalObject$lzycompute$1(barParam$1: String, MethodLocalObject$module$1: runtime.VolatileObjectRef): T#MethodLocalObject$2.type = {
42+
{
43+
T.this.synchronized({
44+
if (MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]().eq(null))
45+
{
46+
MethodLocalObject$module$1.elem = new T#MethodLocalObject$2.type(T.this, barParam$1);
47+
()
48+
};
49+
scala.runtime.BoxedUnit.UNIT
50+
});
51+
()
52+
};
4353
MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]()
4454
};
55+
final <stable> private[this] def MethodLocalObject$1(barParam$1: String, MethodLocalObject$module$1: runtime.VolatileObjectRef): T#MethodLocalObject$2.type = if (MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]().eq(null))
56+
T.this.MethodLocalObject$lzycompute$1(barParam$1, MethodLocalObject$module$1)
57+
else
58+
MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]();
4559
abstract trait MethodLocalTrait$1$class extends Object with T#MethodLocalTrait$1 {
4660
def /*MethodLocalTrait$1$class*/$init$(barParam$1: String): Unit = {
4761
()

‎test/files/run/t6028.check‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,24 @@ package <empty> {
5050
<synthetic> <stable> <artifact> def $outer(): T = MethodLocalObject$2.this.$outer;
5151
<synthetic> <stable> <artifact> def $outer(): T = MethodLocalObject$2.this.$outer
5252
};
53-
final <stable> private[this] def MethodLocalObject$1(barParam$1: Int, MethodLocalObject$module$1: runtime.VolatileObjectRef): T#MethodLocalObject$2.type = {
54-
MethodLocalObject$module$1.elem = new T#MethodLocalObject$2.type(T.this, barParam$1);
53+
final <stable> private[this] def MethodLocalObject$lzycompute$1(barParam$1: Int, MethodLocalObject$module$1: runtime.VolatileObjectRef): T#MethodLocalObject$2.type = {
54+
{
55+
T.this.synchronized({
56+
if (MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]().eq(null))
57+
{
58+
MethodLocalObject$module$1.elem = new T#MethodLocalObject$2.type(T.this, barParam$1);
59+
()
60+
};
61+
scala.runtime.BoxedUnit.UNIT
62+
});
63+
()
64+
};
5565
MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]()
5666
};
67+
final <stable> private[this] def MethodLocalObject$1(barParam$1: Int, MethodLocalObject$module$1: runtime.VolatileObjectRef): T#MethodLocalObject$2.type = if (MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]().eq(null))
68+
T.this.MethodLocalObject$lzycompute$1(barParam$1, MethodLocalObject$module$1)
69+
else
70+
MethodLocalObject$module$1.elem.$asInstanceOf[T#MethodLocalObject$2.type]();
5771
abstract trait MethodLocalTrait$1$class extends Object with T#MethodLocalTrait$1 {
5872
def /*MethodLocalTrait$1$class*/$init$(barParam$1: Int): Unit = {
5973
()

0 commit comments

Comments
 (0)