Correctly handle serial name conflict for different classes (#2856)

in SerializersModule.overwriteWith.

Previous KClass should be removed from the map as well to avoid creating a module
with asymmetric mappings.

Fixes #2820

Author: sandwwraith

core/commonMain/src/kotlinx/serialization/modules/SerializersModuleBuilders.kt

@@ -192,39 +192,30 @@ public class SerializersModuleBuilder @PublishedApi internal constructor() : Ser
         concreteSerializer: KSerializer<Sub>,
         allowOverwrite: Boolean = false
     ) {
-        // Check for overwrite
         val name = concreteSerializer.descriptor.serialName
         val baseClassSerializers = polyBase2Serializers.getOrPut(baseClass, ::hashMapOf)
-        val previousSerializer = baseClassSerializers[concreteClass]
         val names = polyBase2NamedSerializers.getOrPut(baseClass, ::hashMapOf)
-        if (allowOverwrite) {
-            // Remove previous serializers from name mapping
-            if (previousSerializer != null) {
-                names.remove(previousSerializer.descriptor.serialName)
-            }
-            // Update mappings
-            baseClassSerializers[concreteClass] = concreteSerializer
-            names[name] = concreteSerializer
-            return
-        }
-        // Overwrite prohibited
-        if (previousSerializer != null) {
-            if (previousSerializer != concreteSerializer) {
-                throw SerializerAlreadyRegisteredException(baseClass, concreteClass)
-            } else {
-                // Cleanup name mapping
-                names.remove(previousSerializer.descriptor.serialName)
-            }
+
+        // Check KClass conflict
+        val previousSerializer = baseClassSerializers[concreteClass]
+        if (previousSerializer != null && previousSerializer != concreteSerializer) {
+            if (allowOverwrite) names.remove(previousSerializer.descriptor.serialName)
+            else throw SerializerAlreadyRegisteredException(baseClass, concreteClass)
         }
+
+        // Check SerialName conflict
         val previousByName = names[name]
-        if (previousByName != null) {
-            val conflictingClass = polyBase2Serializers[baseClass]!!.asSequence().find { it.value === previousByName }
-            throw IllegalArgumentException(
-                "Multiple polymorphic serializers for base class '$baseClass' " +
-                        "have the same serial name '$name': '$concreteClass' and '$conflictingClass'"
+        if (previousByName != null && previousByName != concreteSerializer) {
+            val previousClass = baseClassSerializers.asSequence().find { it.value === previousByName }?.key
+                ?: error("Name $name is registered in the module but no Kotlin class is associated with it.")
+
+            if (allowOverwrite) baseClassSerializers.remove(previousClass)
+            else throw IllegalArgumentException(
+                "Multiple polymorphic serializers in a scope of '$baseClass' " +
+                        "have the same serial name '$name': $concreteSerializer for '$concreteClass' and $previousByName for '$previousClass'"
             )
         }
-        // Overwrite if no conflicts
+
         baseClassSerializers[concreteClass] = concreteSerializer
         names[name] = concreteSerializer
     }

core/commonTest/src/kotlinx/serialization/modules/ModuleBuildersTest.kt

@@ -10,6 +10,11 @@ import kotlinx.serialization.*
 import kotlinx.serialization.builtins.*
 import kotlinx.serialization.descriptors.*
 import kotlinx.serialization.encoding.*
+import kotlinx.serialization.test.Platform
+import kotlinx.serialization.test.assertFailsWithMessage
+import kotlinx.serialization.test.currentPlatform
+import kotlinx.serialization.test.isJs
+import kotlinx.serialization.test.isJvm
 import kotlin.reflect.*
 import kotlin.test.*
 
@@ -176,6 +181,10 @@ class ModuleBuildersTest {
     @SerialName("C")
     class C
 
+    @Serializable
+    @SerialName("C")
+    class C2
+
     @Serializer(forClass = C::class)
     object CSerializer : KSerializer<C> {
         override val descriptor: SerialDescriptor = buildSerialDescriptor("AnotherName", StructureKind.OBJECT)
@@ -206,6 +215,27 @@ class ModuleBuildersTest {
         assertNull(result.getPolymorphic(Any::class, serializedClassName = "AnotherName"))
     }
 
+    @Test
+    fun testOverwriteWithDifferentClass() {
+        val c1 = SerializersModule {
+            polymorphic<Any>(Any::class) {
+                subclass(C::class)
+            }
+        }
+        val c2 = SerializersModule {
+            polymorphic<Any>(Any::class) {
+                subclass(C2::class)
+            }
+        }
+        val classNameMsg = if (currentPlatform == Platform.JS || currentPlatform == Platform.WASM) "class Any" else "class kotlin.Any"
+        assertFailsWithMessage<IllegalArgumentException>("Multiple polymorphic serializers in a scope of '$classNameMsg' have the same serial name 'C'") { c1 + c2 }
+        val module = c1 overwriteWith c2
+        // C should not be registered at all, C2 should be registered both under "C" and C2::class
+        assertEquals(C2.serializer(), module.getPolymorphic(Any::class, serializedClassName = "C"))
+        assertNull(module.getPolymorphic(Any::class, C()))
+        assertEquals(C2.serializer(), module.getPolymorphic(Any::class, C2()))
+    }
+
     @Test
     fun testOverwriteWithSameSerialName() {
         val m1 = SerializersModule {

core/commonTest/src/kotlinx/serialization/test/TestHelpers.kt

@@ -38,3 +38,7 @@ inline fun jvmOnly(test: () -> Unit) {
     if (isJvm()) test()
 }
 
+inline fun <reified T : Throwable> assertFailsWithMessage(message: String, block: () -> Unit) {
+    val exception = assertFailsWith(T::class, null, block)
+    assertTrue(exception.message!!.contains(message), "Expected message '${exception.message}' to contain substring '$message'")
+}