From d25abb1ee59d9d540fb13d72a2e648f019526b38 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Fri, 8 Nov 2019 14:35:13 -0800 Subject: [PATCH] Fix incorrect type variance being applied on generated adapters (#1010) * Fix: force object type for type arguments * Add outDeclaration regression test for #1009 * Create mapTypesUtility for reusing recursive type mapping * Strip typevar variance where appropriate Resolves #1009 --- .../kotlin/codegen/api/AdapterGenerator.kt | 4 +- .../moshi/kotlin/codegen/api/kotlintypes.kt | 49 +++++++++++++++++ .../squareup/moshi/kotlin/codegen/metadata.kt | 53 +++++-------------- .../squareup/moshi/kotlin/DualKotlinTest.kt | 18 +++++++ .../kotlin/reflect/-MoshiKotlinExtensions.kt | 10 ++-- 5 files changed, 90 insertions(+), 44 deletions(-) diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt index 8719662..931790d 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt +++ b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt @@ -69,7 +69,7 @@ internal class AdapterGenerator( private val nameAllocator = NameAllocator() private val adapterName = "${className.simpleNames.joinToString(separator = "_")}JsonAdapter" - private val originalTypeName = target.typeName + private val originalTypeName = target.typeName.stripTypeVarVariance() private val originalRawTypeName = originalTypeName.rawType() private val moshiParam = ParameterSpec.builder( @@ -130,7 +130,7 @@ internal class AdapterGenerator( result.superclass(jsonAdapterTypeName) if (typeVariables.isNotEmpty()) { - result.addTypeVariables(typeVariables) + result.addTypeVariables(typeVariables.map { it.stripTypeVarVariance() as TypeVariableName }) } // TODO make this configurable. Right now it just matches the source model diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt index 82d886b..1d61aa8 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt +++ b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/kotlintypes.kt @@ -29,11 +29,15 @@ import com.squareup.kotlinpoet.KModifier import com.squareup.kotlinpoet.LONG import com.squareup.kotlinpoet.NOTHING import com.squareup.kotlinpoet.ParameterizedTypeName +import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy import com.squareup.kotlinpoet.SHORT +import com.squareup.kotlinpoet.STAR import com.squareup.kotlinpoet.TypeName import com.squareup.kotlinpoet.TypeVariableName import com.squareup.kotlinpoet.UNIT +import com.squareup.kotlinpoet.WildcardTypeName import com.squareup.kotlinpoet.asTypeName +import kotlin.reflect.KClass internal fun TypeName.rawType(): ClassName { return when (this) { @@ -95,3 +99,48 @@ internal fun KModifier.checkIsVisibility() { "Visibility must be one of ${(0..ordinal).joinToString { KModifier.values()[it].name }}. Is $name" } } + +internal inline fun TypeName.mapTypes(noinline transform: T.() -> TypeName?): TypeName { + return mapTypes(T::class, transform) +} + +@Suppress("UNCHECKED_CAST") +internal fun TypeName.mapTypes(target: KClass, transform: T.() -> TypeName?): TypeName { + if (target.java == javaClass) { + return (this as T).transform() ?: return this + } + return when (this) { + is ClassName -> this + is ParameterizedTypeName -> { + (rawType.mapTypes(target, transform) as ClassName).parameterizedBy(typeArguments.map { it.mapTypes(target, transform) }) + .copy(nullable = isNullable, annotations = annotations) + } + is TypeVariableName -> { + copy(bounds = bounds.map { it.mapTypes(target, transform) }) + } + is WildcardTypeName -> { + // TODO Would be nice if KotlinPoet modeled these easier. + // Producer type - empty inTypes, single element outTypes + // Consumer type - single element inTypes, single ANY element outType. + when { + this == STAR -> this + outTypes.isNotEmpty() && inTypes.isEmpty() -> { + WildcardTypeName.producerOf(outTypes[0].mapTypes(target, transform)) + .copy(nullable = isNullable, annotations = annotations) + } + inTypes.isNotEmpty() -> { + WildcardTypeName.consumerOf(inTypes[0].mapTypes(target, transform)) + .copy(nullable = isNullable, annotations = annotations) + } + else -> throw UnsupportedOperationException("Not possible.") + } + } + else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.") + } +} + +internal fun TypeName.stripTypeVarVariance(): TypeName { + return mapTypes { + TypeVariableName(name = name, bounds = bounds.map { it.mapTypes(TypeVariableName::stripTypeVarVariance) }, variance = null) + } +} \ No newline at end of file diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt index 1f4e9ac..21d6fe6 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt +++ b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt @@ -48,6 +48,7 @@ import com.squareup.moshi.kotlin.codegen.api.TargetConstructor import com.squareup.moshi.kotlin.codegen.api.TargetParameter import com.squareup.moshi.kotlin.codegen.api.TargetProperty import com.squareup.moshi.kotlin.codegen.api.TargetType +import com.squareup.moshi.kotlin.codegen.api.mapTypes import java.lang.annotation.ElementType import java.lang.annotation.Retention import java.lang.annotation.RetentionPolicy @@ -59,6 +60,7 @@ import javax.lang.model.element.TypeElement import javax.lang.model.util.Elements import javax.lang.model.util.Types import javax.tools.Diagnostic +import kotlin.reflect.KClass private val JSON_QUALIFIER = JsonQualifier::class.java private val JSON = Json::class.asClassName() @@ -320,46 +322,19 @@ private fun String.escapeDollarSigns(): String { return replace("\$", "\${\'\$\'}") } -private fun TypeName.unwrapTypeAlias(): TypeName { - return when (this) { - is ClassName -> { - tag()?.type?.let { unwrappedType -> - // If any type is nullable, then the whole thing is nullable - var isAnyNullable = isNullable - // Keep track of all annotations across type levels. Sort them too for consistency. - val runningAnnotations = TreeSet(compareBy { it.toString() }).apply { - addAll(annotations) - } - val nestedUnwrappedType = unwrappedType.unwrapTypeAlias() - runningAnnotations.addAll(nestedUnwrappedType.annotations) - isAnyNullable = isAnyNullable || nestedUnwrappedType.isNullable - nestedUnwrappedType.copy(nullable = isAnyNullable, annotations = runningAnnotations.toList()) - } ?: this - } - is ParameterizedTypeName -> { - return (rawType.unwrapTypeAlias() as ClassName).parameterizedBy(typeArguments.map { it.unwrapTypeAlias() }) - .copy(nullable = isNullable, annotations = annotations) - } - is TypeVariableName -> { - return copy(bounds = bounds.map { it.unwrapTypeAlias() }) - } - is WildcardTypeName -> { - // TODO Would be nice if KotlinPoet modeled these easier. - // Producer type - empty inTypes, single element outTypes - // Consumer type - single element inTypes, single ANY element outType. - return when { - this == STAR -> this - outTypes.isNotEmpty() && inTypes.isEmpty() -> { - WildcardTypeName.producerOf(outTypes[0].unwrapTypeAlias()) - .copy(nullable = isNullable, annotations = annotations) - } - inTypes.isNotEmpty() -> { - WildcardTypeName.consumerOf(inTypes[0].unwrapTypeAlias()) - .copy(nullable = isNullable, annotations = annotations) - } - else -> throw UnsupportedOperationException("Not possible.") +internal fun TypeName.unwrapTypeAlias(): TypeName { + return mapTypes { + tag()?.type?.let { unwrappedType -> + // If any type is nullable, then the whole thing is nullable + var isAnyNullable = isNullable + // Keep track of all annotations across type levels. Sort them too for consistency. + val runningAnnotations = TreeSet(compareBy { it.toString() }).apply { + addAll(annotations) } + val nestedUnwrappedType = unwrappedType.unwrapTypeAlias() + runningAnnotations.addAll(nestedUnwrappedType.annotations) + isAnyNullable = isAnyNullable || nestedUnwrappedType.isNullable + nestedUnwrappedType.copy(nullable = isAnyNullable, annotations = runningAnnotations.toList()) } - else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.") } } diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt index 05dd50a..0e44eed 100644 --- a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt +++ b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt @@ -522,6 +522,24 @@ class DualKotlinTest(useReflection: Boolean) { val convolutedMultiNullableShouldBeNullable: NullableB?, val deepNestedNullableShouldBeNullable: E ) + + // Regression test for https://github.com/square/moshi/issues/1009 + @Test fun outDeclaration() { + val adapter = moshi.adapter>() + + @Language("JSON") + val testJson = """{"input":3}""" + + val instance = OutDeclaration(3) + assertThat(adapter.serializeNulls().toJson(instance)) + .isEqualTo(testJson) + + val result = adapter.fromJson(testJson)!! + assertThat(result).isEqualTo(instance) + } + + @JsonClass(generateAdapter = true) + data class OutDeclaration(val input: T) } typealias TypeAlias = Int diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/reflect/-MoshiKotlinExtensions.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/reflect/-MoshiKotlinExtensions.kt index ba67a99..9983846 100644 --- a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/reflect/-MoshiKotlinExtensions.kt +++ b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/reflect/-MoshiKotlinExtensions.kt @@ -57,12 +57,16 @@ fun Moshi.adapter(ktype: KType): JsonAdapter { } @PublishedApi -internal fun KType.toType(): Type { +internal fun KType.toType(allowPrimitives: Boolean = true): Type { classifier?.let { when (it) { is KTypeParameter -> throw IllegalArgumentException("Type parameters are not supported") is KClass<*> -> { - val javaType = it.java + val javaType = if (allowPrimitives) { + it.java + } else { + it.javaObjectType + } if (javaType.isArray) { return Types.arrayOf(javaType.componentType) } @@ -88,7 +92,7 @@ internal fun KType.toType(): Type { } internal fun KTypeProjection.toType(): Type { - val javaType = type?.toType() ?: return Any::class.java + val javaType = type?.toType(allowPrimitives = false) ?: return Any::class.java return when (variance) { null -> Any::class.java KVariance.INVARIANT -> javaType