From c637535ea976ff9ee82fea2e17c02309bf72f948 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Jul 2017 17:18:24 +0200 Subject: [PATCH 1/4] Fix #2869: Have constructors run after group of Memoize As explained in the code comment, the two get too entangled otherwise which leads to parameters being stored as class fields when they should not. --- compiler/src/dotty/tools/dotc/Compiler.scala | 4 +-- .../tools/dotc/transform/Constructors.scala | 31 +++++++++++++------ tests/run/capturing.check | 1 + tests/run/capturing.scala | 9 ++++++ 4 files changed, 34 insertions(+), 11 deletions(-) create mode 100644 tests/run/capturing.check create mode 100644 tests/run/capturing.scala diff --git a/compiler/src/dotty/tools/dotc/Compiler.scala b/compiler/src/dotty/tools/dotc/Compiler.scala index 4566aaad486a..381fb3b7869d 100644 --- a/compiler/src/dotty/tools/dotc/Compiler.scala +++ b/compiler/src/dotty/tools/dotc/Compiler.scala @@ -87,8 +87,8 @@ class Compiler { new LazyVals, // Expand lazy vals new Memoize, // Add private fields to getters and setters new NonLocalReturns, // Expand non-local returns - new CapturedVars, // Represent vars captured by closures as heap objects - new Constructors, // Collect initialization code in primary constructors + new CapturedVars), // Represent vars captured by closures as heap objects + List(new Constructors, // Collect initialization code in primary constructors // Note: constructors changes decls in transformTemplate, no InfoTransformers should be added after it new FunctionalInterfaces, // Rewrites closures to implement @specialized types of Functions. new GetClass, // Rewrites getClass calls on primitive types. diff --git a/compiler/src/dotty/tools/dotc/transform/Constructors.scala b/compiler/src/dotty/tools/dotc/transform/Constructors.scala index f4c800e342e7..f1d9b40286ba 100644 --- a/compiler/src/dotty/tools/dotc/transform/Constructors.scala +++ b/compiler/src/dotty/tools/dotc/transform/Constructors.scala @@ -30,7 +30,24 @@ class Constructors extends MiniPhaseTransform with IdentityDenotTransformer { th import tpd._ override def phaseName: String = "constructors" - override def runsAfter: Set[Class[_ <: Phase]] = Set(classOf[Memoize], classOf[HoistSuperArgs]) + override def runsAfter: Set[Class[_ <: Phase]] = Set(classOf[HoistSuperArgs]) + override def runsAfterGroupsOf: Set[Class[_ <: Phase]] = Set(classOf[Memoize]) + // Memoized needs to be finished because we depend on the ownerchain after Memoize + // when checking whether an ident is an access in a constructor or outside it. + // This test is done in the right-hand side of a value definition. If Memoize + // was in the same group as Constructors, the test on the rhs ident would be + // performed before the rhs undergoes the owner change. This would lead + // to more symbls being retained as parameters. Test case is + // + // dotc tests/pos/captuing.scala -Xprint:constr + // + // `sf` should not be retained as a filed of class `MT`. + + /** The private vals that are known to be retained as class fields */ + private val retainedPrivateVals = mutable.Set[Symbol]() + + /** The private vals whose definition comes before the current focus */ + private val seenPrivateVals = mutable.Set[Symbol]() // Collect all private parameter accessors and value definitions that need // to be retained. There are several reasons why a parameter accessor or @@ -40,14 +57,10 @@ class Constructors extends MiniPhaseTransform with IdentityDenotTransformer { th // 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 val retainedPrivateVals = mutable.Set[Symbol]() - private val seenPrivateVals = mutable.Set[Symbol]() - private def markUsedPrivateSymbols(tree: RefTree)(implicit ctx: Context): Unit = { val sym = tree.symbol - def retain() = - retainedPrivateVals.add(sym) + def retain() = retainedPrivateVals.add(sym) if (sym.exists && sym.owner.isClass && mightBeDropped(sym)) { val owner = sym.owner.asClass @@ -58,7 +71,8 @@ class Constructors extends MiniPhaseTransform with IdentityDenotTransformer { th val method = ctx.owner.enclosingMethod method.isPrimaryConstructor && ctx.owner.enclosingClass == owner } - if (inConstructor && (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) { + if (inConstructor && + (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) { // used inside constructor, accessed on this, // could use constructor argument instead, no need to retain field } @@ -79,8 +93,7 @@ class Constructors extends MiniPhaseTransform with IdentityDenotTransformer { th } 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 + if (mightBeDropped(tree.symbol)) seenPrivateVals += tree.symbol tree } diff --git a/tests/run/capturing.check b/tests/run/capturing.check new file mode 100644 index 000000000000..66aa5dce5a1d --- /dev/null +++ b/tests/run/capturing.check @@ -0,0 +1 @@ +private final java.lang.String MT.s diff --git a/tests/run/capturing.scala b/tests/run/capturing.scala new file mode 100644 index 000000000000..7f00a8322129 --- /dev/null +++ b/tests/run/capturing.scala @@ -0,0 +1,9 @@ +class MT(sf: MT => String) { + // `s` is retained as a field, but `sf` should not be. + val s = sf(this) +} +object Test extends App { + def printFields(obj: Any) = + println(obj.getClass.getDeclaredFields.map(_.toString).sorted.deep.mkString("\n")) + printFields(new MT(_ => "")) +} From 97d7bbf3a24b7a1a264f4db5ee9c741de1e62bc7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Jul 2017 17:44:56 +0200 Subject: [PATCH 2/4] Update check file It turns out with the fix to Constructors we don't generate the a value for the intermediate field anymore. This seems great! (scalac does generate the field, but it looks redundant) --- tests/run/variable-pattern-access.check | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/run/variable-pattern-access.check b/tests/run/variable-pattern-access.check index 8ab43375fc97..cfc1a6110516 100644 --- a/tests/run/variable-pattern-access.check +++ b/tests/run/variable-pattern-access.check @@ -1,7 +1,6 @@ # Fields of A: private final int A.a private final int A.b -private final scala.Tuple2 A.$1$ # Methods of A: public int A.a() public int A.b() From b865cee256c5aa9867a9ef21ffd3bd6f714dde4d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Jul 2017 18:53:21 +0200 Subject: [PATCH 3/4] Fix typos --- compiler/src/dotty/tools/dotc/transform/Constructors.scala | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Constructors.scala b/compiler/src/dotty/tools/dotc/transform/Constructors.scala index f1d9b40286ba..4698930691e2 100644 --- a/compiler/src/dotty/tools/dotc/transform/Constructors.scala +++ b/compiler/src/dotty/tools/dotc/transform/Constructors.scala @@ -37,11 +37,7 @@ class Constructors extends MiniPhaseTransform with IdentityDenotTransformer { th // This test is done in the right-hand side of a value definition. If Memoize // was in the same group as Constructors, the test on the rhs ident would be // performed before the rhs undergoes the owner change. This would lead - // to more symbls being retained as parameters. Test case is - // - // dotc tests/pos/captuing.scala -Xprint:constr - // - // `sf` should not be retained as a filed of class `MT`. + // to more symbols being retained as parameters. Test case in run/capturing.scala. /** The private vals that are known to be retained as class fields */ private val retainedPrivateVals = mutable.Set[Symbol]() From daef8ecb2291380b5636691d6b7ff0e37d58c1e4 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Jul 2017 20:00:20 +0200 Subject: [PATCH 4/4] Update runsAfter info for LambdaLift I verified experimentally that LamdaLift and Constructors cannot run in the same group. --- compiler/src/dotty/tools/dotc/transform/LambdaLift.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala b/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala index 17772df9b91c..0cb4f3e208f2 100644 --- a/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -66,7 +66,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform override def relaxedTyping = true - override def runsAfter: Set[Class[_ <: Phase]] = Set(classOf[Constructors], classOf[HoistSuperArgs]) + override def runsAfterGroupsOf: Set[Class[_ <: Phase]] = Set(classOf[Constructors], classOf[HoistSuperArgs]) // Constructors has to happen before LambdaLift because the lambda lift logic // becomes simpler if it can assume that parameter accessors have already been // converted to parameters in super calls. Without this it is very hard to get