Clean up and document some usages of flags in the backend

9 files changed, 154 insertions, 44 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
index bffa4bc51d..b79cf01f41 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
@@ -977,7 +977,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
         if (!isModuleInitialized &&
             jMethodName == INSTANCE_CONSTRUCTOR_NAME &&
             jname == INSTANCE_CONSTRUCTOR_NAME &&
-            isStaticModule(siteSymbol)) {
+            isStaticModuleClass(siteSymbol)) {
           isModuleInitialized = true
           mnode.visitVarInsn(asm.Opcodes.ALOAD, 0)
           mnode.visitFieldInsn(
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
index effc68c5e3..f8ecb8e643 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
@@ -95,7 +95,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
 
       claszSymbol       = cd.symbol
       isCZParcelable    = isAndroidParcelableClass(claszSymbol)
-      isCZStaticModule  = isStaticModule(claszSymbol)
+      isCZStaticModule  = isStaticModuleClass(claszSymbol)
       isCZRemote        = isRemote(claszSymbol)
       thisName          = internalName(claszSymbol)
 
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala
index 9b131cb123..b373f8d74d 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala
@@ -627,20 +627,52 @@ abstract class BCodeTypes extends BCodeIdiomatic {
     false
   }
 
-  /*
+  /**
    * must-single-thread
+   *
+   * True for module classes of package level objects. The backend will generate a mirror class for
+   * such objects.
    */
-  def isTopLevelModule(sym: Symbol): Boolean = {
-    exitingPickler { sym.isModuleClass && !sym.isImplClass && !sym.isNestedClass }
+  def isTopLevelModuleClass(sym: Symbol): Boolean = exitingPickler {
+    // phase travel to pickler required for isNestedClass (looks at owner)
+    val r = sym.isModuleClass && !sym.isNestedClass
+    // The mixin phase adds the `lateMODULE` flag to trait implementation classes. Since the flag
+    // is late, it should not be visible here inside the time travel. We check this.
+    if (r) assert(!sym.isImplClass, s"isModuleClass should be false for impl class $sym")
+    r
   }
 
-  /*
+  /**
    * must-single-thread
+   *
+   * True for module classes of modules that are top-level or owned only by objects. Module classes
+   * for such objects will get a MODULE$ flag and a corresponding static initializer.
    */
-  def isStaticModule(sym: Symbol): Boolean = {
-    sym.isModuleClass && !sym.isImplClass && !sym.isLifted
+  def isStaticModuleClass(sym: Symbol): Boolean = {
+    /* The implementation of this method is tricky because it is a source-level property. Various
+     * phases changed the symbol's properties in the meantime.
+     *
+     * (1) Phase travel to to pickler is required to exclude implementation classes; they have the
+     * lateMODULEs after mixin, so isModuleClass would be true.
+     *
+     * (2) We cannot use `sym.isStatic` because lambdalift modified (destructively) the owner. For
+     * example, in
+     *   object T { def f { object U } }
+     * the owner of U is T, so UModuleClass.isStatic is true. Phase travel does not help here.
+     * So we basically re-implement `sym.isStaticOwner`, but using the original owner chain.
+     */
+
+    def isOriginallyStaticOwner(sym: Symbol): Boolean = {
+      sym.isPackageClass || sym.isModuleClass && isOriginallyStaticOwner(sym.originalOwner)
+    }
+
+    exitingPickler { // (1)
+      sym.isModuleClass &&
+      isOriginallyStaticOwner(sym.originalOwner) // (2)
+    }
   }
 
+
   // ---------------------------------------------------------------------
   // ---------------- InnerClasses attribute (JVMS 4.7.6) ----------------
   // ---------------------------------------------------------------------
@@ -743,7 +775,7 @@ abstract class BCodeTypes extends BCodeIdiomatic {
         null
       else {
         val outerName = innerSym.rawowner.javaBinaryName
-        if (isTopLevelModule(innerSym.rawowner)) nme.stripModuleSuffix(outerName)
+        if (isTopLevelModuleClass(innerSym.rawowner)) nme.stripModuleSuffix(outerName)
         else outerName
       }
     }
@@ -825,7 +857,7 @@ abstract class BCodeTypes extends BCodeIdiomatic {
     // new instances via outerClassInstance.new InnerModuleClass$().
     // TODO: do this early, mark the symbol private.
     val privateFlag =
-      sym.isPrivate || (sym.isPrimaryConstructor && isTopLevelModule(sym.owner))
+      sym.isPrivate || (sym.isPrimaryConstructor && isTopLevelModuleClass(sym.owner))
 
     // Symbols marked in source as `final` have the FINAL flag. (In the past, the flag was also
     // added to modules and module classes, not anymore since 296b706).
@@ -854,7 +886,7 @@ abstract class BCodeTypes extends BCodeIdiomatic {
     // emit ACC_FINAL.
 
     val finalFlag = (
-         (((sym.rawflags & symtab.Flags.FINAL) != 0) || isTopLevelModule(sym))
+         (((sym.rawflags & symtab.Flags.FINAL) != 0) || isTopLevelModuleClass(sym))
       && !sym.enclClass.isInterface
       && !sym.isClassConstructor
       && !sym.isMutable // lazy vals and vars both
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala
index 6b192556d9..af5d01458a 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala
@@ -165,7 +165,7 @@ abstract class GenBCode extends BCodeSyncAndTry {
 
         // -------------- mirror class, if needed --------------
         val mirrorC =
-          if (isStaticModule(claszSymbol) && isTopLevelModule(claszSymbol)) {
+          if (isTopLevelModuleClass(claszSymbol)) {
             if (claszSymbol.companionClass == NoSymbol) {
               mirrorCodeGen.genMirrorClass(claszSymbol, cunit)
             } else {
diff --git a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
index 447fa66ae4..82c2a4d6ed 100644
--- a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
+++ b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -240,6 +240,12 @@ abstract class SymbolLoaders {
     }
   }
 
+  private def phaseBeforeRefchecks: Phase = {
+    var resPhase = phase
+    while (resPhase.refChecked) resPhase = resPhase.prev
+    resPhase
+  }
+
   /**
    * Load contents of a package
    */
@@ -248,19 +254,24 @@ abstract class SymbolLoaders {
 
     protected def doComplete(root: Symbol) {
       assert(root.isPackageClass, root)
-      root.setInfo(new PackageClassInfoType(newScope, root))
-
-      if (!root.isRoot) {
-        for (classRep <- classpath.classes) {
-          initializeFromClassPath(root, classRep)
-        }
-      }
-      if (!root.isEmptyPackageClass) {
-        for (pkg <- classpath.packages) {
-          enterPackage(root, pkg.name, new PackageLoader(pkg))
+      // Time travel to a phase before refchecks avoids an initialization issue. `openPackageModule`
+      // creates a module symbol and invokes invokes `companionModule` while the `infos` field is
+      // still null. This calls `isModuleNotMethod`, which forces the `info` if run after refchecks.
+      enteringPhase(phaseBeforeRefchecks) {
+        root.setInfo(new PackageClassInfoType(newScope, root))
+
+        if (!root.isRoot) {
+          for (classRep <- classpath.classes) {
+            initializeFromClassPath(root, classRep)
+          }
         }
+        if (!root.isEmptyPackageClass) {
+          for (pkg <- classpath.packages) {
+            enterPackage(root, pkg.name, new PackageLoader(pkg))
+          }
 
-        openPackageModule(root)
+          openPackageModule(root)
+        }
       }
     }
   }
@@ -290,7 +301,13 @@ abstract class SymbolLoaders {
 
     protected def doComplete(root: Symbol) {
       val start = if (Statistics.canEnable) Statistics.startTimer(classReadNanos) else null
-      classfileParser.parse(classfile, root)
+
+      // Running the classfile parser after refchecks can lead to "illegal class file dependency"
+      // errors. More concretely, the classfile parser calls "sym.companionModule", which calls
+      // "isModuleNotMethod" on the companion. After refchecks, this method forces the info, which
+      // may run the classfile parser. This produces the error.
+      enteringPhase(phaseBeforeRefchecks)(classfileParser.parse(classfile, root))
+
       if (root.associatedFile eq NoAbstractFile) {
         root match {
           // In fact, the ModuleSymbol forwards its setter to the module class
diff --git a/src/compiler/scala/tools/nsc/transform/Flatten.scala b/src/compiler/scala/tools/nsc/transform/Flatten.scala
index c3fbfae322..fa53ef48b5 100644
--- a/src/compiler/scala/tools/nsc/transform/Flatten.scala
+++ b/src/compiler/scala/tools/nsc/transform/Flatten.scala
@@ -76,8 +76,17 @@ abstract class Flatten extends InfoTransform {
             for (sym <- decls) {
               if (sym.isTerm && !sym.isStaticModule) {
                 decls1 enter sym
-                if (sym.isModule)
+                if (sym.isModule) {
+                  // Nested, non-static moduls are transformed to methods.
+                  assert(sym.isMethod, s"Non-static $sym should have the lateMETHOD flag from RefChecks")
+                  // Note that module classes are not entered into the 'decls' of the ClassInfoType
+                  // of the outer class, only the module symbols are. So the current loop does
+                  // not visit module classes. Therefore we set the LIFTED flag here for module
+                  // classes.
+                  // TODO: should we also set the LIFTED flag for static, nested module classes?
+                  // currently they don't get the flag, even though they are lifted to the package
                   sym.moduleClass setFlag LIFTED
+                }
               } else if (sym.isClass)
                 liftSymbol(sym)
             }
diff --git a/src/compiler/scala/tools/nsc/transform/LazyVals.scala b/src/compiler/scala/tools/nsc/transform/LazyVals.scala
index b71d14a04f..38671ebaae 100644
--- a/src/compiler/scala/tools/nsc/transform/LazyVals.scala
+++ b/src/compiler/scala/tools/nsc/transform/LazyVals.scala
@@ -192,13 +192,15 @@ abstract class LazyVals extends Transform with TypingTransformers with ast.TreeD
 
     def mkSlowPathDef(clazz: Symbol, lzyVal: Symbol, cond: Tree, syncBody: List[Tree],
                       stats: List[Tree], retVal: Tree): Tree = {
+      // Q: is there a reason to first set owner to `clazz` (by using clazz.newMethod), and then
+      // changing it to lzyVal.owner very soon after? Could we just do lzyVal.owner.newMethod?
       val defSym = clazz.newMethod(nme.newLazyValSlowComputeName(lzyVal.name.toTermName), lzyVal.pos, STABLE | PRIVATE)
       defSym setInfo MethodType(List(), lzyVal.tpe.resultType)
       defSym.owner = lzyVal.owner
       debuglog(s"crete slow compute path $defSym with owner ${defSym.owner} for lazy val $lzyVal")
       if (bitmaps.contains(lzyVal))
         bitmaps(lzyVal).map(_.owner = defSym)
-      val rhs: Tree = (gen.mkSynchronizedCheck(clazz, cond, syncBody, stats)).changeOwner(currentOwner -> defSym)
+      val rhs: Tree = gen.mkSynchronizedCheck(clazz, cond, syncBody, stats).changeOwner(currentOwner -> defSym)
 
       DefDef(defSym, addBitmapDefs(lzyVal, BLOCK(rhs, retVal)))
     }
diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
index cc750fb88a..b6cce4524b 100644
--- a/src/reflect/scala/reflect/internal/Symbols.scala
+++ b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -55,23 +55,29 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
   def newFreeTypeSymbol(name: TypeName, flags: Long = 0L, origin: String): FreeTypeSymbol =
     new FreeTypeSymbol(name, origin) initFlags flags
 
-  /** The original owner of a class. Used by the backend to generate
-   *  EnclosingMethod attributes.
+  /**
+   * This map stores the original owner the the first time the owner of a symbol is re-assigned.
+   * The original owner of a symbol is needed in some places in the backend. Ideally, owners should
+   * be versioned like the type history.
    */
-  val originalOwner = perRunCaches.newMap[Symbol, Symbol]()
+  private val originalOwnerMap = perRunCaches.newMap[Symbol, Symbol]()
 
   // TODO - don't allow the owner to be changed without checking invariants, at least
   // when under some flag. Define per-phase invariants for owner/owned relationships,
   // e.g. after flatten all classes are owned by package classes, there are lots and
   // lots of these to be declared (or more realistically, discovered.)
-  protected def saveOriginalOwner(sym: Symbol) {
-    if (originalOwner contains sym) ()
-    else originalOwner(sym) = sym.rawowner
+  protected def saveOriginalOwner(sym: Symbol): Unit = {
+    // some synthetic symbols have NoSymbol as owner initially
+    if (sym.owner != NoSymbol) {
+      if (originalOwnerMap contains sym) ()
+      else originalOwnerMap(sym) = sym.rawowner
+    }
   }
+
   protected def originalEnclosingMethod(sym: Symbol): Symbol = {
     if (sym.isMethod || sym == NoSymbol) sym
     else {
-      val owner = originalOwner.getOrElse(sym, sym.rawowner)
+      val owner = sym.originalOwner
       if (sym.isLocalDummy) owner.enclClass.primaryConstructor
       else originalEnclosingMethod(owner)
     }
@@ -757,8 +763,22 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
      *  So "isModuleNotMethod" exists not for its achievement in
      *  brevity, but to encapsulate the relevant condition.
      */
-    def isModuleNotMethod = isModule && !isMethod
-    def isStaticModule    = isModuleNotMethod && isStatic
+    def isModuleNotMethod = {
+      if (isModule) {
+        if (phase.refChecked) this.info // force completion to make sure lateMETHOD is there.
+        !isMethod
+      } else false
+    }
+
+    // After RefChecks, the `isStatic` check is mostly redundant: all non-static modules should
+    // be methods (and vice versa). There's a corner case on the vice-versa with mixed-in module
+    // symbols:
+    //   trait T { object A }
+    //   object O extends T
+    // The module symbol A is cloned into T$impl (addInterfaces), and then cloned into O (mixin).
+    // Since the original A is not static, it's turned into a method. The clone in O however is
+    // static (owned by a module), but it's also a method.
+    def isStaticModule = isModuleNotMethod && isStatic
 
     final def isInitializedToDefault = !isType && hasAllFlags(DEFAULTINIT | ACCESSOR)
     final def isThisSym = isTerm && owner.thisSym == this
@@ -909,10 +929,31 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     )
     final def isModuleVar = hasFlag(MODULEVAR)
 
-    /** Is this symbol static (i.e. with no outer instance)?
-     *  Q: When exactly is a sym marked as STATIC?
-     *  A: If it's a member of a toplevel object, or of an object contained in a toplevel object, or any number of levels deep.
-     *  http://groups.google.com/group/scala-internals/browse_thread/thread/d385bcd60b08faf6
+    /**
+     * Is this symbol static (i.e. with no outer instance)?
+     * Q: When exactly is a sym marked as STATIC?
+     * A: If it's a member of a toplevel object, or of an object contained in a toplevel object, or
+     * any number of levels deep.
+     * http://groups.google.com/group/scala-internals/browse_thread/thread/d385bcd60b08faf6
+     *
+     * TODO: should this only be invoked on class / module symbols? because there's also `isStaticMember`.
+     *
+     * Note: the result of `isStatic` changes over time.
+     *  - Lambdalift local definitions to the class level, the `owner` field is modified.
+     *      object T { def foo { object O } }
+     *    After lambdalift, the OModule.isStatic is true.
+     *
+     *  - After flatten, nested classes are moved to the package level. Invoking `owner` on a
+     *    class returns a package class, for which `isStaticOwner` is true. For example,
+     *      class C { object O }
+     *    OModuleClass.isStatic is true after flatten. Using phase travel to get before flatten,
+     *    method `owner` returns the class C.
+     *
+     * Why not make a stable version of `isStatic`? Maybe some parts of the compiler depend on the
+     * current implementation. For example
+     *   trait T { def foo = 1 }
+     * The method `foo` in the implementation class T$impl will be `isStatic`, because trait
+     * impl classes get the `lateMODULE` flag (T$impl.isStaticOwner is true).
      */
     def isStatic = (this hasFlag STATIC) || owner.isStaticOwner
 
@@ -1106,7 +1147,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
      *  - After lambdalift, all local method and class definitions (those not owned by a class
      *    or package class) change their owner to the enclosing class. This is done through
      *    a destructive "sym.owner = sym.owner.enclClass". The old owner is saved by
-     *    saveOriginalOwner for the backend (needed to generate the EnclosingMethod attribute).
+     *    saveOriginalOwner.
      *  - After flatten, all classes are owned by a PackageClass. This is done through a
      *    phase check (if after flatten) in the (overridden) method "def owner" in
      *    ModuleSymbol / ClassSymbol. The `rawowner` field is not modified.
@@ -1129,6 +1170,11 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     final def safeOwner: Symbol   = if (this eq NoSymbol) NoSymbol else owner
     final def assertOwner: Symbol = if (this eq NoSymbol) abort("no-symbol does not have an owner") else owner
 
+    /**
+     * The initial owner of this symbol.
+     */
+    def originalOwner: Symbol = originalOwnerMap.getOrElse(this, rawowner)
+
     // TODO - don't allow the owner to be changed without checking invariants, at least
     // when under some flag. Define per-phase invariants for owner/owned relationships,
     // e.g. after flatten all classes are owned by package classes, there are lots and
@@ -1142,7 +1188,6 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     }
 
     def ownerChain: List[Symbol] = this :: owner.ownerChain
-    def originalOwnerChain: List[Symbol] = this :: originalOwner.getOrElse(this, rawowner).originalOwnerChain
 
     // Non-classes skip self and return rest of owner chain; overridden in ClassSymbol.
     def enclClassChain: List[Symbol] = owner.enclClassChain
diff --git a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
index 7ba68b8733..50ea8d9868 100644
--- a/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
+++ b/src/reflect/scala/reflect/runtime/SymbolLoaders.scala
@@ -65,10 +65,15 @@ private[reflect] trait SymbolLoaders { self: SymbolTable =>
   class LazyPackageType extends LazyType with FlagAgnosticCompleter {
     override def complete(sym: Symbol) {
       assert(sym.isPackageClass)
-      sym setInfo new ClassInfoType(List(), new PackageScope(sym), sym)
+      // Time travel to a phase before refchecks avoids an initialization issue. `openPackageModule`
+      // creates a module symbol and invokes invokes `companionModule` while the `infos` field is
+      // still null. This calls `isModuleNotMethod`, which forces the `info` if run after refchecks.
+      slowButSafeEnteringPhaseNotLaterThan(picklerPhase) {
+        sym setInfo new ClassInfoType(List(), new PackageScope(sym), sym)
         // override def safeToString = pkgClass.toString
-      openPackageModule(sym)
-      markAllCompleted(sym)
+        openPackageModule(sym)
+        markAllCompleted(sym)
+      }
     }
   }