From 04f414f600c0184150cfa6b66e22ee5f8a573c32 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 18 Jan 2020 22:46:54 -0500 Subject: [PATCH 1/6] Add ComplexGenericsInheritanceTest --- .../codegen/ComplexGenericsInheritanceTest.kt | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt new file mode 100644 index 0000000..a69f7d0 --- /dev/null +++ b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt @@ -0,0 +1,132 @@ +@file:Suppress("UNUSED", "UNUSED_PARAMETER") + +package com.squareup.moshi.kotlin.codegen + +import com.squareup.moshi.JsonClass +import com.squareup.moshi.Moshi +import com.squareup.moshi.kotlin.reflect.adapter +import org.assertj.core.api.Assertions.assertThat +import org.intellij.lang.annotations.Language +import org.junit.Test + +@ExperimentalStdlibApi +class ComplexGenericsInheritanceTest { + + private val moshi = Moshi.Builder().build() + + @Test + fun simple() { + val adapter = moshi.adapter() + + @Language("JSON") + val json = """{"data":{"name":"foo"},"data2":"bar","data3":"baz"}""" + + val instance = adapter.fromJson(json)!! + val testInstance = PersonResponse().apply { + data = Person("foo") + } + assertThat(instance).isEqualTo(testInstance) + assertThat(adapter.toJson(instance)).isEqualTo(json) + } + + @Test + fun nested() { + val adapter = moshi.adapter() + + @Language("JSON") + val json = """{"data":{"name":"foo"},"data2":"bar","data3":"baz"}""" + + val instance = adapter.fromJson(json)!! + val testInstance = NestedPersonResponse().apply { + data = Person("foo") + } + assertThat(instance).isEqualTo(testInstance) + assertThat(adapter.toJson(instance)).isEqualTo(json) + } + + @Test + fun untyped() { + val adapter = moshi.adapter>() + + @Language("JSON") + val json = """{"data":{"name":"foo"},"data2":"bar","data3":"baz"}""" + + val instance = adapter.fromJson(json)!! + val testInstance = UntypedNestedPersonResponse().apply { + data = Person("foo") + } + assertThat(instance).isEqualTo(testInstance) + assertThat(adapter.toJson(instance)).isEqualTo(json) + } + + @Test + fun complex() { + val adapter = moshi.adapter>>() + + @Language("JSON") + val json = """{"layer4E":{"name":"layer4E"},"layer4F":{"data":{"name":"layer4F"},"data2":"layer4F","data3":"layer4F"},"layer3C":[1,2,3],"layer3D":"layer3D","layer2":"layer2","layer1":"layer1"}""" + + val instance = adapter.fromJson(json)!! + val testInstance = Layer4( + layer4E = Person("layer4E"), + layer4F = UntypedNestedPersonResponse().apply { + data = Person("layer4F") + data2 = "layer4F" + data3 = "layer4F" + } + ).apply { + layer3C = listOf(1, 2, 3) + layer3D = "layer3D" + layer2 = "layer2" + layer1 = "layer1" + } + assertThat(instance).isEqualTo(testInstance) + assertThat(adapter.toJson(testInstance)).isEqualTo(json) + } +} + +open class ResponseWithSettableProperty { + var data: T? = null + var data2: R? = null + var data3: R? = null +} + +interface Personable + +@JsonClass(generateAdapter = true) +data class Person(val name: String) : Personable + +@JsonClass(generateAdapter = true) +data class PersonResponse( + val extra: String? = null) : ResponseWithSettableProperty() + +abstract class NestedResponse : ResponseWithSettableProperty() + +@JsonClass(generateAdapter = true) +data class NestedPersonResponse(val extra: String? = null) : NestedResponse() + +@JsonClass(generateAdapter = true) +data class UntypedNestedPersonResponse( + val extra: String? = null +) : NestedResponse() + +interface LayerInterface + +abstract class Layer1 { + var layer1: A? = null +} + +abstract class Layer2 : Layer1(), LayerInterface { + var layer2: B? = null +} + +abstract class Layer3 : Layer2() { + var layer3C: C? = null + var layer3D: D? = null +} + +@JsonClass(generateAdapter = true) +data class Layer4( + val layer4E: E, + val layer4F: F? = null +) : Layer3, String>(), LayerInterface From a73b9324295fc3397db9e97d2d828ed2470721cd Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 18 Jan 2020 22:47:56 -0500 Subject: [PATCH 2/6] Always emit full types Effectively reverts https://github.com/square/moshi/pull/1041 --- .../squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt | 7 ++++--- 1 file changed, 4 insertions(+), 3 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 00214ac..2f93040 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 @@ -18,7 +18,6 @@ package com.squareup.moshi.kotlin.codegen.api import com.squareup.kotlinpoet.ARRAY import com.squareup.kotlinpoet.AnnotationSpec import com.squareup.kotlinpoet.CodeBlock -import com.squareup.kotlinpoet.CodeBlock.Companion import com.squareup.kotlinpoet.FileSpec import com.squareup.kotlinpoet.FunSpec import com.squareup.kotlinpoet.INT @@ -478,8 +477,10 @@ internal class AdapterGenerator( localConstructorProperty ) } else { - // Standard constructor call. Can omit generics as they're inferred - result.addCode("«%L%T(", returnOrResultAssignment, originalTypeName.rawType()) + // Standard constructor call. Don't omit generics for parameterized types even if they can be + // inferred, as calculating the right condition for inference exceeds the value gained from + // being less pedantic. + result.addCode("«%L%T(", returnOrResultAssignment, originalTypeName) } for (input in components.filterIsInstance()) { From 4e84451d6e3c465b47756f659e0e38320524ae1c Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 18 Jan 2020 22:52:18 -0500 Subject: [PATCH 3/6] Resolve type arguments correctly for supertype settable properties Resolves #1046 --- .../squareup/moshi/kotlin/codegen/metadata.kt | 114 +++++++++++++++++- 1 file changed, 109 insertions(+), 5 deletions(-) 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 87a2201..2a1e2b9 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 @@ -18,8 +18,10 @@ package com.squareup.moshi.kotlin.codegen import com.squareup.kotlinpoet.AnnotationSpec import com.squareup.kotlinpoet.ClassName import com.squareup.kotlinpoet.KModifier +import com.squareup.kotlinpoet.ParameterizedTypeName import com.squareup.kotlinpoet.TypeName import com.squareup.kotlinpoet.TypeSpec +import com.squareup.kotlinpoet.TypeVariableName import com.squareup.kotlinpoet.asClassName import com.squareup.kotlinpoet.asTypeName import com.squareup.kotlinpoet.metadata.ImmutableKmConstructor @@ -53,6 +55,7 @@ import javax.lang.model.element.AnnotationMirror import javax.lang.model.element.Element import javax.lang.model.element.ElementKind import javax.lang.model.element.TypeElement +import javax.lang.model.type.DeclaredType import javax.lang.model.util.Elements import javax.lang.model.util.Types import javax.tools.Diagnostic.Kind.ERROR @@ -192,6 +195,8 @@ internal fun targetType(messager: Messager, } val properties = mutableMapOf() + + val resolvedTypes = mutableListOf() val superTypes = appliedType.supertypes(types) .filterNot { supertype -> supertype.element.asClassName() == OBJECT_CLASS || // Don't load properties for java.lang.Object. @@ -207,15 +212,53 @@ internal fun targetType(messager: Messager, } .associateWithTo(LinkedHashMap()) { supertype -> // Load the kotlin API cache into memory eagerly so we can reuse the parsed APIs - if (supertype.element == element) { + val api = if (supertype.element == element) { // We've already parsed this api above, reuse it kotlinApi } else { cachedClassInspector.toTypeSpec(supertype.element) } + + val apiSuperClass = api.superclass + if (apiSuperClass is ParameterizedTypeName) { + // + // This extends a typed generic superclass. We want to construct a mapping of the + // superclass typevar names to their materialized types here. + // + // class Foo extends Bar + // class Bar + // + // We will store {Foo : {T : [String]}}. + // + // Then when we look at Bar later, we'll look up to the descendent Foo and extract its + // materialized type from there. + // + val superSuperClass = supertype.element.superclass as DeclaredType + + // Convert to an element and back to wipe the typed generics off of this + val untyped = superSuperClass.asElement().asType().asTypeName() as ParameterizedTypeName + resolvedTypes += ResolvedTypeMapping( + target = untyped.rawType, + args = untyped.typeArguments.asSequence() + .cast() + .map(TypeVariableName::name) + .zip(apiSuperClass.typeArguments.asSequence()) + .associate { it } + ) + } + + return@associateWithTo api } - for (supertypeApi in superTypes.values) { - val supertypeProperties = declaredProperties(constructor, supertypeApi) + + for ((localAppliedType, supertypeApi) in superTypes.entries) { + val appliedClassName = localAppliedType.element.asClassName() + val supertypeProperties = declaredProperties( + constructor = constructor, + kotlinApi = supertypeApi, + allowTypeVars = typeVariables.isNotEmpty(), + currentClass = appliedClassName, + resolvedTypes = resolvedTypes + ) for ((name, property) in supertypeProperties) { properties.putIfAbsent(name, property) } @@ -243,16 +286,70 @@ internal fun targetType(messager: Messager, visibility = resolvedVisibility) } +/** + * Represents a resolved raw class to type arguments where [args] are a map of the parent type var + * name to its resolved [TypeName]. + */ +private data class ResolvedTypeMapping(val target: ClassName, val args: Map) + +private fun resolveTypeArgs( + targetClass: ClassName, + propertyType: TypeName, + resolvedTypes: List, + allowTypeVars: Boolean, + entryStartIndex: Int = resolvedTypes.indexOfLast { it.target == targetClass } +): TypeName { + val unwrappedType = propertyType.unwrapTypeAlias() + + if (unwrappedType !is TypeVariableName) { + return unwrappedType + } else if (entryStartIndex == -1) { + return unwrappedType + } + + val targetMappingIndex = resolvedTypes[entryStartIndex] + val targetMappings = targetMappingIndex.args + + // Try to resolve the real type of this property based on mapped generics in the subclass. + // We need to us a non-nullable version for mapping since we're just mapping based on raw java + // type vars, but then can re-copy nullability back if it is found. + val resolvedType = targetMappings[unwrappedType.name] + ?.copy(nullable = unwrappedType.isNullable) + ?: unwrappedType + + return when { + resolvedType !is TypeVariableName -> resolvedType + entryStartIndex != 0 -> { + // We need to go deeper + resolveTypeArgs(targetClass, resolvedType, resolvedTypes, allowTypeVars, entryStartIndex - 1) + } + allowTypeVars -> { + // TODO this is ok but it would be nice to verify the typevar here is the one defined on our target class + resolvedType + } + else -> error("Could not find $resolvedType in $resolvedTypes") + } +} + /** Returns the properties declared by `typeElement`. */ @KotlinPoetMetadataPreview private fun declaredProperties( constructor: TargetConstructor, - kotlinApi: TypeSpec + kotlinApi: TypeSpec, + allowTypeVars: Boolean, + currentClass: ClassName, + resolvedTypes: List ): Map { val result = mutableMapOf() for (initialProperty in kotlinApi.propertySpecs) { - val property = initialProperty.toBuilder(type = initialProperty.type.unwrapTypeAlias()).build() + val resolvedType = resolveTypeArgs( + targetClass = currentClass, + propertyType = initialProperty.type, + resolvedTypes = resolvedTypes, + allowTypeVars = allowTypeVars + ) + val property = initialProperty.toBuilder(type = resolvedType).build() val name = property.name val parameter = constructor.parameters[name] result[name] = TargetProperty( @@ -380,3 +477,10 @@ internal val TypeElement.metadata: Metadata return getAnnotation(Metadata::class.java) ?: throw IllegalStateException("Not a kotlin type! $this") } + +private fun Sequence<*>.cast(): Sequence { + return map { + @Suppress("UNCHECKED_CAST") + it as E + } +} From 3f0d763ce1d472e2d0a95e415604d9a521ed8c7a Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 18 Jan 2020 23:08:44 -0500 Subject: [PATCH 4/6] Opportunistic - suppress RedundantExplicitType --- .../com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt | 3 +++ 1 file changed, 3 insertions(+) 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 2f93040..64272f6 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 @@ -73,6 +73,9 @@ internal class AdapterGenerator( // Because we generate redundant `out` variance for some generics and there's no way // for us to know when it's redundant. "REDUNDANT_PROJECTION", + // Because we may generate redundant explicit types for local vars with default values. + // Example: 'var fooSet: Boolean = false' + "RedundantExplicitType", // NameAllocator will just add underscores to differentiate names, which Kotlin doesn't // like for stylistic reasons. "LocalVariableName" From 76b4ec9f925dfba59cda27790b616eeb60b7847d Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 18 Jan 2020 23:10:40 -0500 Subject: [PATCH 5/6] Add another twist --- .../moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt index a69f7d0..a3a16f8 100644 --- a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt +++ b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/ComplexGenericsInheritanceTest.kt @@ -126,7 +126,7 @@ abstract class Layer3 : Layer2() { } @JsonClass(generateAdapter = true) -data class Layer4( +data class Layer4( val layer4E: E, val layer4F: F? = null ) : Layer3, String>(), LayerInterface From 1edcb77f76b04c8dc86dc3410c00dfaae22b3f75 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sat, 18 Jan 2020 23:11:17 -0500 Subject: [PATCH 6/6] Specify exactly which typevars are allowed to top level --- .../squareup/moshi/kotlin/codegen/metadata.kt | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) 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 2a1e2b9..08c630f 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 @@ -255,7 +255,7 @@ internal fun targetType(messager: Messager, val supertypeProperties = declaredProperties( constructor = constructor, kotlinApi = supertypeApi, - allowTypeVars = typeVariables.isNotEmpty(), + allowedTypeVars = typeVariables.toSet(), currentClass = appliedClassName, resolvedTypes = resolvedTypes ) @@ -296,7 +296,7 @@ private fun resolveTypeArgs( targetClass: ClassName, propertyType: TypeName, resolvedTypes: List, - allowTypeVars: Boolean, + allowedTypeVars: Set, entryStartIndex: Int = resolvedTypes.indexOfLast { it.target == targetClass } ): TypeName { val unwrappedType = propertyType.unwrapTypeAlias() @@ -321,13 +321,14 @@ private fun resolveTypeArgs( resolvedType !is TypeVariableName -> resolvedType entryStartIndex != 0 -> { // We need to go deeper - resolveTypeArgs(targetClass, resolvedType, resolvedTypes, allowTypeVars, entryStartIndex - 1) + resolveTypeArgs(targetClass, resolvedType, resolvedTypes, allowedTypeVars, entryStartIndex - 1) } - allowTypeVars -> { - // TODO this is ok but it would be nice to verify the typevar here is the one defined on our target class + resolvedType.copy(nullable = false) in allowedTypeVars -> { + // This is a generic type in the top-level declared class. This is fine to leave in because + // this will be handled by the `Type` array passed in at runtime. resolvedType } - else -> error("Could not find $resolvedType in $resolvedTypes") + else -> error("Could not find $resolvedType in $resolvedTypes. Also not present in allowable top-level type vars $allowedTypeVars") } } @@ -336,7 +337,7 @@ private fun resolveTypeArgs( private fun declaredProperties( constructor: TargetConstructor, kotlinApi: TypeSpec, - allowTypeVars: Boolean, + allowedTypeVars: Set, currentClass: ClassName, resolvedTypes: List ): Map { @@ -347,7 +348,7 @@ private fun declaredProperties( targetClass = currentClass, propertyType = initialProperty.type, resolvedTypes = resolvedTypes, - allowTypeVars = allowTypeVars + allowedTypeVars = allowedTypeVars ) val property = initialProperty.toBuilder(type = resolvedType).build() val name = property.name