From 82cff1a94674e44e7a48a005f3a5ce84581800eb Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Fri, 15 Feb 2019 17:37:35 +0100 Subject: [PATCH 1/4] Fix #5924: don't move static fields to interfaces --- compiler/src/dotty/tools/dotc/transform/LazyVals.scala | 2 -- .../src/dotty/tools/dotc/transform/MoveStatics.scala | 6 ++++-- tests/run/i5924.scala | 9 +++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 tests/run/i5924.scala diff --git a/compiler/src/dotty/tools/dotc/transform/LazyVals.scala b/compiler/src/dotty/tools/dotc/transform/LazyVals.scala index b440804f9794..473197bc8eff 100644 --- a/compiler/src/dotty/tools/dotc/transform/LazyVals.scala +++ b/compiler/src/dotty/tools/dotc/transform/LazyVals.scala @@ -36,8 +36,6 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer { override def changesMembers: Boolean = true // the phase adds lazy val accessors - def transformer: LazyVals = new LazyVals - val containerFlags: FlagSet = Synthetic | Mutable | Lazy val initFlags: FlagSet = Synthetic | Method diff --git a/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala b/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala index 059ab19165cf..1356c60683ea 100644 --- a/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala +++ b/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala @@ -24,7 +24,8 @@ class MoveStatics extends MiniPhase with SymTransformer { override def phaseName: String = MoveStatics.name def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = { - if (sym.hasAnnotation(defn.ScalaStaticAnnot) && sym.owner.is(Flags.Module) && sym.owner.companionClass.exists) { + if (sym.hasAnnotation(defn.ScalaStaticAnnot) && sym.owner.is(Flags.Module) && + sym.owner.companionClass.exists && !sym.owner.companionClass.is(Flags.Trait)) { sym.owner.asClass.delete(sym.symbol) sym.owner.companionClass.asClass.enter(sym.symbol) val flags = if (sym.is(Flags.Method)) sym.flags else sym.flags | Flags.Mutable @@ -60,9 +61,10 @@ class MoveStatics extends MiniPhase with SymTransformer { def move(module: TypeDef, companion: TypeDef): List[Tree] = { assert(companion ne module) if (!module.symbol.is(Flags.Module)) move(companion, module) + else if (companion != null && companion.symbol.is(Flags.Trait)) List(module, companion) else { val allMembers = - (if(companion ne null) {companion.rhs.asInstanceOf[Template].body} else Nil) ++ + (if(companion != null) {companion.rhs.asInstanceOf[Template].body} else Nil) ++ module.rhs.asInstanceOf[Template].body val (newModuleBody, newCompanionBody) = allMembers.partition(x => {assert(x.symbol.exists); x.symbol.owner == module.symbol}) Trees.flatten(rebuild(companion, newCompanionBody) :: rebuild(module, newModuleBody) :: Nil) diff --git a/tests/run/i5924.scala b/tests/run/i5924.scala new file mode 100644 index 000000000000..595ed5f41070 --- /dev/null +++ b/tests/run/i5924.scala @@ -0,0 +1,9 @@ +trait Matchers { + object Helper +} + +object Matchers extends Matchers + +object Test { + def main(args: Array[String]): Unit = Matchers +} From 70041d29000d65f1f40c7555cb3e18704f14c580 Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Sun, 17 Feb 2019 00:49:11 +0100 Subject: [PATCH 2/4] Move @static immutable fields & methods to companion traits The original code in addition adds the flag `Mutable` while moving, which does not respect Scala semantics. --- .../tools/dotc/transform/MoveStatics.scala | 10 ++-- .../tools/backend/jvm/DottyBytecodeTest.scala | 4 ++ .../backend/jvm/DottyBytecodeTests.scala | 46 +++++++++++++++++++ tests/run/i5924b.scala | 13 ++++++ tests/run/i5924c.scala | 15 ++++++ 5 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 tests/run/i5924b.scala create mode 100644 tests/run/i5924c.scala diff --git a/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala b/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala index 1356c60683ea..e9e9cf6aeed4 100644 --- a/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala +++ b/compiler/src/dotty/tools/dotc/transform/MoveStatics.scala @@ -24,12 +24,11 @@ class MoveStatics extends MiniPhase with SymTransformer { override def phaseName: String = MoveStatics.name def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = { - if (sym.hasAnnotation(defn.ScalaStaticAnnot) && sym.owner.is(Flags.Module) && - sym.owner.companionClass.exists && !sym.owner.companionClass.is(Flags.Trait)) { + if (sym.hasAnnotation(defn.ScalaStaticAnnot) && sym.owner.is(Flags.Module) && sym.owner.companionClass.exists && + (sym.is(Flags.Method) || !(sym.is(Flags.Mutable) && sym.owner.companionClass.is(Flags.Trait)))) { sym.owner.asClass.delete(sym.symbol) sym.owner.companionClass.asClass.enter(sym.symbol) - val flags = if (sym.is(Flags.Method)) sym.flags else sym.flags | Flags.Mutable - sym.copySymDenotation(owner = sym.owner.companionClass, initFlags = flags) + sym.copySymDenotation(owner = sym.owner.companionClass) } else sym } @@ -59,9 +58,8 @@ class MoveStatics extends MiniPhase with SymTransformer { } def move(module: TypeDef, companion: TypeDef): List[Tree] = { - assert(companion ne module) + assert(companion != module) if (!module.symbol.is(Flags.Module)) move(companion, module) - else if (companion != null && companion.symbol.is(Flags.Trait)) List(module, companion) else { val allMembers = (if(companion != null) {companion.rhs.asInstanceOf[Template].body} else Nil) ++ diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala index 1c8648455c7e..842164339b24 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala @@ -73,6 +73,10 @@ trait DottyBytecodeTest { classNode.methods.asScala.find(_.name == name) getOrElse sys.error(s"Didn't find method '$name' in class '${classNode.name}'") + protected def getField(classNode: ClassNode, name: String): FieldNode = + classNode.fields.asScala.find(_.name == name) getOrElse + sys.error(s"Didn't find field '$name' in class '${classNode.name}'") + def diffInstructions(isa: List[Instruction], isb: List[Instruction]): String = { val len = Math.max(isa.length, isb.length) val sb = new StringBuilder diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala index 75c90fe36a0d..ad5e443b919d 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala @@ -695,4 +695,50 @@ class TestBCode extends DottyBytecodeTest { "`test` was not properly generated\n" + diffInstructions(instructions, expected)) } } + + @Test def i5924b = { + val source = + """|import scala.annotation.static + |trait Base + | + |object Base { + | @static val x = 10 + | @static final val y = 10 + | @static def f: Int = 30 + |} + """.stripMargin + + checkBCode(source) { dir => + val clsIn = dir.lookupName("Base.class", directory = false).input + val clsNode = loadClassNode(clsIn) + getMethod(clsNode, "f") + getField(clsNode, "x") + getField(clsNode, "y") + } + } + + @Test def i5924c = { + val source = + """|import scala.annotation.static + |class Base + | + |object Base { + | @static val x = 10 + | @static final val y = 10 + | @static var a = 10 + | @static final var b = 10 + | @static def f: Int = 30 + |} + """.stripMargin + + checkBCode(source) { dir => + val clsIn = dir.lookupName("Base.class", directory = false).input + val clsNode = loadClassNode(clsIn) + getMethod(clsNode, "f") + getField(clsNode, "x") + getField(clsNode, "y") + getField(clsNode, "a") + getField(clsNode, "b") + } + } } diff --git a/tests/run/i5924b.scala b/tests/run/i5924b.scala new file mode 100644 index 000000000000..9d76a53a1da5 --- /dev/null +++ b/tests/run/i5924b.scala @@ -0,0 +1,13 @@ +import scala.annotation.static + +trait Base + +object Base { + @static val x = 10 + @static final val y = 10 + @static def f: Int = 30 +} + +object Test { + def main(args: Array[String]): Unit = Base +} \ No newline at end of file diff --git a/tests/run/i5924c.scala b/tests/run/i5924c.scala new file mode 100644 index 000000000000..c86076d7e4e8 --- /dev/null +++ b/tests/run/i5924c.scala @@ -0,0 +1,15 @@ +import scala.annotation.static + +class Base + +object Base { + @static val x = 10 + @static final val y = 10 + @static var a = 10 + @static final var b = 10 + @static def f: Int = 30 +} + +object Test { + def main(args: Array[String]): Unit = Base +} \ No newline at end of file From 32e43f75435719f868693350bf7513f891c99dd8 Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Sun, 17 Feb 2019 01:18:10 +0100 Subject: [PATCH 3/4] Check never emit non-final fields in traits --- compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala index dd04f7ddec2d..52952976094e 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala @@ -231,6 +231,9 @@ trait BCodeSkelBuilder extends BCodeHelpers { val javagensig = getGenericSignature(f, claszSymbol) val flags = javaFieldFlags(f) + assert(!f.isStaticMember || !claszSymbol.isInterface || !f.isMutable, + s"interface $claszSymbol cannot have non-final static field $f") + val jfield = new asm.tree.FieldNode( flags, f.javaSimpleName.toString, From ca0da0f69ba24fd47ae3ee214161c91c3db9d485 Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Tue, 19 Feb 2019 16:14:09 +0100 Subject: [PATCH 4/4] Address review --- .../backend/jvm/DottyBytecodeTests.scala | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala index ad5e443b919d..ecb876bfae2a 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala @@ -711,9 +711,14 @@ class TestBCode extends DottyBytecodeTest { checkBCode(source) { dir => val clsIn = dir.lookupName("Base.class", directory = false).input val clsNode = loadClassNode(clsIn) - getMethod(clsNode, "f") - getField(clsNode, "x") - getField(clsNode, "y") + val f = getMethod(clsNode, "f") + val x = getField(clsNode, "x") + val y = getField(clsNode, "y") + assert((f.access & Opcodes.ACC_STATIC) != 0) + List(x, y).foreach { node => + assert((node.access & Opcodes.ACC_STATIC) != 0) + assert((node.access & Opcodes.ACC_FINAL) != 0) + } } } @@ -734,11 +739,19 @@ class TestBCode extends DottyBytecodeTest { checkBCode(source) { dir => val clsIn = dir.lookupName("Base.class", directory = false).input val clsNode = loadClassNode(clsIn) - getMethod(clsNode, "f") - getField(clsNode, "x") - getField(clsNode, "y") - getField(clsNode, "a") - getField(clsNode, "b") + val f = getMethod(clsNode, "f") + val x = getField(clsNode, "x") + val y = getField(clsNode, "y") + val a = getField(clsNode, "a") + val b = getField(clsNode, "b") + assert((f.access & Opcodes.ACC_STATIC) != 0) + List(x, y).foreach { node => + assert((node.access & Opcodes.ACC_STATIC) != 0) + assert((node.access & Opcodes.ACC_FINAL) != 0) + } + List(a, b).foreach { node => + assert((node.access & Opcodes.ACC_STATIC) != 0) + } } } }