Fix support for classes w/ multiple constructors in code gen (#976)

* Fix broken test

This test suite doesn't run on CI builds but fails locally since the method was moved

* Add multiple constructors test case

* Implement TypeName.asTypeBlock()

* Make DEFAULT_CONSTRUCTOR_MARKER public

* Look up constructor via getDeclaredConstructor with exact param types

Resolves #975

* Remove dead code
This commit is contained in:
Zac Sweers
2019-10-30 00:39:08 -04:00
committed by GitHub
parent 6acebfaca6
commit 4f1c8a5eda
5 changed files with 82 additions and 10 deletions

View File

@@ -20,6 +20,7 @@ import com.squareup.kotlinpoet.AnnotationSpec
import com.squareup.kotlinpoet.CodeBlock import com.squareup.kotlinpoet.CodeBlock
import com.squareup.kotlinpoet.FileSpec import com.squareup.kotlinpoet.FileSpec
import com.squareup.kotlinpoet.FunSpec import com.squareup.kotlinpoet.FunSpec
import com.squareup.kotlinpoet.INT
import com.squareup.kotlinpoet.KModifier import com.squareup.kotlinpoet.KModifier
import com.squareup.kotlinpoet.MemberName import com.squareup.kotlinpoet.MemberName
import com.squareup.kotlinpoet.NameAllocator import com.squareup.kotlinpoet.NameAllocator
@@ -31,6 +32,7 @@ import com.squareup.kotlinpoet.TypeSpec
import com.squareup.kotlinpoet.TypeVariableName import com.squareup.kotlinpoet.TypeVariableName
import com.squareup.kotlinpoet.asClassName import com.squareup.kotlinpoet.asClassName
import com.squareup.kotlinpoet.asTypeName import com.squareup.kotlinpoet.asTypeName
import com.squareup.kotlinpoet.joinToCode
import com.squareup.moshi.JsonAdapter import com.squareup.moshi.JsonAdapter
import com.squareup.moshi.JsonReader import com.squareup.moshi.JsonReader
import com.squareup.moshi.JsonWriter import com.squareup.moshi.JsonWriter
@@ -48,6 +50,14 @@ internal class AdapterGenerator(
target: TargetType, target: TargetType,
private val propertyList: List<PropertyGenerator> private val propertyList: List<PropertyGenerator>
) { ) {
companion object {
private val DEFAULT_CONSTRUCTOR_EXTRA_PARAMS = arrayOf(
CodeBlock.of("%T::class.javaPrimitiveType", INT),
CodeBlock.of("%T.DEFAULT_CONSTRUCTOR_MARKER", Util::class)
)
}
private val nonTransientProperties = propertyList.filterNot { it.isTransient } private val nonTransientProperties = propertyList.filterNot { it.isTransient }
private val className = target.typeName.rawType() private val className = target.typeName.rawType()
private val visibility = target.visibility private val visibility = target.visibility
@@ -56,6 +66,7 @@ internal class AdapterGenerator(
private val nameAllocator = NameAllocator() private val nameAllocator = NameAllocator()
private val adapterName = "${className.simpleNames.joinToString(separator = "_")}JsonAdapter" private val adapterName = "${className.simpleNames.joinToString(separator = "_")}JsonAdapter"
private val originalTypeName = target.typeName private val originalTypeName = target.typeName
private val originalRawTypeName = originalTypeName.rawType()
private val moshiParam = ParameterSpec.builder( private val moshiParam = ParameterSpec.builder(
nameAllocator.newName("moshi"), nameAllocator.newName("moshi"),
@@ -158,7 +169,7 @@ internal class AdapterGenerator(
} }
private fun generateToStringFun(): FunSpec { private fun generateToStringFun(): FunSpec {
val name = originalTypeName.rawType().simpleNames.joinToString(".") val name = originalRawTypeName.simpleNames.joinToString(".")
val size = TO_STRING_SIZE_BASE + name.length val size = TO_STRING_SIZE_BASE + name.length
return FunSpec.builder("toString") return FunSpec.builder("toString")
.addModifiers(KModifier.OVERRIDE) .addModifiers(KModifier.OVERRIDE)
@@ -201,10 +212,12 @@ internal class AdapterGenerator(
// JsonReader.Options. // JsonReader.Options.
var propertyIndex = 0 var propertyIndex = 0
var maskIndex = 0 var maskIndex = 0
val constructorPropertyTypes = mutableListOf<CodeBlock>()
for (property in propertyList) { for (property in propertyList) {
if (property.isTransient) { if (property.isTransient) {
if (property.hasConstructorParameter) { if (property.hasConstructorParameter) {
maskIndex++ maskIndex++
constructorPropertyTypes += property.target.type.asTypeBlock()
} }
continue continue
} }
@@ -238,6 +251,9 @@ internal class AdapterGenerator(
exception) exception)
} }
} }
if (property.hasConstructorParameter) {
constructorPropertyTypes += property.target.type.asTypeBlock()
}
propertyIndex++ propertyIndex++
maskIndex++ maskIndex++
} }
@@ -267,12 +283,13 @@ internal class AdapterGenerator(
if (useDefaultsConstructor) { if (useDefaultsConstructor) {
classBuilder.addProperty(constructorProperty) classBuilder.addProperty(constructorProperty)
// Dynamic default constructor call // Dynamic default constructor call
val rawOriginalTypeName = originalTypeName.rawType()
val nonNullConstructorType = constructorProperty.type.copy(nullable = false) val nonNullConstructorType = constructorProperty.type.copy(nullable = false)
val args = constructorPropertyTypes.plus(DEFAULT_CONSTRUCTOR_EXTRA_PARAMS)
.joinToCode(", ")
val coreLookupBlock = CodeBlock.of( val coreLookupBlock = CodeBlock.of(
"%T.lookupDefaultsConstructor(%T::class.java)", "%T::class.java.getDeclaredConstructor(%L)",
MOSHI_UTIL, originalRawTypeName,
rawOriginalTypeName args
) )
val lookupBlock = if (originalTypeName is ParameterizedTypeName) { val lookupBlock = if (originalTypeName is ParameterizedTypeName) {
CodeBlock.of("(%L·as·%T)", coreLookupBlock, nonNullConstructorType) CodeBlock.of("(%L·as·%T)", coreLookupBlock, nonNullConstructorType)
@@ -280,7 +297,7 @@ internal class AdapterGenerator(
coreLookupBlock coreLookupBlock
} }
val initializerBlock = CodeBlock.of( val initializerBlock = CodeBlock.of(
"this.%1N ?:·%2L.also·{ this.%1N·= it }", "this.%1N·?: %2L.also·{ this.%1N·= it }",
constructorProperty, constructorProperty,
lookupBlock lookupBlock
) )
@@ -310,7 +327,7 @@ internal class AdapterGenerator(
// We have to use the default primitive for the available type in order for // We have to use the default primitive for the available type in order for
// invokeDefaultConstructor to properly invoke it. Just using "null" isn't safe because // invokeDefaultConstructor to properly invoke it. Just using "null" isn't safe because
// the transient type may be a primitive type. // the transient type may be a primitive type.
result.addCode(property.target.type.defaultPrimitiveValue()) result.addCode(property.target.type.rawType().defaultPrimitiveValue())
} else { } else {
result.addCode("%N", property.localName) result.addCode("%N", property.localName)
} }

View File

@@ -15,6 +15,8 @@
*/ */
package com.squareup.moshi.kotlin.codegen.api package com.squareup.moshi.kotlin.codegen.api
import com.squareup.kotlinpoet.ANY
import com.squareup.kotlinpoet.ARRAY
import com.squareup.kotlinpoet.BOOLEAN import com.squareup.kotlinpoet.BOOLEAN
import com.squareup.kotlinpoet.BYTE import com.squareup.kotlinpoet.BYTE
import com.squareup.kotlinpoet.CHAR import com.squareup.kotlinpoet.CHAR
@@ -25,9 +27,11 @@ import com.squareup.kotlinpoet.FLOAT
import com.squareup.kotlinpoet.INT import com.squareup.kotlinpoet.INT
import com.squareup.kotlinpoet.KModifier import com.squareup.kotlinpoet.KModifier
import com.squareup.kotlinpoet.LONG import com.squareup.kotlinpoet.LONG
import com.squareup.kotlinpoet.NOTHING
import com.squareup.kotlinpoet.ParameterizedTypeName import com.squareup.kotlinpoet.ParameterizedTypeName
import com.squareup.kotlinpoet.SHORT import com.squareup.kotlinpoet.SHORT
import com.squareup.kotlinpoet.TypeName import com.squareup.kotlinpoet.TypeName
import com.squareup.kotlinpoet.TypeVariableName
import com.squareup.kotlinpoet.UNIT import com.squareup.kotlinpoet.UNIT
import com.squareup.kotlinpoet.asTypeName import com.squareup.kotlinpoet.asTypeName
@@ -49,10 +53,41 @@ internal fun TypeName.defaultPrimitiveValue(): CodeBlock =
FLOAT -> CodeBlock.of("0f") FLOAT -> CodeBlock.of("0f")
LONG -> CodeBlock.of("0L") LONG -> CodeBlock.of("0L")
DOUBLE -> CodeBlock.of("0.0") DOUBLE -> CodeBlock.of("0.0")
UNIT, Void::class.asTypeName() -> throw IllegalStateException("Parameter with void or Unit type is illegal") UNIT, Void::class.asTypeName(), NOTHING -> throw IllegalStateException("Parameter with void, Unit, or Nothing type is illegal")
else -> CodeBlock.of("null") else -> CodeBlock.of("null")
} }
internal fun TypeName.asTypeBlock(): CodeBlock {
when (this) {
is ParameterizedTypeName -> {
return if (rawType == ARRAY) {
CodeBlock.of("%T::class.java", copy(nullable = false))
} else {
rawType.asTypeBlock()
}
}
is TypeVariableName -> {
val bound = bounds.firstOrNull() ?: ANY
return bound.asTypeBlock()
}
is ClassName -> {
return when (this) {
BOOLEAN, CHAR, BYTE, SHORT, INT, FLOAT, LONG, DOUBLE -> {
if (isNullable) {
// Remove nullable but keep the java object type
CodeBlock.of("%T::class.javaObjectType", copy(nullable = false))
} else {
CodeBlock.of("%T::class.javaPrimitiveType", this)
}
}
UNIT, Void::class.asTypeName(), NOTHING -> throw IllegalStateException("Parameter with void, Unit, or Nothing type is illegal")
else -> CodeBlock.of("%T::class.java", copy(nullable = false))
}
}
else -> throw UnsupportedOperationException("Parameter with type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, or type variables are allowed.")
}
}
internal fun KModifier.checkIsVisibility() { internal fun KModifier.checkIsVisibility() {
require(ordinal <= ordinal) { require(ordinal <= ordinal) {
"Visibility must be one of ${(0..ordinal).joinToString { KModifier.values()[it].name }}. Is $name" "Visibility must be one of ${(0..ordinal).joinToString { KModifier.values()[it].name }}. Is $name"

View File

@@ -15,7 +15,7 @@ class KotlinJsonAdapterTest {
val moshi = Moshi.Builder() val moshi = Moshi.Builder()
.add(KotlinJsonAdapterFactory()) .add(KotlinJsonAdapterFactory())
.build() .build()
val adapter = moshi.adapter<Data>() val adapter = moshi.adapter(Data::class.java)
assertThat(adapter.toString()).isEqualTo( assertThat(adapter.toString()).isEqualTo(
"KotlinJsonAdapter(com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterTest.Data).nullSafe()" "KotlinJsonAdapter(com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterTest.Data).nullSafe()"
) )

View File

@@ -322,6 +322,26 @@ class DualKotlinTest(useReflection: Boolean) {
@JsonClass(generateAdapter = true) @JsonClass(generateAdapter = true)
class InternalAbstractProperty(override val test: String) : InternalAbstractPropertyBase() class InternalAbstractProperty(override val test: String) : InternalAbstractPropertyBase()
// Regression test for https://github.com/square/moshi/issues/975
@Test fun multipleConstructors() {
val adapter = moshi.adapter<MultipleConstructorsB>()
assertThat(adapter.toJson(MultipleConstructorsB(6))).isEqualTo("""{"f":{"f":6},"b":6}""")
@Language("JSON")
val testJson = """{"b":6}"""
val result = adapter.fromJson(testJson)!!
assertThat(result.b).isEqualTo(6)
}
@JsonClass(generateAdapter = true)
class MultipleConstructorsA(val f: Int)
@JsonClass(generateAdapter = true)
class MultipleConstructorsB(val f: MultipleConstructorsA = MultipleConstructorsA(5), val b: Int) {
constructor(f: Int, b: Int = 6): this(MultipleConstructorsA(f), b)
}
} }
// Has to be outside since inline classes are only allowed on top level // Has to be outside since inline classes are only allowed on top level

View File

@@ -47,7 +47,7 @@ import static com.squareup.moshi.Types.supertypeOf;
public final class Util { public final class Util {
public static final Set<Annotation> NO_ANNOTATIONS = Collections.emptySet(); public static final Set<Annotation> NO_ANNOTATIONS = Collections.emptySet();
public static final Type[] EMPTY_TYPE_ARRAY = new Type[] {}; public static final Type[] EMPTY_TYPE_ARRAY = new Type[] {};
@Nullable private static final Class<?> DEFAULT_CONSTRUCTOR_MARKER; @Nullable public static final Class<?> DEFAULT_CONSTRUCTOR_MARKER;
@Nullable private static final Class<? extends Annotation> METADATA; @Nullable private static final Class<? extends Annotation> METADATA;
static { static {