-
Notifications
You must be signed in to change notification settings - Fork 666
Add polymorphic default serializers (as opposed to deserializers) #1686
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
Add polymorphic default serializers (as opposed to deserializers) #1686
Conversation
sandwwraith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! Overall PR is good, however, I have a concern regarding deprecation: since this PR is likely to land in the patch release (1.3.x), it's incorrect to deprecate default function. Therefore, I have a suggestion: let's mark new functions with ExperimentalSerializationApi and don't deprecate the current function. In 1.4 we can deprecate it and lift experimentality
| * Default serializers provider affects only serialization process. | ||
| */ | ||
| @Suppress("UNCHECKED_CAST") | ||
| public fun <T : Base> defaultSerializer(defaultSerializerProvider: (value: T) -> SerializationStrategy<T>?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need type parameter? I think defaultSerializerProvider: (value: Base) -> SerializationStrategy<Base>? is more appropriate here.
Imagine the situation: we have Base; A: Base(); B:Base(). If we register defaultSerializer<A> { return A.serializer() } which is allowed by this signature, when we try to serialize B, we'd get ClassCastException, because our lambda can accept only A type. Therefore, default serializer should be able to select between all subclasses of base, i.e. accept value: Base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to use @UnsafeVariance for that, but sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the problem is more complicated than it looks at the first sight. @UnsafeVariance is needed here because PolymorphicModuleBuilder has IN variance: <in Base : Any>. Why does it need it? The answer lies in this sample: https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md#registering-multiple-superclasses
If you have a vast hierarchy (in the sample, it is Any and Project, but instead of Any, it can be different multiple super-interfaces), it is logical to register subclasses of the lowest common interface for polymorphic serialization in all bases (registerProjectSubclasses method in the sample). Naturally, it should accept PolymorphicModuleBuilder<Project>, because any of the Project subclasses can be serialized and deserialized in bigger scopes (say Any or other super-interface). It is also possible to return some deserializer as default — if it always returns a subclass of Project, it is possible to assign it into any variable up to Any.
However, it is not the case for the default serializer: since it accepts an instance, if we register it using some lambda that accepts a Project, we can't accept arbitrary Any. SerializationStrategy would also break: we know how to serialize Project, but not our super-interface. Both problems will lead to ClassCastException. If we add this function with @UnsafeVariance, the following code is error-prone:
val module = SerializersModule {
fun PolymorphicModuleBuilder<Project>.registerProjectSubclasses() {
subclass(OwnedProject::class)
defaultSerializer { it: Project -> SomeDefaultProjectSerializer } // will throw ClassCastException if we serialize String as Any
}
polymorphic(Any::class) { registerProjectSubclasses() }
polymorphic(Project::class) { registerProjectSubclasses() }
}There are multiple ways to solve this problem:
- Leave
@UnsafeVarianceand document that this function may cause problems, probably annotate it with special opt-in annotation. Not a clean solution, since most people don't read the documentation. It probably would be more helpful if we can suppress CCE and throw SerializationException instead about smth like 'serializer not found, default serializer is not applicable', but I'm not sure if this can be done accurately — need further investigation. In any case, stacktrace of the exception won't pinpoint the actual line with the problem. - Remove
in Base: Anyin polymorphic module builder. It would solve the problem, because Kotlin compiler is smart enough. We still can declare the helper function asfun PolymorphicModuleBuilder<in Project>.registerProjectSubclasses()(use-site variance), but compiler would infer that actualBaseindefaultSerializerisAnyand thus would require to accept Any and returnSerializationStrategy<Any>. This is a good solution, but unfortunately removing variance is a source-incompatible breaking change we can't afford to do. (In the sample,polymorphic(Any::class) { registerProjectSubclasses() }would not compile with 'Unresolved reference' ) - Make
defaultSerializerwith fixed types, e.g.defaultSerializer(defaultSerializerProvider: (value: Any) -> SerializationStrategy<Any?>?). Possible and type-safe, but very inconvenient to use. - Do not provide
defaultSerializerat all. Note thatpolymorphicDefaultSerializeron the regularSerializersModuleBuilderis still a thing as it doesn't have such problems. By doing this, we're causing minor inconvenience — people are forced to writepolymorphicDefaultSerializeroutside ofpolymorphic {}scope, but we're saving them from accidental exceptions that are hard to grasp.
I think that option 4 is the way to go, despite all inconveniences. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry I read your comment and then got sidetracked and forgot about it. I agree, 4 is the best solution
core/commonMain/src/kotlinx/serialization/modules/SerializersModuleCollector.kt
Show resolved
Hide resolved
core/commonMain/src/kotlinx/serialization/modules/SerializersModuleCollector.kt
Show resolved
Hide resolved
core/commonMain/src/kotlinx/serialization/modules/SerializersModuleCollector.kt
Outdated
Show resolved
Hide resolved
sandwwraith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address my last comment (#1686 (comment))
|
Done 👍 |
sandwwraith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I think it's ready when you fix minor comments
core/commonMain/src/kotlinx/serialization/modules/PolymorphicModuleBuilder.kt
Outdated
Show resolved
Hide resolved
core/commonMain/src/kotlinx/serialization/modules/SerializersModuleBuilders.kt
Outdated
Show resolved
Hide resolved
core/commonMain/src/kotlinx/serialization/modules/PolymorphicModuleBuilder.kt
Show resolved
Hide resolved
|
Done |
sandwwraith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
…icModuleBuilder.default Replaced with default polymorphicDefaultDeserializer and defaultDeserializer respectively. Remove experimentality from SerializersModuleCollector.polymorphicDefaultSerializer. This is a follow-up for #1686 — finishing migration path
…icModuleBuilder.default Replaced with default polymorphicDefaultDeserializer and defaultDeserializer respectively. Remove experimentality from SerializersModuleCollector.polymorphicDefaultSerializer. This is a follow-up for #1686 — finishing migration path
…icModuleBuilder.default (Kotlin#2076) Replaced with default polymorphicDefaultDeserializer and defaultDeserializer respectively. Remove experimentality from SerializersModuleCollector.polymorphicDefaultSerializer. This is a follow-up for Kotlin#1686 — finishing migration path
Closes #1317. Possible usecases for this are described in that issue.
I am by no means a kotlin expert so please let me know if there's something that needs changing.