From 2d9deac6502ea638214ebe1f87fd2ea34603fe5a Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Fri, 19 Oct 2018 18:46:16 +0200 Subject: [PATCH] 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 --- .../arguments/K2JVMCompilerArguments.kt | 4 +- .../impl/classFiles/BinaryJavaClass.kt | 4 +- .../classFiles/ClassifierResolutionContext.kt | 37 ++----------------- .../java/structure/impl/classFiles/Other.kt | 35 ++++++++++-------- compiler/testData/cli/jvm/extraHelp.out | 3 +- .../compiledJava/TopLevel$Class.fast.txt | 6 +++ .../kotlin/gradle/SimpleKotlinGradleIT.kt | 9 ++++- .../groovyTraitsWithFields/build.gradle | 34 +++++++++++++++++ .../src/main/groovy/MyTrait.groovy | 11 ++++++ .../src/main/groovy/MyTraitAccessor.groovy | 3 ++ .../src/main/kotlin/MyKotlinClass.kt | 3 ++ 11 files changed, 95 insertions(+), 54 deletions(-) create mode 100644 compiler/testData/loadJava/compiledJava/TopLevel$Class.fast.txt create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/build.gradle create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTrait.groovy create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTraitAccessor.groovy create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/kotlin/MyKotlinClass.kt diff --git a/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/arguments/K2JVMCompilerArguments.kt b/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/arguments/K2JVMCompilerArguments.kt index 03a5995be65e1..cccac38c98beb 100644 --- a/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/arguments/K2JVMCompilerArguments.kt +++ b/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) diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/BinaryJavaClass.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/BinaryJavaClass.kt index 2dbcc1bb1bdc8..84581ede88feb 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/BinaryJavaClass.kt +++ b/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 } diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/ClassifierResolutionContext.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/ClassifierResolutionContext.kt index 97948dabe2b46..cf322afacf643 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/ClassifierResolutionContext.kt +++ b/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() - 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) } diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Other.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Other.kt index 071a736711cc3..2e43b0ba6bea6 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Other.kt +++ b/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? - ) { - 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? + ) { + 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 } diff --git a/compiler/testData/cli/jvm/extraHelp.out b/compiler/testData/cli/jvm/extraHelp.out index 0ab2579802e07..6c5ee97354e73 100644 --- a/compiler/testData/cli/jvm/extraHelp.out +++ b/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 diff --git a/compiler/testData/loadJava/compiledJava/TopLevel$Class.fast.txt b/compiler/testData/loadJava/compiledJava/TopLevel$Class.fast.txt new file mode 100644 index 0000000000000..eb2c94383665c --- /dev/null +++ b/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 +} diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/SimpleKotlinGradleIT.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/SimpleKotlinGradleIT.kt index 3d87ded812205..d8d7e16c8634b 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/SimpleKotlinGradleIT.kt +++ b/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") } } -} \ No newline at end of file + + @Test + fun testGroovyTraitsWithFields() { + Project("groovyTraitsWithFields").build("build") { + assertSuccessful() + } + } +} diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/build.gradle b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/build.gradle new file mode 100644 index 0000000000000..18ecef3b40310 --- /dev/null +++ b/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" +} diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTrait.groovy b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTrait.groovy new file mode 100644 index 0000000000000..7b535c59ee7a2 --- /dev/null +++ b/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 someField + + def foo() { + return 1 + } +} diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTraitAccessor.groovy b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/groovy/MyTraitAccessor.groovy new file mode 100644 index 0000000000000..d2951c8253fb3 --- /dev/null +++ b/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 +} diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/kotlin/MyKotlinClass.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/groovyTraitsWithFields/src/main/kotlin/MyKotlinClass.kt new file mode 100644 index 0000000000000..0f51db3c2297f --- /dev/null +++ b/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) { + System.out.println(MyTraitAccessor().myField) +}