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, 42 insertions, 18 deletions

diff --git a/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala b/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala
index c03094bc6a..408b457d5b 100644
--- a/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala
@@ -121,6 +121,7 @@ trait MethodSynthesis {
     }
 
     // TODO: see if we can link symbol creation & tree derivation by sharing the Field/Getter/Setter factories
+    // maybe we can at least reuse some variant of standardAccessors?
     def enterGetterSetter(tree: ValDef): Unit = {
       tree.symbol =
         if (tree.mods.isLazy) {
@@ -131,15 +132,14 @@ trait MethodSynthesis {
           val getterSym = getter.createAndEnterSymbol()
 
           // Create the setter if necessary.
-          if (getter.needsSetter)
-            Setter(tree).createAndEnterSymbol()
+          if (getter.needsSetter) Setter(tree).createAndEnterSymbol()
 
-          // If the getter's abstract the tree gets the getter's symbol,
-          // otherwise, create a field (assume the getter requires storage).
+          // If the getter's abstract, the tree gets the getter's symbol,
+          // otherwise, create a field (we have to assume the getter requires storage for now).
           // NOTE: we cannot look at symbol info, since we're in the process of deriving them
           // (luckily, they only matter for lazy vals, which we've ruled out in this else branch,
           // and `doNotDeriveField` will skip them if `!mods.isLazy`)
-          if (Field.noFieldFor(tree)) getterSym setPos tree.pos
+          if (Field.noFieldFor(tree)) getterSym setPos tree.pos // TODO: why do setPos? `createAndEnterSymbol` already gave `getterSym` the position `tree.pos.focus`
           else enterStrictVal(tree)
         }
 
@@ -282,14 +282,15 @@ trait MethodSynthesis {
       final def enclClass = basisSym.enclClass
 
 
-      /* Explicit isSetter required for bean setters (beanSetterSym.isSetter is false) */
-      final def completer(sym: Symbol) = namerOf(sym).accessorTypeCompleter(tree, isSetter)
+      // There's no reliable way to detect all kinds of setters from flags or name!!!
+      // A BeanSetter's name does not end in `_=` -- it does begin with "set", but so could the getter
+      // for a regular Scala field... TODO: can we add a flag to distinguish getter/setter accessors?
+      final def completer(sym: Symbol) = namerOf(sym).accessorTypeCompleter(tree, this.isInstanceOf[DerivedSetter])
       final def fieldSelection         = Select(This(enclClass), basisSym)
 
       def derivedSym: Symbol = tree.symbol
       def derivedTree: Tree  = EmptyTree
 
-      def isSetter   = false
       def isDeferred = mods.isDeferred
       def validate() { }
       def createAndEnterSymbol(): MethodSymbol = {
@@ -304,6 +305,7 @@ trait MethodSynthesis {
 
         result
       }
+
       final def derive(initial: List[AnnotationInfo]): Tree = {
         validate()
 
@@ -311,7 +313,9 @@ trait MethodSynthesis {
         // Annotations on ValDefs can be targeted towards the following: field, getter, setter, beanGetter, beanSetter, param.
         // The defaults are:
         //   - (`val`-, `var`- or plain) constructor parameter annotations end up on the parameter, not on any other entity.
-        //   - val/var member annotations solely end up on the underlying field.
+        //   - val/var member annotations solely end up on the underlying field, except in traits (@since 2.12),
+        //     where there is no field, and the getter thus holds annotations targetting both getter & field.
+        //     As soon as there is a field/getter (in subclasses mixing in the trait), we triage the annotations.
         //
         // TODO: these defaults can be surprising for annotations not meant for accessors/fields -- should we revisit?
         // (In order to have `@foo val X` result in the X getter being annotated with `@foo`, foo needs to be meta-annotated with @getter)
@@ -319,9 +323,11 @@ trait MethodSynthesis {
           case _: Param                       => annotationFilter(ParamTargetClass,      defaultRetention = true)
           // By default annotations go to the field, except if the field is generated for a class parameter (PARAMACCESSOR).
           case _: Field                       => annotationFilter(FieldTargetClass,      defaultRetention = !mods.isParamAccessor)
+          case _: BaseGetter if owner.isTrait => annotationFilter(List(FieldTargetClass, GetterTargetClass), defaultRetention = true)
           case _: BaseGetter                  => annotationFilter(GetterTargetClass,     defaultRetention = false)
           case _: Setter                      => annotationFilter(SetterTargetClass,     defaultRetention = false)
           case _: BeanSetter                  => annotationFilter(BeanSetterTargetClass, defaultRetention = false)
+          // TODO do bean getters need special treatment to collect field-targeting annotations in traits?
           case _: AnyBeanGetter               => annotationFilter(BeanGetterTargetClass, defaultRetention = false)
         }
 
@@ -329,21 +335,23 @@ trait MethodSynthesis {
         // should be propagated to this kind of accessor.
         derivedSym setAnnotations (initial filter annotFilter)
 
+        if (derivedSym.isSetter && owner.isTrait && !isDeferred)
+          derivedSym addAnnotation TraitSetterAnnotationClass
+
         logDerived(derivedTree)
       }
     }
+
     sealed trait DerivedGetter extends DerivedFromValDef {
-      // A getter must be accompanied by a setter if the ValDef is mutable.
       def needsSetter = mods.isMutable
     }
     sealed trait DerivedSetter extends DerivedFromValDef {
-      override def isSetter = true
-      private def setterParam = derivedSym.paramss match {
+      protected def setterParam = derivedSym.paramss match {
         case (p :: Nil) :: _  => p
         case _                => NoSymbol
       }
 
-      private def setterRhs = {
+      protected def setterRhs = {
         assert(!derivedSym.isOverloaded, s"Unexpected overloaded setter $derivedSym for $basisSym in $enclClass")
         if (Field.noFieldFor(tree) || derivedSym.isOverloaded) EmptyTree
         else Assign(fieldSelection, Ident(setterParam))
@@ -390,6 +398,7 @@ trait MethodSynthesis {
       override def derivedSym = if (Field.noFieldFor(tree)) basisSym else basisSym.getterIn(enclClass)
       private def derivedRhs  = if (Field.noFieldFor(tree)) tree.rhs else fieldSelection
 
+      // TODO: more principled approach -- this is a bit bizarre
       private def derivedTpt = {
         // For existentials, don't specify a type for the getter, even one derived
         // from the symbol! This leads to incompatible existentials for the field and
@@ -457,6 +466,7 @@ trait MethodSynthesis {
       def flagsMask  = SetterFlags
       def flagsExtra = ACCESSOR
 
+      // TODO: double check logic behind need for name expansion in context of new fields phase
       override def derivedSym = basisSym.setterIn(enclClass)
     }
 
@@ -464,17 +474,25 @@ trait MethodSynthesis {
       // No field for these vals (either never emitted or eliminated later on):
       //   - abstract vals have no value we could store (until they become concrete, potentially)
       //   - lazy vals of type Unit
-      //   - [Emitted, later removed during AddInterfaces/Mixins] concrete vals in traits can't have a field
-      //   - [Emitted, later removed during Constructors] a concrete val with a statically known value (Unit / ConstantType)
+      //   - concrete vals in traits don't yield a field here either (their getter's RHS has the initial value)
+      //     Constructors will move the assignment to the constructor, abstracting over the field using the field setter,
+      //     and Fields will add a field to the class that mixes in the trait, implementing the accessors in terms of it
+      //   - [Emitted, later removed during Constructors] a concrete val with a statically known value (ConstantType)
       //     performs its side effect according to lazy/strict semantics, but doesn't need to store its value
       //     each access will "evaluate" the RHS (a literal) again
       // We would like to avoid emitting unnecessary fields, but the required knowledge isn't available until after typer.
       // The only way to avoid emitting & suppressing, is to not emit at all until we are sure to need the field, as dotty does.
       // NOTE: do not look at `vd.symbol` when called from `enterGetterSetter` (luckily, that call-site implies `!mods.isLazy`),
+      // similarly, the `def field` call-site breaks when you add `|| vd.symbol.owner.isTrait` (detected in test suite)
       // as the symbol info is in the process of being created then.
       // TODO: harmonize tree & symbol creation
-      // TODO: the `def field` call-site breaks when you add `|| vd.symbol.owner.isTrait` (detected in test suite)
-      def noFieldFor(vd: ValDef) = vd.mods.isDeferred || (vd.mods.isLazy && isUnitType(vd.symbol.info))
+      // the middle  `&& !owner.isTrait` is needed after `isLazy` because non-unit-typed lazy vals in traits still get a field -- see neg/t5455.scala
+      def noFieldFor(vd: ValDef) = (vd.mods.isDeferred
+        || (vd.mods.isLazy && !owner.isTrait && isUnitType(vd.symbol.info))
+        || (owner.isTrait && !traitFieldFor(vd)))
+
+      // TODO: never emit any fields in traits -- only use getter for lazy/presuper ones as well
+      private def traitFieldFor(vd: ValDef): Boolean = vd.mods.hasFlag(PRESUPER | LAZY)
     }
 
     case class Field(tree: ValDef) extends DerivedFromValDef {
@@ -482,6 +500,9 @@ trait MethodSynthesis {
       def flagsMask  = FieldFlags
       def flagsExtra = PrivateLocal
 
+      // TODO: override def createAndEnterSymbol (currently never called on Field)
+      // and do `enterStrictVal(tree)`, so that enterGetterSetter and addDerivedTrees can share some logic...
+
       // handle lazy val first for now (we emit a Field even though we probably shouldn't...)
       override def derivedTree =
         if (mods.isLazy) copyValDef(tree)(mods = mods | flagsExtra, name = this.name, rhs = EmptyTree).setPos(tree.pos.focus)
@@ -528,7 +549,10 @@ trait MethodSynthesis {
     }
     case class BooleanBeanGetter(tree: ValDef) extends BeanAccessor("is") with AnyBeanGetter { }
     case class BeanGetter(tree: ValDef) extends BeanAccessor("get") with AnyBeanGetter { }
-    case class BeanSetter(tree: ValDef) extends BeanAccessor("set") with DerivedSetter
+    case class BeanSetter(tree: ValDef) extends BeanAccessor("set") with DerivedSetter {
+      // TODO: document, motivate
+      override protected def setterRhs = Apply(Ident(tree.name.setterName), List(Ident(setterParam)))
+    }
 
     // No Symbols available.
     private def beanAccessorsFromNames(tree: ValDef) = {