Merge pull request #2870 from dotty-staging/fix-param-captures

Fix #2869: Have constructors run after group of Memoize

Author: odersky

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.

compiler/src/dotty/tools/dotc/transform/Constructors.scala

@@ -30,7 +30,20 @@ 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 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]()
+
+  /** 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 +53,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 +67,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 +89,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
   }
 

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

tests/run/capturing.check

@@ -0,0 +1 @@
+private final java.lang.String MT.s

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(_ => ""))
+}

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()