kotlinx.serialization icon indicating copy to clipboard operation
kotlinx.serialization copied to clipboard

Documentation for `serializer(KType)` doesn't mention that it's unsound

Open dkhalanskyjb opened this issue 9 months ago • 9 comments

import kotlinx.serialization.json.*
import kotlinx.serialization.*
import kotlin.reflect.*

fun main() {
    println(Json.encodeToString(serializer(typeOf<Double>()), "string"))
}

compiles just fine, but throws

Exception in thread "main" java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Number (java.lang.String and java.lang.Number are in module java.base of loader 'bootstrap')
	at kotlinx.serialization.internal.DoubleSerializer.serialize(Primitives.kt:113)
	at kotlinx.serialization.json.internal.StreamingJsonEncoder.encodeSerializableValue(StreamingJsonEncoder.kt:259)
	at kotlinx.serialization.json.internal.JsonStreamsKt.encodeByWriter(JsonStreams.kt:99)
	at kotlinx.serialization.json.Json.encodeToString(Json.kt:125)

The documentation doesn't mention the intended usage pattern (which is probably to do an unchecked cast of the result into a KSerializer of the required type?) and doesn't explain that the returned value is prone to crashes (or surprising results!) otherwise.

Some fun combinations:

  • Json.encodeToString(serializer(typeOf<Int>()), Long.MAX_VALUE) == "-1"
  • Json.encodeToString(serializer(typeOf<Int>()), 3.1415) == "3"

dkhalanskyjb avatar Mar 07 '25 11:03 dkhalanskyjb

The function's return type is KSerializer<Any?>. Is that not enough to deduce that it may produce unchecked casts/class cast exceptions?

sandwwraith avatar Mar 07 '25 11:03 sandwwraith

Not really. The general contract of KSerializer allows serialize() to throw IllegalArgumentException/SerializationException if it doesn't like the input, and purely from the documentation, I would assume that's what should happen if I pass an incompatible type. I can see how it's non-viable, though.

dkhalanskyjb avatar Mar 07 '25 11:03 dkhalanskyjb

Still, it doesn't imply that serializer(KType) documentation is wrong. You've asked for Double serializer, you got the Double serializer.

sandwwraith avatar Mar 07 '25 11:03 sandwwraith

I would agree if the signature were public fun serializer(type: KType): KSerializer<*>: then, the type of the function would give no false promises about what the returned serializer can do. It has a descriptor, it has a serialize() you can't call, it has a deserialize() whose result you can't interpret. All's fair.

But today, the situation is different:

val mySerializer: KSerializer<Any?> = ... // I'm in a sauna, and this part of the screen is foggy
println(Json.encodeToString(mySerializer, "myValue"))

What is the reasonable behavior of this code? According to the documentation of serialize, it may:

  • Throw an IllegalArgumentException.
  • Throw a SerializationException.
  • Return a string representing the provided value.

Is that correct?

Yet if ... is actually serializer(typeOf<Double>()), then other behaviors occur. The guarantee at the type level got broken. This means the returned value is not a KSerializer<Any?>, and the function is unsound.

dkhalanskyjb avatar Mar 07 '25 12:03 dkhalanskyjb

From my perspective it would perhaps be better to have the function return KSerializer<*> rather than KSerializer<Any?>, however I'm not sure it is correct to change it at this point (source compatibility).

pdvrieze avatar Mar 07 '25 13:03 pdvrieze

KSerializer<*> is less convenient in cases like Json.encodeToString(serializer(typeOf<String>()), "string"), because you know that you are allowed to pass "string", but have no easy way of convincing the compiler of that.

dkhalanskyjb avatar Mar 07 '25 13:03 dkhalanskyjb

@dkhalanskyjb This function is for those cases where the type of the serializer is determined dynamically. The whole point is that you don't know the type of the resulting serializer. Returning KSerializer<Any?> is convenient, but a lie.

pdvrieze avatar Mar 07 '25 14:03 pdvrieze

Having KSerializer<*> will be not usable in real life, because it will be coerced into Nothing as the serialize function argument.

I do not see the point of adding ClassCastException to the list of exceptions to encodeToString, tho. It may happen even if you do not use serializer(KType) and e.g. perform an unchecked cast yourself.

sandwwraith avatar Mar 11 '25 11:03 sandwwraith

Having KSerializer<*> will be not usable in real life

It's possible to use KSerializer<*>, one would just have to perform an unchecked cast, and then, ClassCastException or weird behavior would be both completely expected and the user's responsibility.

I do not see the point of adding ClassCastException to the list of exceptions to encodeToString, tho.

Me neither. I think serializer(KType) should document the fact that serializers obtained using it are faulty.

It may happen even if you do not use serializer(KType) and e.g. perform an unchecked cast yourself.

Unchecked casts in user code emit a warning. Here, the issue is a combination of this library's entities that compiles without any errors or warnings, but fails at runtime with an exception that's not declared anywhere at all.

dkhalanskyjb avatar Mar 11 '25 13:03 dkhalanskyjb