From 96d2bd1680ef5ddb0e2d49d16eb1f5daca7fccbe Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Mon, 24 Aug 2015 15:11:17 +0200 Subject: [PATCH 1/7] Memoize: call transformFollowingDeep with correct owner. --- src/dotty/tools/dotc/transform/Memoize.scala | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Memoize.scala b/src/dotty/tools/dotc/transform/Memoize.scala index 728005cabf11..63466dc461ee 100644 --- a/src/dotty/tools/dotc/transform/Memoize.scala +++ b/src/dotty/tools/dotc/transform/Memoize.scala @@ -70,16 +70,16 @@ import Decorators._ lazy val field = sym.field.orElse(newField).asTerm if (sym.is(Accessor, butNot = NoFieldNeeded)) if (sym.isGetter) { - var rhs = tree.rhs.changeOwnerAfter(sym, field, thisTransform) - if (isWildcardArg(rhs)) rhs = EmptyTree - val fieldDef = transformFollowing(ValDef(field, rhs)) - val getterDef = cpy.DefDef(tree)(rhs = transformFollowingDeep(ref(field))) - Thicket(fieldDef, getterDef) - } + var rhs = tree.rhs.changeOwnerAfter(sym, field, thisTransform) + if (isWildcardArg(rhs)) rhs = EmptyTree + val fieldDef = transformFollowing(ValDef(field, rhs)) + val getterDef = cpy.DefDef(tree)(rhs = transformFollowingDeep(ref(field))(ctx.withOwner(sym), info)) + Thicket(fieldDef, getterDef) + } else if (sym.isSetter) { if (!sym.is(ParamAccessor)) { val Literal(Constant(())) = tree.rhs } // this is intended as an assertion val initializer = Assign(ref(field), ref(tree.vparamss.head.head.symbol)) - cpy.DefDef(tree)(rhs = transformFollowingDeep(initializer)) + cpy.DefDef(tree)(rhs = transformFollowingDeep(initializer)(ctx.withOwner(sym), info)) } else tree // curiously, some accessors from Scala2 have ' ' suffixes. They count as // neither getters nor setters From 030ab74be769177977d344a309f982d052a1adfb Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Mon, 24 Aug 2015 15:49:04 +0200 Subject: [PATCH 2/7] Constructors: gather retained private vals in advance. Allows not to go deep into tree, additionally fixes bugs with fields that are only used in inner classes being removed. --- .../tools/dotc/transform/Constructors.scala | 76 ++++++++++++++++++- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala index ad3422232c58..f097f7e8659f 100644 --- a/src/dotty/tools/dotc/transform/Constructors.scala +++ b/src/dotty/tools/dotc/transform/Constructors.scala @@ -33,6 +33,62 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor override def runsAfter: Set[Class[_ <: Phase]] = Set(classOf[Memoize]) + // Collect all private parameter accessors and value definitions that need + // to be retained. There are several reasons why a parameter accessor or + // definition might need to be retained: + // 1. It is accessed after the constructor has finished + // 2. It is accessed before it is defined + // 3. It is accessed on an object other than `this` + // 4. It is a mutable parameter accessor + // 5. It is has a wildcard initializer `_` + + private var retainedPrivateVals = mutable.Set[Symbol]() + private var seenPrivateVals = mutable.Set[Symbol]() + private var insideConstructor = false + + private def markUsedPrivateSymbols(tree: RefTree)(implicit ctx: Context): Unit = { + + val sym = tree.symbol + def retain = { + if (sym.toString.contains("initialValues")) + println("hooooo") + retainedPrivateVals.add(sym) + } + + if (mightBeDropped(sym) && sym.owner.isClass) { + val owner = sym.owner.asClass + + tree match { + case Ident(_) | Select(This(_), _) => + def inConstructor = ctx.owner.enclosingMethod.isPrimaryConstructor && ctx.owner.enclosingClass == owner + if (inConstructor && (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) { + // used inside constructor, accessed on this, + // could use constructor argument instead, no need to retain field + println("hoha") + } + else retain + case _ => retain + } + } + } + + override def transformIdent(tree: tpd.Ident)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = { + markUsedPrivateSymbols(tree) + tree + } + + override def transformSelect(tree: tpd.Select)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = { + markUsedPrivateSymbols(tree) + tree + } + + + override def transformValDef(tree: tpd.ValDef)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = { + if (mightBeDropped(tree.symbol)) + (if (isWildcardStarArg(tree.rhs)) retainedPrivateVals else seenPrivateVals) += tree.symbol + tree + } + /** All initializers for non-lazy fields should be moved into constructor. * All non-abstract methods should be implemented (this is assured for constructors * in this phase and for other methods in memoize). @@ -152,7 +208,15 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor } usage.collect(tree.body) - def isRetained(acc: Symbol) = !mightBeDropped(acc) || usage.retained(acc) + def isRetained(acc: Symbol) = { + !mightBeDropped(acc) || { + val a = usage.retained(acc) + val b = retainedPrivateVals(acc) + if (a != b) + println("fail") + b + } + } val constrStats, clsStats = new mutable.ListBuffer[Tree] @@ -170,6 +234,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor } } + val dropped = mutable.Set[Symbol]() + // Split class body into statements that go into constructor and // definitions that are kept as members of the class. def splitStats(stats: List[Tree]): Unit = stats match { @@ -183,6 +249,7 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor clsStats += cpy.ValDef(stat)(rhs = EmptyTree) } else if (!stat.rhs.isEmpty) { + dropped += sym sym.copySymDenotation( initFlags = sym.flags &~ Private, owner = constr.symbol).installAfter(thisTransform) @@ -203,8 +270,10 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor // The initializers for the retained accessors */ val copyParams = accessors flatMap { acc => - if (!isRetained(acc)) Nil - else { + if (!isRetained(acc)) { + dropped += acc + Nil + } else { val target = if (acc.is(Method)) acc.field else acc if (!target.exists) Nil // this case arises when the parameter accessor is an alias else { @@ -224,7 +293,6 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor } // Drop accessors that are not retained from class scope - val dropped = usage.dropped if (dropped.nonEmpty) { val clsInfo = cls.classInfo // TODO investigate: expand clsInfo to cls.info => dotty type error cls.copy( From 86e83aff4c06d837912e248edfe08ea8e99c731d Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Mon, 24 Aug 2015 17:30:14 +0200 Subject: [PATCH 3/7] Constructors: Do not store private fields used only to initialise other fields --- src/dotty/tools/dotc/transform/Constructors.scala | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala index f097f7e8659f..76046f0019a6 100644 --- a/src/dotty/tools/dotc/transform/Constructors.scala +++ b/src/dotty/tools/dotc/transform/Constructors.scala @@ -60,7 +60,10 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor tree match { case Ident(_) | Select(This(_), _) => - def inConstructor = ctx.owner.enclosingMethod.isPrimaryConstructor && ctx.owner.enclosingClass == owner + def inConstructor = { + val method = ctx.owner.enclosingMethod + method.isPrimaryConstructor && ctx.owner.enclosingClass == owner + } if (inConstructor && (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) { // used inside constructor, accessed on this, // could use constructor argument instead, no need to retain field @@ -131,6 +134,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo): Tree = { val cls = ctx.owner.asClass + if (cls.toString.contains("VarianceChecker")) + println("hoho") val constr @ DefDef(nme.CONSTRUCTOR, Nil, vparams :: Nil, _, EmptyTree) = tree.constr // Produce aligned accessors and constructor parameters. We have to adjust @@ -298,6 +303,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor cls.copy( info = clsInfo.derivedClassInfo( decls = clsInfo.decls.filteredScope(!dropped.contains(_)))) + + // TODO: this happens to work only because Constructors is the last phase in group } val (superCalls, followConstrStats) = constrStats.toList match { From a33eece6c72d2c9b36b0d279fe0e3718b7dda817 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Mon, 24 Aug 2015 17:33:30 +0200 Subject: [PATCH 4/7] Constructors: remove self validation. Curiously enough, it only found bugs in old scheme It was deleting accessors, that used to be vals but became defs without anybody raising the `Method` flag. --- .../tools/dotc/transform/Constructors.scala | 58 +------------------ 1 file changed, 3 insertions(+), 55 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala index 76046f0019a6..3c7a94a7d8a2 100644 --- a/src/dotty/tools/dotc/transform/Constructors.scala +++ b/src/dotty/tools/dotc/transform/Constructors.scala @@ -41,7 +41,6 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor // 3. It is accessed on an object other than `this` // 4. It is a mutable parameter accessor // 5. It is has a wildcard initializer `_` - private var retainedPrivateVals = mutable.Set[Symbol]() private var seenPrivateVals = mutable.Set[Symbol]() private var insideConstructor = false @@ -49,11 +48,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor private def markUsedPrivateSymbols(tree: RefTree)(implicit ctx: Context): Unit = { val sym = tree.symbol - def retain = { - if (sym.toString.contains("initialValues")) - println("hooooo") + def retain = retainedPrivateVals.add(sym) - } if (mightBeDropped(sym) && sym.owner.isClass) { val owner = sym.owner.asClass @@ -134,8 +130,7 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo): Tree = { val cls = ctx.owner.asClass - if (cls.toString.contains("VarianceChecker")) - println("hoho") + val constr @ DefDef(nme.CONSTRUCTOR, Nil, vparams :: Nil, _, EmptyTree) = tree.constr // Produce aligned accessors and constructor parameters. We have to adjust @@ -174,53 +169,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor } } - // Collect all private parameter accessors and value definitions that need - // to be retained. There are several reasons why a parameter accessor or - // definition might need to be retained: - // 1. It is accessed after the constructor has finished - // 2. It is accessed before it is defined - // 3. It is accessed on an object other than `this` - // 4. It is a mutable parameter accessor - // 5. It is has a wildcard initializer `_` - object usage extends TreeTraverser { - private var inConstr: Boolean = true - private val seen = mutable.Set[Symbol](accessors: _*) - val retained = mutable.Set[Symbol]() - def dropped: collection.Set[Symbol] = seen -- retained - override def traverse(tree: Tree)(implicit ctx: Context) = { - val sym = tree.symbol - tree match { - case Ident(_) | Select(This(_), _) if inConstr && seen(tree.symbol) => - // could refer to definition in constructors, so no retention necessary - case tree: RefTree => - if (mightBeDropped(sym)) retained += sym - case _ => - } - if (!noDirectRefsFrom(tree)) traverseChildren(tree) - } - def collect(stats: List[Tree]): Unit = stats foreach { - case stat: ValDef if !stat.symbol.is(Lazy) => - traverse(stat) - if (mightBeDropped(stat.symbol)) - (if (isWildcardStarArg(stat.rhs)) retained else seen) += stat.symbol - case stat: DefTree => - inConstr = false - traverse(stat) - inConstr = true - case stat => - traverse(stat) - } - } - usage.collect(tree.body) - def isRetained(acc: Symbol) = { - !mightBeDropped(acc) || { - val a = usage.retained(acc) - val b = retainedPrivateVals(acc) - if (a != b) - println("fail") - b - } + !mightBeDropped(acc) || retainedPrivateVals(acc) } val constrStats, clsStats = new mutable.ListBuffer[Tree] @@ -303,8 +253,6 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor cls.copy( info = clsInfo.derivedClassInfo( decls = clsInfo.decls.filteredScope(!dropped.contains(_)))) - - // TODO: this happens to work only because Constructors is the last phase in group } val (superCalls, followConstrStats) = constrStats.toList match { From 56b1951b5763a3a77230f8b405e17b2e3ed2c988 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Mon, 24 Aug 2015 17:34:47 +0200 Subject: [PATCH 5/7] Add comment to Compiler.scala about behaviour of Constructors --- src/dotty/tools/dotc/Compiler.scala | 2 +- src/dotty/tools/dotc/transform/Constructors.scala | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/Compiler.scala b/src/dotty/tools/dotc/Compiler.scala index f753b7614a45..e4b328a82058 100644 --- a/src/dotty/tools/dotc/Compiler.scala +++ b/src/dotty/tools/dotc/Compiler.scala @@ -71,7 +71,7 @@ class Compiler { new LinkScala2ImplClasses, new NonLocalReturns, new CapturedVars, // capturedVars has a transformUnit: no phases should introduce local mutable vars here - new Constructors, + new Constructors, // constructors changes decls in transformTemplate, no InfoTransformers should be added after it new FunctionalInterfaces, new GetClass), // getClass transformation should be applied to specialized methods List(new LambdaLift, // in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala index 3c7a94a7d8a2..7ba2ccf4d20d 100644 --- a/src/dotty/tools/dotc/transform/Constructors.scala +++ b/src/dotty/tools/dotc/transform/Constructors.scala @@ -253,6 +253,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor cls.copy( info = clsInfo.derivedClassInfo( decls = clsInfo.decls.filteredScope(!dropped.contains(_)))) + + // TODO: this happens to work only because Constructors is the last phase in group } val (superCalls, followConstrStats) = constrStats.toList match { From d1ecc22865b201a94a18c6a4f3404ab8bbb5945a Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Tue, 25 Aug 2015 10:45:28 +0200 Subject: [PATCH 6/7] Address review comments #774 --- src/dotty/tools/dotc/transform/Constructors.scala | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala index 7ba2ccf4d20d..2f78eb9c60d5 100644 --- a/src/dotty/tools/dotc/transform/Constructors.scala +++ b/src/dotty/tools/dotc/transform/Constructors.scala @@ -41,9 +41,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor // 3. It is accessed on an object other than `this` // 4. It is a mutable parameter accessor // 5. It is has a wildcard initializer `_` - private var retainedPrivateVals = mutable.Set[Symbol]() - private var seenPrivateVals = mutable.Set[Symbol]() - private var insideConstructor = false + private val retainedPrivateVals = mutable.Set[Symbol]() + private val seenPrivateVals = mutable.Set[Symbol]() private def markUsedPrivateSymbols(tree: RefTree)(implicit ctx: Context): Unit = { @@ -63,7 +62,6 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor if (inConstructor && (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) { // used inside constructor, accessed on this, // could use constructor argument instead, no need to retain field - println("hoha") } else retain case _ => retain @@ -81,7 +79,6 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor tree } - override def transformValDef(tree: tpd.ValDef)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = { if (mightBeDropped(tree.symbol)) (if (isWildcardStarArg(tree.rhs)) retainedPrivateVals else seenPrivateVals) += tree.symbol From 863d72dea0ada792ec3813fce588a996c532d295 Mon Sep 17 00:00:00 2001 From: Dmitry Petrashko Date: Wed, 9 Sep 2015 17:18:47 +0200 Subject: [PATCH 7/7] Address review comments of #774 --- src/dotty/tools/dotc/transform/Constructors.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala index 2f78eb9c60d5..265ad32174d4 100644 --- a/src/dotty/tools/dotc/transform/Constructors.scala +++ b/src/dotty/tools/dotc/transform/Constructors.scala @@ -47,10 +47,10 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor private def markUsedPrivateSymbols(tree: RefTree)(implicit ctx: Context): Unit = { val sym = tree.symbol - def retain = + def retain() = retainedPrivateVals.add(sym) - if (mightBeDropped(sym) && sym.owner.isClass) { + if (sym.exists && sym.owner.isClass && mightBeDropped(sym)) { val owner = sym.owner.asClass tree match { @@ -63,8 +63,8 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor // used inside constructor, accessed on this, // could use constructor argument instead, no need to retain field } - else retain - case _ => retain + else retain() + case _ => retain() } } }