From f7d87f6d707fcbd31818002a2d4dac123c1ddab8 Mon Sep 17 00:00:00 2001 From: Leonid Startsev Date: Wed, 22 Nov 2023 18:03:24 +0100 Subject: [PATCH] Use type parameters' source information for kotlinx.serialization diagnostics To be more consistent with K1, certain diagnostics should be reported not on the whole properties' types, but on type arguments inside them. Note that there is still a difference with K2 because K2 reports on a type argument including its annotations, while K1 used KtTypeReference.typeElement. IMO, K2 conveys the same or better meaning here, so I am willing to leave this difference instead of providing PositioningStrategy. #KT-53861 Fixed --- .../FirSerializationPluginClassChecker.kt | 38 ++++++++++--------- .../diagnostics/NonSerializable.fir.kt | 18 --------- .../testData/diagnostics/NonSerializable.kt | 5 ++- .../testData/diagnostics/NonSerializable.txt | 30 +++++++++++++++ .../SerializerTypeIncompatible.fir.kt | 5 ++- .../diagnostics/SerializerTypeIncompatible.kt | 3 ++ 6 files changed, 61 insertions(+), 38 deletions(-) delete mode 100644 plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.fir.kt diff --git a/plugins/kotlinx-serialization/kotlinx-serialization.k2/src/org/jetbrains/kotlinx/serialization/compiler/fir/checkers/FirSerializationPluginClassChecker.kt b/plugins/kotlinx-serialization/kotlinx-serialization.k2/src/org/jetbrains/kotlinx/serialization/compiler/fir/checkers/FirSerializationPluginClassChecker.kt index 97e97f54c0e..d6a1d173bb2 100644 --- a/plugins/kotlinx-serialization/kotlinx-serialization.k2/src/org/jetbrains/kotlinx/serialization/compiler/fir/checkers/FirSerializationPluginClassChecker.kt +++ b/plugins/kotlinx-serialization/kotlinx-serialization.k2/src/org/jetbrains/kotlinx/serialization/compiler/fir/checkers/FirSerializationPluginClassChecker.kt @@ -11,10 +11,9 @@ import org.jetbrains.kotlin.descriptors.ClassKind import org.jetbrains.kotlin.diagnostics.DiagnosticReporter import org.jetbrains.kotlin.diagnostics.SourceElementPositioningStrategies import org.jetbrains.kotlin.diagnostics.reportOn +import org.jetbrains.kotlin.fir.analysis.checkers.* import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext import org.jetbrains.kotlin.fir.analysis.checkers.declaration.FirClassChecker -import org.jetbrains.kotlin.fir.analysis.checkers.isSingleFieldValueClass -import org.jetbrains.kotlin.fir.analysis.checkers.toRegularClassSymbol import org.jetbrains.kotlin.fir.declarations.* import org.jetbrains.kotlin.fir.declarations.annotationPlatformSupport import org.jetbrains.kotlin.fir.declarations.utils.* @@ -489,7 +488,7 @@ object FirSerializationPluginClassChecker : FirClassChecker() { ) checkSerializerNullability(propertyType, customSerializerType, source, reporter) } else { - checkType(propertyType, source, reporter) + checkType(typeRef, source, reporter) checkGenericArrayType(propertyType, source, reporter) } } @@ -506,13 +505,15 @@ object FirSerializationPluginClassChecker : FirClassChecker() { } context(CheckerContext) - private fun checkTypeArguments(type: ConeKotlinType, source: KtSourceElement?, reporter: DiagnosticReporter) { - for (typeArgument in type.typeArguments) { - checkType( - typeArgument.type?.fullyExpandedType(session) ?: continue, - source, - reporter - ) + private fun checkTypeArguments( + typeRef: FirTypeRef, + fallbackSource: KtSourceElement?, + reporter: DiagnosticReporter + ) { + val argsRefs = extractArgumentsTypeRefAndSource(typeRef) ?: return + for (typeArgument in argsRefs) { + val argTypeRef = typeArgument.typeRef ?: continue + checkType(argTypeRef, typeArgument.source ?: fallbackSource, reporter) } } @@ -526,11 +527,12 @@ object FirSerializationPluginClassChecker : FirClassChecker() { get() = isSingleFieldValueClass(session) && !isPrimitiveOrNullablePrimitive context(CheckerContext) - private fun checkType(type: ConeKotlinType, source: KtSourceElement?, reporter: DiagnosticReporter) { + private fun checkType(typeRef: FirTypeRef, typeSource: KtSourceElement?, reporter: DiagnosticReporter) { + val type = typeRef.coneType.fullyExpandedType(session) if (type.lowerBoundIfFlexible().isTypeParameter) return // type parameters always have serializer stored in class' field if (type.isUnsupportedInlineType && !canSupportInlineClasses()) { reporter.reportOn( - source, + typeRef.source ?: typeSource, FirSerializationErrors.INLINE_CLASSES_NOT_SUPPORTED, RuntimeVersions.MINIMAL_VERSION_FOR_INLINE_CLASSES.toString(), session.versionReader.runtimeVersions?.implementationVersion.toString() @@ -541,15 +543,15 @@ object FirSerializationPluginClassChecker : FirClassChecker() { if (serializer != null) { val classSymbol = type.toRegularClassSymbol(session) ?: return type.serializableWith?.fullyExpandedType(session)?.let { serializerType -> - checkCustomSerializerMatch(classSymbol, source, type, serializerType, reporter) - checkCustomSerializerIsNotLocal(source, classSymbol, serializerType, reporter) - checkSerializerNullability(type, serializerType, source, reporter) + checkCustomSerializerMatch(classSymbol, typeSource, type, serializerType, reporter) + checkCustomSerializerIsNotLocal(typeSource, classSymbol, serializerType, reporter) + checkSerializerNullability(type, serializerType, typeSource, reporter) } - checkTypeArguments(type, source, reporter) + checkTypeArguments(typeRef, typeSource, reporter) } else { if (type.toRegularClassSymbol(session)?.isEnumClass != true) { // enums are always serializable - reporter.reportOn(source, FirSerializationErrors.SERIALIZER_NOT_FOUND, type) + reporter.reportOn(typeSource, FirSerializationErrors.SERIALIZER_NOT_FOUND, type) } } } @@ -566,7 +568,6 @@ object FirSerializationPluginClassChecker : FirClassChecker() { val declarationTypeClassId = declarationType.classId if (declarationTypeClassId == null || declarationTypeClassId != serializerForType.classId) { - // TODO: report on specific type argument (KT-53861) reporter.reportOn( source ?: containingClassSymbol.serializableOrMetaAnnotationSource, FirSerializationErrors.SERIALIZER_TYPE_INCOMPATIBLE, @@ -576,6 +577,7 @@ object FirSerializationPluginClassChecker : FirClassChecker() { ) } } + context(CheckerContext) private fun checkCustomSerializerNotAbstract( containingClassSymbol: FirClassSymbol<*>, diff --git a/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.fir.kt b/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.fir.kt deleted file mode 100644 index 0c9305915cd..00000000000 --- a/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.fir.kt +++ /dev/null @@ -1,18 +0,0 @@ -// FIR_DISABLE_LAZY_RESOLVE_CHECKS -// WITH_STDLIB -// FIR_DIFFERENCE: KT-53861 -// FILE: test.kt -import kotlinx.serialization.* - -class NonSerializable - -@Serializable -class Basic(val foo: NonSerializable) - -@Serializable -class Inside(val foo: List) - -@Serializable -class WithImplicitType { - val foo = NonSerializable() -} diff --git a/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.kt b/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.kt index fddb34330d3..c30dd3b7720 100644 --- a/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.kt +++ b/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.kt @@ -1,6 +1,6 @@ // FIR_DISABLE_LAZY_RESOLVE_CHECKS // WITH_STDLIB -// FIR_DIFFERENCE: KT-53861 +// FIR_IDENTICAL // FILE: test.kt import kotlinx.serialization.* @@ -12,6 +12,9 @@ class Basic(val foo: NonSerializable< @Serializable class Inside(val foo: List<NonSerializable>) +@Serializable +class Inside2(val foo: ListNonSerializable>>) + @Serializable class WithImplicitType { val foo = NonSerializable() diff --git a/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.txt b/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.txt index 363dd420520..c9a0dbb2b53 100644 --- a/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.txt +++ b/plugins/kotlinx-serialization/testData/diagnostics/NonSerializable.txt @@ -60,6 +60,36 @@ package } } +@kotlinx.serialization.Serializable public final class Inside2 { + @kotlin.Deprecated(level = DeprecationLevel.HIDDEN, message = "This synthesized declaration should not be used directly", replaceWith = kotlin.ReplaceWith(expression = "", imports = {})) internal /*synthesized*/ constructor Inside2(/*0*/ seen1: kotlin.Int, /*1*/ foo: kotlin.collections.List>?, /*2*/ serializationConstructorMarker: kotlinx.serialization.internal.SerializationConstructorMarker?) + public constructor Inside2(/*0*/ foo: kotlin.collections.List>) + public final val foo: kotlin.collections.List> + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + @kotlin.jvm.JvmStatic internal final /*synthesized*/ fun `write$Self`(/*0*/ self: Inside2, /*1*/ output: kotlinx.serialization.encoding.CompositeEncoder, /*2*/ serialDesc: kotlinx.serialization.descriptors.SerialDescriptor): kotlin.Unit + + @kotlin.Deprecated(level = DeprecationLevel.HIDDEN, message = "This synthesized declaration should not be used directly", replaceWith = kotlin.ReplaceWith(expression = "", imports = {})) public object `$serializer` : kotlinx.serialization.internal.GeneratedSerializer { + private constructor `$serializer`() + public open override /*1*/ /*synthesized*/ val descriptor: kotlinx.serialization.descriptors.SerialDescriptor + public open override /*1*/ /*synthesized*/ fun childSerializers(): kotlin.Array> + public open override /*1*/ /*synthesized*/ fun deserialize(/*0*/ decoder: kotlinx.serialization.encoding.Decoder): Inside2 + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*synthesized*/ fun serialize(/*0*/ encoder: kotlinx.serialization.encoding.Encoder, /*1*/ value: Inside2): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + public open override /*1*/ /*fake_override*/ fun typeParametersSerializers(): kotlin.Array> + } + + public companion object Companion { + private constructor Companion() + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public final /*synthesized*/ fun serializer(): kotlinx.serialization.KSerializer + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } +} + public final class NonSerializable { public constructor NonSerializable() public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean diff --git a/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.fir.kt b/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.fir.kt index 71b835ad82f..dfb57cc907c 100644 --- a/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.fir.kt +++ b/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.fir.kt @@ -21,7 +21,10 @@ class Biz(val i: Int) class Foo(@Serializable(with = BazSerializer::class) val i: Bar) @Serializable -class Foo2(val li: List<@Serializable(with = BazSerializer::class) Bar>) +class Foo2(val li: List<@Serializable(with = BazSerializer::class) Bar>) + +@Serializable +class Foo25(val i: @Serializable(with = BazSerializer::class) Bar) @Serializable class Foo3(@Serializable(with = BazSerializer::class) val i: Baz) diff --git a/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.kt b/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.kt index b33eefeb397..19611d0cb4a 100644 --- a/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.kt +++ b/plugins/kotlinx-serialization/testData/diagnostics/SerializerTypeIncompatible.kt @@ -23,6 +23,9 @@ class Foo(@Serializable(with = BazSerializer::class) val i: Bar>) +@Serializable +class Foo25(val i: @Serializable(with = BazSerializer::class) Bar) + @Serializable class Foo3(@Serializable(with = BazSerializer::class) val i: Baz)