Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dkhalanskyjb opened this issue Mar 7, 2025 · 9 comments
Open

Comments

@dkhalanskyjb
Copy link

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"
@sandwwraith
Copy link
Member

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

@dkhalanskyjb
Copy link
Author

dkhalanskyjb commented Mar 7, 2025

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.

@sandwwraith
Copy link
Member

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

@dkhalanskyjb
Copy link
Author

dkhalanskyjb commented Mar 7, 2025

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.

@pdvrieze
Copy link
Contributor

pdvrieze commented Mar 7, 2025

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).

@dkhalanskyjb
Copy link
Author

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.

@pdvrieze
Copy link
Contributor

pdvrieze commented Mar 7, 2025

@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.

@sandwwraith
Copy link
Member

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.

@dkhalanskyjb
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants