From ab3f8db74339be1d4373ebf452db643911dc99a8 Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Fri, 31 Aug 2018 19:06:42 +0200 Subject: [PATCH] Consider property accessor non-default if it has annotations Otherwise we're not trying to load annotations on the parameter of the property setter in MemberDeserializer.loadProperty. Note that after this commit, we could now also assume that if getter/setter is default, it has no annotations, and thus use Annotations.EMPTY for default getter/setter in loadProperty. However, this would cause reflection to work incorrectly on classes compiled by an older Kotlin compiler, so we'll still try to load annotations on default accessors for an indefinite time. #KT-25499 Fixed --- .../checkers/JvmFieldApplicabilityChecker.kt | 17 +++++++--- .../kotlin/resolve/DescriptorResolver.java | 31 +++++++++++-------- .../resolve/checkers/ConstModifierChecker.kt | 5 +-- .../annotationsOnNonExistentAccessors.kt | 2 +- .../deserialization/MemberDeserializer.kt | 2 +- 5 files changed, 36 insertions(+), 21 deletions(-) diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmFieldApplicabilityChecker.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmFieldApplicabilityChecker.kt index 66570d59243..1a3b3328463 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmFieldApplicabilityChecker.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmFieldApplicabilityChecker.kt @@ -50,18 +50,18 @@ class JvmFieldApplicabilityChecker : DeclarationChecker { } override fun check(declaration: KtDeclaration, descriptor: DeclarationDescriptor, context: DeclarationCheckerContext) { - if (descriptor !is PropertyDescriptor) return + if (descriptor !is PropertyDescriptor || declaration !is KtProperty) return val annotation = descriptor.findJvmFieldAnnotation() ?: descriptor.delegateField?.annotations?.findAnnotation(JvmAbi.JVM_FIELD_ANNOTATION_FQ_NAME) ?: return val problem = when { - declaration is KtProperty && declaration.hasDelegate() -> DELEGATE + declaration.hasDelegate() -> DELEGATE !descriptor.hasBackingField(context.trace.bindingContext) -> return descriptor.isOverridable -> NOT_FINAL Visibilities.isPrivate(descriptor.visibility) -> PRIVATE - descriptor.hasCustomAccessor() -> CUSTOM_ACCESSOR + declaration.hasCustomAccessor() -> CUSTOM_ACCESSOR descriptor.overriddenDescriptors.isNotEmpty() -> OVERRIDES descriptor.isLateInit -> LATEINIT descriptor.isConst -> CONST @@ -95,7 +95,16 @@ class JvmFieldApplicabilityChecker : DeclarationChecker { return true } - private fun PropertyDescriptor.hasCustomAccessor() = !(getter?.isDefault ?: true) || !(setter?.isDefault ?: true) + // This logic has been effectively used since 1.0. It'd be nice to call [PropertyAccessorDescriptor.isDefault] here instead, + // but that would be a breaking change because in the following case: + // + // @JvmField + // @get:Anno + // val foo = 42 + // + // we'd start considering foo as having a custom getter and disallow the JvmField annotation on it + private fun KtProperty.hasCustomAccessor(): Boolean = + getter != null || setter != null private fun PropertyDescriptor.hasBackingField(bindingContext: BindingContext) = bindingContext.get(BindingContext.BACKING_FIELD_REQUIRED, this) ?: false diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/DescriptorResolver.java b/compiler/frontend/src/org/jetbrains/kotlin/resolve/DescriptorResolver.java index 4cf7e9ac1ef..eae48a2e2b0 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/DescriptorResolver.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/DescriptorResolver.java @@ -1038,10 +1038,13 @@ public class DescriptorResolver { boolean hasDelegate ) { PropertySetterDescriptorImpl setterDescriptor = null; + Annotations setterTargetedAnnotations = annotationSplitter.getAnnotationsForTarget(PROPERTY_SETTER); + Annotations parameterTargetedAnnotations = annotationSplitter.getAnnotationsForTarget(SETTER_PARAMETER); if (setter != null) { Annotations annotations = new CompositeAnnotations(CollectionsKt.listOf( - annotationSplitter.getAnnotationsForTarget(PROPERTY_SETTER), - annotationResolver.resolveAnnotationsWithoutArguments(scopeWithTypeParameters, setter.getModifierList(), trace))); + setterTargetedAnnotations, + annotationResolver.resolveAnnotationsWithoutArguments(scopeWithTypeParameters, setter.getModifierList(), trace) + )); KtParameter parameter = setter.getParameter(); setterDescriptor = new PropertySetterDescriptorImpl( @@ -1082,8 +1085,7 @@ public class DescriptorResolver { } ValueParameterDescriptorImpl valueParameterDescriptor = resolveValueParameterDescriptor( - scopeWithTypeParameters, setterDescriptor, parameter, 0, type, trace, - annotationSplitter.getAnnotationsForTarget(SETTER_PARAMETER) + scopeWithTypeParameters, setterDescriptor, parameter, 0, type, trace, parameterTargetedAnnotations ); setterDescriptor.initialize(valueParameterDescriptor); } @@ -1095,10 +1097,9 @@ public class DescriptorResolver { } else if (property.isVar()) { setterDescriptor = DescriptorFactory.createSetter( - propertyDescriptor, - annotationSplitter.getAnnotationsForTarget(PROPERTY_SETTER), - annotationSplitter.getAnnotationsForTarget(SETTER_PARAMETER), - !hasDelegate, false, property.hasModifier(KtTokens.INLINE_KEYWORD), + propertyDescriptor, setterTargetedAnnotations, parameterTargetedAnnotations, + !hasDelegate && setterTargetedAnnotations.isEmpty() && parameterTargetedAnnotations.isEmpty(), + false, property.hasModifier(KtTokens.INLINE_KEYWORD), propertyDescriptor.getSource() ); } @@ -1124,10 +1125,12 @@ public class DescriptorResolver { ) { PropertyGetterDescriptorImpl getterDescriptor; KotlinType getterType; + Annotations getterTargetedAnnotations = annotationSplitter.getAnnotationsForTarget(PROPERTY_GETTER); if (getter != null) { Annotations getterAnnotations = new CompositeAnnotations(CollectionsKt.listOf( - annotationSplitter.getAnnotationsForTarget(PROPERTY_GETTER), - annotationResolver.resolveAnnotationsWithoutArguments(scopeForDeclarationResolution, getter.getModifierList(), trace))); + getterTargetedAnnotations, + annotationResolver.resolveAnnotationsWithoutArguments(scopeForDeclarationResolution, getter.getModifierList(), trace) + )); getterDescriptor = new PropertyGetterDescriptorImpl( propertyDescriptor, getterAnnotations, @@ -1142,9 +1145,11 @@ public class DescriptorResolver { trace.record(BindingContext.PROPERTY_ACCESSOR, getter, getterDescriptor); } else { - Annotations getterAnnotations = annotationSplitter.getAnnotationsForTarget(PROPERTY_GETTER); - getterDescriptor = DescriptorFactory.createGetter(propertyDescriptor, getterAnnotations, !hasDelegate, - /* isExternal = */ false, property.hasModifier(KtTokens.INLINE_KEYWORD)); + getterDescriptor = DescriptorFactory.createGetter( + propertyDescriptor, getterTargetedAnnotations, + !hasDelegate && getterTargetedAnnotations.isEmpty(), + /* isExternal = */ false, property.hasModifier(KtTokens.INLINE_KEYWORD) + ); getterType = propertyTypeIfKnown; } diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/ConstModifierChecker.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/ConstModifierChecker.kt index 345923a138e..950bc7cd5b5 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/ConstModifierChecker.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/ConstModifierChecker.kt @@ -60,8 +60,9 @@ object ConstModifierChecker : DeclarationChecker { return Errors.CONST_VAL_WITH_DELEGATE.on(declaration.delegate!!).nonApplicable() } - if (!descriptor.getter!!.isDefault) { - return Errors.CONST_VAL_WITH_GETTER.on(declaration.getter!!).nonApplicable() + val getter = declaration.getter + if (!descriptor.getter!!.isDefault && getter != null) { + return Errors.CONST_VAL_WITH_GETTER.on(getter).nonApplicable() } if (descriptor.type.isError) return ConstApplicability.NonApplicable() diff --git a/compiler/testData/codegen/box/annotations/annotationsOnNonExistentAccessors.kt b/compiler/testData/codegen/box/annotations/annotationsOnNonExistentAccessors.kt index 833f4c5f78a..2622bb636f1 100644 --- a/compiler/testData/codegen/box/annotations/annotationsOnNonExistentAccessors.kt +++ b/compiler/testData/codegen/box/annotations/annotationsOnNonExistentAccessors.kt @@ -135,7 +135,7 @@ class Statics { check(::x2.getter, annotationExists = true) check(::x2.setter, annotationExists = true) - check(::x2.setter.parameters.first(), annotationExists = false /* TODO */) + check(::x2.setter.parameters.first(), annotationExists = true) check(::x3.getter, annotationExists = false) diff --git a/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/MemberDeserializer.kt b/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/MemberDeserializer.kt index dcb91f2c92f..62392308b93 100644 --- a/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/MemberDeserializer.kt +++ b/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/MemberDeserializer.kt @@ -128,7 +128,7 @@ class MemberDeserializer(private val c: DeserializationContext) { } else { DescriptorFactory.createDefaultSetter( property, annotations, - Annotations.EMPTY /* Otherwise presumably the setter is not default */ + Annotations.EMPTY /* Otherwise the setter is not default, see DescriptorResolver.resolvePropertySetterDescriptor */ ) } } else {