Minor cleanups and comments in GenBCode

6 files changed, 83 insertions, 21 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala
index 51bfdf0027..07890dd5f8 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala
@@ -460,6 +460,7 @@ abstract class BCodeHelpers extends BCodeTypes with BytecodeWriters {
         // Can't call .toInterface (at this phase) or we trip an assertion.
         // See PackratParser#grow for a method which fails with an apparent mismatch
         // between "object PackratParsers$class" and "trait PackratParsers"
+        // TODO @lry do we have a test for that?
         if (sym.isImplClass) {
           // pos/spec-List.scala is the sole failure if we don't check for NoSymbol
           val traitSym = sym.owner.info.decl(tpnme.interfaceName(sym.name))
@@ -469,6 +470,8 @@ abstract class BCodeHelpers extends BCodeTypes with BytecodeWriters {
           }
         }
 
+        // TODO @lry: code duplication between here and method asmClassType.
+
         assert(hasInternalName(sym), s"Invoked for a symbol lacking JVM internal name: ${sym.fullName}")
         assert(!phantomTypeMap.contains(sym), "phantom types not supposed to reach here.")
 
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala
index 62dfb4917d..22cb189c70 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeTypes.scala
@@ -21,8 +21,9 @@ abstract class BCodeTypes extends BCodeIdiomatic {
   import global._
   import bTypes._
 
-  // when compiling the Scala library, some assertions don't hold (e.g., scala.Boolean has null superClass although it's not an interface)
-  val isCompilingStdLib = !(settings.sourcepath.isDefault)
+  // Used only for assertions. When compiling the Scala library, some assertions don't hold
+  // (e.g., scala.Boolean has null superClass although it's not an interface)
+  private val isCompilingStdLib = !(settings.sourcepath.isDefault)
 
   // special names
   var StringReference             : ClassBType = null
@@ -175,12 +176,21 @@ abstract class BCodeTypes extends BCodeIdiomatic {
   // ------------------------------------------------
 
   /**
-   * TODO @lry should probably be a map form ClassBType to Tracked
+   * Type information for classBTypes.
+   *
+   * TODO rename Tracked
    */
-  val exemplars = new java.util.concurrent.ConcurrentHashMap[BType, Tracked]
+  val exemplars = new java.util.concurrent.ConcurrentHashMap[ClassBType, Tracked]
 
   /**
    * Maps class symbols to their corresponding `Tracked` instance.
+   *
+   * This map is only used during the first backend phase (Worker1) where ClassDef trees are
+   * transformed into ClassNode asm trees. In this phase, ClassBTypes and their Tracked are created
+   * and added to the `exemplars` map. The `symExemplars` map is only used to know if a symbol has
+   * already been visited.
+   *
+   * TODO move this map to the builder class. it's only used during building. can be gc'd with the builder.
    */
   val symExemplars = new java.util.concurrent.ConcurrentHashMap[Symbol, Tracked]
 
@@ -313,7 +323,7 @@ abstract class BCodeTypes extends BCodeIdiomatic {
   final def isDeprecated(sym: Symbol): Boolean = { sym.annotations exists (_ matches definitions.DeprecatedAttr) }
 
   /* must-single-thread */
-  final def hasInternalName(sym: Symbol) = { sym.isClass || (sym.isModule && !sym.isMethod) }
+  final def hasInternalName(sym: Symbol) = sym.isClass || sym.isModuleNotMethod
 
   /* must-single-thread */
   def getSuperInterfaces(csym: Symbol): List[Symbol] = {
@@ -702,6 +712,10 @@ abstract class BCodeTypes extends BCodeIdiomatic {
     var x = ics
     while (x ne NoSymbol) {
       assert(x.isClass, s"not a class symbol: ${x.fullName}")
+      // Uses `rawowner` because `owner` reflects changes in the owner chain due to flattening.
+      // The owner chain of a class only contains classes. This is because the lambdalift phase
+      // changes the `rawowner` destructively to point to the enclosing class. Before, the owner
+      // might be for example a method.
       val isInner = !x.rawowner.isPackageClass
       if (isInner) {
         chain ::= x
@@ -794,14 +808,23 @@ abstract class BCodeTypes extends BCodeIdiomatic {
    * must-single-thread
    */
   def javaFlags(sym: Symbol): Int = {
-    // constructors of module classes should be private
-    // PP: why are they only being marked private at this stage and not earlier?
+    // constructors of module classes should be private. introduced in b06edbc, probably to prevent
+    // creating module instances from java. for nested modules, the constructor needs to be public
+    // since they are created by the outer class and stored in a field. a java client can create
+    // new instances via outerClassInstance.new InnerModuleClass$().
+    // TODO: do this early, mark the symbol private.
     val privateFlag =
       sym.isPrivate || (sym.isPrimaryConstructor && isTopLevelModule(sym.owner))
 
-    // Final: the only fields which can receive ACC_FINAL are eager vals.
-    // Neither vars nor lazy vals can, because:
+    // 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).
+    // Note that the presence of the `FINAL` flag on a symbol does not correspond 1:1 to emitting
+    // ACC_FINAL in bytecode.
+    //
+    // Top-level modules are marked ACC_FINAL in bytecode (even without the FINAL flag). Nested
+    // objects don't get the flag to allow overriding (under -Yoverride-objects, SI-5676).
     //
+    // For fields, only eager val fields can receive ACC_FINAL. vars or lazy vals can't:
     // Source: http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5.3
     // "Another problem is that the specification allows aggressive
     // optimization of final fields. Within a thread, it is permissible to
@@ -818,7 +841,6 @@ abstract class BCodeTypes extends BCodeIdiomatic {
     // we can exclude lateFINAL. Such symbols are eligible for inlining, but to
     // avoid breaking proxy software which depends on subclassing, we do not
     // emit ACC_FINAL.
-    // Nested objects won't receive ACC_FINAL in order to allow for their overriding.
 
     val finalFlag = (
          (((sym.rawflags & symtab.Flags.FINAL) != 0) || isTopLevelModule(sym))
@@ -845,6 +867,10 @@ abstract class BCodeTypes extends BCodeIdiomatic {
       if (sym.isVarargsMethod) ACC_VARARGS else 0,
       if (sym.hasFlag(symtab.Flags.SYNCHRONIZED)) ACC_SYNCHRONIZED else 0
     )
+    // TODO @lry should probably also check / add "deprectated"
+    // all call sites of "javaFlags" seem to check for deprecation rigth after.
+    // Exception: the call below in javaFieldFlags. However, the caller of javaFieldFlags then
+    // does the check.
   }
 
   /*
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
index 5b0fa6f7a8..aac4f9cbfc 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
@@ -235,10 +235,24 @@ abstract class BTypes[G <: Global](val __global_dont_use: G) {
   /**
    * Class or Interface type.
    *
-   * Classes are represented using their name as a slice of the `chrs` array. This representation is
-   * efficient because the JVM class name is initially created using `classSymbol.javaBinaryName`.
-   * This already adds the necessary string to the `chrs` array, so it makes sense to reuse the same
-   * name table in the backend.
+   * The information for creating a ClassBType (superClass, interfaces, etc) is obtained
+   *   - either from a ClassSymbol, for classes being compiled or referenced from source (see
+   *     BCodeTypes)
+   *   - or, during inlining, from ASM ClassNodes that are parsed from class files.
+   *
+   * The class name is represented as a slice of the `chrs` array. This representation is efficient
+   * because the JVM class name is obtained through `classSymbol.javaBinaryName`. This already adds
+   * the necessary string to the `chrs` array, so it makes sense to reuse the same name table in the
+   * backend.
+   *
+   * Not a case class because that would expose the constructor that takes (offset, length)
+   * parameters (I didn't find a way to make it private, also the factory in the companion).
+   *
+   * @param offset     See below
+   * @param length     The class name is represented as offset and length in the `chrs` array.
+   *                   The (public) constructors of ClassBType take a BTypeName, which are
+   *                   hash-consed. This ensures that two ClassBType instances for the same name
+   *                   have the same offset and length.
    *
    * Not a case class because that would expose the (Int, Int) constructor (didn't find a way to
    * make it private, also the factory in the companion).
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
index 988c04f514..ce7e7c49ff 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
@@ -636,6 +636,9 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM {
           innerSym.rawname + innerSym.moduleSuffix
 
       // add inner classes which might not have been referenced yet
+      // TODO @lry according to the spec, all nested classes should be added, also local and
+      // anonymous. This seems to add only member classes - or not? it's exitingErasure, so maybe
+      // local / anonymous classes have been lifted by lambdalift. are they in the "decls" though?
       exitingErasure {
         for (sym <- List(csym, csym.linkedClassOfClass); m <- sym.info.decls.map(innerClassSymbolFor) if m.isClass)
           innerClassBuffer += m
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala
index 9b292fee7f..6b192556d9 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala
@@ -271,7 +271,7 @@ abstract class GenBCode extends BCodeSyncAndTry {
     override def run() {
 
       arrivalPos = 0 // just in case
-      scalaPrimitives.init
+      scalaPrimitives.init()
       initBCodeTypes()
 
       // initBytecodeWriter invokes fullName, thus we have to run it before the typer-dependent thread is activated.
diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
index 2ce54d2259..cc750fb88a 100644
--- a/src/reflect/scala/reflect/internal/Symbols.scala
+++ b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -1099,13 +1099,28 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
 
 // ------ owner attribute --------------------------------------------------------------
 
-    /** In general when seeking the owner of a symbol, one should call `owner`.
-     *  The other possibilities include:
-     *    - call `safeOwner` if it is expected that the target may be NoSymbol
-     *    - call `assertOwner` if it is an unrecoverable error if the target is NoSymbol
+    /**
+     * The owner of a symbol. Changes over time to adapt to the structure of the trees:
+     *  - Up to lambdalift, the owner is the lexically enclosing definition. For definitions
+     *    in a local block, the owner is also the next enclosing definition.
+     *  - 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).
+     *  - 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.
+     *  - Owners are also changed in other situations, for example when moving trees into a new
+     *    lexical context, e.g. in the named/default arguments tranformation, or when translating
+     *    extension method definitions.
+     *
+     * In general when seeking the owner of a symbol, one should call `owner`.
+     * The other possibilities include:
+     *   - call `safeOwner` if it is expected that the target may be NoSymbol
+     *   - call `assertOwner` if it is an unrecoverable error if the target is NoSymbol
      *
-     *  `owner` behaves like `safeOwner`, but logs NoSymbol.owner calls under -Xdev.
-     *  `assertOwner` aborts compilation immediately if called on NoSymbol.
+     * `owner` behaves like `safeOwner`, but logs NoSymbol.owner calls under -Xdev.
+     * `assertOwner` aborts compilation immediately if called on NoSymbol.
      */
     def owner: Symbol = {
       if (Statistics.hotEnabled) Statistics.incCounter(ownerCount)
@@ -2811,6 +2826,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
 
     override def owner = {
       if (Statistics.hotEnabled) Statistics.incCounter(ownerCount)
+      // a module symbol may have the lateMETHOD flag after refchecks, see isModuleNotMethod
       if (!isMethod && needsFlatClasses) rawowner.owner
       else rawowner
     }