Move private fields into constructor

Private fields that are accessed only from the constructor,
and are accessed only after they are properly initialized are now
moved into the constructor. This avoids creating a redundant objetc field.
Good example: gcd in Rationals (see constrs.scala).

3 files changed, 193 insertions, 72 deletions

diff --git a/src/dotty/tools/dotc/transform/Constructors.scala b/src/dotty/tools/dotc/transform/Constructors.scala
index 3d2f5a0d7..9cb348dff 100644
--- a/src/dotty/tools/dotc/transform/Constructors.scala
+++ b/src/dotty/tools/dotc/transform/Constructors.scala
@@ -21,6 +21,8 @@ import collection.mutable
 /** This transform
  *   - moves initializers from body to constructor.
  *   - makes all supercalls explicit
+ *   - also moves private fields that are accessed only from constructor
+ *     into the constructor if possible.
  */
 class Constructors extends MiniPhaseTransform with SymTransformer { thisTransform =>
   import tpd._
@@ -35,7 +37,7 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor
    */
   override def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = {
     def ownerBecomesConstructor(owner: Symbol): Boolean =
-      (owner.isLocalDummy || owner.isTerm && !owner.is(Method)) &&
+      (owner.isLocalDummy || owner.isTerm && !owner.is(Method | Lazy)) &&
       owner.owner.isClass
     if (ownerBecomesConstructor(sym.owner))
       sym.copySymDenotation(owner = sym.owner.enclosingClass.primaryConstructor)
@@ -48,38 +50,14 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor
   private def noDirectRefsFrom(tree: Tree)(implicit ctx: Context) =
     tree.isDef && tree.symbol.isClass && !tree.symbol.is(InSuperCall)
 
-  /** Adjustments performed when moving code into the constructor:
-   *  (1) Replace references to param accessors by constructor parameters
-   *      except possibly references to mutable variables, if `excluded = Mutable`.
-   *      (Mutable parameters should be replaced only during the super call)
-   *  (2) If the parameter accessor reference was to an alias getter,
-   *      drop the () when replacing by the parameter.
+  /** Class members that can be eliminated if referenced only from their own
+   *  constructor.
    */
-  class IntoConstrMap(accessors: List[Symbol], params: List[Symbol]) extends TreeMap {
-    private var excluded: FlagSet = _
-    override def transform(tree: Tree)(implicit ctx: Context): Tree = tree match {
-      case Ident(_) | Select(This(_), _) =>
-        val sym = tree.symbol
-        if (sym is (ParamAccessor, butNot = excluded)) {
-          val param = sym.subst(accessors, params)
-          if (param ne sym) ref(param).withPos(tree.pos)
-          else tree
-        }
-        else tree
-      case Apply(fn, Nil) =>
-        val fn1 = transform(fn)
-        if ((fn1 ne fn) && fn1.symbol.is(Param) && fn1.symbol.owner.isPrimaryConstructor)
-          fn1 // in this case, fn1.symbol was an alias for a parameter in a superclass
-        else cpy.Apply(tree)(fn1, Nil)
-      case _ =>
-        if (noDirectRefsFrom(tree)) tree else super.transform(tree)
-    }
+  private def mightBeDropped(sym: Symbol)(implicit ctx: Context) =
+    sym.is(Private, butNot = KeeperFlags) && !sym.is(MutableParamAccessor)
 
-    def apply(tree: Tree, excluded: FlagSet)(implicit ctx: Context): Tree = {
-      this.excluded = excluded
-      transform(tree)
-    }
-  }
+  private final val KeeperFlags = Method | Lazy | NotJavaPrivate
+  private final val MutableParamAccessor = allOf(Mutable, ParamAccessor)
 
   override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo): Tree = {
     val cls = ctx.owner.asClass
@@ -102,7 +80,34 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor
     }
     val paramSyms = vparamsWithOuter map (_.symbol)
 
-    val intoConstr = new IntoConstrMap(accessors, paramSyms)
+    // Adjustments performed when moving code into the constructor:
+    //  (1) Replace references to param accessors by constructor parameters
+    //      except possibly references to mutable variables, if `excluded = Mutable`.
+    //      (Mutable parameters should be replaced only during the super call)
+    //  (2) If the parameter accessor reference was to an alias getter,
+    //      drop the () when replacing by the parameter.
+    object intoConstr extends TreeMap {
+      private var excluded: FlagSet = _
+      override def transform(tree: Tree)(implicit ctx: Context): Tree = tree match {
+        case Ident(_) | Select(This(_), _) =>
+          var sym = tree.symbol
+          if (sym is (ParamAccessor, butNot = excluded)) sym = sym.subst(accessors, paramSyms)
+          if (sym.owner.isConstructor) ref(sym).withPos(tree.pos) else tree
+        case Apply(fn, Nil) =>
+          val fn1 = transform(fn)
+          if ((fn1 ne fn) && fn1.symbol.is(Param) && fn1.symbol.owner.isPrimaryConstructor)
+            fn1 // in this case, fn1.symbol was an alias for a parameter in a superclass
+          else cpy.Apply(tree)(fn1, Nil)
+        case _ =>
+          if (noDirectRefsFrom(tree)) tree else super.transform(tree)
+      }
+
+      def apply(tree: Tree, inSuperCall: Boolean = false)(implicit ctx: Context): Tree = {
+        this.excluded = if (inSuperCall) EmptyFlags else Mutable
+        transform(tree)
+      }
+    }
+
     val superCalls = new mutable.ListBuffer[Tree]
 
     // If parent is a constructor call, pull out the call into a separate
@@ -115,12 +120,12 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor
           nme.CONSTRUCTOR),
         superArgs) =>
         val toClass = !superType.symbol.is(Trait)
-        val mappedArgs = superArgs.map(
-          intoConstr(_, excluded = if (toClass) Mutable else EmptyFlags))
+        val mappedArgs = superArgs.map(intoConstr(_, inSuperCall = toClass))
         val receiver =
           if (toClass) Super(This(cls), tpnme.EMPTY, inConstrCall = true)
           else This(cls)
-        superCalls += cpy.Apply(superApp)(
+        superCalls +=
+          cpy.Apply(superApp)(
             receiver.withPos(superNew.pos)
               .select(superSel.symbol).withPos(superSel.pos),
             mappedArgs)
@@ -129,67 +134,97 @@ class Constructors extends MiniPhaseTransform with SymTransformer { thisTransfor
     }
     val parentTypeTrees = tree.parents.map(normalizeParent)
 
+    // 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) = {
+        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(superCalls.toList ++ tree.body)
+
+    def isRetained(acc: Symbol) = !mightBeDropped(acc) || usage.retained(acc)
+
+    val constrStats, clsStats = new mutable.ListBuffer[Tree]
+
     // Split class body into statements that go into constructor and
     // definitions that are kept as members of the class.
-    def splitStats(stats: List[Tree]): (List[Tree], List[Tree]) = stats match {
+    def splitStats(stats: List[Tree]): Unit = stats match {
       case stat :: stats1 =>
-        val (constrStats, clsStats) = splitStats(stats1)
         stat match {
-          case stat @ ValDef(mods, name, tpt, rhs) =>
-            val inits =
-              if (rhs.isEmpty || isWildcardArg(rhs)) Nil
-              else Assign(ref(stat.symbol), intoConstr(rhs, excluded = Mutable))
-                .withPos(stat.pos) :: Nil
-            (inits ::: constrStats, cpy.ValDef(stat)(rhs = EmptyTree) :: clsStats)
+          case stat @ ValDef(mods, name, tpt, rhs) if !stat.symbol.is(Lazy) =>
+            val sym = stat.symbol
+            if (isRetained(sym)) {
+              if (!rhs.isEmpty && !isWildcardArg(rhs))
+                constrStats += Assign(ref(sym), intoConstr(rhs)).withPos(stat.pos)
+              clsStats += cpy.ValDef(stat)(rhs = EmptyTree)
+            }
+            else if (!rhs.isEmpty) {
+              sym.copySymDenotation(
+                initFlags = sym.flags &~ Private,
+                owner = constr.symbol).installAfter(thisTransform)
+              constrStats += intoConstr(stat)
+            }
           case _: DefTree =>
-            (constrStats, stat :: clsStats)
+            clsStats += stat
           case _ =>
-            (intoConstr(stat, excluded = Mutable) :: constrStats, clsStats)
+            constrStats += intoConstr(stat)
         }
+        splitStats(stats1)
       case Nil =>
         (Nil, Nil)
     }
-    val (constrStats, clsStats) = splitStats(tree.body)
-
-    // Collect all private parameter accessors that need to be retained
-    // because they are accessed after the constructor has finished.
-    val collectRetained = new TreeAccumulator[Set[Symbol]] {
-      override def apply(retained: Set[Symbol], tree: Tree) = tree match {
-        case tree: RefTree =>
-          val sym = tree.symbol
-          foldOver(
-            if (sym.is(PrivateParamAccessor) && sym.owner == cls) retained + sym else retained,
-            tree)
-        case _ =>
-          if (noDirectRefsFrom(tree)) retained else foldOver(retained, tree)
-      }
-    }
-    val retainedPrivate = collectRetained(collectRetained(Set[Symbol](), constrStats), clsStats)
-    def isRetained(acc: Symbol) =
-      (!acc.is(Private) || acc.is(NotJavaPrivate) || retainedPrivate(acc))
+    splitStats(tree.body)
 
     val accessorFields = accessors.filterNot(_ is Method)
-    val (retainedAccessors, droppedAccessors) = accessorFields.partition(isRetained)
 
     // The initializers for the retained accessors */
-    val copyParams = retainedAccessors.map(acc =>
+    val copyParams = accessorFields.filter(isRetained).map(acc =>
       Assign(ref(acc), ref(acc.subst(accessors, paramSyms))).withPos(tree.pos))
 
     // Drop accessors that are not retained from class scope
-    if (droppedAccessors.nonEmpty) {
+    val dropped = usage.dropped
+    if (dropped.nonEmpty) {
       val clsInfo = cls.classInfo // TODO investigate: expand clsInfo to cls.info => dotty type error
       cls.copy(
         info = clsInfo.derivedClassInfo(
-          decls = clsInfo.decls.filteredScope(!droppedAccessors.contains(_))))
+          decls = clsInfo.decls.filteredScope(!dropped.contains(_))))
     }
 
     cpy.Template(tree)(
       constr = cpy.DefDef(constr)(
-        rhs = Block(superCalls.toList ::: copyParams ::: constrStats, unitLiteral)),
+        rhs = Block(superCalls.toList ::: copyParams ::: constrStats.toList, unitLiteral)),
       parents = parentTypeTrees,
-      body = clsStats filter {
-        case vdef: ValDef => !droppedAccessors.contains(vdef.symbol)
-        case _ => true
-      })
+      body = clsStats.toList)
   }
 }
\ No newline at end of file
diff --git a/tests/pos/Fileish.scala b/tests/pos/Fileish.scala
new file mode 100644
index 000000000..d226bb0bd
--- /dev/null
+++ b/tests/pos/Fileish.scala
@@ -0,0 +1,53 @@
+// Inspired by the original Fileish,
+// testing combinations of lazy and non-lazy vals for their treatment in constructors
+package dotty.tools
+package io
+
+import java.io.{ InputStream }
+import java.util.jar.JarEntry
+import language.postfixOps
+
+/** A common interface for File-based things and Stream-based things.
+ *  (In particular, io.File and JarEntry.)
+ */
+class Fileish(val path: Path, val input: () => InputStream) extends Streamable.Chars {
+  def inputStream() = input()
+
+  def parent       = path.parent
+  def name         = path.name
+  def isSourceFile = path.hasExtension("java", "scala")
+
+  private lazy val pkgLines = lines() collect { case x if x startsWith "package " => x stripPrefix "package" trim }
+  lazy val pkgFromPath      = parent.path.replaceAll("""[/\\]""", ".")
+  lazy val pkgFromSource    = pkgLines map (_ stripSuffix ";") mkString "."
+
+  override def toString = path.path
+}
+class Fileish2(val path: Path, val input: () => InputStream) extends Streamable.Chars {
+  def inputStream() = input()
+
+  def parent       = path.parent
+  def name         = path.name
+  def isSourceFile = path.hasExtension("java", "scala")
+
+  private val pkgLines = lines() collect { case x if x startsWith "package " => x stripPrefix "package" trim }
+  lazy val pkgFromPath      = parent.path.replaceAll("""[/\\]""", ".")
+  lazy val pkgFromSource    = pkgLines map (_ stripSuffix ";") mkString "."
+
+  override def toString = path.path
+}
+
+class Fileish3(val path: Path, val input: () => InputStream) extends Streamable.Chars {
+  def inputStream() = input()
+
+  def parent       = path.parent
+  def name         = path.name
+  def isSourceFile = path.hasExtension("java", "scala")
+
+  private val pkgLines = lines() collect { case x if x startsWith "package " => x stripPrefix "package" trim }
+  private val pkgFromPath      = parent.path.replaceAll("""[/\\]""", ".")
+  private val pkgFromSource    = pkgLines map (_ stripSuffix ";") mkString "."
+
+  override def toString = path.path
+}
+
diff --git a/tests/pos/constrs.scala b/tests/pos/constrs.scala
new file mode 100644
index 000000000..dc0e1a369
--- /dev/null
+++ b/tests/pos/constrs.scala
@@ -0,0 +1,33 @@
+class Foo(x: Int, var y: Int) {
+
+  val z: Int = 0
+
+  var u: Int = _
+
+  def f = x
+
+}
+
+class Baz(val base: Int) {
+
+}
+
+
+class Bar(base: Int, byName: => String, local: Int) extends Baz(base + local) {
+
+  def f() = println(base.toString + byName)
+
+}
+
+class Rational(n: Int, d: Int) {
+  def gcd(x: Int, y: Int): Int = ???
+  private val x = gcd(n, d)
+  def numer = n / x
+  def denom = d / x
+}
+class Rational2(n: Int, d: Int) {
+  def gcd(x: Int, y: Int): Int = ???
+  private val x = gcd(n, d)
+  val numer = n / x
+  val denom = d / x
+}