Simplify and correctify calculation of the InnerClass attribute

The InnerClass attribute needs to contain an entry for every nested
class that is defined or referenced in a class. Details are in a
doc comment in BTypes.scala.

Instead of collecting ClassBTypes of nested classes into a hash map
during code generation, traverse the class before writing it out to
disk. The previous approach was incorrect as soon as the generated
bytecode was modified by the optimzier (DCE, inlining).

Author: WojciechMazur

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

@@ -39,7 +39,6 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
   import DottyBackendInterface.symExtensions
   import bTypes._
   import coreBTypes._
-  import BCodeBodyBuilder._
 
   protected val primitives: DottyPrimitives
 
@@ -1753,9 +1752,9 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
 
       val metafactory =
         if (flags != 0)
-          lambdaMetaFactoryAltMetafactoryHandle // altMetafactory required to be able to pass the flags and additional arguments if needed
+          jliLambdaMetaFactoryAltMetafactoryHandle // altMetafactory required to be able to pass the flags and additional arguments if needed
         else
-          lambdaMetaFactoryMetafactoryHandle
+          jliLambdaMetaFactoryMetafactoryHandle
 
       bc.jmethod.visitInvokeDynamicInsn(methodName, desc, metafactory, bsmArgs: _*)
 
@@ -1772,27 +1771,5 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
   private def isEmittedInterface(sym: Symbol): Boolean = sym.isInterface ||
     sym.is(JavaDefined) && (toDenot(sym).isAnnotation || sym.is(ModuleClass) && (sym.companionClass.is(PureInterface)) || sym.companionClass.is(Trait))
 
-}
 
-object BCodeBodyBuilder {
-  val lambdaMetaFactoryMetafactoryHandle = new Handle(
-    Opcodes.H_INVOKESTATIC,
-    "java/lang/invoke/LambdaMetafactory",
-    "metafactory",
-    "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;",
-    /* itf = */ false)
-
-  val lambdaMetaFactoryAltMetafactoryHandle = new Handle(
-    Opcodes.H_INVOKESTATIC,
-    "java/lang/invoke/LambdaMetafactory",
-    "altMetafactory",
-    "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;",
-    /* itf = */ false)
-
-  val lambdaDeserializeBootstrapHandle = new Handle(
-    Opcodes.H_INVOKESTATIC,
-    "scala/runtime/LambdaDeserialize",
-    "bootstrap",
-    "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/invoke/CallSite;",
-    /* itf = */ false)
 }

compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala

@@ -132,24 +132,24 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
   }
 
   /*
-   * Populates the InnerClasses JVM attribute with `refedInnerClasses`.
-   * In addition to inner classes mentioned somewhere in `jclass` (where `jclass` is a class file being emitted)
-   * `refedInnerClasses` should contain those inner classes defined as direct member classes of `jclass`
-   * but otherwise not mentioned in `jclass`.
+   * Populates the InnerClasses JVM attribute with `refedInnerClasses`. See also the doc on inner
+   * classes in BTypes.scala.
    *
-   * `refedInnerClasses` may contain duplicates,
-   * need not contain the enclosing inner classes of each inner class it lists (those are looked up for consistency).
+   * `refedInnerClasses` may contain duplicates, need not contain the enclosing inner classes of
+   * each inner class it lists (those are looked up and included).
    *
-   * This method serializes in the InnerClasses JVM attribute in an appropriate order,
+   * This method serializes in the InnerClasses JVM attribute in an appropriate order, 
    * not necessarily that given by `refedInnerClasses`.
    *
    * can-multi-thread
    */
-  final def addInnerClassesASM(jclass: asm.ClassVisitor, refedInnerClasses: List[ClassBType]): Unit = {
-    val allNestedClasses = refedInnerClasses.flatMap(_.enclosingNestedClassesChain).distinct
-
+  final def addInnerClasses(jclass: asm.ClassVisitor, declaredInnerClasses: List[ClassBType], refedInnerClasses: List[ClassBType]): Unit = {
     // sorting ensures nested classes are listed after their enclosing class thus satisfying the Eclipse Java compiler
-    for (nestedClass <- allNestedClasses.sortBy(_.internalName.toString)) {
+    val allNestedClasses = new mutable.TreeSet[ClassBType]()(Ordering.by(_.internalName))
+    allNestedClasses ++= declaredInnerClasses
+    refedInnerClasses.foreach(allNestedClasses ++= _.enclosingNestedClassesChain)
+    for nestedClass <- allNestedClasses
+    do {
       // Extract the innerClassEntry - we know it exists, enclosingNestedClassesChain only returns nested classes.
       val Some(e) = nestedClass.innerClassAttributeEntry
       jclass.visitInnerClass(e.name, e.outerName, e.innerName, e.flags)
@@ -218,26 +218,16 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     final val emitLines  = debugLevel >= 2
     final val emitVars   = debugLevel >= 3
 
-    /*
-     *  Contains class-symbols that:
-     *    (a) are known to denote inner classes
-     *    (b) are mentioned somewhere in the class being generated.
-     *
-     *  In other words, the lifetime of `innerClassBufferASM` is associated to "the class being generated".
-     */
-    final val innerClassBufferASM = mutable.Set.empty[ClassBType]
-
     /**
-     * The class internal name for a given class symbol. If the symbol describes a nested class, the
-     * ClassBType is added to the innerClassBufferASM.
+     * The class internal name for a given class symbol.
      */
     final def internalName(sym: Symbol): String = {
       // For each java class, the scala compiler creates a class and a module (thus a module class).
-      // If the `sym` is a java module class, we use the java class instead. This ensures that we
-      // register the class (instead of the module class) in innerClassBufferASM.
+      // If the `sym` is a java module class, we use the java class instead. This ensures that the
+      // ClassBType is created from the main class (instead of the module class).
       // The two symbols have the same name, so the resulting internalName is the same.
       val classSym = if (sym.is(JavaDefined) && sym.is(ModuleClass)) sym.linkedClass else sym
-      getClassBTypeAndRegisterInnerClass(classSym).internalName
+      getClassBType(classSym).internalName
     }
 
     private def assertClassNotArray(sym: Symbol): Unit = {
@@ -251,8 +241,7 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     }
 
     /**
-     * The ClassBType for a class symbol. If the class is nested, the ClassBType is added to the
-     * innerClassBufferASM.
+     * The ClassBType for a class symbol.
      *
      * The class symbol scala.Nothing is mapped to the class scala.runtime.Nothing$. Similarly,
      * scala.Null is mapped to scala.runtime.Null$. This is because there exist no class files
@@ -264,16 +253,12 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
      * the class descriptor of the receiver (the implementation class) is obtained by creating the
      * ClassBType.
      */
-    final def getClassBTypeAndRegisterInnerClass(sym: Symbol): ClassBType = {
+    final def getClassBType(sym: Symbol): ClassBType = {
       assertClassNotArrayNotPrimitive(sym)
 
       if (sym == defn.NothingClass) srNothingRef
       else if (sym == defn.NullClass) srNullRef
-      else {
-        val r = classBTypeFromSymbol(sym)
-        if (r.isNestedClass) innerClassBufferASM += r
-        r
-      }
+      else classBTypeFromSymbol(sym)
     }
 
     /*
@@ -288,16 +273,14 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     }
 
     /**
-     * The jvm descriptor of a type. If `t` references a nested class, its ClassBType is added to
-     * the innerClassBufferASM.
+     * The jvm descriptor of a type.
      */
     final def typeDescriptor(t: Type): String = { toTypeKind(t).descriptor   }
 
     /**
-     * The jvm descriptor for a symbol. If `sym` represents a nested class, its ClassBType is added
-     * to the innerClassBufferASM.
+     * The jvm descriptor for a symbol.
      */
-    final def symDescriptor(sym: Symbol): String = { getClassBTypeAndRegisterInnerClass(sym).descriptor }
+    final def symDescriptor(sym: Symbol): String = getClassBType(sym).descriptor
 
     final def toTypeKind(tp: Type): BType = typeToTypeKind(tp)(BCodeHelpers.this)(this)
 
@@ -691,16 +674,15 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     def genMirrorClass(moduleClass: Symbol, cunit: CompilationUnit): asm.tree.ClassNode = {
       assert(moduleClass.is(ModuleClass))
       assert(moduleClass.companionClass == NoSymbol, moduleClass)
-      innerClassBufferASM.clear()
       this.cunit = cunit
+      val bType      = mirrorClassBTypeFromSymbol(moduleClass)
       val moduleName = internalName(moduleClass) // + "$"
-      val mirrorName = moduleName.substring(0, moduleName.length() - 1)
+      val mirrorName = bType.internalName
 
-      val flags = (asm.Opcodes.ACC_SUPER | asm.Opcodes.ACC_PUBLIC | asm.Opcodes.ACC_FINAL)
       val mirrorClass = new asm.tree.ClassNode
       mirrorClass.visit(
         classfileVersion,
-        flags,
+        bType.info.flags,
         mirrorName,
         null /* no java-generic-signature */,
         ObjectRef.internalName,
@@ -717,10 +699,6 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
       emitAnnotations(mirrorClass, moduleClass.annotations ++ ssa)
 
       addForwarders(mirrorClass, mirrorName, moduleClass)
-
-      innerClassBufferASM ++= classBTypeFromSymbol(moduleClass).info.memberClasses
-      addInnerClassesASM(mirrorClass, innerClassBufferASM.toList)
-
       mirrorClass.visitEnd()
 
       moduleClass.name // this side-effect is necessary, really.
@@ -754,8 +732,7 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
      * must-single-thread
      */
     def legacyAddCreatorCode(clinit: asm.MethodVisitor, cnode: asm.tree.ClassNode, thisName: String): Unit = {
-      // this tracks the inner class in innerClassBufferASM, if needed.
-      val androidCreatorType = getClassBTypeAndRegisterInnerClass(AndroidCreatorClass)
+      val androidCreatorType = getClassBType(AndroidCreatorClass)
       val tdesc_creator = androidCreatorType.descriptor
 
       cnode.visitField(
@@ -818,8 +795,7 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
     def primitiveOrClassToBType(sym: Symbol): BType = {
       assert(sym.isClass, sym)
       assert(sym != defn.ArrayClass || compilingArray, sym)
-      primitiveTypeMap.getOrElse(sym,
-        storage.getClassBTypeAndRegisterInnerClass(sym)).asInstanceOf[BType]
+      primitiveTypeMap.getOrElse(sym, storage.getClassBType(sym)).asInstanceOf[BType]
     }
 
     /**
@@ -862,7 +838,7 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
 
         tp match {
           case tp: ThisType if tp.cls == defn.ArrayClass => ObjectRef.asInstanceOf[ct.bTypes.ClassBType] // was introduced in 9b17332f11 to fix SI-999, but this code is not reached in its test, or any other test
-          case tp: ThisType                         => storage.getClassBTypeAndRegisterInnerClass(tp.cls)
+          case tp: ThisType                         => storage.getClassBType(tp.cls)
           // case t: SingletonType                   => primitiveOrClassToBType(t.classSymbol)
           case t: SingletonType                     => typeToTypeKind(t.underlying)(ct)(storage)
           case t: RefinedType                       => typeToTypeKind(t.parent)(ct)(storage) //parents.map(_.toTypeKind(ct)(storage).asClassBType).reduceLeft((a, b) => a.jvmWiseLUB(b))

compiler/src/dotty/tools/backend/jvm/BCodeIdiomatic.scala

@@ -268,8 +268,10 @@ trait BCodeIdiomatic {
      * can-multi-thread
      */
     final def genStringBuilderEnd: Unit = {
-      invokevirtual(JavaStringBuilderClassName, "toString", "()Ljava/lang/String;")
+      invokevirtual(JavaStringBuilderClassName, "toString", genStringBuilderEndDesc)
     }
+    // Use ClassBType refs instead of plain string literal to make sure that needed ClassBTypes are initialized and reachable
+    private lazy val genStringBuilderEndDesc = MethodBType(Nil, StringRef).descriptor
 
     /* Concatenate top N arguments on the stack with `StringConcatFactory#makeConcatWithConstants`
      * (only works for JDK 9+)
@@ -284,13 +286,7 @@ trait BCodeIdiomatic {
       jmethod.visitInvokeDynamicInsn(
         "makeConcatWithConstants",
         asm.Type.getMethodDescriptor(StringRef.toASMType, argTypes:_*),
-        new asm.Handle(
-          asm.Opcodes.H_INVOKESTATIC,
-          "java/lang/invoke/StringConcatFactory",
-          "makeConcatWithConstants",
-          "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;",
-          false
-        ),
+        coreBTypes.jliStringConcatFactoryMakeConcatWithConstantsHandle,
         (recipe +: constants):_*
       )
     }

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

@@ -112,7 +112,6 @@ trait BCodeSkelBuilder extends BCodeHelpers {
     def genPlainClass(cd0: TypeDef) = cd0 match {
       case TypeDef(_, impl: Template) =>
       assert(cnode == null, "GenBCode detected nested methods.")
-      innerClassBufferASM.clear()
 
       claszSymbol       = cd0.symbol
       isCZParcelable    = isAndroidParcelableClass(claszSymbol)
@@ -231,15 +230,7 @@ trait BCodeSkelBuilder extends BCodeHelpers {
       if (optSerial.isDefined) { addSerialVUID(optSerial.get, cnode)}
 
       addClassFields()
-
-      innerClassBufferASM ++= classBTypeFromSymbol(claszSymbol).info.memberClasses
-
-      val companion = claszSymbol.companionClass
-      if companion.isTopLevelModuleClass then
-        innerClassBufferASM ++= classBTypeFromSymbol(companion).info.memberClasses
-
       gen(cd.rhs)
-      addInnerClassesASM(cnode, innerClassBufferASM.toList)
 
       if (AsmUtils.traceClassEnabled && cnode.name.contains(AsmUtils.traceClassPattern))
         AsmUtils.traceClass(cnode)
@@ -256,11 +247,7 @@ trait BCodeSkelBuilder extends BCodeHelpers {
 
       val ps = claszSymbol.info.parents
       val superClass: String = if (ps.isEmpty) ObjectRef.internalName else internalName(ps.head.typeSymbol)
-      val interfaceNames0 = classBTypeFromSymbol(claszSymbol).info.interfaces map {
-        case classBType =>
-          if (classBType.isNestedClass) { innerClassBufferASM += classBType }
-          classBType.internalName
-      }
+      val interfaceNames0 = classBTypeFromSymbol(claszSymbol).info.interfaces.map(_.internalName)
       /* To avoid deadlocks when combining objects, lambdas and multi-threading,
        * lambdas in objects are compiled to instance methods of the module class
        * instead of static methods (see tests/run/deadlock.scala and
@@ -881,7 +868,7 @@ trait BCodeSkelBuilder extends BCodeHelpers {
       // android creator code
       if (isCZParcelable) {
         // add a static field ("CREATOR") to this class to cache android.os.Parcelable$Creator
-        val andrFieldDescr = getClassBTypeAndRegisterInnerClass(AndroidCreatorClass).descriptor
+        val andrFieldDescr = classBTypeFromSymbol(AndroidCreatorClass).descriptor
         cnode.visitField(
           asm.Opcodes.ACC_STATIC | asm.Opcodes.ACC_FINAL,
           "CREATOR",

compiler/src/dotty/tools/backend/jvm/BTypesFromSymbols.scala

@@ -13,6 +13,7 @@ import dotty.tools.dotc.core.Phases._
 import dotty.tools.dotc.core.Symbols._
 import dotty.tools.dotc.core.Phases.Phase
 import dotty.tools.dotc.transform.SymUtils._
+import dotty.tools.dotc.core.StdNames
 
 /**
  * This class mainly contains the method classBTypeFromSymbol, which extracts the necessary
@@ -91,6 +92,20 @@ class BTypesFromSymbols[I <: DottyBackendInterface](val int: I) extends BTypes {
     })
   }
 
+  final def mirrorClassBTypeFromSymbol(moduleClassSym: Symbol): ClassBType = {
+    assert(moduleClassSym.isTopLevelModuleClass, s"not a top-level module class: $moduleClassSym")
+    val internalName = moduleClassSym.javaBinaryName.stripSuffix(StdNames.str.MODULE_SUFFIX)
+    val bType = ClassBType(internalName)
+    bType.info = ClassInfo(
+      superClass = Some(ObjectRef),
+      interfaces = Nil,
+      flags = asm.Opcodes.ACC_SUPER | asm.Opcodes.ACC_PUBLIC | asm.Opcodes.ACC_FINAL,
+      memberClasses = getMemberClasses(moduleClassSym).map(classBTypeFromSymbol),
+      nestedInfo = None
+    )
+    bType
+  }
+
   private def setClassInfo(classSym: Symbol, classBType: ClassBType): ClassBType = {
     val superClassSym: Symbol =  {
       val t = classSym.asClass.superClass
@@ -138,13 +153,6 @@ class BTypesFromSymbols[I <: DottyBackendInterface](val int: I) extends BTypes {
     /* The InnerClass table of a class C must contain all nested classes of C, even if they are only
      * declared but not otherwise referenced in C (from the bytecode or a method / field signature).
      * We collect them here.
-     *
-     * Nested classes that are also referenced in C will be added to the innerClassBufferASM during
-     * code generation, but those duplicates will be eliminated when emitting the InnerClass
-     * attribute.
-     *
-     * Why doe we need to collect classes into innerClassBufferASM at all? To collect references to
-     * nested classes, but NOT nested in C, that are used within C.
      */
     val nestedClassSymbols = {
       // The lambdalift phase lifts all nested classes to the enclosing class, so if we collect