From 8c8f0639f83e87f53c5236936d3c9cf4e49812f1 Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Mon, 27 Aug 2018 19:43:56 +0200 Subject: [PATCH] Remove unneeded CallerImpl implementations Also fix a bug where nullability of the assigned value was not checked in ClassCompanionFieldSetter because it wasn't a subclass of FieldSetter where this check occurred --- .../call/disallowNullValueForNotNullField.kt | 22 +++++++ .../reflect/jvm/internal/KPropertyImpl.kt | 48 ++++++---------- .../reflect/jvm/internal/calls/CallerImpl.kt | 57 +++---------------- 3 files changed, 47 insertions(+), 80 deletions(-) diff --git a/compiler/testData/codegen/box/reflection/call/disallowNullValueForNotNullField.kt b/compiler/testData/codegen/box/reflection/call/disallowNullValueForNotNullField.kt index c3367ca9f5e..d9f42ff8bfc 100644 --- a/compiler/testData/codegen/box/reflection/call/disallowNullValueForNotNullField.kt +++ b/compiler/testData/codegen/box/reflection/call/disallowNullValueForNotNullField.kt @@ -23,6 +23,14 @@ class CounterTest(t: T) { private var generic: T = t } +class C { + companion object { + private var z: String = "" + + fun getBoundZ() = this::z + } +} + fun box(): String { val p = A::class.memberProperties.single() as KMutableProperty1 p.isAccessible = true @@ -47,5 +55,19 @@ fun box(): String { d.isAccessible = true d.setter.call(CounterTest(""), null) // Also should not fail, because we can't be sure about nullability of 'generic' + val z = C.Companion::class.memberProperties.single { it.name == "z" } as KMutableProperty1 + z.isAccessible = true + try { + z.setter.call(C, null) + return "Fail: exception should have been thrown" + } catch (e: IllegalArgumentException) {} + + val zz = C.getBoundZ() as KMutableProperty0 + zz.isAccessible = true + try { + zz.setter.call(null) + return "Fail: exception should have been thrown" + } catch (e: IllegalArgumentException) {} + return "OK" } 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 83ffa8bd58b..aa36f8bbf55 100644 --- a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt +++ b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/KPropertyImpl.kt @@ -179,42 +179,14 @@ private fun KPropertyImpl.Accessor<*, *>.computeCallerForAccessor(isGetter: Bool return ThrowingCaller } - fun isInsideClassCompanionObject(): Boolean { - val possibleCompanionObject = property.descriptor.containingDeclaration - return DescriptorUtils.isCompanionObject(possibleCompanionObject) && - !DescriptorUtils.isInterface(possibleCompanionObject.containingDeclaration) - } + fun isJvmStaticProperty(): Boolean = + property.descriptor.annotations.hasAnnotation(JVM_STATIC) - fun isInsideJvmInterfaceCompanionObject(): Boolean { - val possibleCompanionObject = property.descriptor.containingDeclaration - return DescriptorUtils.isCompanionObject(possibleCompanionObject) && - (DescriptorUtils.isInterface(possibleCompanionObject.containingDeclaration) || - DescriptorUtils.isAnnotationClass(possibleCompanionObject.containingDeclaration)) - } - - fun isInsideInterfaceCompanionObjectWithJvmField(): Boolean { - val propertyDescriptor = property.descriptor - if (propertyDescriptor !is DeserializedPropertyDescriptor || !isInsideJvmInterfaceCompanionObject()) return false - return JvmProtoBufUtil.isMovedFromInterfaceCompanion(propertyDescriptor.proto) - } - - fun isJvmStaticProperty() = - property.descriptor.annotations.findAnnotation(JVM_STATIC) != null - - fun isNotNullProperty() = + fun isNotNullProperty(): Boolean = !TypeUtils.isNullableType(property.descriptor.type) fun computeFieldCaller(field: Field): Caller = when { - isInsideClassCompanionObject() || isInsideInterfaceCompanionObjectWithJvmField() -> { - val klass = (descriptor.containingDeclaration as ClassDescriptor).toJavaClass()!! - if (isGetter) - if (isBound) CallerImpl.BoundClassCompanionFieldGetter(field, klass) - else CallerImpl.ClassCompanionFieldGetter(field, klass) - else - if (isBound) CallerImpl.BoundClassCompanionFieldSetter(field, klass) - else CallerImpl.ClassCompanionFieldSetter(field, klass) - } - !Modifier.isStatic(field.modifiers) -> + property.descriptor.isJvmFieldPropertyInCompanionObject() || !Modifier.isStatic(field.modifiers) -> if (isGetter) if (isBound) CallerImpl.BoundInstanceFieldGetter(field, property.boundReceiver) else CallerImpl.InstanceFieldGetter(field) @@ -293,3 +265,15 @@ private fun KPropertyImpl.Accessor<*, *>.computeCallerForAccessor(isGetter: Bool } } } + +private fun PropertyDescriptor.isJvmFieldPropertyInCompanionObject(): Boolean { + val container = containingDeclaration + if (!DescriptorUtils.isCompanionObject(container)) return false + + val outerClass = container.containingDeclaration + return when { + DescriptorUtils.isInterface(outerClass) || DescriptorUtils.isAnnotationClass(outerClass) -> + this is DeserializedPropertyDescriptor && JvmProtoBufUtil.isMovedFromInterfaceCompanion(proto) + else -> true + } +} diff --git a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/calls/CallerImpl.kt b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/calls/CallerImpl.kt index 82863fdfcb3..e3a25c1179a 100644 --- a/core/reflection.jvm/src/kotlin/reflect/jvm/internal/calls/CallerImpl.kt +++ b/core/reflection.jvm/src/kotlin/reflect/jvm/internal/calls/CallerImpl.kt @@ -128,7 +128,7 @@ internal abstract class CallerImpl( abstract class FieldGetter( field: ReflectField, - requiresInstance: Boolean = !Modifier.isStatic(field.modifiers) + requiresInstance: Boolean ) : CallerImpl( field, field.genericType, @@ -144,7 +144,7 @@ internal abstract class CallerImpl( abstract class FieldSetter( field: ReflectField, private val notNull: Boolean, - requiresInstance: Boolean = !Modifier.isStatic(field.modifiers) + requiresInstance: Boolean ) : CallerImpl( field, Void.TYPE, @@ -164,9 +164,9 @@ internal abstract class CallerImpl( } } - class StaticFieldGetter(field: ReflectField) : FieldGetter(field) + class StaticFieldGetter(field: ReflectField) : FieldGetter(field, requiresInstance = false) - class InstanceFieldGetter(field: ReflectField) : FieldGetter(field) + class InstanceFieldGetter(field: ReflectField) : FieldGetter(field, requiresInstance = true) class JvmStaticInObjectFieldGetter(field: ReflectField) : FieldGetter(field, requiresInstance = true) { override fun checkArguments(args: Array<*>) { @@ -175,18 +175,7 @@ internal abstract class CallerImpl( } } - class ClassCompanionFieldGetter(field: ReflectField, klass: Class<*>) : CallerImpl( - field, field.genericType, klass, emptyArray() - ) { - override fun call(args: Array<*>): Any? { - checkArguments(args) - return member.get(args.first()) - } - } - - class BoundInstanceFieldGetter(field: ReflectField, private val boundReceiver: Any?) : FieldGetter( - field, requiresInstance = false - ) { + class BoundInstanceFieldGetter(field: ReflectField, private val boundReceiver: Any?) : FieldGetter(field, requiresInstance = false) { override fun call(args: Array<*>): Any? { checkArguments(args) return member.get(boundReceiver) @@ -195,18 +184,9 @@ internal abstract class CallerImpl( class BoundJvmStaticInObjectFieldGetter(field: ReflectField) : FieldGetter(field, requiresInstance = false) - class BoundClassCompanionFieldGetter(field: ReflectField, private val boundReceiver: Any?) : FieldGetter( - field, requiresInstance = false - ) { - override fun call(args: Array<*>): Any? { - checkArguments(args) - return member.get(boundReceiver) - } - } + class StaticFieldSetter(field: ReflectField, notNull: Boolean) : FieldSetter(field, notNull, requiresInstance = false) - class StaticFieldSetter(field: ReflectField, notNull: Boolean) : FieldSetter(field, notNull) - - class InstanceFieldSetter(field: ReflectField, notNull: Boolean) : FieldSetter(field, notNull) + class InstanceFieldSetter(field: ReflectField, notNull: Boolean) : FieldSetter(field, notNull, requiresInstance = true) class JvmStaticInObjectFieldSetter(field: ReflectField, notNull: Boolean) : FieldSetter(field, notNull, requiresInstance = true) { override fun checkArguments(args: Array<*>) { @@ -215,18 +195,8 @@ internal abstract class CallerImpl( } } - class ClassCompanionFieldSetter(field: ReflectField, klass: Class<*>) : CallerImpl( - field, Void.TYPE, klass, arrayOf(field.genericType) - ) { - override fun call(args: Array<*>): Any? { - checkArguments(args) - return member.set(null, args.last()) - } - } - - class BoundInstanceFieldSetter(field: ReflectField, notNull: Boolean, private val boundReceiver: Any?) : FieldSetter( - field, notNull, false - ) { + class BoundInstanceFieldSetter(field: ReflectField, notNull: Boolean, private val boundReceiver: Any?) : + FieldSetter(field, notNull, requiresInstance = false) { override fun call(args: Array<*>): Any? { checkArguments(args) return member.set(boundReceiver, args.first()) @@ -242,15 +212,6 @@ internal abstract class CallerImpl( } } - class BoundClassCompanionFieldSetter(field: ReflectField, klass: Class<*>) : CallerImpl( - field, Void.TYPE, klass, arrayOf(field.genericType) - ) { - override fun call(args: Array<*>): Any? { - checkArguments(args) - return member.set(null, args.last()) - } - } - companion object { @Suppress("UNCHECKED_CAST") inline fun Array.dropFirst(): Array =