From c5275f178a1828ff6422a8bb775f06dead3ca1d1 Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Tue, 30 Oct 2018 18:35:45 +0100 Subject: [PATCH] Do not use getMethod/getConstructor in reflection Use only getDeclaredMethod/getDeclaredConstructor instead. The reason is that getMethod/getConstructor only finds public-API (public or protected on JVM) declarations, and to determine if a declaration is public-API in the class file we used isPublicInBytecode, which was trying to load annotations on the declaration to see if it was InlineOnly, and that required lots of time-consuming actions and worsened the stack trace (as can be seen e.g. in KT-27878). In fact, the implementation of Class.getMethod is not supposed to do anything complicated except loading annotations from each superclass and superinterface of the given class. Doing it in our codebase simplifies implementation and probably improves performance --- .../mapping/methodsFromSuperInterface.kt | 27 +++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 5 ++ .../LightAnalysisModeTestGenerated.java | 5 ++ .../ir/IrBlackBoxCodegenTestGenerated.java | 5 ++ .../descriptors/annotations/annotationUtil.kt | 1 + .../jvm/internal/KDeclarationContainerImpl.kt | 74 +++++++++---------- .../reflect/jvm/internal/KFunctionImpl.kt | 12 +-- .../reflect/jvm/internal/KPropertyImpl.kt | 10 +-- .../src/kotlin/reflect/jvm/internal/util.kt | 7 -- 9 files changed, 85 insertions(+), 61 deletions(-) create mode 100644 compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt diff --git a/compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt b/compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt new file mode 100644 index 00000000000..4ed0856007e --- /dev/null +++ b/compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt @@ -0,0 +1,27 @@ +// TARGET_BACKEND: JVM +// WITH_REFLECT + +import kotlin.reflect.jvm.javaMethod +import kotlin.test.assertEquals + +interface A1 { + fun a1() +} + +interface A2 { + fun a2() +} + +interface B1 : A1 +interface B2 : A1, A2 + +interface C : B2 + +abstract class D : B1, C + +fun box(): String { + assertEquals("public abstract void A1.a1()", D::a1.javaMethod!!.toString()) + assertEquals("public abstract void A2.a2()", D::a2.javaMethod!!.toString()) + + return "OK" +} diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index 29ed5603231..9eadd7b37d9 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -20130,6 +20130,11 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/reflection/mapping/methodsFromObject.kt"); } + @TestMetadata("methodsFromSuperInterface.kt") + public void testMethodsFromSuperInterface() throws Exception { + runTest("compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt"); + } + @TestMetadata("openSuspendFun.kt") public void testOpenSuspendFun() throws Exception { runTest("compiler/testData/codegen/box/reflection/mapping/openSuspendFun.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index ab296a3a137..0217352f625 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -20130,6 +20130,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/reflection/mapping/methodsFromObject.kt"); } + @TestMetadata("methodsFromSuperInterface.kt") + public void testMethodsFromSuperInterface() throws Exception { + runTest("compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt"); + } + @TestMetadata("openSuspendFun.kt") public void testOpenSuspendFun() throws Exception { runTest("compiler/testData/codegen/box/reflection/mapping/openSuspendFun.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index cca6499a82a..48b8edb3603 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java @@ -20135,6 +20135,11 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/reflection/mapping/methodsFromObject.kt"); } + @TestMetadata("methodsFromSuperInterface.kt") + public void testMethodsFromSuperInterface() throws Exception { + runTest("compiler/testData/codegen/box/reflection/mapping/methodsFromSuperInterface.kt"); + } + @TestMetadata("openSuspendFun.kt") public void testOpenSuspendFun() throws Exception { runTest("compiler/testData/codegen/box/reflection/mapping/openSuspendFun.kt"); diff --git a/core/descriptors/src/org/jetbrains/kotlin/descriptors/annotations/annotationUtil.kt b/core/descriptors/src/org/jetbrains/kotlin/descriptors/annotations/annotationUtil.kt index 0cbe33166ab..44efabd0f00 100644 --- a/core/descriptors/src/org/jetbrains/kotlin/descriptors/annotations/annotationUtil.kt +++ b/core/descriptors/src/org/jetbrains/kotlin/descriptors/annotations/annotationUtil.kt @@ -73,6 +73,7 @@ private val INLINE_ONLY_ANNOTATION_FQ_NAME = FqName("kotlin.internal.InlineOnly" fun MemberDescriptor.isInlineOnlyOrReifiable(): Boolean = this is CallableMemberDescriptor && (isReifiable() || DescriptorUtils.getDirectMember(this).isReifiable() || isInlineOnly()) +// TODO: move to compiler modules fun MemberDescriptor.isEffectivelyInlineOnly(): Boolean = isInlineOnlyOrReifiable() || (this is FunctionDescriptor && isSuspend && isInline && (valueParameters.any { it.isCrossinline } || visibility == Visibilities.PRIVATE)) diff --git a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KDeclarationContainerImpl.kt b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KDeclarationContainerImpl.kt index da9f207fd8f..dfb3922f91f 100644 --- a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KDeclarationContainerImpl.kt +++ b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KDeclarationContainerImpl.kt @@ -165,35 +165,34 @@ internal abstract class KDeclarationContainerImpl : ClassBasedDeclarationContain return functions.single() } - private fun Class<*>.lookupMethod(name: String, parameterTypes: List>, returnType: Class<*>, isPublic: Boolean): Method? { - val parametersArray = parameterTypes.toTypedArray() + private fun Class<*>.lookupMethod(name: String, parameterTypes: List>, returnType: Class<*>): Method? { + lookupMethod(name, parameterTypes.toTypedArray(), returnType)?.let { return it } - // If we're looking for a public method, use Java reflection's getMethod/getMethods first - if (isPublic) { - val result = tryGetMethod(name, parametersArray, returnType, declared = false) - if (result != null) return result - - // Methods from java.lang.Object cannot be found in the interface via Class.getMethod/getDeclaredMethod - if (isInterface) { - val fromObject = Any::class.java.lookupMethod(name, parameterTypes, returnType, isPublic) - if (fromObject != null) return fromObject - } - } - - // If we're looking for a non-public method, it might be located not only in this class, but also in any of its superclasses - var klass: Class<*>? = this - while (klass != null) { - val method = klass.tryGetMethod(name, parametersArray, returnType, declared = true) - if (method != null) return method - klass = klass.superclass + // Methods from java.lang.Object (equals, hashCode, toString) cannot be found in the interface via + // Class.getMethod/getDeclaredMethod, so for interfaces, we also look in java.lang.Object. + if (isInterface) { + Any::class.java.lookupMethod(name, parameterTypes.toTypedArray(), returnType)?.let { return it } } return null } - private fun Class<*>.tryGetMethod(name: String, parameterTypes: Array>, returnType: Class<*>, declared: Boolean): Method? = + private fun Class<*>.lookupMethod(name: String, parameterTypes: Array>, returnType: Class<*>): Method? { + tryGetMethod(name, parameterTypes, returnType)?.let { return it } + + superclass?.lookupMethod(name, parameterTypes, returnType)?.let { return it } + + // TODO: avoid exponential complexity here + for (superInterface in interfaces) { + superInterface.lookupMethod(name, parameterTypes, returnType)?.let { return it } + } + + return null + } + + private fun Class<*>.tryGetMethod(name: String, parameterTypes: Array>, returnType: Class<*>): Method? = try { - val result = if (declared) getDeclaredMethod(name, *parameterTypes) else getMethod(name, *parameterTypes) + val result = getDeclaredMethod(name, *parameterTypes) if (result.returnType == returnType) result else { @@ -201,8 +200,7 @@ internal abstract class KDeclarationContainerImpl : ClassBasedDeclarationContain // with the given parameter types and Java reflection API has returned not the one we're looking for. // Falling back to enumerating all methods in the class in this (rather rare) case. // Example: class A(val x: Int) { fun getX(): String = ... } - val allMethods = if (declared) declaredMethods else methods - allMethods.firstOrNull { method -> + declaredMethods.firstOrNull { method -> method.name == name && method.returnType == returnType && method.parameterTypes!!.contentEquals(parameterTypes) @@ -212,21 +210,20 @@ internal abstract class KDeclarationContainerImpl : ClassBasedDeclarationContain null } - private fun Class<*>.tryGetConstructor(parameterTypes: List>, declared: Boolean): Constructor<*>? = + private fun Class<*>.tryGetConstructor(parameterTypes: List>): Constructor<*>? = try { - if (declared) getDeclaredConstructor(*parameterTypes.toTypedArray()) - else getConstructor(*parameterTypes.toTypedArray()) + getDeclaredConstructor(*parameterTypes.toTypedArray()) } catch (e: NoSuchMethodException) { null } - fun findMethodBySignature(name: String, desc: String, isPublic: Boolean): Method? { + fun findMethodBySignature(name: String, desc: String): Method? { if (name == "") return null - return methodOwner.lookupMethod(name, loadParameterTypes(desc), loadReturnType(desc), isPublic) + return methodOwner.lookupMethod(name, loadParameterTypes(desc), loadReturnType(desc)) } - fun findDefaultMethod(name: String, desc: String, isMember: Boolean, isPublic: Boolean): Method? { + fun findDefaultMethod(name: String, desc: String, isMember: Boolean): Method? { if (name == "") return null val parameterTypes = arrayListOf>() @@ -235,19 +232,16 @@ internal abstract class KDeclarationContainerImpl : ClassBasedDeclarationContain } addParametersAndMasks(parameterTypes, desc, false) - return methodOwner.lookupMethod(name + JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX, parameterTypes, loadReturnType(desc), isPublic) + return methodOwner.lookupMethod(name + JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX, parameterTypes, loadReturnType(desc)) } - fun findConstructorBySignature(desc: String, isPublic: Boolean): Constructor<*>? { - return jClass.tryGetConstructor(loadParameterTypes(desc), declared = !isPublic) - } + fun findConstructorBySignature(desc: String): Constructor<*>? = + jClass.tryGetConstructor(loadParameterTypes(desc)) - fun findDefaultConstructor(desc: String, isPublic: Boolean): Constructor<*>? { - val parameterTypes = arrayListOf>() - addParametersAndMasks(parameterTypes, desc, true) - - return jClass.tryGetConstructor(parameterTypes, declared = !isPublic) - } + fun findDefaultConstructor(desc: String): Constructor<*>? = + jClass.tryGetConstructor(arrayListOf>().also { parameterTypes -> + addParametersAndMasks(parameterTypes, desc, true) + }) private fun addParametersAndMasks(result: MutableList>, desc: String, isConstructor: Boolean) { val valueParameters = loadParameterTypes(desc) diff --git a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KFunctionImpl.kt b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KFunctionImpl.kt index 7f223d520c3..451b0bdd05f 100644 --- a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KFunctionImpl.kt +++ b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KFunctionImpl.kt @@ -63,10 +63,9 @@ internal class KFunctionImpl private constructor( is KotlinConstructor -> { if (isAnnotationConstructor) return@caller AnnotationConstructorCaller(container.jClass, parameters.map { it.name!! }, POSITIONAL_CALL, KOTLIN) - container.findConstructorBySignature(jvmSignature.constructorDesc, descriptor.isPublicInBytecode) + container.findConstructorBySignature(jvmSignature.constructorDesc) } - is KotlinFunction -> - container.findMethodBySignature(jvmSignature.methodName, jvmSignature.methodDesc, descriptor.isPublicInBytecode) + is KotlinFunction -> container.findMethodBySignature(jvmSignature.methodName, jvmSignature.methodDesc) is JavaMethod -> jvmSignature.method is JavaConstructor -> jvmSignature.constructor is FakeJavaAnnotationConstructor -> { @@ -94,15 +93,12 @@ internal class KFunctionImpl private constructor( val jvmSignature = RuntimeTypeMapper.mapSignature(descriptor) val member: Member? = when (jvmSignature) { is KotlinFunction -> { - container.findDefaultMethod( - jvmSignature.methodName, jvmSignature.methodDesc, - !Modifier.isStatic(caller.member!!.modifiers), descriptor.isPublicInBytecode - ) + container.findDefaultMethod(jvmSignature.methodName, jvmSignature.methodDesc, !Modifier.isStatic(caller.member!!.modifiers)) } is KotlinConstructor -> { if (isAnnotationConstructor) return@defaultCaller AnnotationConstructorCaller(container.jClass, parameters.map { it.name!! }, CALL_BY_NAME, KOTLIN) - container.findDefaultConstructor(jvmSignature.constructorDesc, descriptor.isPublicInBytecode) + container.findDefaultConstructor(jvmSignature.constructorDesc) } is FakeJavaAnnotationConstructor -> { val methods = jvmSignature.methods diff --git a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt index 0b3c6b9546d..9ddd9cd030f 100644 --- a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt +++ b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt @@ -221,8 +221,7 @@ private fun KPropertyImpl.Accessor<*, *>.computeCallerForAccessor(isGetter: Bool val accessor = accessorSignature?.let { signature -> property.container.findMethodBySignature( jvmSignature.nameResolver.getString(signature.name), - jvmSignature.nameResolver.getString(signature.desc), - descriptor.isPublicInBytecode + jvmSignature.nameResolver.getString(signature.desc) ) } @@ -257,10 +256,9 @@ private fun KPropertyImpl.Accessor<*, *>.computeCallerForAccessor(isGetter: Bool val signature = if (isGetter) jvmSignature.getterSignature else (jvmSignature.setterSignature ?: throw KotlinReflectionInternalError("No setter found for property $property")) - val accessor = property.container.findMethodBySignature( - signature.methodName, signature.methodDesc, descriptor.isPublicInBytecode - ) ?: throw KotlinReflectionInternalError("No accessor found for property $property") - + val accessor = + property.container.findMethodBySignature(signature.methodName, signature.methodDesc) + ?: throw KotlinReflectionInternalError("No accessor found for property $property") assert(!Modifier.isStatic(accessor.modifiers)) { "Mapped property cannot have a static accessor: $property" } return if (isBound) CallerImpl.Method.BoundInstance(accessor, boundReceiver) diff --git a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/util.kt b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/util.kt index 64492628ffa..2ad9415d885 100644 --- a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/util.kt +++ b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/util.kt @@ -20,7 +20,6 @@ import org.jetbrains.kotlin.builtins.jvm.JavaToKotlinClassMap import org.jetbrains.kotlin.descriptors.* import org.jetbrains.kotlin.descriptors.annotations.Annotated import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor -import org.jetbrains.kotlin.descriptors.annotations.isEffectivelyInlineOnly import org.jetbrains.kotlin.load.java.JvmAbi import org.jetbrains.kotlin.load.kotlin.KotlinJvmBinarySourceElement import org.jetbrains.kotlin.load.kotlin.header.KotlinClassHeader @@ -187,12 +186,6 @@ internal val ReflectKotlinClass.packageModuleName: String? } } -internal val CallableMemberDescriptor.isPublicInBytecode: Boolean - get() { - val visibility = visibility - return (visibility == Visibilities.PUBLIC || visibility == Visibilities.INTERNAL) && !isEffectivelyInlineOnly() - } - internal val CallableDescriptor.instanceReceiverParameter: ReceiverParameterDescriptor? get() = if (dispatchReceiverParameter != null) (containingDeclaration as ClassDescriptor).thisAsReceiverParameter