Fix serializing nulls for a property of a parameterized type with a nullable upper bound with Protobuf (#2561)

This is done by setting `nullableMode` depending on `serializer.descriptor.isNullable` in `encodeSerializableElement`.

Because of https://youtrack.jetbrains.com/issue/KT-66206 nullable types can still be encountered in `encodeSerializableElement` despite having explicit `encodeNullableSerializableElement`

5 files changed, 146 insertions, 22 deletions

diff --git a/formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufTaggedEncoder.kt b/formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufTaggedEncoder.kt
index 84e58399..d061e40a 100644
--- a/formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufTaggedEncoder.kt
+++ b/formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufTaggedEncoder.kt
@@ -123,13 +123,27 @@ internal abstract class ProtobufTaggedEncoder : ProtobufTaggedBase(), Encoder, C
     public final override fun encodeStringElement(descriptor: SerialDescriptor, index: Int, value: String): Unit =
         encodeTaggedString(descriptor.getTag(index), value)
 
+    private fun SerialKind.isMapOrList() =
+        this == StructureKind.MAP || this == StructureKind.LIST
+
     public final override fun <T : Any?> encodeSerializableElement(
         descriptor: SerialDescriptor,
         index: Int,
         serializer: SerializationStrategy<T>,
         value: T
     ) {
-        nullableMode = NullableMode.NOT_NULL
+        nullableMode =
+            if (descriptor.isElementOptional(index))
+                NullableMode.OPTIONAL
+            else {
+                val elementDescriptor = descriptor.getElementDescriptor(index)
+                if (elementDescriptor.kind.isMapOrList())
+                    NullableMode.COLLECTION
+                else if (!descriptor.kind.isMapOrList() && elementDescriptor.isNullable) // or: `serializer.descriptor`
+                    NullableMode.ACCEPTABLE
+                else
+                    NullableMode.NOT_NULL
+            }
 
         pushTag(descriptor.getTag(index))
         encodeSerializableValue(serializer, value)
@@ -141,14 +155,12 @@ internal abstract class ProtobufTaggedEncoder : ProtobufTaggedBase(), Encoder, C
         serializer: SerializationStrategy<T>,
         value: T?
     ) {
-        val elementKind = descriptor.getElementDescriptor(index).kind
-        nullableMode = if (descriptor.isElementOptional(index)) {
+        nullableMode = if (descriptor.isElementOptional(index))
             NullableMode.OPTIONAL
-        } else if (elementKind == StructureKind.MAP || elementKind == StructureKind.LIST) {
+        else if (descriptor.getElementDescriptor(index).kind.isMapOrList())
             NullableMode.COLLECTION
-        } else {
+        else
             NullableMode.ACCEPTABLE
-        }
 
         pushTag(descriptor.getTag(index))
         encodeNullableSerializableValue(serializer, value)
diff --git a/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufNothingTest.kt b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufNothingTest.kt
index e90ff2be..bdc93c23 100644
--- a/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufNothingTest.kt
+++ b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufNothingTest.kt
@@ -5,7 +5,6 @@
 package kotlinx.serialization.protobuf
 
 import kotlinx.serialization.*
-import kotlinx.serialization.test.*
 import kotlin.test.*
 
 class ProtobufNothingTest {
@@ -13,17 +12,16 @@ class ProtobufNothingTest {
     /*private*/ data class NullableNothingBox(val value: Nothing?) // `private` doesn't work on the JS legacy target
 
     @Serializable
-    private data class ParameterizedBox<T : Any>(val value: T?)
+    private data class NullablePropertyNotNullUpperBoundParameterizedBox<T : Any>(val value: T?)
+
+    @Serializable
+    private data class NullableUpperBoundParameterizedBox<T : Any?>(val value: T)
 
-    private inline fun <reified T> testConversion(data: T, expectedHexString: String) {
-        val string = ProtoBuf.encodeToHexString(data).uppercase()
-        assertEquals(expectedHexString, string)
-        assertEquals(data, ProtoBuf.decodeFromHexString(string))
-    }
 
     @Test
     fun testNothing() {
         testConversion(NullableNothingBox(null), "")
-        testConversion(ParameterizedBox(null), "")
+        testConversion(NullablePropertyNotNullUpperBoundParameterizedBox(null), "")
+        testConversion(NullableUpperBoundParameterizedBox(null), "")
     }
 }
diff --git a/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufPrimitivesTest.kt b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufPrimitivesTest.kt
index 56c7bfab..f8716446 100644
--- a/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufPrimitivesTest.kt
+++ b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufPrimitivesTest.kt
@@ -3,18 +3,10 @@
  */
 package kotlinx.serialization.protobuf
 
-import kotlinx.serialization.*
 import kotlinx.serialization.builtins.*
 import kotlin.test.*
 
 class ProtobufPrimitivesTest {
-
-    private fun <T> testConversion(data: T, serializer: KSerializer<T>, expectedHexString: String) {
-        val string = ProtoBuf.encodeToHexString(serializer, data).uppercase()
-        assertEquals(expectedHexString, string)
-        assertEquals(data, ProtoBuf.decodeFromHexString(serializer, string))
-    }
-
     @Test
     fun testPrimitiveTypes() {
         testConversion(true, Boolean.serializer(), "01")
diff --git a/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufTypeParameterTest.kt b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufTypeParameterTest.kt
new file mode 100644
index 00000000..0b0f19f4
--- /dev/null
+++ b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/ProtobufTypeParameterTest.kt
@@ -0,0 +1,102 @@
+/*
+ * Copyright 2017-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package kotlinx.serialization.protobuf
+
+import kotlinx.serialization.*
+import kotlin.test.*
+
+class ProtobufTypeParameterTest {
+    @Serializable
+    data class Box<T>(val value: T)
+
+    @Serializable
+    data class ExplicitNullableUpperBoundBox<T : Any?>(val value: T)
+
+    @Serializable
+    data class ExplicitNullableUpperNullablePropertyBoundBox<T : Any?>(val value: T?)
+
+    inline fun <reified T> testBox(value: T, expectedHexString: String) {
+        testConversion(Box(value), expectedHexString)
+        testConversion(ExplicitNullableUpperBoundBox(value), expectedHexString)
+        testConversion(ExplicitNullableUpperNullablePropertyBoundBox(value), expectedHexString)
+    }
+
+    @Serializable
+    private data class DefaultArgPair<T>(val first: T, val second: T = first)
+
+    companion object {
+        val testList0 = emptyList<Int>()
+        val testList1 = listOf(0)
+        val testMap0 = emptyMap<Int, Int>()
+        val testMap1 = mapOf(0 to 0)
+    }
+
+
+    @Test
+    fun testNothingBoxesWithNull() {
+        // Cannot use 'Nothing?' as reified type parameter
+        //testBox(null, "")
+        testConversion(Box(null), "")
+        testConversion(ExplicitNullableUpperBoundBox(null), "")
+        @Suppress("RemoveExplicitTypeArguments")
+        testConversion(ExplicitNullableUpperNullablePropertyBoundBox<Nothing>(null), "")
+        testConversion(ExplicitNullableUpperNullablePropertyBoundBox<Nothing?>(null), "")
+    }
+
+    @Test
+    fun testIntBoxes() {
+        testBox(0, "0800")
+        testBox(1, "0801")
+    }
+
+    @Test
+    fun testNullableIntBoxes() {
+        testBox<Int?>(null, "")
+        testBox<Int?>(0, "0800")
+    }
+
+    @Test
+    fun testCollectionBoxes() {
+        testBox(testList0, "")
+        testBox(testList1, "0800")
+        testBox(testMap0, "")
+        testBox(testMap1, "0A0408001000")
+    }
+
+    @Test
+    fun testNullableCollectionBoxes() {
+        fun assertFailsForNullForCollectionTypes(block: () -> Unit) {
+            try {
+                block()
+                fail()
+            } catch (e: SerializationException) {
+                assertEquals(
+                    "'null' is not supported for collection types in ProtoBuf", e.message
+                )
+            }
+        }
+        assertFailsForNullForCollectionTypes {
+            testBox<List<Int>?>(null, "")
+        }
+        assertFailsForNullForCollectionTypes {
+            testBox<Map<Int, Int>?>(null, "")
+        }
+        testBox<List<Int>?>(testList0, "")
+        testBox<Map<Int, Int>?>(testMap0, "")
+    }
+
+    @Test
+    fun testWithDefaultArguments() {
+        testConversion(DefaultArgPair(null), "")
+        testConversion(DefaultArgPair(1), "0801")
+        testConversion(DefaultArgPair(null, null), "")
+        testConversion(DefaultArgPair(null, 1), "1001")
+        assertFailsWith<SerializationException> {
+            testConversion(DefaultArgPair(1, null), "0801")
+        }
+        testConversion(DefaultArgPair(1, 1), "0801")
+        testConversion(DefaultArgPair(1, 2), "08011002")
+    }
+}
\ No newline at end of file
diff --git a/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/TestFunctions.kt b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/TestFunctions.kt
new file mode 100644
index 00000000..7302c995
--- /dev/null
+++ b/formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/TestFunctions.kt
@@ -0,0 +1,20 @@
+/*
+ * Copyright 2017-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package kotlinx.serialization.protobuf
+
+import kotlinx.serialization.*
+import kotlin.test.*
+
+fun <T> testConversion(data: T, serializer: KSerializer<T>, expectedHexString: String) {
+    val string = ProtoBuf.encodeToHexString(serializer, data).uppercase()
+    assertEquals(expectedHexString, string)
+    assertEquals(data, ProtoBuf.decodeFromHexString(serializer, string))
+}
+
+inline fun <reified T> testConversion(data: T, expectedHexString: String) {
+    val string = ProtoBuf.encodeToHexString(data).uppercase()
+    assertEquals(expectedHexString, string)
+    assertEquals(data, ProtoBuf.decodeFromHexString(string))
+}