From 2b12ba28ec50140286e38db2223cdcfd5a6166eb Mon Sep 17 00:00:00 2001 From: Lukellmann <47486203+Lukellmann@users.noreply.github.com> Date: Wed, 29 Mar 2023 09:09:38 +0200 Subject: [PATCH] Add inspection for missing Optional default values (#797) If a dev.kord.common.entity.optional.Optional is used in a @Serializable class, it should always have a default value so that deserialization works if the value is missing in the JSON. Until now this was easy to forget which could lead to deserialization errors down the line. By introducing a KSP processor that looks at all primary constructor parameters of @Serializable classes, there is now a static check executed at build time that verifies the default value is not forgotten in any place. --- .../kotlin/entity/optional/OptionalTest.kt | 4 +- core/api/core.api | 3 ++ core/build.gradle.kts | 2 + .../cache/data/ApplicationCommandData.kt | 10 ++-- core/src/main/kotlin/cache/data/RegionData.kt | 2 +- core/src/main/kotlin/cache/data/RoleData.kt | 4 +- gateway/build.gradle.kts | 2 + ksp-processors/src/main/kotlin/KSPUtils.kt | 11 +++- .../OptionalDefaultInspectionProcessor.kt | 52 +++++++++++++++++++ ...ols.ksp.processing.SymbolProcessorProvider | 1 + rest/build.gradle.kts | 2 + 11 files changed, 82 insertions(+), 11 deletions(-) create mode 100644 ksp-processors/src/main/kotlin/inspection/OptionalDefaultInspectionProcessor.kt diff --git a/common/src/test/kotlin/entity/optional/OptionalTest.kt b/common/src/test/kotlin/entity/optional/OptionalTest.kt index 144a03872889..c06c47e4ae37 100644 --- a/common/src/test/kotlin/entity/optional/OptionalTest.kt +++ b/common/src/test/kotlin/entity/optional/OptionalTest.kt @@ -28,7 +28,7 @@ internal class OptionalTest { @Serializable - private class NullOptionalEntity(val value: Optional) + private class NullOptionalEntity(val value: Optional = Optional.Missing()) @Test fun `deserializing null in nullable optional assigns Null`() { @@ -82,4 +82,4 @@ internal class OptionalTest { } } -} \ No newline at end of file +} diff --git a/core/api/core.api b/core/api/core.api index 2a7ef8298710..597b70022af4 100644 --- a/core/api/core.api +++ b/core/api/core.api @@ -2451,6 +2451,7 @@ public final class dev/kord/core/cache/data/ApplicationCommandParameterData { public static final field Companion Ldev/kord/core/cache/data/ApplicationCommandParameterData$Companion; public synthetic fun (ILjava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;Ldev/kord/common/entity/optional/Optional;Lkotlinx/serialization/internal/SerializationConstructorMarker;)V public fun (Ljava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;Ldev/kord/common/entity/optional/Optional;)V + public synthetic fun (Ljava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;Ldev/kord/common/entity/optional/Optional;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun component1 ()Ljava/lang/String; public final fun component2 ()Ljava/lang/String; public final fun component3 ()Ldev/kord/common/entity/optional/OptionalBoolean; @@ -2488,6 +2489,7 @@ public final class dev/kord/core/cache/data/ApplicationCommandSubcommandData { public static final field Companion Ldev/kord/core/cache/data/ApplicationCommandSubcommandData$Companion; public synthetic fun (ILjava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;Lkotlinx/serialization/internal/SerializationConstructorMarker;)V public fun (Ljava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;)V + public synthetic fun (Ljava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun component1 ()Ljava/lang/String; public final fun component2 ()Ljava/lang/String; public final fun component3 ()Ldev/kord/common/entity/optional/OptionalBoolean; @@ -4740,6 +4742,7 @@ public final class dev/kord/core/cache/data/RegionData { public static final field Companion Ldev/kord/core/cache/data/RegionData$Companion; public synthetic fun (ILjava/lang/String;Ldev/kord/common/entity/optional/OptionalSnowflake;Ljava/lang/String;ZZZLkotlinx/serialization/internal/SerializationConstructorMarker;)V public fun (Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalSnowflake;Ljava/lang/String;ZZZ)V + public synthetic fun (Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalSnowflake;Ljava/lang/String;ZZZILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun component1 ()Ljava/lang/String; public final fun component2 ()Ldev/kord/common/entity/optional/OptionalSnowflake; public final fun component3 ()Ljava/lang/String; diff --git a/core/build.gradle.kts b/core/build.gradle.kts index acddb513a9aa..e65161ba483c 100644 --- a/core/build.gradle.kts +++ b/core/build.gradle.kts @@ -24,6 +24,8 @@ dependencies { api(libs.kord.cache.api) api(libs.kord.cache.map) + ksp(projects.kspProcessors) + samplesImplementation(libs.slf4j.simple) testImplementation(libs.bundles.test.implementation) diff --git a/core/src/main/kotlin/cache/data/ApplicationCommandData.kt b/core/src/main/kotlin/cache/data/ApplicationCommandData.kt index c787df8e1dba..c1deb359669d 100644 --- a/core/src/main/kotlin/cache/data/ApplicationCommandData.kt +++ b/core/src/main/kotlin/cache/data/ApplicationCommandData.kt @@ -112,8 +112,8 @@ public fun ApplicationCommandGroupData(data: ApplicationCommandOptionData): Appl public data class ApplicationCommandSubcommandData( val name: String, val description: String, - val isDefault: OptionalBoolean, - val parameters: Optional> + val isDefault: OptionalBoolean = OptionalBoolean.Missing, + val parameters: Optional> = Optional.Missing(), ) @@ -133,9 +133,9 @@ public fun ApplicationCommandSubCommandData(data: ApplicationCommandOptionData): public data class ApplicationCommandParameterData( val name: String, val description: String, - val required: OptionalBoolean, - val choices: Optional>, - val channelTypes: Optional> + val required: OptionalBoolean = OptionalBoolean.Missing, + val choices: Optional> = Optional.Missing(), + val channelTypes: Optional> = Optional.Missing(), ) diff --git a/core/src/main/kotlin/cache/data/RegionData.kt b/core/src/main/kotlin/cache/data/RegionData.kt index ee296eecfe1a..7750640ff72a 100644 --- a/core/src/main/kotlin/cache/data/RegionData.kt +++ b/core/src/main/kotlin/cache/data/RegionData.kt @@ -7,7 +7,7 @@ import kotlinx.serialization.Serializable @Serializable public data class RegionData( val id: String, - val guildId: OptionalSnowflake, + val guildId: OptionalSnowflake = OptionalSnowflake.Missing, val name: String, val optimal: Boolean, val deprecated: Boolean, diff --git a/core/src/main/kotlin/cache/data/RoleData.kt b/core/src/main/kotlin/cache/data/RoleData.kt index 2a2aa995d517..9e7dab10bf38 100644 --- a/core/src/main/kotlin/cache/data/RoleData.kt +++ b/core/src/main/kotlin/cache/data/RoleData.kt @@ -17,8 +17,8 @@ public data class RoleData( val name: String, val color: Int, val hoisted: Boolean, - val icon: Optional, - val unicodeEmoji: Optional, + val icon: Optional = Optional.Missing(), + val unicodeEmoji: Optional = Optional.Missing(), val position: Int, val permissions: Permissions, val managed: Boolean, diff --git a/gateway/build.gradle.kts b/gateway/build.gradle.kts index b38ed983bec7..8019d729397b 100644 --- a/gateway/build.gradle.kts +++ b/gateway/build.gradle.kts @@ -11,6 +11,8 @@ dependencies { api(libs.ktor.client.websockets) api(libs.ktor.client.cio) + ksp(projects.kspProcessors) + testImplementation(libs.bundles.test.implementation) testRuntimeOnly(libs.bundles.test.runtime) } diff --git a/ksp-processors/src/main/kotlin/KSPUtils.kt b/ksp-processors/src/main/kotlin/KSPUtils.kt index be0176599880..716786a52b52 100644 --- a/ksp-processors/src/main/kotlin/KSPUtils.kt +++ b/ksp-processors/src/main/kotlin/KSPUtils.kt @@ -1,7 +1,7 @@ package dev.kord.ksp import com.google.devtools.ksp.processing.Resolver -import com.google.devtools.ksp.symbol.KSAnnotation +import com.google.devtools.ksp.symbol.* import kotlin.reflect.KProperty1 internal inline fun Resolver.getSymbolsWithAnnotation(inDepth: Boolean = false) = @@ -18,3 +18,12 @@ internal class AnnotationArguments private constructor(private val map: Map false + is KSClassifierReference -> true + is KSDefNonNullReference -> enclosedType.isClassifierReference + is KSParenthesizedReference -> element.isClassifierReference + else -> error("Unexpected KSReferenceElement: $this") + } diff --git a/ksp-processors/src/main/kotlin/inspection/OptionalDefaultInspectionProcessor.kt b/ksp-processors/src/main/kotlin/inspection/OptionalDefaultInspectionProcessor.kt new file mode 100644 index 000000000000..993fbbe965d3 --- /dev/null +++ b/ksp-processors/src/main/kotlin/inspection/OptionalDefaultInspectionProcessor.kt @@ -0,0 +1,52 @@ +package dev.kord.ksp.inspection + +import com.google.devtools.ksp.findActualType +import com.google.devtools.ksp.processing.* +import com.google.devtools.ksp.symbol.* +import dev.kord.ksp.getSymbolsWithAnnotation +import dev.kord.ksp.isClassifierReference +import kotlinx.serialization.Serializable + +/** [SymbolProcessorProvider] for [OptionalDefaultInspectionProcessor]. */ +class OptionalDefaultInspectionProcessorProvider : SymbolProcessorProvider { + override fun create(environment: SymbolProcessorEnvironment): SymbolProcessor = + OptionalDefaultInspectionProcessor(environment.logger) +} + +private val OPTIONAL_TYPES = + listOf("Optional", "OptionalBoolean", "OptionalInt", "OptionalLong", "OptionalSnowflake") + .map { "dev.kord.common.entity.optional.$it" } + .toSet() + +/** + * [SymbolProcessor] that verifies that every primary constructor parameter with `Optional` type of a [Serializable] + * class has a default value. + */ +private class OptionalDefaultInspectionProcessor(private val logger: KSPLogger) : SymbolProcessor { + override fun process(resolver: Resolver): List { + resolver.getSymbolsWithAnnotation() + .filterIsInstance() + .forEach { it.verifySerializableClassPrimaryConstructor() } + + return emptyList() // we never have to defer any symbols + } + + private fun KSClassDeclaration.verifySerializableClassPrimaryConstructor() { + primaryConstructor?.parameters?.forEach { parameter -> + if (parameter.hasDefault) return@forEach + + val type = parameter.type + if (type.element?.isClassifierReference == false) return@forEach + + val clazz = when (val declaration = type.resolve().declaration) { + is KSTypeParameter -> return@forEach + is KSClassDeclaration -> declaration + is KSTypeAlias -> declaration.findActualType() + else -> error("Unexpected KSDeclaration: $declaration") + } + if (clazz.qualifiedName?.asString() in OPTIONAL_TYPES) { + logger.error("Missing default for parameter ${parameter.name?.asString()}.", symbol = parameter) + } + } + } +} diff --git a/ksp-processors/src/main/resources/META-INF/services/com.google.devtools.ksp.processing.SymbolProcessorProvider b/ksp-processors/src/main/resources/META-INF/services/com.google.devtools.ksp.processing.SymbolProcessorProvider index 29c136e0b63e..0b05d6c8f5e6 100644 --- a/ksp-processors/src/main/resources/META-INF/services/com.google.devtools.ksp.processing.SymbolProcessorProvider +++ b/ksp-processors/src/main/resources/META-INF/services/com.google.devtools.ksp.processing.SymbolProcessorProvider @@ -1 +1,2 @@ +dev.kord.ksp.inspection.OptionalDefaultInspectionProcessorProvider dev.kord.ksp.kordenum.KordEnumProcessorProvider diff --git a/rest/build.gradle.kts b/rest/build.gradle.kts index da30cf30ef42..cfdf92efda3d 100644 --- a/rest/build.gradle.kts +++ b/rest/build.gradle.kts @@ -10,6 +10,8 @@ dependencies { api(libs.bundles.ktor.client.serialization) api(libs.ktor.client.cio) + ksp(projects.kspProcessors) + testImplementation(libs.bundles.test.implementation) testImplementation(libs.ktor.client.mock) testRuntimeOnly(libs.bundles.test.runtime)