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

Implement support for KotlinX Serialization #196

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Feb 18, 2024

This change adds support for a new code-gen argument, implementKSerializable, which results in the annotation kotlinx.serialization.Serializable being added to data classes during codegen.

New discussion opened here

Approach

A new argument has been added to the Kotlin code generator and Gradle plugin, at implementKSerializable. When set to true, the code generator annotates data classes with @kotlinx.serialization.Serializable.

If both JVM serialization and KotlinX serialization are enabled, the code generator properly qualifies with java.io.Serializable and import kotlinx.serialization.Serializable. The two are optional and usable independent of each other.

For the moment, this is filed on top of these other PRs to avoid conflicts, since they all touch the code generator; these can be rebased away, though:

Transitive dependency needs

Users of this code will need to include the KotlinX Serialization compiler plugin in their build, with:

plugins {
  kotlin("plugin.serialization")

  // -- or --
  alias(libs.plugins.kotlin.serialization)  // version catalog

  // -- or --
  id(libs.plugins.kotlin.serialization.get().pluginId)  // multi-module build with version catalog
}

And a dependency on KotlinX Serialization Core, for the annotation, at version 1.5.0 or greater:

dependencies {
  api("org.jetbrains.kotlinx:kotlinx-serialization-core:1.5.0")

  // -- or --
  api(libs.kotlinx.serialization.core)
}
[versions]
# ...
kotlinx-serialization = "1.5.0"  # or greater for newer Kotlin metadata versions
# ...

[libraries]
# ...
kotlinx-serialization = { module = "org.jetbrains.kotlinx:kotlinx-serialization-core", version.ref = "kotlinx-serialization" }
# ...

I stopped short of adding an api dependency to Pkl's Kotlin runtime library to avoid adding it to consumer dependency graphs downstream. This could easily be added, though, if preferable. Gradle will generally select the newest available version in a user's build, but builds that don't include KotlinX Serialization already will have to download it if it is added this way, of course.

IDEA and Kotlin warn appropriately if the compiler is not included.

Changelog

  • feat(codegen): add support for kotlin Serializable annotation
  • feat(gradle): add implementKSerializable argument
  • test(codegen): add test for KotlinX serialization support
  • test(codegen): add test for both Java and KotlinX serialization
  • test(gradle): add test for compiling with KotlinX serialization
  • chore: update lockfiles

Fixes and closes apple#191

Signed-off-by: Sam Gammon <[email protected]>
Adds a setting to the Kotlin code generator which controls the
target Kotlin package for codegen. Applied as a prefix to the
normal generated package name.

The current versions of the `kotlinx.serialization` and
`kotlinx.html` libraries are declared dynamically, which causes
a failure when refreshing dependencies. To avoid Kotlin metadata
build issues, these have been pinned.

- feat(codegen): use `kotlinPackage` as prefix for kotlin codegen
- feat(gradle): `kotlinPackage` property in gradle plugin
- test(codegen): add tests for custom kotlin package
- test(gradle): add tests for generating with custom kotlin package

Signed-off-by: Sam Gammon <[email protected]>
This change adds support for a new code-gen argument,
`implementKSerializable`, which results in the annotation
`kotlinx.serialization.Serializable` being added to `data`
classes during codegen.

Relates to discussion apple#185

- feat(codegen): add support for kotlin `Serializable` annotation
- feat(gradle): add `implementKSerializable` argument
- test(codegen): add test for KotlinX serialization support
- test(codegen): add test for both Java and KotlinX serialization
- test(gradle): add test for compiling with KotlinX serialization

Signed-off-by: Sam Gammon <[email protected]>
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase your changes on top of main? It's a little hard to review at the moment.

One general comment here is that we shouldn't need to actually depend on kotlinx.serialization; we can reference the class by string.

Also another note: Our codegen includes types that aren't Serializable. This includes org.pkl.core.DataSize, org.pkl.core.Duration, and other subclasses of org.pkl.core.Value.

Maybe those properties should be marked @Contextual, if using kotlinx serialization? And we would leave it to end users to decide how these should be serialized.

val implementSerializable: Boolean = false,

/** Whether to annotate data classes with [kotlinx.serialization.Serializable] */
val implementKSerializable: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, and everywhere else:

Suggested change
val implementKSerializable: Boolean = false,
val generateKotlinSerializable: Boolean = false,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okm..

@@ -933,6 +1170,7 @@ class KotlinCodeGeneratorTest {
fun `user defined type aliases`() {
val kotlinCode =
generateKotlinCode(
// language=text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// language=text

return this
}

val spec = AnnotationSpec.builder(Serializable::class).build()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid adding a dependency on kotlinx serialization by referencing kotlinx serialization by string:

Suggested change
val spec = AnnotationSpec.builder(Serializable::class).build()
val spec = AnnotationSpec
.builder(ClassName("kotlinx.serialization", "Serializable"))
.build()

@sgammon
Copy link
Contributor Author

sgammon commented Mar 20, 2024

Hey @bioball, sorry I missed your review here! I'll take a look. The problem with sealed classes is a bigger issue that may need some design attention before this feature ships.

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

Successfully merging this pull request may close these issues.

3 participants