Support Groovy $Trait$FieldHelper classes in new class file reader

Preface: for Groovy traits with fields which are extended by classes,
the Groovy compiler generates synthetic "$Trait$FieldHelper" classes
which posed several problems to our class file reader, caused by the
fact the contents of the InnerClasses attribute broke some assumptions
about how names on the JVM are formed.

For a trait named `A`, the Groovy compiler will additionally generate a
synthetic class file `A$Trait$FieldHelper` with the following in the
InnerClasses attribute:

InnerClasses:
     public static #15= #2 of #14; //FieldHelper=class A$Trait$FieldHelper of class A

i.e. the simple name of the class is `FieldHelper`, the name of its
outer class is `A`, but the full internal name is `A$Trait$FieldHelper`,
which is surprising considering that the names are usually obtained by
separating the outer and inner names via the dollar sign.

Another detail is that in some usages of this synthetic class, the
InnerClasses attribute was missing at all. For example, if an empty
class `B` extends `A`, then there's no InnerClasses attribute in `B`'s
class file, which is surprising because we might decode the same name
differently depending on the class file we encounter it in.

In this change, we attempt to treat these synthetic classes as top-level
by refusing to read "invalid" InnerClasses attribute values (they are
not technically invalid because they still conform to JVMS), fixing the
problem of "unresolved supertypes" error which occrurred when these
classes were used as supertypes in a class file in a dependency.

1) In ClassifierResolutionContext.mapInternalNameToClassId, do not use
   the ad-hoc logic (copy-pasted from intellij-core) to determine class
   id heuristically from the internal name. For $Trait$FieldHelper
   classes this logic attempted to replace all dollar signs with dots,
   which was semantically incorrect: dollars there were used as
   synthetic characters, not as a separator between outer and inner
   classes.
2) In isNotTopLevelClass (Other.kt), only consider "valid" InnerClasses
   attribute values, where the full name of the class is obtained by
   separating the outer name and the inner name with a dollar character.
   This way, we'll be able to treat class files with invalid attribute
   values as top-level and avoid breaking any other assumptions in the
   class file loader.
3) In BinaryJavaClass.visitInnerClass, record all valid InnerClasses
   attribute values present in the class file, not just those related to
   the class in question itself. This is needed now because previously,
   the removed heuristics (see p.1) transformed mentioned inner class
   names to class ids correctly >99% of the time. Now that the
   heuristics are gone, we'll use the information present in the class
   file to map names correctly and predictably. According to JVMS, this
   attribute should contain information about all inner classes
   mentioned in the class file, and this is true at least for class
   files produced by javac.

 #KT-18592 Fixed

compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/arguments/K2JVMCompilerArguments.kt

@@ -149,8 +149,8 @@ class K2JVMCompilerArguments : CommonCompilerArguments() {
 
     @Argument(
         value = "-Xuse-old-class-files-reading",
-        description = "Use old class files reading implementation " +
-                "(may slow down the build and should be used in case of problems with the new implementation)"
+        description = "Use old class files reading implementation. This may slow down the build and cause problems with Groovy interop.\n" +
+                "Should be used in case of problems with the new implementation"
     )
     var useOldClassFilesReading: Boolean by FreezableVar(false)
 

compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/BinaryJavaClass.kt

@@ -100,7 +100,9 @@ class BinaryJavaClass(
         if (access.isSet(Opcodes.ACC_SYNTHETIC)) return
         if (innerName == null || outerName == null) return
 
-        if (myInternalName == outerName) {
+        // Do not read InnerClasses attribute values where full name != outer + $ + inner; treat those classes as top level instead.
+        // This is possible for example for Groovy-generated $Trait$FieldHelper classes.
+        if (name == "$outerName$$innerName") {
             context.addInnerClass(name, outerName, innerName)
             innerClassNameToAccess[context.mapInternalNameToClassId(name).shortClassName] = access
         }

compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/ClassifierResolutionContext.kt

@@ -16,7 +16,6 @@
 
 package org.jetbrains.kotlin.load.java.structure.impl.classFiles
 
-import com.intellij.util.containers.ContainerUtil
 import org.jetbrains.kotlin.load.java.structure.JavaClass
 import org.jetbrains.kotlin.load.java.structure.JavaClassifier
 import org.jetbrains.kotlin.load.java.structure.JavaTypeParameter
@@ -78,45 +77,17 @@ class ClassifierResolutionContext private constructor(
         }
 
         if ('$' in internalName) {
-            val innerClassInfo = innerClasses.getOrNull(internalName) ?: return mapInternalNameToClassIdNaively(internalName)
-            if (Name.isValidIdentifier(innerClassInfo.simpleName)) {
+            val innerClassInfo = innerClasses.getOrNull(internalName)
+            if (innerClassInfo != null && Name.isValidIdentifier(innerClassInfo.simpleName)) {
                 val outerClassId = mapInternalNameToClassId(innerClassInfo.outerInternalName)
                 return outerClassId.createNestedClassId(Name.identifier(innerClassInfo.simpleName))
             }
         }
 
-        return createClassIdForTopLevel(internalName)
+        return ClassId.topLevel(FqName(internalName.replace('/', '.')))
     }
 
-    // See com.intellij.psi.impl.compiled.StubBuildingVisitor.GUESSING_MAPPER
-    private fun mapInternalNameToClassIdNaively(internalName: String): ClassId {
-        val splitPoints = ContainerUtil.newSmartList<Int>()
-        for (p in 0..internalName.length - 1) {
-            val c = internalName[p]
-            if (c == '$' && p > 0 && internalName[p - 1] != '/' && p < internalName.length - 1 && internalName[p + 1] != '$') {
-                splitPoints.add(p)
-            }
-        }
-
-        if (splitPoints.isNotEmpty()) {
-            val substrings = (listOf(-1) + splitPoints).zip(splitPoints + internalName.length).map { (from, to) ->
-                internalName.substring(from + 1, to)
-            }
-
-            val outerFqName = FqName(substrings[0].replace('/', '.'))
-            val packageFqName = outerFqName.parent()
-            val relativeName =
-                FqName(outerFqName.shortName().asString() + "." + substrings.subList(1, substrings.size).joinToString("."))
-
-            return ClassId(packageFqName, relativeName, false)
-        }
-
-        return createClassIdForTopLevel(internalName)
-    }
-
-    private fun createClassIdForTopLevel(internalName: String) = ClassId.topLevel(FqName(internalName.replace('/', '.')))
-
-    internal fun resolveByInternalName(c: String) = resolveClass(mapInternalNameToClassId(c))
+    internal fun resolveByInternalName(c: String): Result = resolveClass(mapInternalNameToClassId(c))
 
     internal fun mapDescToClassId(desc: String): ClassId = mapInternalNameToClassId(Type.getType(desc).internalName)
 }

compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Other.kt

@@ -67,24 +67,27 @@ class BinaryJavaValueParameter(
 fun isNotTopLevelClass(classContent: ByteArray): Boolean {
     var isNotTopLevelClass = false
     ClassReader(classContent).accept(
-            object : ClassVisitor(ASM_API_VERSION_FOR_CLASS_READING) {
-                private var internalName: String? = null
-                override fun visit(
-                        version: Int,
-                        access: Int,
-                        name: String?,
-                        signature: String?,
-                        superName: String?,
-                        interfaces: Array<out String>?
-                ) {
-                    internalName = name
-                }
+        object : ClassVisitor(ASM_API_VERSION_FOR_CLASS_READING) {
+            private var internalName: String? = null
+            override fun visit(
+                version: Int,
+                access: Int,
+                name: String?,
+                signature: String?,
+                superName: String?,
+                interfaces: Array<out String>?
+            ) {
+                internalName = name
+            }
 
-                override fun visitInnerClass(name: String?, outerName: String?, innerName: String?, access: Int) {
-                    isNotTopLevelClass = isNotTopLevelClass or (name == internalName)
+            override fun visitInnerClass(name: String?, outerName: String?, innerName: String?, access: Int) {
+                // Do not read InnerClasses attribute values where full name != outer + $ + inner; treat those classes as top level instead.
+                if (name == internalName && (innerName == null || name == "$outerName$$innerName")) {
+                    isNotTopLevelClass = true
                 }
-            },
-            ClassReader.SKIP_CODE or ClassReader.SKIP_DEBUG or ClassReader.SKIP_FRAMES
+            }
+        },
+        ClassReader.SKIP_CODE or ClassReader.SKIP_DEBUG or ClassReader.SKIP_FRAMES
     )
     return isNotTopLevelClass
 }

compiler/testData/cli/jvm/extraHelp.out

@@ -61,7 +61,8 @@ where advanced options include:
                              Default value is 'enable'
   -Xuse-ir                   Use the IR backend
   -Xuse-javac                Use javac for Java source and class files analysis
-  -Xuse-old-class-files-reading Use old class files reading implementation (may slow down the build and should be used in case of problems with the new implementation)
+  -Xuse-old-class-files-reading Use old class files reading implementation. This may slow down the build and cause problems with Groovy interop.
+                             Should be used in case of problems with the new implementation
   -Xuse-type-table           Use type table in metadata serialization
   -Xallow-kotlin-package     Allow compiling code in package 'kotlin' and allow not requiring kotlin.stdlib in module-info
   -Xallow-result-return-type Allow compiling code when `kotlin.Result` is used as a return type

compiler/testData/loadJava/compiledJava/TopLevel$Class.fast.txt

@@ -0,0 +1,6 @@
+package test
+
+public open class `TopLevel$Class` {
+    public constructor `TopLevel$Class`()
+    public open fun foo(/*0*/ p0: test.`TopLevel$Class`!): kotlin.Unit
+}

libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/SimpleKotlinGradleIT.kt

@@ -107,4 +107,11 @@ class SimpleKotlinGradleIT : BaseGradleIT() {
             assertNoSuchFile("build")
         }
     }
-}
+
+    @Test
+    fun testGroovyTraitsWithFields() {
+        Project("groovyTraitsWithFields").build("build") {
+            assertSuccessful()
+        }
+    }
+}

libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/build.gradle

@@ -0,0 +1,34 @@
+buildscript {
+    repositories {
+        mavenLocal()
+        mavenCentral()
+    }
+    dependencies {
+        classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
+    }
+}
+
+apply plugin: "kotlin"
+apply plugin: "groovy"
+apply plugin: "java"
+
+repositories {
+    mavenLocal()
+    mavenCentral()
+}
+
+dependencies {
+    compile "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
+    compile 'org.codehaus.groovy:groovy-all:2.5.3'
+}
+
+compileGroovy.dependsOn = compileGroovy.taskDependencies.values - 'compileJava'
+
+compileKotlin {
+    dependsOn compileGroovy
+    classpath += files(compileGroovy.destinationDir)
+}
+
+compileKotlin {
+    kotlinOptions.jvmTarget = "1.8"
+}

libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTrait.groovy

@@ -0,0 +1,11 @@
+import groovy.transform.CompileStatic
+
+@CompileStatic
+trait MyTrait {
+    private transient boolean somePrivateField = false
+    List<String> someField
+
+    def foo() {
+        return 1
+    }
+}

libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTraitAccessor.groovy

@@ -0,0 +1,3 @@
+class MyTraitAccessor implements MyTrait {
+    def myField = 1
+}

libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/kotlin/MyKotlinClass.kt

@@ -0,0 +1,3 @@
+fun main(args: Array<String>) {
+    System.out.println(MyTraitAccessor().myField)
+}