From 59ea12c7f5010a18fe2318896067b4fa6748ee26 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Fri, 4 May 2018 14:53:04 +1000 Subject: [PATCH] Mixin fields with trait setters shouldn't be JVM final Before: ``` scala> trait T { val foo = 24 }; class C extends T defined trait T defined class C scala> :javap -private -c C Compiled from "" public class $line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T { private final int foo; public int foo(); Code: 0: aload_0 1: getfield #21 // Field foo:I 4: ireturn public void $line3$$read$$iw$$iw$T$_setter_$foo_$eq(int); Code: 0: aload_0 1: iload_1 2: putfield #21 // Field foo:I 5: return public $line3.$read$$iw$$iw$C(); Code: 0: aload_0 1: invokespecial #30 // Method java/lang/Object."":()V 4: aload_0 5: invokestatic #34 // InterfaceMethod $line3/$read$$iw$$iw$T.$init$:(L$line3/$read$$iw$$iw$T;)V 8: return } ``` The assignment to the final field `foo` has always contravened the JVM spec, and this rule is enforced for any classfiles of format 53 and higher. After this patch: ``` scala> trait T { val foo = 24 }; class C extends T defined trait T defined class C scala> :javap -private -c C Compiled from "" public class $line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T { private int foo; public int foo(); Code: 0: aload_0 1: getfield #21 // Field foo:I 4: ireturn public void $line3$$read$$iw$$iw$T$_setter_$foo_$eq(int); Code: 0: aload_0 1: iload_1 2: putfield #21 // Field foo:I 5: return public $line3.$read$$iw$$iw$C(); Code: 0: aload_0 1: invokespecial #30 // Method java/lang/Object."":()V 4: aload_0 5: invokestatic #34 // InterfaceMethod $line3/$read$$iw$$iw$T.$init$:(L$line3/$read$$iw$$iw$T;)V 8: getstatic #40 // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$; 11: invokevirtual #43 // Method scala/runtime/ScalaRunTime$.releaseFence:()V 14: return } ``` --- .../tools/nsc/transform/Constructors.scala | 7 +++++- .../scala/tools/nsc/transform/Fields.scala | 16 +++++++++---- .../reflect/internal/StdAttachments.scala | 2 ++ .../scala/reflect/internal/StdNames.scala | 1 + .../reflect/runtime/JavaUniverseForce.scala | 1 + test/files/run/lazy-locals-2.scala | 23 +++++++++++++++---- test/files/run/t10075b.check | 4 ++-- 7 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/compiler/scala/tools/nsc/transform/Constructors.scala b/src/compiler/scala/tools/nsc/transform/Constructors.scala index e3d33fb2b548..99b3315b69c0 100644 --- a/src/compiler/scala/tools/nsc/transform/Constructors.scala +++ b/src/compiler/scala/tools/nsc/transform/Constructors.scala @@ -756,11 +756,16 @@ abstract class Constructors extends Statics with Transform with TypingTransforme if (isDelayedInitSubclass && remainingConstrStats.nonEmpty) delayedInitDefsAndConstrStats(defs, remainingConstrStats) else (Nil, remainingConstrStats) + val fence = if (clazz.primaryConstructor.hasAttachment[ConstructorNeedsFence.type]) { + val tree = localTyper.typedPos(clazz.primaryConstructor.pos)(gen.mkRuntimeCall(nme.releaseFence, Nil)) + tree :: Nil + } else Nil + // Assemble final constructor val primaryConstructor = deriveDefDef(primaryConstr)(_ => { treeCopy.Block( primaryConstrBody, - paramInits ::: constructorPrefix ::: uptoSuperStats ::: guardSpecializedInitializer(remainingConstrStatsDelayedInit), + paramInits ::: constructorPrefix ::: uptoSuperStats ::: guardSpecializedInitializer(remainingConstrStatsDelayedInit) ::: fence, primaryConstrBody.expr) }) diff --git a/src/compiler/scala/tools/nsc/transform/Fields.scala b/src/compiler/scala/tools/nsc/transform/Fields.scala index 0d6b2de65cfd..2b1e0e6dc311 100644 --- a/src/compiler/scala/tools/nsc/transform/Fields.scala +++ b/src/compiler/scala/tools/nsc/transform/Fields.scala @@ -422,12 +422,12 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor // println(s"expanded modules for $clazz: $expandedModules") // afterOwnPhase, so traits receive trait setters for vals (needs to be at finest grain to avoid looping) - val synthInSubclass = + val synthInSubclass: List[Symbol] = clazz.mixinClasses.flatMap(mixin => afterOwnPhase{mixin.info}.decls.toList.filter(accessorImplementedInSubclass)) // mixin field accessors -- // invariant: (accessorsMaybeNeedingImpl, mixedInAccessorAndFields).zipped.forall(case (acc, clone :: _) => `clone` is clone of `acc` case _ => true) - val mixedInAccessorAndFields = synthInSubclass.map{ member => + val mixedInAccessorAndFields: List[List[Symbol]] = synthInSubclass.map{ member => def cloneAccessor() = { val clonedAccessor = (member cloneSymbol clazz) setPos clazz.pos setMixedinAccessorFlags(member, clonedAccessor) @@ -652,9 +652,15 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor // trait val/var setter mixed into class else fieldAccess(setter) match { case NoSymbol => EmptyTree - case fieldSel => afterOwnPhase { // the assign only type checks after our phase (assignment to val) - mkAccessor(setter)(Assign(Select(This(clazz), fieldSel), castHack(Ident(setter.firstParam), fieldSel.info))) - } + case fieldSel => + if (!fieldSel.hasFlag(MUTABLE)) { + fieldSel.setFlag(MUTABLE) + fieldSel.owner.primaryConstructor.updateAttachment(ConstructorNeedsFence) + } + + afterOwnPhase { // the assign only type checks after our phase (assignment to val) + mkAccessor(setter)(Assign(Select(This(clazz), fieldSel), castHack(Ident(setter.firstParam), fieldSel.info))) + } } def moduleAccessorBody(module: Symbol): Tree = diff --git a/src/reflect/scala/reflect/internal/StdAttachments.scala b/src/reflect/scala/reflect/internal/StdAttachments.scala index e704632b4991..bd560c7ef1b0 100644 --- a/src/reflect/scala/reflect/internal/StdAttachments.scala +++ b/src/reflect/scala/reflect/internal/StdAttachments.scala @@ -107,4 +107,6 @@ trait StdAttachments { case object KnownDirectSubclassesCalled extends PlainAttachment class QualTypeSymAttachment(val sym: Symbol) + + case object ConstructorNeedsFence extends PlainAttachment } diff --git a/src/reflect/scala/reflect/internal/StdNames.scala b/src/reflect/scala/reflect/internal/StdNames.scala index a86895d4d313..b3f7ba8971cf 100644 --- a/src/reflect/scala/reflect/internal/StdNames.scala +++ b/src/reflect/scala/reflect/internal/StdNames.scala @@ -771,6 +771,7 @@ trait StdNames { val productPrefix: NameType = "productPrefix" val raw_ : NameType = "raw" val readResolve: NameType = "readResolve" + val releaseFence: NameType = "releaseFence" val reify : NameType = "reify" val reificationSupport : NameType = "reificationSupport" val rootMirror : NameType = "rootMirror" diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala index 8c59a96081a2..88bbc529878e 100644 --- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala +++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala @@ -51,6 +51,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.UseInvokeSpecial this.TypeParamVarargsAttachment this.KnownDirectSubclassesCalled + this.ConstructorNeedsFence this.noPrint this.typeDebug this.Range diff --git a/test/files/run/lazy-locals-2.scala b/test/files/run/lazy-locals-2.scala index d6c33cffcb85..526376fb4d91 100644 --- a/test/files/run/lazy-locals-2.scala +++ b/test/files/run/lazy-locals-2.scala @@ -281,8 +281,8 @@ object Test { lzyComputeMethods == expComputeMethods, s"wrong lzycompute methods. expected:\n$expComputeMethods\nfound:\n$lzyComputeMethods") - val fields = c.getClass.getDeclaredFields.toList.sortBy(_.getName).map(_.toString) - val expFields = List( + val fields: List[String] = c.getClass.getDeclaredFields.toList.sortBy(_.getName).map(_.toString) + val expFields = List[String]( "private volatile byte C.bitmap$0", "private int C.lvl1", "private java.lang.String C.lvl2", @@ -305,9 +305,22 @@ object Test { d.run() val dFields = d.getClass.getDeclaredFields.toList.sortBy(_.getName).map(_.toString) - assert( - dFields == expFields.map(_.replaceAll(" C.", " D.")), - s"wrong fields. expected:\n$expFields\nfound:\n$fields") + val expDFields = List[String]( + "private volatile byte D.bitmap$0", + "private int D.lvl1", + "private java.lang.String D.lvl2", + "private scala.runtime.BoxedUnit D.lvl3", + "private int D.t1", + "private java.lang.String D.t2", + "private scala.runtime.BoxedUnit D.t3", + "private int D.vl1", + "private java.lang.String D.vl2", + "private scala.runtime.BoxedUnit D.vl3", + "private int D.vr1", + "private java.lang.String D.vr2", + "private scala.runtime.BoxedUnit D.vr3") + assert(dFields == expDFields, + s"wrong fields. expected:\n$expDFields\nfound:\n$dFields") val d1 = new D1 diff --git a/test/files/run/t10075b.check b/test/files/run/t10075b.check index dc64e95ac7aa..c1801c3c7761 100644 --- a/test/files/run/t10075b.check +++ b/test/files/run/t10075b.check @@ -45,9 +45,9 @@ @RetainedAnnotation() public int TMix.lzyValGetterAnnotation() private int TMix.lzyValGetterAnnotation$lzycompute() @RetainedAnnotation() public int TMix.method() -@RetainedAnnotation() private final int TMix.valFieldAnnotation +@RetainedAnnotation() private int TMix.valFieldAnnotation public int TMix.valFieldAnnotation() - private final int TMix.valGetterAnnotation + private int TMix.valGetterAnnotation @RetainedAnnotation() public int TMix.valGetterAnnotation() @RetainedAnnotation() private int TMix.varFieldAnnotation public int TMix.varFieldAnnotation()