Skip to content

pkl-config-java: Migrate nullness to jSpecify#1528

Merged
bioball merged 2 commits intoapple:mainfrom
odenix:config-java-jspecify
Apr 17, 2026
Merged

pkl-config-java: Migrate nullness to jSpecify#1528
bioball merged 2 commits intoapple:mainfrom
odenix:config-java-jspecify

Conversation

@odenix
Copy link
Copy Markdown
Contributor

@odenix odenix commented Apr 16, 2026

Questions that came up while working on this PR:

  1. Should Config be thread-safe? Currently, it isn't, as some Converter implementations aren't.
  2. Should Config.makeConfig() be part of the public API? Currently, it is.

@odenix odenix force-pushed the config-java-jspecify branch from 604caa2 to 0d663de Compare April 16, 2026 19:46
@HT154
Copy link
Copy Markdown
Contributor

HT154 commented Apr 16, 2026

Looks like changes are needed in pkl-config-kotlin for this to compile. Is this expected?

diff --git a/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/ConfigExtensions.kt b/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/ConfigExtensions.kt
index 6bf47437..16513274 100644
--- a/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/ConfigExtensions.kt
+++ b/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/ConfigExtensions.kt
@@ -36,10 +36,10 @@ import org.pkl.config.kotlin.mapper.KotlinConverterFactories
  * * easier to use with parameterized types: `to<List<String>>()` vs.
  *   `as(JavaType.listOf(String::class.java))`
  */
-inline fun <reified T> Config.to(): T {
+inline fun <reified T : Any> Config.to(): T {
   val javaType = object : JavaType<T>() {}
   // `as T?` may no longer be required after switching to JSpecify
-  val result = `as`<T>(javaType.type) as T?
+  val result = `as`(javaType.type) as T?
   if (result == null && null !is T) {
     throw ConversionException(
       "Expected a non-null value but got `null`. " +
diff --git a/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/mapper/PPairToKotlinPair.kt b/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/mapper/PPairToKotlinPair.kt
index cf93536a..7e38a8e0 100644
--- a/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/mapper/PPairToKotlinPair.kt
+++ b/pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/mapper/PPairToKotlinPair.kt
@@ -42,7 +42,7 @@ internal class PPairToKotlinPair : ConverterFactory {
     )
   }
 
-  private class ConverterImpl<F, S>(
+  private class ConverterImpl<F : Any, S : Any>(
     private val firstTargetType: Type,
     private val secondTargetType: Type,
   ) : Converter<PPair<Any, Any>, Pair<F, S>> {

@odenix
Copy link
Copy Markdown
Contributor Author

odenix commented Apr 16, 2026

Yes, I'm on it. kotlinc has direct support for jSpecify and is very strict about nullness. Unfortunately, a local gradlew clean build didn't catch this (didn't rerun the failing compileKotlin task). Only --rerun-tasks did the job.

@odenix odenix force-pushed the config-java-jspecify branch from 0d663de to 6a6ce39 Compare April 16, 2026 22:59
@odenix odenix force-pushed the config-java-jspecify branch from 6a6ce39 to 88cbe91 Compare April 16, 2026 23:24
@odenix
Copy link
Copy Markdown
Contributor Author

odenix commented Apr 16, 2026

@HT154 force-pushed because I had missed some org.pkl.core.utils.Nullable imports

@odenix odenix force-pushed the config-java-jspecify branch from 88cbe91 to 8d9ccec Compare April 17, 2026 01:14
@bioball
Copy link
Copy Markdown
Member

bioball commented Apr 17, 2026

  1. Should Config be thread-safe? Currently, it isn't, as some Converter implementations aren't.

Yeah, I'd consider it a bug if it isn't.

  1. Should Config.makeConfig() be part of the public API? Currently, it is.

I don't think so. Happy to accept a PR to move this out.

Copy link
Copy Markdown
Member

@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.

Nice, thanks!

Comment thread pkl-config-kotlin/src/main/kotlin/org/pkl/config/kotlin/ConfigExtensions.kt Outdated
inline fun <reified T> Config.to(): T {
val javaType = object : JavaType<T>() {}
// `as T?` may no longer be required after switching to JSpecify
val result = `as`<T>(javaType.type) as T?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

@bioball bioball merged commit 1571d72 into apple:main Apr 17, 2026
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