Merge pull request scala#5570 from adriaanm/t10075

SI-10075 propagate annotations to lazy val's underlying field

Author: adriaanm

src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala

@@ -494,7 +494,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
               genDefDef(statified)
             } else {
               val forwarderDefDef = {
-                val dd1 = global.gen.mkStatic(deriveDefDef(dd)(_ => EmptyTree), traitSuperAccessorName(sym), _.cloneSymbol)
+                val dd1 = global.gen.mkStatic(deriveDefDef(dd)(_ => EmptyTree), traitSuperAccessorName(sym), _.cloneSymbol.withoutAnnotations)
                 dd1.symbol.setFlag(Flags.ARTIFACT).resetFlag(Flags.OVERRIDE)
                 val selfParam :: realParams = dd1.vparamss.head.map(_.symbol)
                 deriveDefDef(dd1)(_ =>

src/compiler/scala/tools/nsc/transform/Erasure.scala

@@ -1205,7 +1205,9 @@ abstract class Erasure extends InfoTransform
               treeCopy.ArrayValue(
                 tree1, elemtpt setType specialScalaErasure.applyInArray(elemtpt.tpe), trees map transform).clearType()
             case DefDef(_, _, _, _, tpt, _) =>
-              fields.dropFieldAnnotationsFromGetter(tree.symbol) // TODO: move this in some post-processing transform in the fields phase?
+              // TODO: move this in some post-processing transform in the fields phase?
+              if (fields.symbolAnnotationsTargetFieldAndGetter(tree.symbol))
+                fields.dropFieldAnnotationsFromGetter(tree.symbol)
 
               try super.transform(tree1).clearType()
               finally tpt setType specialErasure(tree1.symbol)(tree1.symbol.tpe).resultType

src/compiler/scala/tools/nsc/transform/Fields.scala

@@ -176,14 +176,25 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
   // NOTE: this only considers type, filter on flags first!
   def fieldMemoizationIn(accessorOrField: Symbol, site: Symbol) = new FieldMemoization(accessorOrField, site)
 
-  // drop field-targeting annotations from getters
+  // drop field-targeting annotations from getters (done during erasure because we first need to create the field symbol)
   // (in traits, getters must also hold annotations that target the underlying field,
   //  because the latter won't be created until the trait is mixed into a class)
   // TODO do bean getters need special treatment to suppress field-targeting annotations in traits?
   def dropFieldAnnotationsFromGetter(sym: Symbol) =
-    if (sym.isGetter && sym.owner.isTrait) {
-      sym setAnnotations (sym.annotations filter AnnotationInfo.mkFilter(GetterTargetClass, defaultRetention = false))
-    }
+    sym setAnnotations (sym.annotations filter AnnotationInfo.mkFilter(GetterTargetClass, defaultRetention = false))
+
+  def symbolAnnotationsTargetFieldAndGetter(sym: Symbol): Boolean = sym.isGetter && (sym.isLazy || sym.owner.isTrait)
+
+  // A trait val/var or a lazy val does not receive an underlying field symbol until this phase.
+  // Since annotations need a carrier symbol from the beginning, both field- and getter-targeting annotations
+  // are kept on the getter symbol for these until they are dropped by dropFieldAnnotationsFromGetter
+  def getterTreeAnnotationsTargetFieldAndGetter(owner: Symbol, mods: Modifiers) = mods.isLazy || owner.isTrait
+
+  // Propagate field-targeting annotations from getter to field.
+  // By the way, we must keep them around long enough to see them here (now that we have created the field),
+  // which is why dropFieldAnnotationsFromGetter is not called until erasure.
+  private def propagateFieldAnnotations(getter: Symbol, field: TermSymbol): Unit =
+    field setAnnotations (getter.annotations filter AnnotationInfo.mkFilter(FieldTargetClass, defaultRetention = true))
 
 
   // can't use the referenced field since it already tracks the module's moduleClass
@@ -241,6 +252,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
     sym
   }
 
+
   private object synthFieldsAndAccessors extends TypeMap {
     private def newTraitSetter(getter: Symbol, clazz: Symbol) = {
       // Add setter for an immutable, memoizing getter
@@ -388,10 +400,12 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
         val accessorSymbolSynth = checkedAccessorSymbolSynth(tp.typeSymbol)
 
         // expand module def in class/object (if they need it -- see modulesNeedingExpansion above)
-        val expandedModulesAndLazyVals = (
+        val expandedModulesAndLazyVals =
           modulesAndLazyValsNeedingExpansion flatMap { member =>
             if (member.isLazy) {
-              List(newLazyVarMember(member), accessorSymbolSynth.newSlowPathSymbol(member))
+              val lazyVar = newLazyVarMember(member)
+              propagateFieldAnnotations(member, lazyVar)
+              List(lazyVar, accessorSymbolSynth.newSlowPathSymbol(member))
             }
             // expanding module def (top-level or nested in static module)
             else List(if (member.isStatic) { // implies m.isOverridingSymbol as per above filter
@@ -404,7 +418,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
               member setFlag NEEDS_TREES
               newModuleVarMember(member)
             })
-          })
+          }
 
 //        println(s"expanded modules for $clazz: $expandedModules")
 
@@ -419,8 +433,9 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
             val clonedAccessor = (member cloneSymbol clazz) setPos clazz.pos
             setMixedinAccessorFlags(member, clonedAccessor)
 
-            if (clonedAccessor.isGetter)
-              clonedAccessor setAnnotations (clonedAccessor.annotations filter AnnotationInfo.mkFilter(GetterTargetClass, defaultRetention = false))
+            // note: check original member when deciding how to triage annotations, then act on the cloned accessor
+            if (symbolAnnotationsTargetFieldAndGetter(member))  // this simplifies to member.isGetter, but the full formulation really ties the triage together
+              dropFieldAnnotationsFromGetter(clonedAccessor)
 
             // if we don't cloneInfo, method argument symbols are shared between trait and subclasses --> lambalift proxy crash
             // TODO: use derive symbol variant?
@@ -450,7 +465,11 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
           }
           else if (member hasFlag LAZY) {
             val mixedinLazy = cloneAccessor()
-            val lazyVar = newLazyVarMember(mixedinLazy)
+            val lazyVar = newLazyVarMember(mixedinLazy) // link lazy var member to the mixedin lazy accessor
+
+            // propagate from original member. since mixed in one has only retained the annotations targeting the getter
+            propagateFieldAnnotations(member, lazyVar)
+
             // println(s"mixing in lazy var: $lazyVar for $member")
             List(lazyVar, accessorSymbolSynth.newSlowPathSymbol(mixedinLazy), newSuperLazy(mixedinLazy, site, lazyVar))
           }
@@ -460,9 +479,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
 
             setFieldFlags(member, field)
 
-            // filter getter's annotations to exclude those only meant for the field
-            // we must keep them around long enough to see them here, though, when we create the field
-            field setAnnotations (member.annotations filter AnnotationInfo.mkFilter(FieldTargetClass, defaultRetention = true))
+            propagateFieldAnnotations(member, field)
 
             List(cloneAccessor(), field)
           } else List(cloneAccessor()) // no field needed (constant-typed getter has constant as its RHS)

src/compiler/scala/tools/nsc/typechecker/Namers.scala

@@ -902,9 +902,10 @@ trait Namers extends 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, except in traits (@since 2.12),
+    //   - val/var member annotations solely end up on the underlying field, except in traits and for all lazy vals (@since 2.12),
     //     where there is no field, and the getter thus holds annotations targeting both getter & field.
-    //     As soon as there is a field/getter (in subclasses mixing in the trait), we triage the annotations.
+    //     As soon as there is a field/getter (in subclasses mixing in the trait, or after expanding the lazy val during the fields phase),
+    //     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)
@@ -918,15 +919,17 @@ trait Namers extends MethodSynthesis {
           BeanPropertyAnnotationLimitationError(tree)
       }
 
+      val canTriageAnnotations = isSetter || !fields.getterTreeAnnotationsTargetFieldAndGetter(owner, mods)
+
       def filterAccessorAnnotations: AnnotationInfo => Boolean =
-        if (isSetter || !owner.isTrait)
+        if (canTriageAnnotations)
           annotationFilter(if (isSetter) SetterTargetClass else GetterTargetClass, defaultRetention = false)
         else (ann =>
           annotationFilter(FieldTargetClass, defaultRetention = true)(ann) ||
             annotationFilter(GetterTargetClass, defaultRetention = true)(ann))
 
       def filterBeanAccessorAnnotations: AnnotationInfo => Boolean =
-        if (isSetter || !owner.isTrait)
+        if (canTriageAnnotations)
           annotationFilter(if (isSetter) BeanSetterTargetClass else BeanGetterTargetClass, defaultRetention = false)
         else (ann =>
           annotationFilter(FieldTargetClass, defaultRetention = true)(ann) ||

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -2027,7 +2027,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
 
       // allow trait accessors: it's the only vehicle we have to hang on to annotations that must be passed down to
       // the field that's mixed into a subclass
-      if (sym.hasAnnotation(definitions.VolatileAttr) && !((sym hasFlag MUTABLE) || (sym hasFlag ACCESSOR) && sym.owner.isTrait))
+      if (sym.hasAnnotation(definitions.VolatileAttr) && !((sym hasFlag MUTABLE | LAZY) || (sym hasFlag ACCESSOR) && sym.owner.isTrait))
         VolatileValueError(vdef)
 
       val rhs1 =

test/files/run/junitForwarders/C_1.scala

@@ -10,6 +10,6 @@ object Test extends App {
     assert(s == e, s"found: $s\nexpected: $e")
   }
   check(classOf[C], "foo - @org.junit.Test()")
-  // TODO scala-dev#213: should `foo$` really carry the @Test annotation?
-  check(classOf[T], "$init$ - ;foo - @org.junit.Test();foo$ - @org.junit.Test()")
+  // scala/scala-dev#213, scala/scala#5570: `foo$` should not have the @Test annotation
+  check(classOf[T], "$init$ - ;foo - @org.junit.Test();foo$ - ")
 }

test/files/run/t10075.scala

@@ -0,0 +1,35 @@
+class NotSerializable
+
+trait SerializableActually {
+  @transient
+  lazy val notSerializedTLV: NotSerializable = new NotSerializable
+
+  @transient
+  val notSerializedTL: NotSerializable = new NotSerializable
+
+  @transient
+  var notSerializedTR: NotSerializable = new NotSerializable
+}
+
+class SerializableBecauseTransient extends Serializable with SerializableActually {
+  @transient
+  lazy val notSerializedLV: NotSerializable = new NotSerializable
+
+  @transient
+  val notSerializedL: NotSerializable = new NotSerializable
+
+  @transient
+  var notSerializedR: NotSerializable = new NotSerializable
+}
+
+// Indirectly check that the @transient annotation on `notSerialized` made it to the underyling field in bytecode.
+// If it doesn't, `writeObject` will fail to serialize the field `notSerialized`, because `NotSerializable` is not serializable
+object Test {
+  def main(args: Array[String]): Unit = {
+    val obj = new SerializableBecauseTransient
+    // must force, since `null` valued field is serialized regardless of its type
+    val forceTLV = obj.notSerializedTLV
+    val forceLV  = obj.notSerializedLV
+    new java.io.ObjectOutputStream(new java.io.ByteArrayOutputStream) writeObject obj
+  }
+}

test/files/run/t10075b.check

@@ -0,0 +1,60 @@
+ private volatile byte C.bitmap$0
+@RetainedAnnotation() private int C.lzyValFieldAnnotation
+ public int C.lzyValFieldAnnotation()
+ private int C.lzyValFieldAnnotation$lzycompute()
+ private int C.lzyValGetterAnnotation
+@RetainedAnnotation() public int C.lzyValGetterAnnotation()
+ private int C.lzyValGetterAnnotation$lzycompute()
+@RetainedAnnotation() private final int C.valFieldAnnotation
+ public int C.valFieldAnnotation()
+ private final int C.valGetterAnnotation
+@RetainedAnnotation() public int C.valGetterAnnotation()
+@RetainedAnnotation() private int C.varFieldAnnotation
+ public int C.varFieldAnnotation()
+ public void C.varFieldAnnotation_$eq(int)
+ private int C.varGetterAnnotation
+@RetainedAnnotation() public int C.varGetterAnnotation()
+ public void C.varGetterAnnotation_$eq(int)
+ private int C.varSetterAnnotation
+ public int C.varSetterAnnotation()
+@RetainedAnnotation() public void C.varSetterAnnotation_$eq(int)
+ public static void T.$init$(T)
+ public abstract void T.T$_setter_$valFieldAnnotation_$eq(int)
+ public abstract void T.T$_setter_$valGetterAnnotation_$eq(int)
+ public default int T.lzyValFieldAnnotation()
+ public static int T.lzyValFieldAnnotation$(T)
+@RetainedAnnotation() public default int T.lzyValGetterAnnotation()
+ public static int T.lzyValGetterAnnotation$(T)
+@RetainedAnnotation() public default int T.method()
+ public static int T.method$(T)
+ public abstract int T.valFieldAnnotation()
+@RetainedAnnotation() public abstract int T.valGetterAnnotation()
+ public abstract int T.varFieldAnnotation()
+ public abstract void T.varFieldAnnotation_$eq(int)
+@RetainedAnnotation() public abstract int T.varGetterAnnotation()
+ public abstract void T.varGetterAnnotation_$eq(int)
+ public abstract int T.varSetterAnnotation()
+@RetainedAnnotation() public abstract void T.varSetterAnnotation_$eq(int)
+ public void TMix.T$_setter_$valFieldAnnotation_$eq(int)
+ public void TMix.T$_setter_$valGetterAnnotation_$eq(int)
+ private volatile byte TMix.bitmap$0
+@RetainedAnnotation() private int TMix.lzyValFieldAnnotation
+ public int TMix.lzyValFieldAnnotation()
+ private int TMix.lzyValFieldAnnotation$lzycompute()
+ private int TMix.lzyValGetterAnnotation
+@RetainedAnnotation() public int TMix.lzyValGetterAnnotation()
+ private int TMix.lzyValGetterAnnotation$lzycompute()
+@RetainedAnnotation() public int TMix.method()
+@RetainedAnnotation() private final int TMix.valFieldAnnotation
+ public int TMix.valFieldAnnotation()
+ private final int TMix.valGetterAnnotation
+@RetainedAnnotation() public int TMix.valGetterAnnotation()
+@RetainedAnnotation() private int TMix.varFieldAnnotation
+ public int TMix.varFieldAnnotation()
+ public void TMix.varFieldAnnotation_$eq(int)
+ private int TMix.varGetterAnnotation
+@RetainedAnnotation() public int TMix.varGetterAnnotation()
+ public void TMix.varGetterAnnotation_$eq(int)
+ private int TMix.varSetterAnnotation
+ public int TMix.varSetterAnnotation()
+@RetainedAnnotation() public void TMix.varSetterAnnotation_$eq(int)

test/files/run/t10075b/RetainedAnnotation_1.java

@@ -0,0 +1,4 @@
+import java.lang.annotation.*;
+
+@Retention(RetentionPolicy.RUNTIME)
+@interface RetainedAnnotation { }

test/files/run/t10075b/Test_2.scala

@@ -0,0 +1,56 @@
+class C {
+  @(RetainedAnnotation @annotation.meta.field)
+  lazy val lzyValFieldAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.getter)
+  lazy val lzyValGetterAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.field)
+  val valFieldAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.getter)
+  val valGetterAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.field)
+  var varFieldAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.getter)
+  var varGetterAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.setter)
+  var varSetterAnnotation = 42
+}
+
+trait T {
+  @(RetainedAnnotation @annotation.meta.field)
+  lazy val lzyValFieldAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.getter)
+  lazy val lzyValGetterAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.field)
+  val valFieldAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.getter)
+  val valGetterAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.field)
+  var varFieldAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.getter)
+  var varGetterAnnotation = 42
+
+  @(RetainedAnnotation @annotation.meta.setter)
+  var varSetterAnnotation = 42
+
+  @RetainedAnnotation
+  def method = 42
+}
+class TMix extends T
+
+object Test extends App {
+  (List(classOf[C], classOf[T], classOf[TMix]).
+    flatMap(cls => cls.getDeclaredFields ++ cls.getDeclaredMethods)).
+    sortBy(x => (x.getDeclaringClass.getName, x.getName, x.toString)).
+    foreach(x => println(x.getAnnotations.toList.mkString(" ") + " " + x))
+}