Fields phase

One step towards teasing apart the mixin phase, making
each phase that adds members to traits responsible for
mixing in those members into subclasses of said traits.

Another design tenet is to not emit symbols or trees
only to later remove them. Therefore, we model a
val in a trait as its accessor. The underlying field
is an implementation detail. It must be mixed into
subclasses, but has no business in a trait (an interface).

Also trying to reduce tree creation by changing less in subtrees
during tree transforms.

A lot of nice fixes fall out from this rework:

  - Correct bridges and more precise generic signatures for
    mixed in accessors, since they are now created before erasure.

  - Correct enclosing method attribute for classes nested in trait fields.

    Trait fields are now created as MethodSymbol (no longer TermSymbol).
    This symbol shows up in the `originalOwner` chain of a class declared
    within the field initializer. This promoted the field getter to
    being the enclosing method of the nested class, which it is not
    (the EnclosingMethod attribute is a source-level property).

  - Signature inference is now more similar between vals and defs

  - No more field for constant-typed vals, or mixed in accessors
    for subclasses. A constant val can be fully implemented in a trait.

TODO:
  - give same treatment to trait lazy vals (only accessors, no fields)
  - remove support for presuper vals in traits
    (they don't have the right init semantics in traits anyway)
  - lambdalift should emit accessors for captured vals in traits,
    not a field

Assorted notes from the full git history before squashing below.

Unit-typed vals: don't suppress field

it affects the memory model -- even a write of unit to a field is relevant...
unit-typed lazy vals should never receive a field
this need was unmasked by test/files/run/t7843-jsr223-service.scala,
which no longer printed the output expected from the `0 to 10 foreach`

Use getter.referenced to track traitsetter

reify's toolbox compiler changes the name of the trait
that owns the accessor between fields and constructors (`$` suffix),
so that the trait setter cannot be found when doing mkAssign in constructors
this could be solved by creating the mkAssign tree immediately during fields

anyway, first experiment: use `referenced` now that fields runs closer
to the constructors phase (I tried this before and something broke)

Infer result type for `val`s, like we do for `def`s

The lack of result type inference caused pos/t6780 to fail
in the new field encoding for traits, as there is no separate accessor,
and method synthesis computes the type signature based on the ValDef tree.
This caused a cyclic error in implicit search, because now the
implicit val's result type was not inferred from the super member,
and inferring it from the RHS would cause implicit search to consider
the member in question, so that a cycle is detected and type checking fails...

Regardless of the new encoding, we should consistently infer result types
for `def`s and `val`s.

Removed test/files/run/t4287inferredMethodTypes.scala and test/files/presentation/t4287c,
since they were relying on inferring argument types from "overridden" constructors
in a test for range positions of default arguments. Constructors don't override,
so that was a mis-feature of -Yinfer-argument-types.

Had to slightly refactor test/files/presentation/doc, as it was relying
on scalac inferring a big intersection type to approximate the anonymous
class that's instantiated for `override lazy val analyzer`.
Now that we infer `Global` as the expected type based on the overridden val,
we make `getComment` private in navigating between good old Skylla and Charybdis.
I'm not sure why we need this restriction for anonymous classes though;
only structural calls are restricted in the way that we're trying to avoid.

The old behavior is maintained nder -Xsource:2.11.

Tests:
  - test/files/{pos,neg}/val_infer.scala
  - test/files/neg/val_sig_infer_match.scala
  - test/files/neg/val_sig_infer_struct.scala

need NMT when inferring sig for accessor
Q: why are we calling valDefSig and not methodSig?
A: traits use defs for vals, but still use valDefSig...
keep accessor and field info in synch

1 files changed, 66 insertions, 66 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala
index b5084cffe1..d98daf0ffb 100644
--- a/src/compiler/scala/tools/nsc/transform/Mixin.scala
+++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala
@@ -45,8 +45,8 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
    *     methods in the impl class (because they can have arbitrary initializers)
    */
   private def isImplementedStatically(sym: Symbol) = (
-       sym.isMethod
-    && (!sym.hasFlag(DEFERRED | SUPERACCESSOR) || (sym hasFlag lateDEFERRED))
+    sym.isMethod
+    && notDeferredOrLate(sym)
     && sym.owner.isTrait
     && (!sym.isModule || sym.hasFlag(PRIVATE | LIFTED))
     && (!(sym hasFlag (ACCESSOR | SUPERACCESSOR)) || sym.isLazy)
@@ -109,16 +109,16 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
 
 // --------- type transformation -----------------------------------------------
 
-  def isConcreteAccessor(member: Symbol) =
-    member.hasAccessorFlag && (!member.isDeferred || (member hasFlag lateDEFERRED))
+  private def notDeferredOrLate(sym: Symbol) = !sym.hasFlag(DEFERRED) || sym.hasFlag(lateDEFERRED)
 
   /** Is member overridden (either directly or via a bridge) in base class sequence `bcs`? */
   def isOverriddenAccessor(member: Symbol, bcs: List[Symbol]): Boolean = beforeOwnPhase {
     def hasOverridingAccessor(clazz: Symbol) = {
       clazz.info.nonPrivateDecl(member.name).alternatives.exists(
         sym =>
-          isConcreteAccessor(sym) &&
+          sym.hasFlag(ACCESSOR) &&
           !sym.hasFlag(MIXEDIN) &&
+          notDeferredOrLate(sym) &&
           matchesType(sym.tpe, member.tpe, alwaysMatchSimple = true))
     }
     (    bcs.head != member.owner
@@ -126,6 +126,8 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
     )
   }
 
+  private def isUnitGetter(sym: Symbol) = sym.tpe.resultType.typeSymbol == UnitClass
+
   /** Add given member to given class, and mark member as mixed-in.
    */
   def addMember(clazz: Symbol, member: Symbol): Symbol = {
@@ -202,6 +204,8 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
 
       clazz.info // make sure info is up to date, so that implClass is set.
 
+      // TODO: is this needed? can there be fields in a class that don't have accessors yet but need them???
+      // can we narrow this down to just getters for lazy vals? param accessors?
       for (member <- clazz.info.decls) {
         if (!member.isMethod && !member.isModule && !member.isModuleVar) {
           assert(member.isTerm && !member.isDeferred, member)
@@ -297,49 +301,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
     def mixinTraitMembers(mixinClass: Symbol) {
       // For all members of a trait's interface do:
       for (mixinMember <- mixinClass.info.decls) {
-        if (isConcreteAccessor(mixinMember)) {
-          if (isOverriddenAccessor(mixinMember, clazz.info.baseClasses))
-            devWarning(s"Overridden concrete accessor: ${mixinMember.fullLocationString}")
-          else {
-            // mixin field accessors
-            val mixedInAccessor = cloneAndAddMixinMember(mixinClass, mixinMember)
-            if (mixinMember.isLazy) {
-              initializer(mixedInAccessor) = (
-                mixinClass.info.decl(mixinMember.name)
-                  orElse abort("Could not find initializer for " + mixinMember.name)
-              )
-            }
-            if (!mixinMember.isSetter)
-              mixinMember.tpe match {
-                case MethodType(Nil, ConstantType(_)) =>
-                  // mixinMember is a constant; only getter is needed
-                  ;
-                case MethodType(Nil, TypeRef(_, UnitClass, _)) =>
-                  // mixinMember is a value of type unit. No field needed
-                  ;
-                case _ => // otherwise mixin a field as well
-                  // enteringPhase: the private field is moved to the implementation class by erasure,
-                  // so it can no longer be found in the mixinMember's owner (the trait)
-                  val accessed = enteringPickler(mixinMember.accessed)
-                  // #3857, need to retain info before erasure when cloning (since cloning only
-                  // carries over the current entry in the type history)
-                  val sym = enteringErasure {
-                    // so we have a type history entry before erasure
-                    clazz.newValue(mixinMember.localName, mixinMember.pos).setInfo(mixinMember.tpe.resultType)
-                  }
-                  sym updateInfo mixinMember.tpe.resultType // info at current phase
-
-                  val newFlags = (
-                      ( PrivateLocal )
-                    | ( mixinMember getFlag MUTABLE | LAZY)
-                    | ( if (mixinMember.hasStableFlag) 0 else MUTABLE )
-                  )
-
-                  addMember(clazz, sym setFlag newFlags setAnnotations accessed.annotations)
-              }
-          }
-        }
-        else if (mixinMember.isSuperAccessor) { // mixin super accessors
+        if (mixinMember.hasFlag(SUPERACCESSOR)) { // mixin super accessors
           val superAccessor = addMember(clazz, mixinMember.cloneSymbol(clazz)) setPos clazz.pos
           assert(superAccessor.alias != NoSymbol, superAccessor)
 
@@ -355,10 +317,53 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
               superAccessor.asInstanceOf[TermSymbol] setAlias alias1
           }
         }
-        else if (mixinMember.isMethod && mixinMember.isModule && mixinMember.hasNoFlags(LIFTED | BRIDGE)) {
+        else if (mixinMember.hasAllFlags(METHOD | MODULE) && mixinMember.hasNoFlags(LIFTED | BRIDGE)) {
           // mixin objects: todo what happens with abstract objects?
           addMember(clazz, mixinMember.cloneSymbol(clazz, mixinMember.flags & ~(DEFERRED | lateDEFERRED)) setPos clazz.pos)
         }
+        else if (mixinMember.hasFlag(ACCESSOR) && notDeferredOrLate(mixinMember)
+                 && (mixinMember hasFlag (LAZY | PARAMACCESSOR))
+                 && !isOverriddenAccessor(mixinMember, clazz.info.baseClasses)) {
+          // pick up where `fields` left off -- it already mixed in fields and accessors for regular vals.
+          // but has ignored lazy vals and constructor parameter accessors
+          // TODO: captures added by lambdalift for local traits?
+          //
+          // mixin accessor for lazy val or constructor parameter
+          // (note that a paramaccessor cannot have a constant type as it must have a user-defined type)
+          val mixedInAccessor = cloneAndAddMixinMember(mixinClass, mixinMember)
+          val name = mixinMember.name
+
+          if (mixinMember.isLazy)
+            initializer(mixedInAccessor) =
+              (mixinClass.info.decl(name) orElse abort(s"Could not find initializer for lazy val $name!"))
+
+          // Add field while we're mixing in the getter (unless it's a Unit-typed lazy val)
+          //
+          // lazy val of type Unit doesn't need a field -- the bitmap is enough.
+          // TODO: constant-typed lazy vals... it's an extreme corner case, but we could also suppress the field in:
+          // `trait T { final lazy val a = "a" }; class C extends T`, but who writes code like that!? :)
+          // we'd also have to change the lazyvals logic if we do this
+          if (!nme.isSetterName(name) && !(mixinMember.isLazy && isUnitGetter(mixinMember))) {
+            // enteringPhase: the private field is moved to the implementation class by erasure,
+            // so it can no longer be found in the mixinMember's owner (the trait)
+            val accessed = enteringPickler(mixinMember.accessed)
+            // #3857, need to retain info before erasure when cloning (since cloning only
+            // carries over the current entry in the type history)
+            val sym = enteringErasure {
+              // so we have a type history entry before erasure
+              clazz.newValue(mixinMember.localName, mixinMember.pos).setInfo(mixinMember.tpe.resultType)
+            }
+            sym updateInfo mixinMember.tpe.resultType // info at current phase
+
+            val newFlags = (
+              (PrivateLocal)
+              | (mixinMember getFlag MUTABLE | LAZY)
+              | (if (mixinMember.hasStableFlag) 0 else MUTABLE)
+              )
+
+            addMember(clazz, sym setFlag newFlags setAnnotations accessed.annotations)
+          }
+        }
       }
     }
 
@@ -478,8 +483,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
 
           tree
 
-        case _ =>
-          tree
+        case _ => tree
       }
     }
 
@@ -763,13 +767,12 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
       def addCheckedGetters(clazz: Symbol, stats: List[Tree]): List[Tree] = {
         def dd(stat: DefDef) = {
           val sym     = stat.symbol
-          def isUnit  = sym.tpe.resultType.typeSymbol == UnitClass
           def isEmpty = stat.rhs == EmptyTree
 
           if (!clazz.isTrait && sym.isLazy && !isEmpty) {
             assert(fieldOffset contains sym, sym)
             deriveDefDef(stat) {
-              case t if isUnit => mkLazyDef(clazz, sym, List(t), UNIT, fieldOffset(sym))
+              case t if isUnitGetter(sym) => mkLazyDef(clazz, sym, List(t), UNIT, fieldOffset(sym))
 
               case Block(stats, res) =>
                 mkLazyDef(clazz, sym, stats, Select(This(clazz), res.symbol), fieldOffset(sym))
@@ -781,8 +784,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
             assert(fieldOffset contains sym, sym)
             deriveDefDef(stat)(rhs =>
               (mkCheckedAccessor(clazz, _: Tree, fieldOffset(sym), stat.pos, sym))(
-                if (sym.tpe.resultType.typeSymbol == UnitClass) UNIT
-                else rhs
+                if (isUnitGetter(sym)) UNIT else rhs
               )
             )
           }
@@ -908,7 +910,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
         }
       }
 
-      def isUnitGetter(getter: Symbol) = getter.tpe.resultType.typeSymbol == UnitClass
       def fieldAccess(accessor: Symbol) = Select(This(clazz), accessor.accessed)
 
       def isOverriddenSetter(sym: Symbol) =
@@ -924,7 +925,12 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
           addDefDef(sym)
         } else {
           // if class is not a trait add accessor definitions
-          if (isConcreteAccessor(sym)) {
+          // used to include `sym` with `sym hasFlag lateDEFERRED` as not deferred,
+          // but I don't think MIXEDIN members ever get this flag
+          assert(!sym.hasFlag(lateDEFERRED), s"mixedin $sym from $clazz has lateDEFERRED flag?!")
+          if (sym.hasFlag(ACCESSOR) && !sym.hasFlag(DEFERRED)) {
+            assert(sym hasFlag (LAZY | PARAMACCESSOR), s"mixed in $sym from $clazz is not lazy/param?!?")
+
             // add accessor definitions
             addDefDef(sym, {
               if (sym.isSetter) {
@@ -1006,20 +1012,14 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
           val parents1 = currentOwner.info.parents map (t => TypeTree(t) setPos tree.pos)
           // mark fields which can be nulled afterward
           lazyValNullables = nullableFields(templ) withDefaultValue Set()
-          // Remove bodies of accessors in traits - TODO: after PR #5141 (fields refactoring), this might be a no-op
-          val bodyEmptyAccessors = if (!sym.enclClass.isTrait) body else body mapConserve {
-            case dd: DefDef if dd.symbol.isAccessor && !dd.symbol.isLazy =>
-              deriveDefDef(dd)(_ => EmptyTree)
-            case tree => tree
-          }
           // add all new definitions to current class or interface
-          val body1 = addNewDefs(currentOwner, bodyEmptyAccessors)
-          body1 foreach {
+          val statsWithNewDefs = addNewDefs(currentOwner, body)
+          statsWithNewDefs foreach {
             case dd: DefDef if isTraitMethodRequiringStaticImpl(dd) =>
               dd.symbol.updateAttachment(NeedStaticImpl)
             case _ =>
           }
-          treeCopy.Template(tree, parents1, self, body1)
+          treeCopy.Template(tree, parents1, self, statsWithNewDefs)
 
         case Select(qual, name) if sym.owner.isTrait && !sym.isMethod =>
           // refer to fields in some trait an abstract getter in the interface.