constructors... what a mess

adriaanm · adriaanm · commit 51f59df2e432 · 2015-08-21T17:58:36.000-07:00
diff --git a/src/compiler/scala/tools/nsc/transform/Constructors.scala b/src/compiler/scala/tools/nsc/transform/Constructors.scala
@@ -417,9 +417,9 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
         }
 
         if (stat1 eq stat) {
-          assert(ctorParams(genericClazz).length == constrInfo.constrParams.length)
+          assert(ctorParams(genericClazz).length == primaryConstrParams.length)
           // this is just to make private fields public
-          (new specializeTypes.ImplementationAdapter(ctorParams(genericClazz), constrInfo.constrParams, null, true))(stat1)
+          (new specializeTypes.ImplementationAdapter(ctorParams(genericClazz), primaryConstrParams, null, true))(stat1)
 
           val stat2 = rewriteArrayUpdate(stat1)
           // statements coming from the original class need retyping in the current context
@@ -456,16 +456,16 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
       //   postfix = postfix.tail
       // }
 
-      if (shouldGuard && usesSpecializedField && stats.nonEmpty) {
+      if (guardSpecializedFieldInit && usesSpecializedField && stats.nonEmpty) {
         // save them for duplication in the specialized subclass
         guardedCtorStats(clazz) = stats
-        ctorParams(clazz) = constrInfo.constrParams
+        ctorParams(clazz) = primaryConstrParams
 
         val tree =
           If(
             Apply(
               CODE.NOT (
-               Apply(gen.mkAttributedRef(specializedFlag), List())),
+               Apply(gen.mkAttributedRef(hasSpecializedFieldsSym), List())),
               List()),
             Block(stats, Literal(Constant(()))),
             EmptyTree)
@@ -493,32 +493,23 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
     with    OmittablesHelper
     with    GuardianOfCtorStmts {
 
-    val clazz = impl.symbol.owner  // the transformed class
-    val inTrait = clazz.isTrait
-    val stats = impl.body          // the transformed template body
+    val clazz      = impl.symbol.owner  // the transformed class
+    val inTrait    = clazz.isTrait
+    val stats      = impl.body          // the transformed template body
     val localTyper = typer.atOwner(impl, clazz)
 
-    val specializedFlag: Symbol = clazz.info.decl(nme.SPECIALIZED_INSTANCE)
-    val shouldGuard = (specializedFlag != NoSymbol) && !clazz.hasFlag(SPECIALIZED)
+    val hasSpecializedFieldsSym   = clazz.info.decl(nme.SPECIALIZED_INSTANCE)
+    // The constructor of non-specialized classes with specialized subclasses should guard
+    // the initialization of specialized fields on the result of the `hasSpecializedFieldsSym` method.
+    val guardSpecializedFieldInit = !((hasSpecializedFieldsSym == NoSymbol) || clazz.hasFlag(SPECIALIZED))
 
-    val isDelayedInitSubclass = (clazz isSubClass DelayedInitClass)
+    val isDelayedInitSubclass = clazz isSubClass DelayedInitClass
 
-    case class ConstrInfo(
-      constr: DefDef,               // The primary constructor
-      constrParams: List[Symbol],   // ... and its parameters
-      constrBody: Block             // ... and its body
-    )
     // decompose primary constructor into the three entities above.
-    val constrInfo: ConstrInfo = {
-      val ddef = (stats find (_.symbol.isPrimaryConstructor))
-      ddef match {
-        case Some(ddef @ DefDef(_, _, _, List(vparams), _, rhs @ Block(_, _))) =>
-          ConstrInfo(ddef, vparams map (_.symbol), rhs)
-        case x =>
-          abort("no constructor in template: impl = " + impl)
-      }
-    }
-    import constrInfo._
+    val (primaryConstr, primaryConstrParams, primaryConstrBody) = stats collectFirst {
+      case dd@ DefDef(_, _, _, vps :: Nil, _, rhs: Block) if dd.symbol.isPrimaryConstructor => (dd, vps map (_.symbol), rhs)
+    } getOrElse abort("no constructor in template: impl = " + impl)
+
 
     // The parameter accessor fields which are members of the class
     val paramAccessors = clazz.constrParamAccessors
@@ -531,19 +522,19 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
     def parameterNamed(name: Name): Symbol = {
       def matchesName(param: Symbol) = param.name == name || param.name.startsWith(name + nme.NAME_JOIN_STRING)
 
-      (constrParams filter matchesName) match {
-        case Nil    => abort(name + " not in " + constrParams)
+      (primaryConstrParams filter matchesName) match {
+        case Nil    => abort(name + " not in " + primaryConstrParams)
         case p :: _ => p
       }
     }
 
     /*
      * `usesSpecializedField` makes a difference in deciding whether constructor-statements
-     * should be guarded in a `shouldGuard` class, ie in a class that's the generic super-class of
+     * should be guarded in a `guardSpecializedFieldInit` class, ie in a class that's the generic super-class of
      * one or more specialized sub-classes.
      *
      * Given that `usesSpecializedField` isn't read for any other purpose than the one described above,
-     * we skip setting `usesSpecializedField` in case the current class isn't `shouldGuard` to start with.
+     * we skip setting `usesSpecializedField` in case the current class isn't `guardSpecializedFieldInit` to start with.
      * That way, trips to a map in `specializeTypes` are saved.
      */
     var usesSpecializedField: Boolean = false
@@ -588,10 +579,8 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
           // references to parameter accessor field of own class become references to parameters
           gen.mkAttributedIdent(parameter(tree.symbol)) setPos tree.pos
 
-        case Select(_, _) if shouldGuard => // reasoning behind this guard in the docu of `usesSpecializedField`
-          if (possiblySpecialized(tree.symbol)) {
-            usesSpecializedField = true
-          }
+        case Select(_, _) if guardSpecializedFieldInit && possiblySpecialized(tree.symbol) => // reasoning behind this guard in the docu of `usesSpecializedField`
+          usesSpecializedField = true
           super.transform(tree)
 
         case _ =>
@@ -604,7 +593,7 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
 
     // Move tree into constructor, take care of changing owner from `oldowner` to constructor symbol
     def intoConstructor(oldowner: Symbol, tree: Tree) =
-      intoConstructorTransformer transform tree.changeOwner(oldowner -> constr.symbol)
+      intoConstructorTransformer transform tree.changeOwner(oldowner -> primaryConstr.symbol)
 
     private def needsCtorEffect(stat: ValOrDefDef): Boolean = !(stat.rhs == EmptyTree || stat.symbol.isLazy)
 
@@ -685,7 +674,7 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
     val classInitStatBuf = new ListBuffer[Tree]
 
     // generate code to copy pre-initialized fields
-    for (stat <- constrBody.stats) {
+    for (stat <- primaryConstrBody.stats) {
       constrStatBuf += stat
       stat match {
         case ValDef(mods, name, _, _) if (mods hasFlag PRESUPER) =>
@@ -700,57 +689,57 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
     }
 
     // Triage all template definitions to go into defBuf/auxConstructorBuf, constrStatBuf, or constrPrefixBuf.
-    for (stat <- stats) stat match {
-      case dd@ DefDef(_,_,_,_,_,rhs) =>
-        // methods with constant result type get literals as their body
+
+    for (stat <- stats) {
+      val sym = stat.symbol
+      stat match {
+        // TODO what is this doing in Constructors? I'd expect it much earlier -- erasure? uncurry?
+        case dd@ DefDef(_,_,_,_,_,rhs) if sym.info.params.isEmpty && sym.info.resultType.isInstanceOf[ConstantType] =>
+          defBuf += deriveDefDef(dd)(rhs => gen.mkAttributedQualifier(sym.info.resultType) setPos rhs.pos )
+
         // all methods except the primary constructor go into template
-        stat.symbol.tpe match {
-          case MethodType(List(), tp @ ConstantType(c)) =>
-            defBuf += deriveDefDef(stat)(Literal(c) setPos _.pos setType tp)
-          case _ =>
-            if (stat.symbol.isPrimaryConstructor) ()
-            else if (stat.symbol.isConstructor) auxConstructorBuf += stat
-            // a concrete getter's RHS is treated like a ValDef (the actual field isn't emitted until Mixin augments the class that inherits the trait)
-            else if (inTrait) {
-              // We can't mark as deferred from the start, as it will be implemented automatically in the subclass
-              // it should be marked deferred in bytecode, though
-
-              var stat_memoizedGetterLosesRhs = stat
-
-              // a trait getter with an empty RHS is marked deferred from the start
-              if (stat.symbol.isGetter && (rhs ne EmptyTree)) {
+        case _: DefDef if sym.isConstructor => if (sym ne primaryConstr.symbol) auxConstructorBuf += stat
+
+        // a concrete getter's RHS is treated like a ValDef (the actual field isn't emitted until Mixin augments the class that inherits the trait)
+        // We can't mark as deferred from the start, as it will be implemented automatically in the subclass
+        // it should be marked deferred in bytecode, though
+        case dd@ DefDef(_,_,_,_,_,rhs) if inTrait && sym.isAccessor =>
+          // a trait getter with an empty RHS is marked deferred from the start
+          defBuf +=
+            if (sym.isGetter) {
+              if (rhs ne EmptyTree) {
                 val memoized = moveEffectToCtor(dd)
                 if (memoized) {
-                  stat.symbol setFlag (DEFERRED | lateDEFERRED)
-                  stat_memoizedGetterLosesRhs = deriveDefDef(dd)(_ => EmptyTree)
-                }
-              } else if (stat.symbol.isSetter) {
-                stat.symbol setFlag (DEFERRED | lateDEFERRED)
-              }
-
-              defBuf += stat_memoizedGetterLosesRhs
+                  sym setFlag (DEFERRED | lateDEFERRED)
+                  deriveDefDef(dd)(_ => EmptyTree)
+                } else stat
+              } else stat
+            } else {
+              sym setFlag (DEFERRED | lateDEFERRED)
+              stat
             }
-            else defBuf += stat
-        }
-      case vd@ ValDef(mods, _, _, rhs) if !mods.hasStaticFlag =>
-        // Move field's effect into the constructor (usually an assignment of rhs to field,
-        // but it could also be the side-effect of the rhs when no storage is needed).
-        val needsField = moveEffectToCtor(vd)
-
-        // Only emit fields that actually need to be stored
-        if (needsField) defBuf += deriveValDef(stat)(_ => EmptyTree)
-
-      case ValDef(_, _, _, rhs) =>
-        // Add static initializer statements to classInitStatBuf and remove the rhs from the val def.
-        classInitStatBuf += mkAssign(stat.symbol, rhs)
-        defBuf += deriveValDef(stat)(_ => EmptyTree)
-
-      case ClassDef(_, _, _, _) =>
-        // classes are treated recursively, and left in the template
-        defBuf += new ConstructorTransformer(unit).transform(stat)
-      case _ =>
-        // all other statements go into the constructor
-        constrStatBuf += intoConstructor(impl.symbol, stat)
+
+        case _: DefDef => defBuf += stat
+        case vd@ ValDef(mods, _, _, rhs) if !mods.hasStaticFlag =>
+          // Move field's effect into the constructor (usually an assignment of rhs to field,
+          // but it could also be the side-effect of the rhs when no storage is needed).
+          val needsField = moveEffectToCtor(vd)
+
+          // Only emit fields that actually need to be stored
+          if (needsField) defBuf += deriveValDef(stat)(_ => EmptyTree)
+
+        case ValDef(_, _, _, rhs) =>
+          // Add static initializer statements to classInitStatBuf and remove the rhs from the val def.
+          classInitStatBuf += mkAssign(sym, rhs)
+          defBuf += deriveValDef(stat)(_ => EmptyTree)
+
+        case ClassDef(_, _, _, _) =>
+          // classes are treated recursively, and left in the template
+          defBuf += new ConstructorTransformer(unit).transform(stat)
+        case _ =>
+          // all other statements go into the constructor
+          constrStatBuf += intoConstructor(impl.symbol, stat)
+      }
     }
 
     populateOmittables()
@@ -768,7 +757,7 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
       copyParam(acc, parameter(acc))
     }
 
-    /* Return a pair consisting of (all statements up to and including superclass and trait constr calls, rest) */
+    /* Return a pair consisting of (all statements up to and including superclass and trait primaryConstr calls, rest) */
     def splitAtSuper(stats: List[Tree]) = {
       def isConstr(tree: Tree): Boolean = tree match {
         case Block(_, expr) => isConstr(expr)  // SI-6481 account for named argument blocks
@@ -785,12 +774,11 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
     rewriteDelayedInit()
 
     // Assemble final constructor
-    defBuf += deriveDefDef(constr)(_ =>
+    defBuf += deriveDefDef(primaryConstr)(_ =>
       treeCopy.Block(
-        constrBody,
-        paramInits ::: constrPrefixBuf.toList ::: uptoSuperStats :::
-          guardSpecializedInitializer(remainingConstrStats),
-        constrBody.expr))
+        primaryConstrBody,
+        paramInits ::: constrPrefixBuf.toList ::: uptoSuperStats ::: guardSpecializedInitializer(remainingConstrStats),
+        primaryConstrBody.expr))
 
     // Followed by any auxiliary constructors
     defBuf ++= auxConstructorBuf
diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala
@@ -397,42 +397,26 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
 
   //  TODO in namers: if(sym.owner.isTrait && sym.isSetter && !sym.isDeferred) sym.addAnnotation(TraitSetterAnnotationClass)
 
-    /** Add calls to supermixin constructors
-      *    `super[mix].$init$()`
-      *  to tree, which is assumed to be the body of a constructor of class clazz.
-      */
-    private def addMixinConstructorCalls(clazz: Symbol)(tree: Tree): Tree = {
-      def mixinConstructorCall(`trait`: Symbol): Tree = typedPos(tree.pos) {
-        Apply(Select(This(clazz), `trait`.primaryConstructor), List())
-      }
-      val mixinConstructorCalls = clazz.mixinClasses.filter(_.isTrait).map(mixinConstructorCall).reverse
-      tree match {
-          // TODO???
-//        case Block(Nil, expr) =>
-//          // AnyVal constructor - have to provide a real body so the
-//          // jvm doesn't throw a VerifyError. But we can't add the
-//          // body until now, because the typer knows that Any has no
-//          // constructor and won't accept a call to super.init.
-//          assert((clazz isSubClass AnyValClass) || clazz.info.parents.isEmpty, clazz)
-//          Block(List(Apply(gen.mkSuperInitCall, Nil)), expr)
-
-        case Block(stats, expr) =>
-          // needs `hasSymbolField` check because `supercall` could be a block (named / default args)
-          val (presuper, supercall :: rest) = stats span (t => t.hasSymbolWhich(_ hasFlag PRESUPER))
-          treeCopy.Block(tree, presuper ::: (supercall :: mixinConstructorCalls ::: rest), expr)
-      }
-    }
-
     /** The first transform; called in a pre-order traversal at phase mixin
       * (that is, every node is processed before its children).
       *
       * For every non-trait class, add all mixed in members to the class info.
       */
     private def preTransform(tree: Tree): Tree = {
       val sym = tree.symbol
+      val clazz = sym.owner
       tree match {
-        case DefDef(_,_,_,_,_,_) if sym.isClassConstructor && sym.isPrimaryConstructor && sym.owner != ArrayClass =>
-          deriveDefDef(tree)(addMixinConstructorCalls(sym.owner))
+        // Add super-trait constructor calls `super[mix].$init$()` (in reverse-linearization order) to the body of the ctor
+        case DefDef(_,_,_,_,_, Block(_ :: _, _)) if sym.isClassConstructor && sym.isPrimaryConstructor && clazz != ArrayClass =>
+          deriveDefDef(tree) { case tree@Block(stats, expr) =>
+            def typedCtorCall(`trait`: Symbol): Tree =
+              typedPos(expr.pos) { Apply(Select(This(clazz), `trait`.primaryConstructor), List()) }
+
+            // check `hasExistingSymbol` (via `hasSymbolWhich`) because `supercall` could be a block (named / default args)
+            val (preSuper, superCall :: postSuperOrig) = stats span (_ hasSymbolWhich (_ hasFlag PRESUPER))
+            val postSuper = (clazz.mixinClasses map typedCtorCall) reverse_::: postSuperOrig
+            treeCopy.Block(tree, preSuper ::: (superCall :: postSuper), expr)
+          }
 
         case Template(parents, self, body) if doImplementTraitMembers(currentOwner) =>
           localTyper = erasure.newTyper(rootContext.make(tree, currentOwner))
diff --git a/test/files/trait-defaults/fields.scala b/test/files/trait-defaults/fields.scala
