From 7a57c6eec6c37e8ca3a7f182f0cf2604d7bc80df Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Wed, 30 Nov 2016 13:42:36 -0800 Subject: SI-10075 annotations go to lazy val's underlying field This likely regressed in #5294. Review feedback from retronym: - Tie annotation triaging a bit closer together durban kindly provided initial version of test/files/run/t10075.scala And pointed out you must force `lazy val`, since `null`-valued field is serializable regardless of its type. Test test/files/run/t10075b courtesy of retronym --- .../scala/tools/nsc/transform/Erasure.scala | 4 +- .../scala/tools/nsc/transform/Fields.scala | 43 +++++++++++----- .../scala/tools/nsc/typechecker/Namers.scala | 11 ++-- .../scala/tools/nsc/typechecker/Typers.scala | 2 +- test/files/run/t10075.scala | 35 +++++++++++++ test/files/run/t10075b.check | 60 ++++++++++++++++++++++ test/files/run/t10075b/RetainedAnnotation_1.java | 4 ++ test/files/run/t10075b/Test_2.scala | 56 ++++++++++++++++++++ 8 files changed, 196 insertions(+), 19 deletions(-) create mode 100644 test/files/run/t10075.scala create mode 100644 test/files/run/t10075b.check create mode 100644 test/files/run/t10075b/RetainedAnnotation_1.java create mode 100644 test/files/run/t10075b/Test_2.scala diff --git a/src/compiler/scala/tools/nsc/transform/Erasure.scala b/src/compiler/scala/tools/nsc/transform/Erasure.scala index 92accaf9dd..3ce7db35d8 100644 --- a/src/compiler/scala/tools/nsc/transform/Erasure.scala +++ b/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 diff --git a/src/compiler/scala/tools/nsc/transform/Fields.scala b/src/compiler/scala/tools/nsc/transform/Fields.scala index b09223110a..fbf1e8cec1 100644 --- a/src/compiler/scala/tools/nsc/transform/Fields.scala +++ b/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) diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala index 78e8c8c073..27a586e543 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala +++ b/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) || diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 78533bdfc5..82733c1643 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -2034,7 +2034,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 = diff --git a/test/files/run/t10075.scala b/test/files/run/t10075.scala new file mode 100644 index 0000000000..e7564c5c8b --- /dev/null +++ b/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 + } +} diff --git a/test/files/run/t10075b.check b/test/files/run/t10075b.check new file mode 100644 index 0000000000..dc64e95ac7 --- /dev/null +++ b/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) diff --git a/test/files/run/t10075b/RetainedAnnotation_1.java b/test/files/run/t10075b/RetainedAnnotation_1.java new file mode 100644 index 0000000000..86ac939ec7 --- /dev/null +++ b/test/files/run/t10075b/RetainedAnnotation_1.java @@ -0,0 +1,4 @@ +import java.lang.annotation.*; + +@Retention(RetentionPolicy.RUNTIME) +@interface RetainedAnnotation { } diff --git a/test/files/run/t10075b/Test_2.scala b/test/files/run/t10075b/Test_2.scala new file mode 100644 index 0000000000..89ba2bd488 --- /dev/null +++ b/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)) +} \ No newline at end of file -- cgit v1.2.3