Skip to content

Commit

Permalink
Add inspection for missing Optional default values (#797)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lukellmann committed Mar 31, 2023
1 parent 98181ac commit af9300c
Show file tree
Hide file tree
Showing 11 changed files with 82 additions and 11 deletions.
4 changes: 2 additions & 2 deletions common/src/test/kotlin/entity/optional/OptionalTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ internal class OptionalTest {


@Serializable
private class NullOptionalEntity(val value: Optional<String?>)
private class NullOptionalEntity(val value: Optional<String?> = Optional.Missing())

@Test
fun `deserializing null in nullable optional assigns Null`() {
Expand Down Expand Up @@ -82,4 +82,4 @@ internal class OptionalTest {
}
}

}
}
3 changes: 3 additions & 0 deletions core/api/core.api
Original file line number Diff line number Diff line change
Expand Up @@ -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 <init> (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 <init> (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 <init> (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;
Expand Down Expand Up @@ -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 <init> (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 <init> (Ljava/lang/String;Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalBoolean;Ldev/kord/common/entity/optional/Optional;)V
public synthetic fun <init> (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;
Expand Down Expand Up @@ -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 <init> (ILjava/lang/String;Ldev/kord/common/entity/optional/OptionalSnowflake;Ljava/lang/String;ZZZLkotlinx/serialization/internal/SerializationConstructorMarker;)V
public fun <init> (Ljava/lang/String;Ldev/kord/common/entity/optional/OptionalSnowflake;Ljava/lang/String;ZZZ)V
public synthetic fun <init> (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;
Expand Down
2 changes: 2 additions & 0 deletions core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions core/src/main/kotlin/cache/data/ApplicationCommandData.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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<List<ApplicationCommandParameterData>>
val isDefault: OptionalBoolean = OptionalBoolean.Missing,
val parameters: Optional<List<ApplicationCommandParameterData>> = Optional.Missing(),
)


Expand All @@ -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<List<ApplicationCommandOptionChoiceData>>,
val channelTypes: Optional<List<ChannelType>>
val required: OptionalBoolean = OptionalBoolean.Missing,
val choices: Optional<List<ApplicationCommandOptionChoiceData>> = Optional.Missing(),
val channelTypes: Optional<List<ChannelType>> = Optional.Missing(),
)


Expand Down
2 changes: 1 addition & 1 deletion core/src/main/kotlin/cache/data/RegionData.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/kotlin/cache/data/RoleData.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ public data class RoleData(
val name: String,
val color: Int,
val hoisted: Boolean,
val icon: Optional<String?>,
val unicodeEmoji: Optional<String?>,
val icon: Optional<String?> = Optional.Missing(),
val unicodeEmoji: Optional<String?> = Optional.Missing(),
val position: Int,
val permissions: Permissions,
val managed: Boolean,
Expand Down
2 changes: 2 additions & 0 deletions gateway/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
11 changes: 10 additions & 1 deletion ksp-processors/src/main/kotlin/KSPUtils.kt
Original file line number Diff line number Diff line change
@@ -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 <reified A : Annotation> Resolver.getSymbolsWithAnnotation(inDepth: Boolean = false) =
Expand All @@ -18,3 +18,12 @@ internal class AnnotationArguments private constructor(private val map: Map<Stri
get() = AnnotationArguments(arguments.associate { it.name!!.getShortName() to it.value!! })
}
}

internal val KSReferenceElement.isClassifierReference: Boolean
get() = when (this) {
is KSDynamicReference, is KSCallableReference -> false
is KSClassifierReference -> true
is KSDefNonNullReference -> enclosedType.isClassifierReference
is KSParenthesizedReference -> element.isClassifierReference
else -> error("Unexpected KSReferenceElement: $this")
}
Original file line number Diff line number Diff line change
@@ -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<KSAnnotated> {
resolver.getSymbolsWithAnnotation<Serializable>()
.filterIsInstance<KSClassDeclaration>()
.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)
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
dev.kord.ksp.inspection.OptionalDefaultInspectionProcessorProvider
dev.kord.ksp.kordenum.KordEnumProcessorProvider
2 changes: 2 additions & 0 deletions rest/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit af9300c

Please sign in to comment.