-
Notifications
You must be signed in to change notification settings - Fork 620
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
Add polymorphic default serializers (as opposed to deserializers) #1686
Conversation
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
@UnsafeVariance
and 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: Any
in 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 actualBase
indefaultSerializer
isAny
and 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
defaultSerializer
with fixed types, e.g.defaultSerializer(defaultSerializerProvider: (value: Any) -> SerializationStrategy<Any?>?)
. Possible and type-safe, but very inconvenient to use. - Do not provide
defaultSerializer
at all. Note thatpolymorphicDefaultSerializer
on the regularSerializersModuleBuilder
is still a thing as it doesn't have such problems. By doing this, we're causing minor inconvenience — people are forced to writepolymorphicDefaultSerializer
outside 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
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 👍 |
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 |
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.