Add checker for companion objects inside serializable class:

Warning for old FE, error in FIR #KT-54441 Fixed Merged-by: Leonid Startsev <leonid.startsev@jetbrains.com>
2022-10-20 10:18:04 +00:00
parent 3832c6a520
commit 4cf50d7d23
12 changed files with 238 additions and 8 deletions
@@ -6,6 +6,7 @@
 package org.jetbrains.kotlinx.serialization.compiler.diagnostic;

 import com.intellij.psi.PsiElement;
+import org.jetbrains.kotlin.descriptors.ClassDescriptor;
 import org.jetbrains.kotlin.diagnostics.*;
 import org.jetbrains.kotlin.psi.KtAnnotationEntry;
 import org.jetbrains.kotlin.types.KotlinType;
@@ -19,6 +20,13 @@ public interface SerializationErrors {
    DiagnosticFactory0<PsiElement> ANONYMOUS_OBJECTS_NOT_SUPPORTED = DiagnosticFactory0.create(ERROR);
    DiagnosticFactory0<PsiElement> INNER_CLASSES_NOT_SUPPORTED = DiagnosticFactory0.create(ERROR);

+
+    DiagnosticFactory1<PsiElement, ClassDescriptor> COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED = DiagnosticFactory1.create(WARNING);
+
+    DiagnosticFactory2<PsiElement, KotlinType, KotlinType> COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS = DiagnosticFactory2.create(WARNING);
+
+    DiagnosticFactory2<PsiElement, KotlinType, KotlinType> COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS = DiagnosticFactory2.create(WARNING);
+
    DiagnosticFactory0<PsiElement> EXPLICIT_SERIALIZABLE_IS_REQUIRED = DiagnosticFactory0.create(WARNING);

    DiagnosticFactory0<KtAnnotationEntry> SERIALIZABLE_ANNOTATION_IGNORED = DiagnosticFactory0.create(ERROR);
@@ -171,6 +171,8 @@ open class SerializationPluginDeclarationChecker : DeclarationChecker {
            return false
        }

+        checkCompanionSerializerDependency(descriptor, declaration, trace)
+
        if (!descriptor.hasSerializableOrMetaAnnotation) return false

        if (!serializationPluginEnabledOn(descriptor)) {
@@ -200,6 +202,7 @@ open class SerializationPluginDeclarationChecker : DeclarationChecker {
            }
            return false
        }
+
        if (!descriptor.hasSerializableOrMetaAnnotationWithoutArgs) {
            // defined custom serializer
            checkClassWithCustomSerializer(descriptor, declaration, trace)
@@ -222,6 +225,53 @@ open class SerializationPluginDeclarationChecker : DeclarationChecker {
        return true
    }

+    private fun checkCompanionSerializerDependency(descriptor: ClassDescriptor, declaration: KtDeclaration, trace: BindingTrace) {
+        val companionObjectDescriptor = descriptor.companionObjectDescriptor ?: return
+        val serializerForInCompanion = companionObjectDescriptor.serializerForClass ?: return
+        val serializerAnnotationSource =
+            companionObjectDescriptor.findAnnotationDeclaration(SerializationAnnotations.serializerAnnotationFqName)
+        val serializableWith = descriptor.serializableWith
+        if (descriptor.hasSerializableOrMetaAnnotationWithoutArgs) {
+            if (serializerForInCompanion == descriptor.defaultType) {
+                // @Serializable class Foo / @Serializer(Foo::class) companion object — prohibited due to problems with recursive resolve
+                descriptor.onSerializableOrMetaAnnotation {
+                    trace.report(SerializationErrors.COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED.on(it, descriptor))
+                }
+            } else {
+                // @Serializable class Foo / @Serializer(Bar::class) companion object — prohibited as vague and confusing
+                trace.report(
+                    SerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS.on(
+                        serializerAnnotationSource ?: declaration,
+                        descriptor.defaultType,
+                        serializerForInCompanion
+                    )
+                )
+            }
+        } else if (serializableWith != null) {
+            if (serializableWith == companionObjectDescriptor.defaultType && serializerForInCompanion == descriptor.defaultType) {
+                // @Serializable(Foo.Companion) class Foo / @Serializer(Foo::class) companion object — the only case that is allowed
+            } else {
+                // @Serializable(anySer) class Foo / @Serializer(anyOtherClass) companion object — prohibited as vague and confusing
+                trace.report(
+                    SerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS.on(
+                        serializerAnnotationSource ?: declaration,
+                        descriptor.defaultType,
+                        serializerForInCompanion
+                    )
+                )
+            }
+        } else {
+            // (regular) class Foo / @Serializer(something) companion object - not recommended
+            trace.report(
+                SerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS.on(
+                    serializerAnnotationSource ?: declaration,
+                    descriptor.defaultType,
+                    serializerForInCompanion
+                )
+            )
+        }
+    }
+
    private fun checkClassWithCustomSerializer(descriptor: ClassDescriptor, declaration: KtDeclaration, trace: BindingTrace) {
        val annotationPsi = descriptor.findSerializableOrMetaAnnotationDeclaration()
        checkCustomSerializerMatch(descriptor.module, descriptor.defaultType, descriptor, annotationPsi, trace, declaration)
@@ -34,6 +34,31 @@ object SerializationPluginErrorsRendering : DefaultErrorMessages.Extension {
            SerializationErrors.INNER_CLASSES_NOT_SUPPORTED,
            "Inner (with reference to outer this) serializable classes are not supported. Remove @Serializable annotation or 'inner' keyword."
        )
+        MAP.put(
+            SerializationErrors.COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED,
+            "Class ''{0}'' has implicit custom serializer as its companion object. This behaviour is not properly reflected by @Serializable annotation without arguments and therefore is deprecated. " +
+            "To be able to use companion object as the ''{0}'' default serializer, please explicitly mention it in the annotation on ''{0}'': @Serializable(''{0}''.Companion::class). " +
+            "For more details, refer to this YouTrack ticket: https://youtrack.jetbrains.com/issue/KT-54441",
+            Renderers.NAME
+        )
+        MAP.put(
+            SerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS,
+            "This class is a Companion object for @Serializable class ''{0}'', but itself is an external serializer for another class ''{1}''. " +
+                    "Such declarations are potentially problematic and user-confusing and therefore are deprecated. " +
+                    "Please define external serializers as non-companion, preferably top-level objects. " +
+                    "For more details, refer to this YouTrack ticket: https://youtrack.jetbrains.com/issue/KT-54441",
+            Renderers.RENDER_TYPE,
+            Renderers.RENDER_TYPE
+        )
+        MAP.put(
+            SerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS,
+            "This class is a Companion object for non-serializable class ''{0}'', but itself is an external serializer for another class ''{1}''. " +
+                    "Such declarations are potentially problematic and user-confusing and therefore are deprecated. " +
+                    "Please define external serializers as non-companion, preferably top-level objects. " +
+                    "For more details, refer to this YouTrack ticket: https://youtrack.jetbrains.com/issue/KT-54441",
+            Renderers.RENDER_TYPE,
+            Renderers.RENDER_TYPE
+        )
        MAP.put(
            SerializationErrors.EXPLICIT_SERIALIZABLE_IS_REQUIRED,
            "Explicit @Serializable annotation on enum class is required when @SerialName or @SerialInfo annotations are used on its members."
@@ -8,6 +8,7 @@ package org.jetbrains.kotlinx.serialization.compiler.fir.checkers
 import com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.diagnostics.*
 import org.jetbrains.kotlin.fir.symbols.impl.FirClassSymbol
+import org.jetbrains.kotlin.fir.symbols.impl.FirRegularClassSymbol
 import org.jetbrains.kotlin.fir.types.ConeKotlinType
 import org.jetbrains.kotlin.psi.KtAnnotationEntry

@@ -20,6 +21,10 @@ object FirSerializationErrors {

    val EXPLICIT_SERIALIZABLE_IS_REQUIRED by warning0<PsiElement>()

+    val COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED by error1<PsiElement, FirRegularClassSymbol>()
+    val COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS by error2<PsiElement, ConeKotlinType, ConeKotlinType>()
+    val COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS by warning2<PsiElement, ConeKotlinType, ConeKotlinType>()
+
    val SERIALIZABLE_ANNOTATION_IGNORED by error0<KtAnnotationEntry>()
    val NON_SERIALIZABLE_PARENT_MUST_HAVE_NOARG_CTOR by error0<KtAnnotationEntry>()
    val PRIMARY_CONSTRUCTOR_PARAMETER_IS_NOT_A_PROPERTY by error0<KtAnnotationEntry>()
@@ -203,9 +203,11 @@ object FirSerializationPluginClassChecker : FirClassChecker() {
            )
            return false
        }
-        
+
+        checkCompanionSerializerDependency(classSymbol, reporter)
+
        if (!with(session) { classSymbol.hasSerializableOrMetaAnnotation }) return false
-        
+
        if (classSymbol.isAnonymousObjectOrInsideIt) {
            reporter.reportOn(classSymbol.serializableOrMetaAnnotationSource, FirSerializationErrors.ANONYMOUS_OBJECTS_NOT_SUPPORTED)
            return false
@@ -241,9 +243,13 @@ object FirSerializationPluginClassChecker : FirClassChecker() {
        if (!classSymbol.isEnumClass) { // enums are inherited from java.lang.Enum and can't be inherited from other classes
            val superClassSymbol = classSymbol.getSuperClassOrAny(session)
            if (with(session) { !superClassSymbol.isInternalSerializable }) {
-                val noArgConstructorSymbol = superClassSymbol.declarationSymbols.firstOrNull { it is FirConstructorSymbol && it.valueParameterSymbols.isEmpty() }
+                val noArgConstructorSymbol =
+                    superClassSymbol.declarationSymbols.firstOrNull { it is FirConstructorSymbol && it.valueParameterSymbols.isEmpty() }
                if (noArgConstructorSymbol == null) {
-                    reporter.reportOn(classSymbol.serializableOrMetaAnnotationSource, FirSerializationErrors.NON_SERIALIZABLE_PARENT_MUST_HAVE_NOARG_CTOR)
+                    reporter.reportOn(
+                        classSymbol.serializableOrMetaAnnotationSource,
+                        FirSerializationErrors.NON_SERIALIZABLE_PARENT_MUST_HAVE_NOARG_CTOR
+                    )
                    return false
                }
            }
@@ -252,6 +258,54 @@ object FirSerializationPluginClassChecker : FirClassChecker() {
        return true
    }

+    context(CheckerContext)
+    private fun checkCompanionSerializerDependency(classSymbol: FirClassSymbol<*>, reporter: DiagnosticReporter) = with(session) {
+        if (classSymbol !is FirRegularClassSymbol) return
+        val companionObjectSymbol = classSymbol.companionObjectSymbol ?: return
+        val serializerForInCompanion = companionObjectSymbol.serializerForClass?.toRegularClassSymbol(session) ?: return
+        val serializableWith: ConeKotlinType? = classSymbol.serializableWith
+        if (classSymbol.hasSerializableOrMetaAnnotationWithoutArgs) {
+            if (serializerForInCompanion.classId == classSymbol.classId) {
+                // @Serializable class Foo / @Serializer(Foo::class) companion object — prohibited due to problems with recursive resolve
+                reporter.reportOn(
+                    classSymbol.serializableOrMetaAnnotationSource,
+                    FirSerializationErrors.COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED,
+                    classSymbol
+                )
+            } else {
+                // @Serializable class Foo / @Serializer(Bar::class) companion object — prohibited as vague and confusing
+                val source = companionObjectSymbol.serializerAnnotation?.source
+                reporter.reportOn(
+                    source,
+                    FirSerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS,
+                    classSymbol.defaultType(),
+                    serializerForInCompanion.defaultType()
+                )
+            }
+        } else if (serializableWith != null) {
+            if (serializableWith.classId == companionObjectSymbol.classId && serializerForInCompanion.classId == classSymbol.classId) {
+                // @Serializable(Foo.Companion) class Foo / @Serializer(Foo::class) companion object — the only case that is allowed
+            } else {
+                // @Serializable(anySer) class Foo / @Serializer(anyOtherClass) companion object — prohibited as vague and confusing
+                reporter.reportOn(
+                    companionObjectSymbol.serializerAnnotation?.source,
+                    FirSerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS,
+                    classSymbol.defaultType(),
+                    serializerForInCompanion.defaultType()
+                )
+            }
+        } else {
+            // (regular) class Foo / @Serializer(something) companion object - not recommended
+            reporter.reportOn(
+                companionObjectSymbol.serializerAnnotation?.source,
+                FirSerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS,
+                classSymbol.defaultType(),
+                serializerForInCompanion.defaultType()
+            )
+        }
+    }
+
+
    context(CheckerContext)
    @Suppress("IncorrectFormatting") // KTIJ-22227
    private fun checkClassWithCustomSerializer(classSymbol: FirClassSymbol<*>, reporter: DiagnosticReporter) {
@@ -303,7 +357,10 @@ object FirSerializationPluginClassChecker : FirClassChecker() {

        val properties = session.serializablePropertiesProvider.getSerializablePropertiesForClass(classSymbol)
        if (!properties.isExternallySerializable) {
-            reporter.reportOn(classSymbol.serializableOrMetaAnnotationSource, FirSerializationErrors.PRIMARY_CONSTRUCTOR_PARAMETER_IS_NOT_A_PROPERTY)
+            reporter.reportOn(
+                classSymbol.serializableOrMetaAnnotationSource,
+                FirSerializationErrors.PRIMARY_CONSTRUCTOR_PARAMETER_IS_NOT_A_PROPERTY
+            )
        }

        // check that all names are unique
@@ -334,7 +391,9 @@ object FirSerializationPluginClassChecker : FirClassChecker() {

    private fun declarationHasInitializer(propertySymbol: FirPropertySymbol): Boolean {
        return when {
-            propertySymbol.fromPrimaryConstructor -> propertySymbol.correspondingValueParameterFromPrimaryConstructor?.hasDefaultValue ?: false
+            propertySymbol.fromPrimaryConstructor -> propertySymbol.correspondingValueParameterFromPrimaryConstructor?.hasDefaultValue
+                ?: false
+
            else -> propertySymbol.hasInitializer || propertySymbol.hasDelegate
        }
    }
@@ -470,7 +529,11 @@ object FirSerializationPluginClassChecker : FirClassChecker() {
    ) {
        val serializerClassId = serializerType.classId ?: return
        if (serializerClassId.isLocal) {
-            reporter.reportOn(source ?: classSymbol.serializableOrMetaAnnotationSource, FirSerializationErrors.LOCAL_SERIALIZER_USAGE, serializerType)
+            reporter.reportOn(
+                source ?: classSymbol.serializableOrMetaAnnotationSource,
+                FirSerializationErrors.LOCAL_SERIALIZER_USAGE,
+                serializerType
+            )
        }
    }

@@ -37,6 +37,31 @@ object KtDefaultErrorMessagesSerialization {
            FirSerializationErrors.INNER_CLASSES_NOT_SUPPORTED,
            "Inner (with reference to outer this) serializable classes are not supported. Remove @Serializable annotation or 'inner' keyword."
        )
+        put(
+            FirSerializationErrors.COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED,
+            "Class {0} has implicit custom serializer as its companion object. This behaviour is not properly reflected by @Serializable annotation without arguments and therefore is deprecated. " +
+                    "To be able to use companion object as the {0} default serializer, please explicitly mention it in the annotation on {0}: @Serializable({0}.Companion::class). " +
+                    "For more details, refer to this YouTrack ticket: https://youtrack.jetbrains.com/issue/KT-54441",
+            FirDiagnosticRenderers.DECLARATION_NAME
+        )
+        put(
+            FirSerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS,
+            "This class is a Companion object for @Serializable class {0}, but itself is an external serializer for another class {1}. " +
+                    "Such declarations are potentially problematic and user-confusing and therefore are deprecated. " +
+                    "Please define external serializers as non-companion, preferably top-level objects. " +
+                    "For more details, refer to this YouTrack ticket: https://youtrack.jetbrains.com/issue/KT-54441",
+            FirDiagnosticRenderers.RENDER_TYPE,
+            FirDiagnosticRenderers.RENDER_TYPE
+        )
+        put(
+            FirSerializationErrors.COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS,
+            "This class is a Companion object for non-serializable class {0}, but itself is an external serializer for another class {1}. " +
+                    "Such declarations are potentially problematic and user-confusing and therefore are deprecated. " +
+                    "Please define external serializers as non-companion, preferably top-level objects. " +
+                    "For more details, refer to this YouTrack ticket: https://youtrack.jetbrains.com/issue/KT-54441",
+            FirDiagnosticRenderers.RENDER_TYPE,
+            FirDiagnosticRenderers.RENDER_TYPE
+        )
        put(
            FirSerializationErrors.EXPLICIT_SERIALIZABLE_IS_REQUIRED,
            "Explicit @Serializable annotation on enum class is required when @SerialName or @SerialInfo annotations are used on its members."
@@ -3,7 +3,7 @@

 import kotlinx.serialization.*

-@Serializable
+<!COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED!>@Serializable<!>
 class Digest() {
    @Serializer(forClass = Digest::class)
    companion object : KSerializer<Digest> {}
@@ -0,0 +1,39 @@
+// FIR_IDENTICAL
+// SKIP_TXT
+
+// FILE: test.kt
+import kotlinx.serialization.*
+
+class Bar
+
+<!COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED!>@Serializable<!>
+class Foo1 {
+    @Serializer(Foo1::class)
+    companion object
+}
+
+@Serializable
+class Foo2 {
+    <!COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS!>@Serializer(Bar::class)<!>
+    companion object
+}
+
+@Serializer(Foo3::class)
+object Foo3Ser
+
+@Serializable(Foo3Ser::class)
+class Foo3 {
+    <!COMPANION_OBJECT_SERIALIZER_INSIDE_OTHER_SERIALIZABLE_CLASS!>@Serializer(Bar::class)<!>
+    companion object
+}
+
+@Serializable(Foo4.Companion::class)
+class Foo4 {
+    @Serializer(Foo4::class)
+    companion object
+}
+
+class NonSerializableFoo {
+    <!COMPANION_OBJECT_SERIALIZER_INSIDE_NON_SERIALIZABLE_CLASS!>@Serializer(Bar::class)<!>
+    companion object
+}
@@ -1,4 +1,5 @@
 FILE: serializerViaCompanion.kt
+    @FILE:R|kotlin/Suppress|(names = vararg(String(COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED)))
    package com.example

    @R|kotlinx/serialization/Serializable|() public final data class WithCompanion : R|kotlin/Any| {
@@ -1,5 +1,7 @@
 // WITH_STDLIB
+// !DIAGNOSTICS: -COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED

+@file:Suppress("COMPANION_OBJECT_AS_CUSTOM_SERIALIZER_DEPRECATED")
 package com.example

 import kotlinx.serialization.*
@@ -26,6 +26,12 @@ public class SerializationFirDiagnosticTestGenerated extends AbstractSerializati
            KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("plugins/kotlinx-serialization/testData/diagnostics"), Pattern.compile("^(.+)\\.kt$"), Pattern.compile("^(.+)\\.fir\\.kts?$"), true);
        }

+        @Test
+        @TestMetadata("companionObjectSerializers.kt")
+        public void testCompanionObjectSerializers() throws Exception {
+            runTest("plugins/kotlinx-serialization/testData/diagnostics/companionObjectSerializers.kt");
+        }
+
        @Test
        @TestMetadata("DuplicateSerialName.kt")
        public void testDuplicateSerialName() throws Exception {
@@ -24,6 +24,12 @@ public class SerializationPluginDiagnosticTestGenerated extends AbstractSerializ
        KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("plugins/kotlinx-serialization/testData/diagnostics"), Pattern.compile("^(.+)\\.kt$"), Pattern.compile("^(.+)\\.fir\\.kts?$"), true);
    }

+    @Test
+    @TestMetadata("companionObjectSerializers.kt")
+    public void testCompanionObjectSerializers() throws Exception {
+        runTest("plugins/kotlinx-serialization/testData/diagnostics/companionObjectSerializers.kt");
+    }
+
    @Test
    @TestMetadata("DuplicateSerialName.kt")
    public void testDuplicateSerialName() throws Exception {