Merge pull request #7270 from retronym/topic/static-object-fields

Make object fields static, and move post-super init to <clinit>

Author: lrytz

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

@@ -563,23 +563,10 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
           generatedType = genTypeApply()
 
         case Apply(fun @ Select(sup @ Super(superQual, _), _), args) =>
-          def initModule(): Unit = {
-            // we initialize the MODULE$ field immediately after the super ctor
-            if (!initModuleInClinit && !isModuleInitialized &&
-              jMethodName == INSTANCE_CONSTRUCTOR_NAME &&
-              fun.symbol.javaSimpleName.toString == INSTANCE_CONSTRUCTOR_NAME &&
-              isStaticModuleClass(claszSymbol)) {
-              isModuleInitialized = true
-              mnode.visitVarInsn(asm.Opcodes.ALOAD, 0)
-              assignModuleInstanceField(mnode)
-            }
-          }
-
           // scala/bug#10290: qual can be `this.$outer()` (not just `this`), so we call genLoad (not just ALOAD_0)
           genLoad(superQual)
           genLoadArguments(args, paramTKs(app))
           generatedType = genCallMethod(fun.symbol, InvokeStyle.Super, app.pos, sup.tpe.typeSymbol)
-          initModule()
 
         // 'new' constructor call: Note: since constructors are
         // thought to return an instance of what they construct,

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

@@ -70,7 +70,6 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
     var claszSymbol: Symbol        = null
     var isCZParcelable             = false
     var isCZStaticModule           = false
-    var initModuleInClinit         = false
 
     /* ---------------- idiomatic way to ask questions to typer ---------------- */
 
@@ -102,26 +101,51 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
 
     /* ---------------- helper utils for generating classes and fields ---------------- */
 
-    def genPlainClass(cd: ClassDef): Unit = {
+    def genPlainClass(cd0: ClassDef): Unit = {
       assert(cnode == null, "GenBCode detected nested methods.")
 
-      claszSymbol       = cd.symbol
+      claszSymbol       = cd0.symbol
       isCZParcelable    = isAndroidParcelableClass(claszSymbol)
       isCZStaticModule  = isStaticModuleClass(claszSymbol)
       thisBType         = classBTypeFromSymbol(claszSymbol)
-      initModuleInClinit = isCZStaticModule && canAssignModuleInClinit(cd, claszSymbol)
 
       cnode = new ClassNode1()
 
       initJClass(cnode)
+      val cd = if (isCZStaticModule) {
+        // Move statements from the primary constructor following the superclass constructor call to
+        // a newly synthesised tree representing the "<clinit>", which also assigns the MODULE$ field.
+        // Because the assigments to both the module instance fields, and the fields of the module itself
+        // are in the <clinit>, these fields can be static + final.
 
-      val hasStaticCtor = methodSymbols(cd) exists (_.isStaticConstructor)
-      if (!hasStaticCtor) {
-        // but needs one ...
-        if (isCZStaticModule || isCZParcelable) {
-          fabricateStaticInit()
+        // TODO should we do this transformation earlier, say in Constructors? Or would that just cause
+        // pain for scala-{js, native}?
+
+        for (f <- fieldSymbols(claszSymbol)) {
+          f.setFlag(Flags.STATIC)
         }
-      }
+        val constructorDefDef = treeInfo.firstConstructor(cd0.impl.body).asInstanceOf[DefDef]
+        val (uptoSuperStats, remainingConstrStats) = treeInfo.splitAtSuper(constructorDefDef.rhs.asInstanceOf[Block].stats, classOnly = true)
+        val clInitSymbol = claszSymbol.newMethod(nme.CLASS_CONSTRUCTOR, claszSymbol.pos, Flags.STATIC).setInfo(NullaryMethodType(definitions.UnitTpe))
+
+        // We don't need to enter this field into the decls of claszSymbol.info as this is added manually to the generated class
+        // in addModuleInstanceField. TODO: try adding it to the decls and making the usual field generation do the right thing.
+        val moduleField = claszSymbol.newValue(nme.MODULE_INSTANCE_FIELD, claszSymbol.pos, Flags.STATIC | Flags.PRIVATE).setInfo(claszSymbol.tpeHK)
+
+        val callConstructor = NewFromConstructor(claszSymbol.primaryConstructor).setType(claszSymbol.tpeHK)
+        val assignModuleField = Assign(global.gen.mkAttributedRef(moduleField).setType(claszSymbol.tpeHK), callConstructor).setType(definitions.UnitTpe)
+        val remainingConstrStatsSubst = remainingConstrStats.map(_.substituteThis(claszSymbol, global.gen.mkAttributedRef(claszSymbol.sourceModule)).changeOwner(claszSymbol.primaryConstructor -> clInitSymbol))
+        val clinit = DefDef(clInitSymbol, Block(assignModuleField :: remainingConstrStatsSubst, Literal(Constant(())).setType(definitions.UnitTpe)).setType(definitions.UnitTpe))
+        deriveClassDef(cd0)(tmpl => deriveTemplate(tmpl)(body =>
+          clinit :: body.map {
+            case `constructorDefDef` => copyDefDef(constructorDefDef)(rhs = Block(uptoSuperStats, constructorDefDef.rhs.asInstanceOf[Block].expr))
+            case tree => tree
+          }
+        ))
+      } else cd0
+
+      val hasStaticCtor = methodSymbols(cd) exists (_.isStaticConstructor)
+      if (!hasStaticCtor && isCZParcelable) fabricateStaticInitAndroid()
 
       val optSerial: Option[Long] = serialVUID(claszSymbol)
       /* serialVersionUID can't be put on interfaces (it's a private field).
@@ -204,17 +228,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
      */
     private def addModuleInstanceField(): Unit = {
       // TODO confirm whether we really don't want ACC_SYNTHETIC nor ACC_DEPRECATED
-      // scala/scala-dev#194:
-      //   This can't be FINAL on JVM 1.9+ because we assign it from within the
-      //   instance constructor, not from <clinit> directly. Assignment from <clinit>,
-      //   after the constructor has completely finished, seems like the principled
-      //   thing to do, but it would change behaviour when "benign" cyclic references
-      //   between modules exist.
-      //
-      // We special case modules with parents that we know don't (and won't ever) refer to
-      // the module during their construction. These can use a final field, and defer the assigment
-      // to <clinit>.
-      val mods = if (initModuleInClinit) GenBCode.PublicStaticFinal else GenBCode.PublicStatic
+      val mods = GenBCode.PublicStaticFinal
       val fv =
         cnode.visitField(mods,
                          strMODULE_INSTANCE_FIELD,
@@ -232,7 +246,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
     /*
      * must-single-thread
      */
-    private def fabricateStaticInit(): Unit = {
+    private def fabricateStaticInitAndroid(): Unit = {
 
       val clinit: asm.MethodVisitor = cnode.visitMethod(
         GenBCode.PublicStatic, // TODO confirm whether we really don't want ACC_SYNTHETIC nor ACC_DEPRECATED
@@ -243,20 +257,9 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
       )
       clinit.visitCode()
 
-      /* "legacy static initialization" */
-      if (isCZStaticModule) {
-        clinit.visitTypeInsn(asm.Opcodes.NEW, thisBType.internalName)
-        if (initModuleInClinit) clinit.visitInsn(asm.Opcodes.DUP)
-
-        clinit.visitMethodInsn(asm.Opcodes.INVOKESPECIAL,
-        thisBType.internalName, INSTANCE_CONSTRUCTOR_NAME, "()V", false)
-        if (initModuleInClinit) {
-          assignModuleInstanceField(clinit)
-        }
-      }
       if (isCZParcelable) { legacyAddCreatorCode(clinit, cnode, thisBType.internalName) }
-      clinit.visitInsn(asm.Opcodes.RETURN)
 
+      clinit.visitInsn(asm.Opcodes.RETURN)
       clinit.visitMaxs(0, 0) // just to follow protocol, dummy arguments
       clinit.visitEnd()
     }

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

@@ -21,7 +21,6 @@ import scala.annotation.{switch, tailrec}
 import scala.collection.JavaConverters._
 import scala.collection.immutable.BitSet
 import scala.collection.mutable
-import scala.reflect.ClassTag
 import scala.reflect.internal.util.Position
 import scala.tools.asm
 import scala.tools.asm.Opcodes._
@@ -348,7 +347,7 @@ abstract class BackendUtils extends PerRunInit {
    *   - methods accessing a public field could be inlined. on the other hand, methods accessing a private
    *     static field should not be inlined.
    */
-  def looksLikeForwarderOrFactoryOrTrivial(method: MethodNode, allowPrivateCalls: Boolean): Int = {
+  def looksLikeForwarderOrFactoryOrTrivial(method: MethodNode, owner: InternalName, allowPrivateCalls: Boolean): Int = {
     val paramTypes = Type.getArgumentTypes(method.desc)
     val numPrimitives = paramTypes.count(_.getSort < Type.ARRAY) + (if (Type.getReturnType(method.desc).getSort < Type.ARRAY) 1 else 0)
 
@@ -379,8 +378,14 @@ abstract class BackendUtils extends PerRunInit {
             callMi = mi
           }
         }
-      } else if (i.getOpcode != GETSTATIC && nonForwarderInstructionTypes(t))
-        numCallsOrNew = 2 // stop here: not forwarder or trivial
+      } else if (nonForwarderInstructionTypes(t)) {
+        if (i.getOpcode == GETSTATIC) {
+          if (!allowPrivateCalls && owner == i.asInstanceOf[FieldInsnNode].owner)
+            numCallsOrNew = 2 // stop here: not forwarder or trivial
+        } else {
+          numCallsOrNew = 2 // stop here: not forwarder or trivial
+        }
+      }
     }
     if (numCallsOrNew > 1 || numBoxConv > paramTypes.length + 1) -1
     else if (numCallsOrNew == 0) if (numBoxConv == 0) 1 else 3

src/compiler/scala/tools/nsc/backend/jvm/opt/InlinerHeuristics.scala

@@ -202,7 +202,7 @@ abstract class InlinerHeuristics extends PerRunInit {
     // or aliases, because otherwise it's too confusing for users looking at generated code, they will
     // write a small test method and think the inliner doesn't work correctly.
     val isGeneratedForwarder =
-      BytecodeUtils.isSyntheticMethod(callsite.callsiteMethod) && backendUtils.looksLikeForwarderOrFactoryOrTrivial(callsite.callsiteMethod, allowPrivateCalls = true) > 0 ||
+      BytecodeUtils.isSyntheticMethod(callsite.callsiteMethod) && backendUtils.looksLikeForwarderOrFactoryOrTrivial(callsite.callsiteMethod, callsite.callsiteClass.internalName, allowPrivateCalls = true) > 0 ||
         backendUtils.isMixinForwarder(callsite.callsiteMethod, callsite.callsiteClass) // seems mixin forwarders are not synthetic...
 
     if (isGeneratedForwarder) None
@@ -248,7 +248,7 @@ abstract class InlinerHeuristics extends PerRunInit {
             val isTraitSuperAccessor = backendUtils.isTraitSuperAccessor(callee.callee, callee.calleeDeclarationClass)
             if (isTraitSuperAccessor) null
             else {
-              val forwarderKind = backendUtils.looksLikeForwarderOrFactoryOrTrivial(callee.callee, allowPrivateCalls = false)
+              val forwarderKind = backendUtils.looksLikeForwarderOrFactoryOrTrivial(callee.callee, callee.calleeDeclarationClass.internalName, allowPrivateCalls = false)
               if (forwarderKind < 0)
                 null
               else if (BytecodeUtils.isSyntheticMethod(callee.callee) || backendUtils.isMixinForwarder(callee.callee, callee.calleeDeclarationClass))

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

@@ -738,18 +738,7 @@ abstract class Constructors extends Statics with Transform with TypingTransforme
         copyParam(accSetter, parameter(acc))
       }
 
-      // Return a pair consisting of (all statements up to and including superclass and trait constr calls, rest)
-      def splitAtSuper(stats: List[Tree]) = {
-        def isConstr(tree: Tree): Boolean = tree match {
-          case Block(_, expr) => isConstr(expr) // scala/bug#6481 account for named argument blocks
-          case _              => (tree.symbol ne null) && tree.symbol.isConstructor
-        }
-        val (pre, rest0)       = stats span (!isConstr(_))
-        val (supercalls, rest) = rest0 span (isConstr(_))
-        (pre ::: supercalls, rest)
-      }
-
-      val (uptoSuperStats, remainingConstrStats) = splitAtSuper(constructorStats)
+      val (uptoSuperStats, remainingConstrStats) = treeInfo.splitAtSuper(constructorStats, classOnly = false)
 
       /* TODO: XXX This condition (`isDelayedInitSubclass && remainingConstrStats.nonEmpty`) is not correct:
       * remainingConstrStats.nonEmpty excludes too much,

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

@@ -674,7 +674,9 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
               // 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)
+              val isInStaticModule = fieldSel.owner.isModuleClass && fieldSel.owner.sourceModule.isStaticModule
+              if (!isInStaticModule) // the <clinit> lock is enough.
+                fieldSel.owner.primaryConstructor.updateAttachment(ConstructorNeedsFence)
             }
 
             afterOwnPhase { // the assign only type checks after our phase (assignment to val)

src/reflect/scala/reflect/internal/StdNames.scala

@@ -353,6 +353,7 @@ trait StdNames {
     // Compiler internal names
     val ANYname: NameType                  = "<anyname>"
     val CONSTRUCTOR: NameType              = "<init>"
+    val CLASS_CONSTRUCTOR: NameType        = "<clinit>"
     val DEFAULT_CASE: NameType             = "defaultCase$"
     val EQEQ_LOCAL_VAR: NameType           = "eqEqTemp$"
     val FAKE_LOCAL_THIS: NameType          = "this$"

src/reflect/scala/reflect/internal/TreeInfo.scala

@@ -1085,4 +1085,16 @@ trait MacroAnnotionTreeInfo { self: TreeInfo =>
       SyntacticClassDef(mods, name, tparams, constrMods, vparamss.map(_.map(_.duplicate)), earlyDefs, parents, selfType, body)
     }
   }
+
+  // Return a pair consisting of (all statements up to and including superclass and trait constr calls, rest)
+  final def splitAtSuper(stats: List[Tree], classOnly: Boolean): (List[Tree], List[Tree]) = {
+    def isConstr(tree: Tree): Boolean = tree match {
+      case Block(_, expr) => isConstr(expr) // scala/bug#6481 account for named argument blocks
+      case _              => (tree.symbol ne null) && (if (classOnly) tree.symbol.isClassConstructor else tree.symbol.isConstructor)
+    }
+    val (pre, rest0)       = stats span (!isConstr(_))
+    val (supercalls, rest) = rest0 span (isConstr(_))
+    (pre ::: supercalls, rest)
+  }
+
 }

src/repl/scala/tools/nsc/interpreter/IMain.scala

@@ -607,7 +607,7 @@ class IMain(val settings: Settings, parentClassLoaderOverride: Option[ClassLoade
       val classNameRegex = s"$lineRegex.*".r
       def isWrapperCode(x: StackTraceElement) = cond(x.getClassName) {
         case classNameRegex() =>
-          x.getMethodName == nme.CONSTRUCTOR.decoded || x.getMethodName == printName
+          x.getMethodName == nme.CONSTRUCTOR.decoded || x.getMethodName == "<clinit>" || x.getMethodName == printName
       }
       val stackTrace = unwrapped.stackTracePrefixString(!isWrapperCode(_))
 

test/files/jvm/scala-concurrent-tck.scala

@@ -37,7 +37,7 @@ trait TestBase {
 }
 
 
-trait FutureCallbacks extends TestBase {
+class FutureCallbacks extends TestBase {
   import ExecutionContext.Implicits._
 
   def testOnSuccess(): Unit = once {
@@ -137,7 +137,7 @@ trait FutureCallbacks extends TestBase {
 }
 
 
-trait FutureCombinators extends TestBase {
+class FutureCombinators extends TestBase {
   import ExecutionContext.Implicits._
 
   def testMapSuccess(): Unit = once {
@@ -604,7 +604,7 @@ def testTransformFailure(): Unit = once {
 }
 
 
-trait FutureProjections extends TestBase {
+class FutureProjections extends TestBase {
   import ExecutionContext.Implicits._
 
   def testFailedFailureOnComplete(): Unit = once {
@@ -696,7 +696,7 @@ trait FutureProjections extends TestBase {
 }
 
 
-trait Blocking extends TestBase {
+class Blocking extends TestBase {
   import ExecutionContext.Implicits._
 
   def testAwaitSuccess(): Unit = once {
@@ -728,7 +728,7 @@ trait Blocking extends TestBase {
   test("testFQCNForAwaitAPI")(testFQCNForAwaitAPI())
 }
 
-trait BlockContexts extends TestBase {
+class BlockContexts extends TestBase {
   import ExecutionContext.Implicits._
   import scala.concurrent.{ Await, Awaitable, BlockContext }
 
@@ -785,7 +785,7 @@ trait BlockContexts extends TestBase {
   test("testPopCustom")(testPopCustom())
 }
 
-trait Promises extends TestBase {
+class Promises extends TestBase {
   import ExecutionContext.Implicits._
 
   def testSuccess(): Unit = once {
@@ -820,7 +820,7 @@ trait Promises extends TestBase {
 }
 
 
-trait Exceptions extends TestBase {
+class Exceptions extends TestBase {
   import java.util.concurrent.{Executors, RejectedExecutionException}
   def interruptHandling(): Unit = {
     implicit val e = ExecutionContext.fromExecutorService(Executors.newFixedThreadPool(1))
@@ -846,7 +846,7 @@ trait Exceptions extends TestBase {
   test("rejectedExecutionException")(rejectedExecutionException())
 }
 
-trait GlobalExecutionContext extends TestBase {
+class GlobalExecutionContext extends TestBase {
   import ExecutionContext.Implicits._
 
   def testNameOfGlobalECThreads(): Unit = once {
@@ -859,7 +859,7 @@ trait GlobalExecutionContext extends TestBase {
   test("testNameOfGlobalECThreads")(testNameOfGlobalECThreads())
 }
 
-trait CustomExecutionContext extends TestBase {
+class CustomExecutionContext extends TestBase {
   import scala.concurrent.{ ExecutionContext, Awaitable }
 
   def defaultEC = ExecutionContext.global
@@ -1008,7 +1008,7 @@ trait CustomExecutionContext extends TestBase {
   test("testCallbackChainCustomEC")(testCallbackChainCustomEC())
 }
 
-trait ExecutionContextPrepare extends TestBase {
+class ExecutionContextPrepare extends TestBase {
   val theLocal = new ThreadLocal[String] {
     override protected def initialValue(): String = ""
   }
@@ -1062,17 +1062,17 @@ trait ExecutionContextPrepare extends TestBase {
 }
 
 object Test
-extends App
-with FutureCallbacks
-with FutureCombinators
-with FutureProjections
-with Promises
-with BlockContexts
-with Exceptions
-with GlobalExecutionContext
-with CustomExecutionContext
-with ExecutionContextPrepare
-{
+extends App {
+  new FutureCallbacks
+  new FutureCombinators
+  new FutureProjections
+  new Promises
+  new BlockContexts
+  new Exceptions
+  new GlobalExecutionContext
+  new CustomExecutionContext
+  new ExecutionContextPrepare
+
   System.exit(0)
 }
 

test/files/run/module-static.check

@@ -0,0 +1,3 @@
+c
+t
+o1