From 43cb17cdd595575eea3b3058622bea6897bbb10c Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Wed, 29 May 2019 18:38:56 +0200 Subject: [PATCH] Check expected type of annotation parameter correctly in AnnotationDeserializer The main idea of this refactoring is to separate two usages of `AnnotationDeserializer.resolveValue`: the one where we load annotation argument values, and the one where we load constant values of properties for JS/Native/Common (`AnnotationAndConstantLoaderImpl.loadPropertyConstant`). In the latter case, `expectedType` is the type of the property and it can be a supertype of the actual value (e.g. see `arrayConst` in compiler/testData/serialization/builtinsSerializer/compileTimeConstants.kt). But in the former case, we need to check that the value conforms to the expected type and disregard it if it's not the case, which is possible if the annotation was recompiled separately. #KT-28927 --- .../AbstractAnnotationDeserializer.kt | 26 +--- .../onTypes/classLiteralWithExpectedType.kt | 24 ++++ .../codegen/BlackBoxCodegenTestGenerated.java | 5 + .../LightAnalysisModeTestGenerated.java | 5 + .../ir/IrBlackBoxCodegenTestGenerated.java | 5 + .../deserialization/AnnotationDeserializer.kt | 114 +++++++----------- 6 files changed, 84 insertions(+), 95 deletions(-) create mode 100644 compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt diff --git a/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/deserialization/AbstractAnnotationDeserializer.kt b/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/deserialization/AbstractAnnotationDeserializer.kt index c7c6af35dbf..39c1f677354 100644 --- a/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/deserialization/AbstractAnnotationDeserializer.kt +++ b/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/deserialization/AbstractAnnotationDeserializer.kt @@ -142,29 +142,7 @@ abstract class AbstractAnnotationDeserializer( ) } // ARRAY -> { -// val expectedIsArray = KotlinBuiltIns.isArray(expectedType) || KotlinBuiltIns.isPrimitiveArray(expectedType) -// val arrayElements = value.arrayElementList -// -// val actualArrayType = -// if (arrayElements.isNotEmpty()) { -// val actualElementType = resolveArrayElementType(arrayElements.first(), nameResolver) -// builtIns.getPrimitiveArrayKotlinTypeByPrimitiveKotlinType(actualElementType) -// ?: builtIns.getArrayType(Variance.INVARIANT, actualElementType) -// } else { -// // In the case of empty array, no element has the element type, so we fall back to the expected type, if any. -// // This is not very accurate when annotation class has been changed without recompiling clients, -// // but should not in fact matter because the value is empty anyway -// if (expectedIsArray) expectedType else builtIns.getArrayType(Variance.INVARIANT, builtIns.anyType) -// } -// -// val expectedElementType = builtIns.getArrayElementType(if (expectedIsArray) expectedType else actualArrayType) -// -// ConstantValueFactory.createArrayValue( -// arrayElements.map { -// resolveValue(expectedElementType, it, nameResolver) -// }, -// actualArrayType -// ) +// TODO: see AnnotationDeserializer // } // else -> error("Unsupported annotation argument type: ${value.type} (expected $expectedType)") else -> return null @@ -173,4 +151,4 @@ abstract class AbstractAnnotationDeserializer( } private fun const(kind: IrConstKind, value: T) = FirConstExpressionImpl(session, null, kind, value) -} \ No newline at end of file +} diff --git a/compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt b/compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt new file mode 100644 index 00000000000..aa457f531f9 --- /dev/null +++ b/compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt @@ -0,0 +1,24 @@ +// TARGET_BACKEND: JVM +// WITH_REFLECT + +import kotlin.reflect.KClass +import kotlin.test.assertEquals + +@Target(AnnotationTarget.TYPE) +annotation class Anno( + val k1: KClass, + val k2: KClass, + val ka: Array> +) + +fun f(): @Anno(String::class, CharSequence::class, [Double::class, Long::class, Int::class]) Unit {} + +fun box(): String { + assertEquals( + "[@Anno(k1=class java.lang.String, k2=interface java.lang.CharSequence, " + + "ka=[class java.lang.Double, class java.lang.Long, class java.lang.Integer])]", + ::f.returnType.annotations.toString() + ) + + return "OK" +} diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index 9647bf73a32..9e4cac8cba0 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -20008,6 +20008,11 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/box/reflection/annotations/onTypes"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.JVM, true); } + @TestMetadata("classLiteralWithExpectedType.kt") + public void testClassLiteralWithExpectedType() throws Exception { + runTest("compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt"); + } + @TestMetadata("differentArgumentTypes.kt") public void testDifferentArgumentTypes() throws Exception { runTest("compiler/testData/codegen/box/reflection/annotations/onTypes/differentArgumentTypes.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 432d2e00f4c..1c8ae04c9d1 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -20008,6 +20008,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/box/reflection/annotations/onTypes"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.JVM, true); } + @TestMetadata("classLiteralWithExpectedType.kt") + public void testClassLiteralWithExpectedType() throws Exception { + runTest("compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt"); + } + @TestMetadata("differentArgumentTypes.kt") public void testDifferentArgumentTypes() throws Exception { runTest("compiler/testData/codegen/box/reflection/annotations/onTypes/differentArgumentTypes.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index f72f1e66592..54451b8298b 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java @@ -20013,6 +20013,11 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/box/reflection/annotations/onTypes"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.JVM_IR, true); } + @TestMetadata("classLiteralWithExpectedType.kt") + public void testClassLiteralWithExpectedType() throws Exception { + runTest("compiler/testData/codegen/box/reflection/annotations/onTypes/classLiteralWithExpectedType.kt"); + } + @TestMetadata("differentArgumentTypes.kt") public void testDifferentArgumentTypes() throws Exception { runTest("compiler/testData/codegen/box/reflection/annotations/onTypes/differentArgumentTypes.kt"); diff --git a/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/AnnotationDeserializer.kt b/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/AnnotationDeserializer.kt index 69afa7ef77e..e72873f80ca 100644 --- a/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/AnnotationDeserializer.kt +++ b/core/deserialization/src/org/jetbrains/kotlin/serialization/deserialization/AnnotationDeserializer.kt @@ -20,7 +20,6 @@ import org.jetbrains.kotlin.builtins.KotlinBuiltIns import org.jetbrains.kotlin.descriptors.* import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptorImpl -import org.jetbrains.kotlin.descriptors.annotations.Annotations import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation.Argument import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation.Argument.Value @@ -31,8 +30,9 @@ import org.jetbrains.kotlin.name.ClassId import org.jetbrains.kotlin.name.Name import org.jetbrains.kotlin.resolve.DescriptorUtils import org.jetbrains.kotlin.resolve.constants.* -import org.jetbrains.kotlin.types.* -import org.jetbrains.kotlin.types.typeUtil.isSubtypeOf +import org.jetbrains.kotlin.resolve.descriptorUtil.classId +import org.jetbrains.kotlin.types.ErrorUtils +import org.jetbrains.kotlin.types.KotlinType class AnnotationDeserializer(private val module: ModuleDescriptor, private val notFoundClasses: NotFoundClasses) { private val builtIns: KotlinBuiltIns @@ -59,13 +59,19 @@ class AnnotationDeserializer(private val module: ModuleDescriptor, private val n nameResolver: NameResolver ): Pair>? { val parameter = parameterByName[nameResolver.getName(proto.nameId)] ?: return null - return Pair(nameResolver.getName(proto.nameId), resolveValue(parameter.type, proto.value, nameResolver)) + return Pair(nameResolver.getName(proto.nameId), resolveValueAndCheckExpectedType(parameter.type, proto.value, nameResolver)) + } + + private fun resolveValueAndCheckExpectedType(expectedType: KotlinType, value: Value, nameResolver: NameResolver): ConstantValue<*> { + return resolveValue(expectedType, value, nameResolver).takeIf { + doesValueConformToExpectedType(it, expectedType, value) + } ?: ErrorValue.create("Unexpected argument value: actual type ${value.type} != expected type $expectedType") } fun resolveValue(expectedType: KotlinType, value: Value, nameResolver: NameResolver): ConstantValue<*> { val isUnsigned = Flags.IS_UNSIGNED.get(value.flags) - val result: ConstantValue<*> = when (value.type) { + return when (value.type) { Type.BYTE -> value.intValue.toByte().letIf(isUnsigned, ::UByteValue, ::ByteValue) Type.CHAR -> CharValue(value.intValue.toChar()) Type.SHORT -> value.intValue.toShort().letIf(isUnsigned, ::UShortValue, ::ShortValue) @@ -74,81 +80,47 @@ class AnnotationDeserializer(private val module: ModuleDescriptor, private val n Type.FLOAT -> FloatValue(value.floatValue) Type.DOUBLE -> DoubleValue(value.doubleValue) Type.BOOLEAN -> BooleanValue(value.intValue != 0L) - Type.STRING -> { - StringValue(nameResolver.getString(value.stringValue)) - } - Type.CLASS -> { - KClassValue(nameResolver.getClassId(value.classId), value.arrayDimensionCount) - } - Type.ENUM -> { - EnumValue(nameResolver.getClassId(value.classId), nameResolver.getName(value.enumValueId)) - } - Type.ANNOTATION -> { - AnnotationValue(deserializeAnnotation(value.annotation, nameResolver)) - } - Type.ARRAY -> { - val expectedIsArray = KotlinBuiltIns.isArray(expectedType) || KotlinBuiltIns.isPrimitiveArray(expectedType) - val arrayElements = value.arrayElementList - - val actualArrayType = - if (arrayElements.isNotEmpty()) { - val actualElementType = resolveArrayElementType(arrayElements.first(), nameResolver) - builtIns.getPrimitiveArrayKotlinTypeByPrimitiveKotlinType(actualElementType) - ?: builtIns.getArrayType(Variance.INVARIANT, actualElementType) - } else { - // In the case of empty array, no element has the element type, so we fall back to the expected type, if any. - // This is not very accurate when annotation class has been changed without recompiling clients, - // but should not in fact matter because the value is empty anyway - if (expectedIsArray) expectedType else builtIns.getArrayType(Variance.INVARIANT, builtIns.anyType) - } - - val expectedElementType = builtIns.getArrayElementType(if (expectedIsArray) expectedType else actualArrayType) - - ConstantValueFactory.createArrayValue( - arrayElements.map { - resolveValue(expectedElementType, it, nameResolver) - }, - actualArrayType - ) - } + Type.STRING -> StringValue(nameResolver.getString(value.stringValue)) + Type.CLASS -> KClassValue(nameResolver.getClassId(value.classId), value.arrayDimensionCount) + Type.ENUM -> EnumValue(nameResolver.getClassId(value.classId), nameResolver.getName(value.enumValueId)) + Type.ANNOTATION -> AnnotationValue(deserializeAnnotation(value.annotation, nameResolver)) + Type.ARRAY -> ConstantValueFactory.createArrayValue( + value.arrayElementList.map { resolveValue(builtIns.anyType, it, nameResolver) }, + expectedType + ) else -> error("Unsupported annotation argument type: ${value.type} (expected $expectedType)") } + } - return if (result.getType(module).isSubtypeOf(expectedType)) { - result - } else { - // This usually means that an annotation class has been changed incompatibly without recompiling clients - ErrorValue.create( - "Unexpected argument value: type ${result.getType(module)} is not a subtype of $expectedType (value.type = ${value.type})" - ) + // This method returns false if the actual value loaded from an annotation argument does not conform to the expected type of the + // corresponding parameter in the annotation class. This usually means that the annotation class has been changed incompatibly + // without recompiling clients, in which case we prefer not to load the annotation argument value at all, to avoid constructing + // an incorrect model and breaking some assumptions in the compiler. + private fun doesValueConformToExpectedType(result: ConstantValue<*>, expectedType: KotlinType, value: Value): Boolean { + return when (value.type) { + Type.CLASS -> { + val expectedClass = expectedType.constructor.declarationDescriptor as? ClassDescriptor + // We could also check that the class value's type is a subtype of the expected type, but loading the definition of the + // referenced class here is undesirable and may even be incorrect (because the module might be different at the + // destination where these constant values are read). This can lead to slightly incorrect model in some edge cases. + expectedClass == null || KotlinBuiltIns.isKClass(expectedClass) + } + Type.ARRAY -> { + check(result is ArrayValue && result.value.size == value.arrayElementList.size) { + "Deserialized ArrayValue should have the same number of elements as the original array value: $result" + } + val expectedElementType = builtIns.getArrayElementType(expectedType) + result.value.indices.all { i -> + doesValueConformToExpectedType(result.value[i], expectedElementType, value.getArrayElement(i)) + } + } + else -> result.getType(module) == expectedType } } private inline fun T.letIf(predicate: Boolean, f: (T) -> R, g: (T) -> R): R = if (predicate) f(this) else g(this) - private fun resolveArrayElementType(value: Value, nameResolver: NameResolver): SimpleType = - with(builtIns) { - when (value.type) { - Type.BYTE -> byteType - Type.CHAR -> charType - Type.SHORT -> shortType - Type.INT -> intType - Type.LONG -> longType - Type.FLOAT -> floatType - Type.DOUBLE -> doubleType - Type.BOOLEAN -> booleanType - Type.STRING -> stringType - Type.CLASS -> KotlinTypeFactory.simpleNotNullType( - Annotations.EMPTY, kClass, listOf(StarProjectionImpl(kClass.declaredTypeParameters.single())) - ) - Type.ENUM -> resolveClass(nameResolver.getClassId(value.classId)).defaultType - Type.ANNOTATION -> resolveClass(nameResolver.getClassId(value.annotation.id)).defaultType - Type.ARRAY -> error("Array of arrays is impossible") - else -> error("Unknown type: ${value.type}") - } - } - private fun resolveClass(classId: ClassId): ClassDescriptor { return module.findNonGenericClassAcrossDependencies(classId, notFoundClasses) }