From 6858134fb01315c13df05fbef1b310443f3dac95 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Fri, 3 Jun 2016 10:45:27 -0700 Subject: 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... --- src/compiler/scala/tools/nsc/transform/Fields.scala | 16 ++++++++-------- .../scala/tools/nsc/typechecker/MethodSynthesis.scala | 13 ++++++++++--- src/compiler/scala/tools/nsc/typechecker/Namers.scala | 11 +++++++++-- 3 files changed, 27 insertions(+), 13 deletions(-) (limited to 'src/compiler/scala/tools/nsc') 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 = -- cgit v1.2.3