Move private fields into constructor

odersky · odersky · commit 74528b959f52 · 2014-09-24T14:26:29.000+02:00
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).
diff --git 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)
   }
 }
diff --git a/tests/pos/Fileish.scala 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
@@ -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
+}
