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

Maybe throw a compile error when using serializer<T>() for a non-Serializable type #2522

Closed
arkivanov opened this issue Dec 3, 2023 · 13 comments

Comments

@arkivanov
Copy link

What is your use-case and why do you need this feature?

Consider the following code:

import kotlinx.serialization.serializer

fun foo() {
    val serializer = serializer<Some>()
}

data class Some(val value: Int)

Currently, it produces the following bytecode for Java (decompiled):

public final class FooKt {
   public static final void foo() {
      KSerializer serializer = SerializersKt.noCompiledSerializer("com.example.Some");
   }
}

Describe the solution you'd like

If I understand it correctly, the generated code just throws an exception at runtime. Maybe it would be better to fail the compilation instead?

@qwwdfsad
Copy link
Member

qwwdfsad commented Dec 4, 2023

Nice idea, would be nice to produce either string warning or plugin-induced error.

@sandwwraith
Copy link
Member

I think we discussed this during the intrinsic design process. The general agreement was that compiler intrinsics are optimization and not a separate feature; therefore, they should not affect users' experience (in a negative AND positive way). So, regarding this particular suggestion: it is a nice idea, but doing so would also result in different experiences between serializer<Some>() (intrinsic enabled) and serializer(typeOf<Some>()) (no intrinsic, plain reflection). So this may bring questions like 'why we can warn in one case and can't in another', while code should do literally the same thing.

@sandwwraith
Copy link
Member

Also there would be differences between serializer<Some>() and myModule.serializer<Some>() — we can't report error on latter

@arkivanov
Copy link
Author

arkivanov commented Dec 4, 2023

Here is my experience with serializer<Some>(). My original use case is that I have the following API in one of my libraries (simplified).

interface StateKeeper {
    fun <T : Any> save(value: T, strategy: SerializationStrategy<T>)
    fun <T : Any> restore(strategy: DeserializationStrategy<T>): T?
}

This works just fine. For convenience, I would like to also some extensions, something like this.

inline fun <reified T : Any> StateKeeper.save(value: T) {
    save(value = value, strategy = serializer<T>())
}

inline fun <reified T : Any> StateKeeper.restore(): T? =
    restore(strategy = serializer<T>())

After reading the docs for serializer<T>(), there was no mention of any intrinsic compiler behaviour. So I assumed this all based on reflection and so may cause issues at runtime. In this case I would avoid adding those extensions. But after checking the bytecode, turned out that serializer<T>() has intrinsic behaviour (i.e. the actual bytecode is not what we can see in the serializer<T>() implementation by Ctrl+Clicking on it).

It would be much safer if we could have a compile time safe option for such use cases. To me, just documenting both serializer<T>() and serializer(typeOf<T>()) variants would be good. But a separate function for this case would be good as well.

@pdvrieze
Copy link
Contributor

pdvrieze commented Dec 4, 2023

For the intrinsic case it would make sense to generate a warning, and fall back to the default (reflective) approach. This would only really affect compilations that treat all warnings as errors, which would not likely be "stable" anyway (as new conditions trigger warnings). But I agree the semantics should remain.
Also the warning could be generated even on reflective lookups when it is known that the specific base type has no serializer (in the current code base). It is surprisingly easy to have missing serializers.

@arkivanov
Copy link
Author

I believe in the specific case of the intrinsic serializer<T>(), there would be no case when a reflective approach would succeed after intrinsic check failed?

Anyway, to me it looks beneficial to have a way of intrinsic-only behaviour, even if implemented via a separate dedicated API. I.e. I want to be sure that if the code compiled, then there is a guaranteed serializer for me. So that I could use this API in a library (as mentioned above), and the user's compilation would fail if they pass a non-serializable type by accident.

@pdvrieze
Copy link
Contributor

pdvrieze commented Dec 5, 2023

@arkivanov It could succeed if T is updated to get a serializer: class loading, separate module, update dynamic library (native, although that may not actually work due to lack of reflection)

@sandwwraith
Copy link
Member

So you actually want to report this diagnostic on save() and restore(), not the serializer<T>()? This is something we also can't do, as inliner 1) can't report diagnostics and 2) isn't called in the IDE.
In short, I believe that the compiler is not powerful enough to cover all the cases, and for a very limited subset of cases, this is simply not worth it. So I'll close this.

@pdvrieze
Copy link
Contributor

pdvrieze commented Dec 7, 2023

Perhaps an approach that could be used to the same effect is to allow the @Serializable annotation to be used on the reified type parameter. This would then be able to check that the type actually is serializable.

@arkivanov
Copy link
Author

arkivanov commented Dec 7, 2023

Nice idea @pdvrieze! However, I don't understand what prevents the compiler plugin from throwing an exception instead of generating the noCompiledSerializer call.

With the latest example API above, if I write the following:

fun foo(stateKeeper: StateKeeper) {
    val some = stateKeeper.restore<Some>()
}

data class Some(val value: Int)

Then I have the following in jar file (decompiled):

public final class FooKt {
   public static final void foo(@NotNull StateKeeper stateKeeper) {
      Intrinsics.checkNotNullParameter(stateKeeper, "stateKeeper");
      int $i$f$restore = false;
      Some some = (Some)stateKeeper.consume("some", (DeserializationStrategy)SerializersKt.noCompiledSerializer("com.arkivanov.essenty.statekeeper.Some"));
   }
}

So the code being generated is 100% failing at runtime. I understand that we can't support this inline case in the IDE, but can this be supported at least at compile time? Perhaps with a separate "intrinsic-only" API like requireSerializer<T>() or something.

@pdvrieze
Copy link
Contributor

pdvrieze commented Dec 7, 2023

@arkivanov As Leonid alluded, the way that inlining works is that inlined code is precompiled and then injected into the caller by the inliner. When the inliner is called, the actual value of T is unknown so it is not possible to determine serializability. As the code has already been pre-compiled the inliner assumes it correct (there is not even reporting capability) and doesn't do further verification. I haven't looked into the specifics, but the intrinsic engages somewhere in this process as well but is assumed correct (because the non-intrinsic call is correct).

@arkivanov
Copy link
Author

the way that inlining works is that inlined code is precompiled

Thanks! I might be missing something, but please consider the following fact.

In a library I have the following API.

inline fun <reified T : Any> StateKeeper.restore(): T? =
    restore(strategy = serializer<T>())

This produces the following in the library jar.

   public static final <T> T restore(StateKeeper $this$restore) {
      Intrinsics.checkNotNullParameter($this$restore, "<this>");
      int $i$f$restore = false;
      Intrinsics.reifiedOperationMarker(6, "T");
      MagicApiIntrinsics.voidMagicApiCall("kotlinx.serialization.serializer.simple");
      return $this$restore.restore((DeserializationStrategy)SerializersKt.serializer((KType)null));
   }

You can see (KType)null here, so I'm pretty sure this can't be called (or inlined) as it is.

Then in my project that depends on the library I have the following code.

fun foo(stateKeeper: StateKeeper) {
    val some = stateKeeper.restore<Some>()
}

data class Some(val value: Int)

fun bar(stateKeeper: StateKeeper) {
    val some = stateKeeper.restore<SomeSerializable>()
}

@Serializable
data class SomeSerializable(val value: Int)

After compiling the project into jar, I have the following output.

public final class FooKt {
   public static final void foo(@NotNull StateKeeper stateKeeper) {
      Intrinsics.checkNotNullParameter(stateKeeper, "stateKeeper");
      int $i$f$restore = false;
      Some some = (Some)stateKeeper.restore((DeserializationStrategy)SerializersKt.noCompiledSerializer("com.arkivanov.essenty.statekeeper.Some"));
   }

   public static final void bar(@NotNull StateKeeper stateKeeper) {
      Intrinsics.checkNotNullParameter(stateKeeper, "stateKeeper");
      int $i$f$restore = false;
      SomeSerializable some = (SomeSerializable)stateKeeper.restore((DeserializationStrategy)SomeSerializable.Companion.serializer());
   }
}

As you can see, in case of foo - the generated code fails 100% by calling noCompiledSerializer, whereas in case of bar it succeeds by calling SomeSerializable.Companion.serializer(). So my question still states, why can't we fail the compilation of the project (using the library) instead of generating a 100% failing code?

@sandwwraith
Copy link
Member

why can't we fail the compilation of the project (using the library)

We can throw InternalCompilerError from the inliner, resulting in build failure with a compiler crash and a stacktrace :) Which is not a nice UX, in my opinion. Other means for nice diagnostics rendering are not available in the inliner, because it works over bytecode, not source files. Also, making behavior different between the CLI compiler and the IDE is another big UX hit.

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

4 participants