Kotlin / kotlinx.serialization

Kotlin multiplatform / multi-format serialization
Apache License 2.0
5.44k stars 624 forks source link

The JPMS boundary should be concerned during access to serializer #2429

Open iseki0 opened 1 year ago

iseki0 commented 1 year ago

Describe the bug When we create a hand-written serializer, it can be linked with some type by @Serializable annotation. The hand-written serializer could be placed to another package which is not exported by module-info.class. The Json.decodeFromString<Type>(data) will be failed with an IllegalAccessException because of the JPMS edge.

Expected behavior Could we add some warning for it?

Environment

iseki0 commented 1 year ago

Before that, I'm trying use JPMS to hide serializers in Java side. (The internal visibility modifier is not working in Java.)

pdvrieze commented 1 year ago

Because serialization is intended to be statically resolved the serializer type is part of the ABI. As such it being (potentially) hidden by JPMS is expected behaviour.

ps. if you want to hide it from Java you can try the @JvmSynthetic annotation as tools will ignore synthetic types/members. pps. there is nothing that prevents serialization to be used from Java or makes it awkward (it isn't like inline classes or coroutines), so exposing the serializer types is valid.

iseki0 commented 1 year ago

@pdvrieze I means there's no compilation error if the serializaer is hidden. As a instead there're runtime exception thrown during try to serialize/deserialize. It's a very bad development experience.

iseki0 commented 1 year ago

🤣I change the title, make it more clearly

pdvrieze commented 1 year ago

@iseki0 I see, I assume that you use the Type.serializer() function. This is however an intrinsic now that will silently be replaced with the actual serializer. Your issue is probably with the intrinsic not observing JPMS.

iseki0 commented 1 year ago

Although I'm not using it.(I'm prefer to using the standalone function serializer<Type>() instead). But the problem you said is correct. The intrinsic ignored the JPMS boundray during compilation. Sorry for my bad English, I use a wrong word. @pdvrieze

sandwwraith commented 1 year ago

Hmm, interesting issue. Shouldn't just using a class reference (e.g. println(MyFooSerializer::class)) also trigger a JPMS error? In that case, this is a missing functionality in Kotlin or IDE, rather than a serialization plugin.

iseki0 commented 1 year ago

@sandwwraith emmm Do you meaning to export a type with an internal serializer is a problem and it should be treat as an error reported by IDE and Kotlin compiler?

sandwwraith commented 1 year ago

No, I mean referencing non-exported type should be an error in the IDE regardless whether it is used in serialization annotations or not.

Or, maybe I misunderstood you and you have a situation like this:

// File a.kt
package my

@Serializable(my.internal.MySerializer::class)
class MyClass

// File b.kt
package my.internal
class MySerializer: KSerializer<MyClass>

// module-info.java
exports my;
pdvrieze commented 1 year ago

@sandwwraith The way I understand the bug is that (given your example) serializer<MyClass>() compiles (and thus also Json.decodeFromString<Type>(data) which is just an inline function calling serializer()). But as the function is an intrinsic this transforms into a direct reference to MySerializer that is not public. But even if not an intrinsic, there would be an annotation that refers to a non-accessible type (I'm not sure what the correct behaviour is for that).

iseki0 commented 1 year ago

I think the problem is clarificated. :)🥰