Address lrytz's review feedback

Remove obsolete hack for BeanSetter's RHS

Use currentOwner.isClass instead of exprOwner.isLocalDummy

Refactor: shortest branches first in if/else

Fix comments from when the prototype ran before refchecks

Also, store `isScala212` as a `val` in `Namer`
since the `def` on `settings` parses the version each time...

3 files changed, 27 insertions, 13 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/Fields.scala b/src/compiler/scala/tools/nsc/transform/Fields.scala
index f5f0b229e4..105bf0410d 100644
--- a/src/compiler/scala/tools/nsc/transform/Fields.scala
+++ b/src/compiler/scala/tools/nsc/transform/Fields.scala
@@ -287,7 +287,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
       case tp@ClassInfoType(parents, oldDecls, clazz) if !clazz.isPackageClass =>
         val site = clazz.thisType
 
-        // TODO: setter conflicts?
+        // setter conflicts cannot arise independently from a getter conflict, since a setter without a getter does not a val definition make
         def accessorConflictsExistingVal(accessor: Symbol): Boolean = {
           val existingGetter = oldDecls.lookup(accessor.name.getterName)
 //          println(s"$existingGetter from $accessor to ${accessor.name.getterName}")
@@ -345,7 +345,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
             val moduleVar = newModuleVar(member)
             List(moduleVar, newModuleAccessor(member, clazz, moduleVar))
           }
-          // when considering whether to mix in the trait setter, forget about conflicts -- they will be reported for the getter
+          // when considering whether to mix in the trait setter, forget about conflicts -- they are reported for the getter
           // a trait setter for an overridden val will receive a unit body in the tree transform
           else if (nme.isTraitSetterName(member.name)) {
             val getter = member.getterIn(member.owner)
@@ -356,8 +356,8 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
 
             List(clone)
           }
-          // avoid creating early errors in case of conflicts (wait until refchecks);
-          // also, skip overridden accessors contributed by supertraits (only act on the last overriding one)
+          // don't cause conflicts, skip overridden accessors contributed by supertraits (only act on the last overriding one)
+          // see pos/trait_fields_dependent_conflict.scala and neg/t1960.scala
           else if (accessorConflictsExistingVal(member) || isOverriddenAccessor(member, clazz)) Nil
           else if (member.isGetter && fieldMemoizationIn(member, clazz).stored) {
             // add field if needed
@@ -370,7 +370,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
             field setAnnotations (member.annotations filter AnnotationInfo.mkFilter(FieldTargetClass, defaultRetention = true))
 
             List(cloneAccessor(), field)
-          } else List(cloneAccessor())
+          } else List(cloneAccessor()) // no field needed (constant-typed getter has constant as its RHS)
         }
 
 //        println(s"mixedInAccessorAndFields for $clazz: $mixedInAccessorAndFields")
@@ -423,7 +423,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
       def fieldAccess(accessor: Symbol): Option[Tree] = {
         val fieldName = accessor.localName
         val field = clazz.info.decl(fieldName)
-        // The `None` result denotes an error, but we defer to refchecks to report it.
+        // The `None` result denotes an error, but it's refchecks' job to report it (this fallback is for robustness).
         // This is the result of overriding a val with a def, so that no field is found in the subclass.
         if (field.exists) Some(Select(This(clazz), field))
         else None
@@ -527,8 +527,8 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
 
     override def transformStats(stats: List[Tree], exprOwner: Symbol): List[Tree] = {
       val addedStats =
-        if (exprOwner.isLocalDummy) afterOwnPhase { fieldsAndAccessors(exprOwner.owner) }
-        else Nil
+        if (!currentOwner.isClass) Nil
+        else afterOwnPhase { fieldsAndAccessors(currentOwner) }
 
       val newStats =
         stats mapConserve (if (exprOwner != currentOwner) transformTermsAtExprOwner(exprOwner) else transform)
diff --git a/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala b/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala
index 408b457d5b..0f79bb60ed 100644
--- a/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/MethodSynthesis.scala
@@ -536,6 +536,8 @@ trait MethodSynthesis {
         super.validate()
       }
     }
+
+    // This trait is mixed into BooleanBeanGetter and BeanGetter by beanAccessorsFromNames, but not by beanAccessors
     trait NoSymbolBeanGetter extends AnyBeanGetter {
       // Derives a tree without attempting to use the original tree's symbol.
       override def derivedTree = {
@@ -547,10 +549,15 @@ trait MethodSynthesis {
       }
       override def createAndEnterSymbol(): MethodSymbol = enterSyntheticSym(derivedTree).asInstanceOf[MethodSymbol]
     }
-    case class BooleanBeanGetter(tree: ValDef) extends BeanAccessor("is") with AnyBeanGetter { }
-    case class BeanGetter(tree: ValDef) extends BeanAccessor("get") with AnyBeanGetter { }
+
+    // NoSymbolBeanGetter synthesizes the getter's RHS (which defers to the regular setter)
+    // (not sure why, but there is one use site of the BeanGetters where NoSymbolBeanGetter is not mixed in)
+    // TODO: clean this up...
+    case class BooleanBeanGetter(tree: ValDef) extends BeanAccessor("is") with AnyBeanGetter
+    case class BeanGetter(tree: ValDef) extends BeanAccessor("get") with AnyBeanGetter
+
+    // the bean setter's RHS delegates to the setter
     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)))
     }
 
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index 784b43ab84..98dca1089c 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -61,6 +61,11 @@ trait Namers extends MethodSynthesis {
     private lazy val innerNamer =
       if (isTemplateContext(context)) createInnerNamer() else this
 
+    // Cached as a val because `settings.isScala212` parses the Scala version each time...
+    // Not in Namers because then we need to go to outer first to check this.
+    // I do think it's ok to check every time we create a Namer instance (so, not a lazy val).
+    private[this] val isScala212 = settings.isScala212
+
     def createNamer(tree: Tree): Namer = {
       val sym = tree match {
         case ModuleDef(_, _, _) => tree.symbol.moduleClass
@@ -1380,7 +1385,9 @@ trait Namers extends MethodSynthesis {
             val pt = {
               val valOwner = owner.owner
               // there's no overriding outside of classes, and we didn't use to do this in 2.11, so provide opt-out
-              if (valOwner.isClass && settings.isScala212) {
+
+              if (!isScala212 || !valOwner.isClass) WildcardType
+              else {
                 // normalize to getter so that we correctly consider a val overriding a def
                 // (a val's name ends in a " ", so can't compare to def)
                 val overridingSym = if (isGetter) vdef.symbol else vdef.symbol.getterIn(valOwner)
@@ -1391,7 +1398,7 @@ trait Namers extends MethodSynthesis {
 
                 if (overridden == NoSymbol || overridden.isOverloaded) WildcardType
                 else valOwner.thisType.memberType(overridden).resultType
-              } else WildcardType
+              }
             }
 
             def patchSymInfo(tp: Type): Unit =