From af9300c019b300cf68534e8dc2ac19b835d63a04 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 144a0387288..c06c47e4ae3 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 e1b41421a8f..92b68e2afdb 100644 --- a/core/api/core.api +++ b/core/api/core.api @@ -2452,6 +2452,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; @@ -2489,6 +2490,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; @@ -4746,6 +4748,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 acddb513a9a..e65161ba483 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 c787df8e1db..c1deb359669 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 ee296eecfe1..7750640ff72 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 2a2aa995d51..9e7dab10bf3 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 b38ed983bec..8019d729397 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 be017659988..716786a52b5 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 00000000000..993fbbe965d --- /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 29c136e0b63..0b05d6c8f5e 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 da30cf30ef4..cfdf92efda3 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)