Skip to content

Unclear error about necessary serialization in case of scoping with interfaces #2174

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

Closed
zarechenskiy opened this issue Jan 27, 2023 · 8 comments
Assignees
Labels

Comments

@zarechenskiy
Copy link
Contributor

zarechenskiy commented Jan 27, 2023

Describe the bug

The problem is that the error happens at runtime and the error message isn't clear

To Reproduce

interface Parent

@Serializable
data class Holder(val value: Int) : Parent

fun main() {
  val data = Holder(42) as Parent
  val string = Json.encodeToString(data)
  println(string)
}

Here the plugin will throw an error:

Exception in thread "main" kotlinx.serialization.SerializationException: Class 'Holder' is not registered for polymorphic serialization in the scope of 'Parent'.
Mark the base class as 'sealed' or register the serializer explicitly.
	at kotlinx.serialization.internal.AbstractPolymorphicSerializerKt.throwSubtypeNotRegistered(AbstractPolymorphicSerializer.kt:102)
	at kotlinx.serialization.internal.AbstractPolymorphicSerializerKt.throwSubtypeNotRegistered(AbstractPolymorphicSerializer.kt:113)

Next, making interface Parent as a sealed one doesn't help, the error is the same, marking interface as Serializable also doesn't work but it helps if you mark the interface as sealed and then put Serializable annotation on it:

@Serializable
sealed interface Parent // OK

Note that if you remove cast as Parent, then everything also works fine:

interface Parent // no serialization annotation

@Serializable
data class Holder(val value: Int) : Parent

fun main() {
  val data = Holder(42) // no cast
  val string = Json.encodeToString(data)
  println(string)
}

Expected behavior

Ideally, it'd be nice to have an error at compile time and in IDE. If it's not possible, then it'd be better to specify an error for interfaces as right now it's slightly misleading. Note that this is minimised example, my real case was more complicated and it was unclear for me what is the problem

Environment

  • Kotlin version: [1.8.0]
  • Library version: [plugin.serialization 1.8.0, kotlinx-serialization-json 1.5.0-RC]
  • Kotlin platforms: [JVM]
@sandwwraith
Copy link
Member

sandwwraith commented Jan 27, 2023

The dependency on static type (as Parent) is a deliberate design choice (see https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md#static-types). It also determines whether you get the type information included in the output.
Unfortunately, it is impossible to make an IDE diagnostic, as with non-sealed classes, the possibility of finding a serializer depends on what the user has written in the SerializersModule {} in the runtime.

Regarding error message — indeed, it is unchanged from the ages when we had only sealed classes, so it can definitely be improved

@martinbonnin
Copy link
Contributor

Came here for the same thing, thanks for improving the error message!

Could there also be more build-time checking on sealed hierarchies? The doc currently says:

All subclasses of a sealed class must be explicitly marked as @Serializable.

If I do something like this:

@Serializable
sealed interface Animal

class Cat(val meow: String): Animal

class Dog(val barf: String): Animal

I only get the error at runtime. Could this be caught at compile time instead?

@sandwwraith
Copy link
Member

I think that doc needs updating. In fact, it is fine if not all sublcasses are serializable — e.g. you may have some subclass that contains private information, and you don't want to serialize it. It is possible with abstract classes (by simply not registering it), but if will be impossible for sealed classes if we force this rule.

@martinbonnin
Copy link
Contributor

martinbonnin commented Feb 20, 2023

Trying to wrap my head around this. I think the doc is OK then? It says "All subclasses of a sealed class must be explicitly marked as @.Serializable" so that requirement is valid for sealed classes right?

@sandwwraith
Copy link
Member

But the framework works fine even if this requirement is not met. It simply ignores non-serializable classes and throws error on them. The exact same thing happens if the base class is abstract and subclass is not registered in the module.

@martinbonnin
Copy link
Contributor

I see. Thanks for clearing that up 🙏
It "feels" a bit weird as I often associate "sealed" interfaces with the possibility to exhaustively check for all possible implementation and I would naively expect this to apply to serialization as well.
Was it considered to enforce this at the compiler level? If you want some subclasses to not be serializable, don't use a sealed interface in the first place?

@sandwwraith
Copy link
Member

sandwwraith commented Feb 20, 2023

Yes, we had considered it while implementing support for sealed classes but decided not to force it — the only way to opt-out of this would be, as you say, not to use sealed class/inteface. However, there's possibility that it has to be sealed for different, serialization-unrelated reasons.

@martinbonnin
Copy link
Contributor

Gotcha. Thanks for the insights!

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

No branches or pull requests

4 participants