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.

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,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
   }
 

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