From 1464a4ac58656b615c0f58e504c5956ea4f06ac6 Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Tue, 17 Jul 2018 14:55:59 +0200 Subject: [PATCH] Load Java parameter names correctly in BinaryJavaMethod PSI-based implementation (accessible via `-Xuse-old-class-files-reading`) loads parameter names from the "MethodParameters" attribute if it's present, so our own implementation should as well. This metadata doesn't seem supported in the java.lang.model.element API though, so SymbolBasedValueParameter (which is used in `-Xuse-javac`) will continue to have incorrect behavior for now #KT-25193 Fixed --- .../structure/impl/classFiles/Annotations.kt | 18 +++++++++- .../java/structure/impl/classFiles/Methods.kt | 12 +++---- .../java/structure/impl/classFiles/Other.kt | 8 ++++- .../box/parametersMetadata/delegation.kt | 35 +++++++++++++++++++ .../compiledJava/ParameterNames.java | 21 +++++++++++ .../compiledJava/ParameterNames.javac.txt | 29 +++++++++++++++ .../loadJava8/compiledJava/ParameterNames.txt | 29 +++++++++++++++ .../jvm/compiler/LoadDescriptorUtil.java | 20 ++++++----- ...BlackBoxWithJava8CodegenTestGenerated.java | 5 +++ .../jvm/compiler/LoadJava8TestGenerated.java | 5 +++ ...ava8WithFastClassReadingTestGenerated.java | 5 +++ .../LoadJava8UsingJavacTestGenerated.java | 5 +++ ...8RuntimeDescriptorLoaderTestGenerated.java | 5 +++ 13 files changed, 181 insertions(+), 16 deletions(-) create mode 100644 compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt create mode 100644 compiler/testData/loadJava8/compiledJava/ParameterNames.java create mode 100644 compiler/testData/loadJava8/compiledJava/ParameterNames.javac.txt create mode 100644 compiler/testData/loadJava8/compiledJava/ParameterNames.txt diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Annotations.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Annotations.kt index a60822e330f..9a1b0390a7d 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Annotations.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Annotations.kt @@ -25,18 +25,34 @@ import org.jetbrains.kotlin.utils.addToStdlib.safeAs import org.jetbrains.org.objectweb.asm.* import java.lang.reflect.Array -internal class AnnotationsCollectorMethodVisitor( +internal class AnnotationsAndParameterCollectorMethodVisitor( private val member: BinaryJavaMethodBase, private val context: ClassifierResolutionContext, private val signatureParser: BinaryClassSignatureParser, private val parametersToSkipNumber: Int ) : MethodVisitor(ASM_API_VERSION_FOR_CLASS_READING) { + private var parameterIndex = 0 + override fun visitAnnotationDefault(): AnnotationVisitor? { member.safeAs()?.hasAnnotationParameterDefaultValue = true // We don't store default value in Java model return null } + override fun visitParameter(name: String?, access: Int) { + if (name != null) { + val index = parameterIndex - parametersToSkipNumber + if (index >= 0) { + val parameter = member.valueParameters.getOrNull(index) ?: error( + "No parameter with index $parameterIndex-$parametersToSkipNumber (name=$name access=$access) " + + "in method ${member.containingClass.fqName}.${member.name}" + ) + parameter.updateName(Name.identifier(name)) + } + } + parameterIndex++ + } + override fun visitAnnotation(desc: String, visible: Boolean) = BinaryJavaAnnotation.addAnnotation( member.annotations as MutableCollection, diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Methods.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Methods.kt index ea71df155f5..6491781bc79 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Methods.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Methods.kt @@ -31,7 +31,7 @@ import java.text.StringCharacterIterator abstract class BinaryJavaMethodBase( override val access: Int, override val containingClass: JavaClass, - val valueParameters: List, + val valueParameters: List, val typeParameters: List, override val name: Name ) : JavaMember, MapBasedJavaAnnotationOwner, BinaryJavaModifierListOwner { @@ -80,13 +80,13 @@ abstract class BinaryJavaMethodBase( } val parameterTypes = info.valueParameterTypes - val parameterList = ContainerUtil.newArrayList() + val parameterList = ContainerUtil.newArrayList() val paramCount = parameterTypes.size for (i in 0..paramCount - 1) { val type = parameterTypes[i] val isEllipsisParam = isVarargs && i == paramCount - 1 - parameterList.add(BinaryJavaValueParameter(null, type, isEllipsisParam)) + parameterList.add(BinaryJavaValueParameter(type, isEllipsisParam)) } val member: BinaryJavaMethodBase = @@ -106,7 +106,7 @@ abstract class BinaryJavaMethodBase( else -> 0 } - return member to AnnotationsCollectorMethodVisitor(member, parentContext, signatureParser, paramIgnoreCount) + return member to AnnotationsAndParameterCollectorMethodVisitor(member, parentContext, signatureParser, paramIgnoreCount) } private fun parseMethodDescription( @@ -155,7 +155,7 @@ abstract class BinaryJavaMethodBase( class BinaryJavaMethod( flags: Int, containingClass: JavaClass, - valueParameters: List, + valueParameters: List, typeParameters: List, name: Name, override val returnType: JavaType @@ -168,7 +168,7 @@ class BinaryJavaMethod( class BinaryJavaConstructor( flags: Int, containingClass: JavaClass, - valueParameters: List, + valueParameters: List, typeParameters: List ) : BinaryJavaMethodBase( flags, containingClass, valueParameters, typeParameters, 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 83e92c78438..071a736711c 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 @@ -50,12 +50,18 @@ class BinaryJavaTypeParameter( } class BinaryJavaValueParameter( - override val name: Name?, override val type: JavaType, override val isVararg: Boolean ) : JavaValueParameter, MapBasedJavaAnnotationOwner { override val annotations: MutableCollection = ContainerUtil.newSmartList() override val annotationsByFqName by buildLazyValueForMap() + + override var name: Name? = null + + internal fun updateName(newName: Name) { + assert(name == null) { "Parameter can't have two names: $name and $newName" } + name = newName + } } fun isNotTopLevelClass(classContent: ByteArray): Boolean { diff --git a/compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt b/compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt new file mode 100644 index 00000000000..87b8e418954 --- /dev/null +++ b/compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt @@ -0,0 +1,35 @@ +// WITH_RUNTIME +// FULL_JDK +// JAVAC_OPTIONS: -parameters +// KOTLIN_CONFIGURATION_FLAGS: +JVM.PARAMETERS_METADATA + +// FILE: JavaInterface.java + +public interface JavaInterface { + void plugin(String id); +} + +// FILE: test.kt + +import kotlin.test.assertEquals + +interface KotlinInterface { + fun plugin(id: String) +} + +class KotlinDelegate(impl: KotlinInterface) : KotlinInterface by impl + +class JavaDelegate(impl: JavaInterface) : JavaInterface by impl + +private fun check(javaClass: Class<*>) { + val pluginMethod = javaClass.getDeclaredMethod("plugin", String::class.java) + assertEquals(listOf("id"), pluginMethod.parameters.map { it.name }, "Incorrect parameters for $javaClass") +} + +fun box(): String { + check(JavaInterface::class.java) + check(KotlinInterface::class.java) + check(KotlinDelegate::class.java) + check(JavaDelegate::class.java) + return "OK" +} diff --git a/compiler/testData/loadJava8/compiledJava/ParameterNames.java b/compiler/testData/loadJava8/compiledJava/ParameterNames.java new file mode 100644 index 00000000000..37c86aaa139 --- /dev/null +++ b/compiler/testData/loadJava8/compiledJava/ParameterNames.java @@ -0,0 +1,21 @@ +// JAVAC_OPTIONS: -parameters + +package test; + +public class ParameterNames { + public ParameterNames(long longConstructorParam, String stringConstructorParam) {} + + public void foo(long longMethodParam, int intMethodParam) {} + + static void bar(ParameterNames staticMethodParam) {} + + class Inner { + public Inner(double doubleInnerParam, Object objectInnerParam) {} + } + + enum Enum { + E(0.0, ""); + + Enum(double doubleEnumParam, String stringEnumParam) {} + } +} diff --git a/compiler/testData/loadJava8/compiledJava/ParameterNames.javac.txt b/compiler/testData/loadJava8/compiledJava/ParameterNames.javac.txt new file mode 100644 index 00000000000..ee97c6214a8 --- /dev/null +++ b/compiler/testData/loadJava8/compiledJava/ParameterNames.javac.txt @@ -0,0 +1,29 @@ +package test + +public open class ParameterNames { + public constructor ParameterNames(/*0*/ p0: kotlin.Long, /*1*/ p1: kotlin.String!) + public open fun foo(/*0*/ p0: kotlin.Long, /*1*/ p1: kotlin.Int): kotlin.Unit + + public/*package*/ final enum class Enum : kotlin.Enum { + enum entry E + + private constructor Enum(/*0*/ p0: kotlin.Double, /*1*/ p1: kotlin.String!) + public final override /*1*/ /*fake_override*/ val name: kotlin.String + public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int + protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any + public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: test.ParameterNames.Enum!): kotlin.Int + protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit + public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class! + + // Static members + public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): test.ParameterNames.Enum + public final /*synthesized*/ fun values(): kotlin.Array + } + + public/*package*/ open inner class Inner { + public constructor Inner(/*0*/ p0: kotlin.Double, /*1*/ p1: kotlin.Any!) + } + + // Static members + public/*package*/ open fun bar(/*0*/ p0: test.ParameterNames!): kotlin.Unit +} diff --git a/compiler/testData/loadJava8/compiledJava/ParameterNames.txt b/compiler/testData/loadJava8/compiledJava/ParameterNames.txt new file mode 100644 index 00000000000..c27ec4f8578 --- /dev/null +++ b/compiler/testData/loadJava8/compiledJava/ParameterNames.txt @@ -0,0 +1,29 @@ +package test + +public open class ParameterNames { + public constructor ParameterNames(/*0*/ longConstructorParam: kotlin.Long, /*1*/ stringConstructorParam: kotlin.String!) + public open fun foo(/*0*/ longMethodParam: kotlin.Long, /*1*/ intMethodParam: kotlin.Int): kotlin.Unit + + public/*package*/ final enum class Enum : kotlin.Enum { + enum entry E + + private constructor Enum(/*0*/ doubleEnumParam: kotlin.Double, /*1*/ stringEnumParam: kotlin.String!) + public final override /*1*/ /*fake_override*/ val name: kotlin.String + public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int + protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any + public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: test.ParameterNames.Enum!): kotlin.Int + protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit + public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class! + + // Static members + public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): test.ParameterNames.Enum + public final /*synthesized*/ fun values(): kotlin.Array + } + + public/*package*/ open inner class Inner { + public constructor Inner(/*0*/ doubleInnerParam: kotlin.Double, /*1*/ objectInnerParam: kotlin.Any!) + } + + // Static members + public/*package*/ open fun bar(/*0*/ staticMethodParam: test.ParameterNames!): kotlin.Unit +} diff --git a/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/LoadDescriptorUtil.java b/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/LoadDescriptorUtil.java index aee4483de65..95eb6f5e803 100644 --- a/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/LoadDescriptorUtil.java +++ b/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/LoadDescriptorUtil.java @@ -19,10 +19,8 @@ package org.jetbrains.kotlin.jvm.compiler; import com.intellij.openapi.Disposable; import com.intellij.openapi.util.Pair; import com.intellij.openapi.util.io.FileUtil; -import com.intellij.util.text.StringKt; import kotlin.collections.CollectionsKt; import kotlin.io.FilesKt; -import kotlin.sequences.SequencesKt; import kotlin.text.Charsets; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -118,14 +116,21 @@ public class LoadDescriptorUtil { } public static void compileJavaWithAnnotationsJar(@NotNull Collection javaFiles, @NotNull File outDir) throws IOException { + List args = new ArrayList<>(Arrays.asList( + "-sourcepath", "compiler/testData/loadJava/include", + "-d", outDir.getPath()) + ); + List classpath = new ArrayList<>(); classpath.add(ForTestCompileRuntime.runtimeJarForTests()); classpath.add(KotlinTestUtils.getAnnotationsJar()); - for (File test: javaFiles) { + for (File test : javaFiles) { String content = FilesKt.readText(test, Charsets.UTF_8); + args.addAll(InTextDirectivesUtils.findListWithPrefixes(content, "JAVAC_OPTIONS:")); + if (InTextDirectivesUtils.isDirectiveDefined(content, "ANDROID_ANNOTATIONS")) { classpath.add(ForTestCompileRuntime.androidAnnotationsForTests()); } @@ -135,11 +140,10 @@ public class LoadDescriptorUtil { } } - KotlinTestUtils.compileJavaFiles(javaFiles, Arrays.asList( - "-classpath", classpath.stream().map(File::getPath).collect(Collectors.joining(File.pathSeparator)), - "-sourcepath", "compiler/testData/loadJava/include", - "-d", outDir.getPath() - )); + args.add("-classpath"); + args.add(classpath.stream().map(File::getPath).collect(Collectors.joining(File.pathSeparator))); + + KotlinTestUtils.compileJavaFiles(javaFiles, args); } @NotNull diff --git a/compiler/tests-java8/tests/org/jetbrains/kotlin/codegen/BlackBoxWithJava8CodegenTestGenerated.java b/compiler/tests-java8/tests/org/jetbrains/kotlin/codegen/BlackBoxWithJava8CodegenTestGenerated.java index c33eb8b791e..85d641cec84 100644 --- a/compiler/tests-java8/tests/org/jetbrains/kotlin/codegen/BlackBoxWithJava8CodegenTestGenerated.java +++ b/compiler/tests-java8/tests/org/jetbrains/kotlin/codegen/BlackBoxWithJava8CodegenTestGenerated.java @@ -711,6 +711,11 @@ public class BlackBoxWithJava8CodegenTestGenerated extends AbstractBlackBoxCodeg runTest("compiler/testData/codegen/java8/box/parametersMetadata/defaultImpls.kt"); } + @TestMetadata("delegation.kt") + public void testDelegation() throws Exception { + runTest("compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt"); + } + @TestMetadata("enum.kt") public void testEnum() throws Exception { runTest("compiler/testData/codegen/java8/box/parametersMetadata/enum.kt"); diff --git a/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8TestGenerated.java b/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8TestGenerated.java index 58ddf7e8afa..a33260a2e7d 100644 --- a/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8TestGenerated.java +++ b/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8TestGenerated.java @@ -41,6 +41,11 @@ public class LoadJava8TestGenerated extends AbstractLoadJava8Test { runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java"); } + @TestMetadata("ParameterNames.java") + public void testParameterNames() throws Exception { + runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java"); + } + @TestMetadata("TypeAnnotations.java") public void testTypeAnnotations() throws Exception { runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java"); diff --git a/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8WithFastClassReadingTestGenerated.java b/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8WithFastClassReadingTestGenerated.java index 1120067674a..ed76bd04ccf 100644 --- a/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8WithFastClassReadingTestGenerated.java +++ b/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8WithFastClassReadingTestGenerated.java @@ -39,6 +39,11 @@ public class LoadJava8WithFastClassReadingTestGenerated extends AbstractLoadJava runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java"); } + @TestMetadata("ParameterNames.java") + public void testParameterNames() throws Exception { + runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java"); + } + @TestMetadata("TypeAnnotations.java") public void testTypeAnnotations() throws Exception { runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java"); diff --git a/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/javac/LoadJava8UsingJavacTestGenerated.java b/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/javac/LoadJava8UsingJavacTestGenerated.java index 1625df6ef68..ea6e9b35fcc 100644 --- a/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/javac/LoadJava8UsingJavacTestGenerated.java +++ b/compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/javac/LoadJava8UsingJavacTestGenerated.java @@ -41,6 +41,11 @@ public class LoadJava8UsingJavacTestGenerated extends AbstractLoadJava8UsingJava runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java"); } + @TestMetadata("ParameterNames.java") + public void testParameterNames() throws Exception { + runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java"); + } + @TestMetadata("TypeAnnotations.java") public void testTypeAnnotations() throws Exception { runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java"); diff --git a/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/Jvm8RuntimeDescriptorLoaderTestGenerated.java b/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/Jvm8RuntimeDescriptorLoaderTestGenerated.java index b1460f3c1ac..6ef4238c531 100644 --- a/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/Jvm8RuntimeDescriptorLoaderTestGenerated.java +++ b/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/Jvm8RuntimeDescriptorLoaderTestGenerated.java @@ -39,6 +39,11 @@ public class Jvm8RuntimeDescriptorLoaderTestGenerated extends AbstractJvm8Runtim runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java"); } + @TestMetadata("ParameterNames.java") + public void testParameterNames() throws Exception { + runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java"); + } + @TestMetadata("TypeAnnotations.java") public void testTypeAnnotations() throws Exception { runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java");