Skip to content

Commit

Permalink
Mixin fields with trait setters shouldn't be JVM final
Browse files Browse the repository at this point in the history
Before:

```
scala> trait T { val foo = 24 }; class C extends T
defined trait T
defined class C

scala> :javap -private -c C
Compiled from "<console>"
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."<init>":()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 "<console>"
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."<init>":()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
}
```
  • Loading branch information
retronym committed Aug 16, 2018
1 parent 00d9e14 commit f9ecece
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 13 deletions.
7 changes: 6 additions & 1 deletion src/compiler/scala/tools/nsc/transform/Constructors.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})

Expand Down
21 changes: 16 additions & 5 deletions src/compiler/scala/tools/nsc/transform/Fields.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -659,9 +659,20 @@ 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)) {
// If the field is mutable, it won't be final, so we can write to it in a setter.
// If it's not, we still need to initialize it, and make sure it's safely published.
// Since initialization is performed (lexically) outside of the constructor (in the trait setter),
// we have to make the field mutable starting with classfile format 53
// (it was never allowed, but the verifier enforces this now).
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 =
Expand Down
2 changes: 2 additions & 0 deletions src/reflect/scala/reflect/internal/StdAttachments.scala
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,6 @@ trait StdAttachments {
case object KnownDirectSubclassesCalled extends PlainAttachment

class QualTypeSymAttachment(val sym: Symbol)

case object ConstructorNeedsFence extends PlainAttachment
}
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/internal/StdNames.scala
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,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"
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse =>
this.UseInvokeSpecial
this.TypeParamVarargsAttachment
this.KnownDirectSubclassesCalled
this.ConstructorNeedsFence
this.noPrint
this.typeDebug
// inaccessible: this.posAssigner
Expand Down
23 changes: 18 additions & 5 deletions test/files/run/lazy-locals-2.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions test/files/run/t10075b.check
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit f9ecece

Please sign in to comment.