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 9167ce7..5b3dd37 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 @@ -30,7 +30,6 @@ import com.squareup.kotlinpoet.TypeSpec import com.squareup.kotlinpoet.TypeVariableName import com.squareup.kotlinpoet.asClassName import com.squareup.kotlinpoet.asTypeName -import com.squareup.kotlinpoet.joinToCode import com.squareup.moshi.JsonAdapter import com.squareup.moshi.JsonReader import com.squareup.moshi.JsonWriter @@ -184,13 +183,29 @@ internal class AdapterGenerator( } } + val maskName = nameAllocator.newName("mask") + val useConstructorDefaults = nonTransientProperties.any { it.hasConstructorDefault } + if (useConstructorDefaults) { + result.addStatement("var %L = -1", maskName) + } result.addStatement("%N.beginObject()", readerParam) result.beginControlFlow("while (%N.hasNext())", readerParam) result.beginControlFlow("when (%N.selectName(%N))", readerParam, optionsProperty) - nonTransientProperties.forEachIndexed { index, property -> - if (property.hasLocalIsPresentName) { - result.beginControlFlow("%L -> ", index) + // We track property index and mask index separately, because mask index is based on _all_ + // constructor arguments, while property index is only based on the index passed into + // JsonReader.Options. + var propertyIndex = 0 + var maskIndex = 0 + for (property in propertyList) { + if (property.isTransient) { + if (property.hasConstructorParameter) { + maskIndex++ + } + continue + } + if (property.hasLocalIsPresentName || property.hasConstructorDefault) { + result.beginControlFlow("%L ->", propertyIndex) if (property.delegateKey.nullable) { result.addStatement("%N = %N.fromJson(%N)", property.localName, nameAllocator[property.delegateKey], readerParam) @@ -199,19 +214,28 @@ internal class AdapterGenerator( result.addStatement("%N = %N.fromJson(%N) ?: throw·%L", property.localName, nameAllocator[property.delegateKey], readerParam, exception) } - result.addStatement("%N = true", property.localIsPresentName) + if (property.hasConstructorDefault) { + val inverted = (1 shl maskIndex).inv() + result.addComment("\$mask = \$mask and (1 shl %L).inv()", maskIndex) + result.addStatement("%1L = %1L and %2L", maskName, inverted) + } else { + // Presence tracker for a mutable property + result.addStatement("%N = true", property.localIsPresentName) + } result.endControlFlow() } else { if (property.delegateKey.nullable) { result.addStatement("%L -> %N = %N.fromJson(%N)", - index, property.localName, nameAllocator[property.delegateKey], readerParam) + propertyIndex, property.localName, nameAllocator[property.delegateKey], readerParam) } else { val exception = unexpectedNull(property.localName, readerParam) result.addStatement("%L -> %N = %N.fromJson(%N) ?: throw·%L", - index, property.localName, nameAllocator[property.delegateKey], readerParam, + propertyIndex, property.localName, nameAllocator[property.delegateKey], readerParam, exception) } } + propertyIndex++ + maskIndex++ } result.beginControlFlow("-1 ->") @@ -236,18 +260,10 @@ internal class AdapterGenerator( } else { CodeBlock.of("return·") } - val maskName = nameAllocator.newName("mask") val localConstructorName = nameAllocator.newName("localConstructor") if (useDefaultsConstructor) { classBuilder.addProperty(constructorProperty) // Dynamic default constructor call - val booleanArrayBlock = parameterProperties.map { param -> - when { - param.isTransient -> CodeBlock.of("false") - param.hasLocalIsPresentName -> CodeBlock.of(param.localIsPresentName) - else -> CodeBlock.of("true") - } - }.joinToCode(", ") result.addStatement( "val %1L·= this.%2N ?: %3T.lookupDefaultsConstructor(%4T::class.java).also·{ this.%2N·= it }", localConstructorName, @@ -255,15 +271,10 @@ internal class AdapterGenerator( MOSHI_UTIL, originalTypeName ) - result.addStatement("val %L = %T.createDefaultValuesParametersMask(%L)", - maskName, MOSHI_UTIL, booleanArrayBlock) result.addCode( - "«%L%T.invokeDefaultConstructor(%T::class.java, %L, %L, ", + "«%L%L.newInstance(", returnOrResultAssignment, - MOSHI_UTIL, - originalTypeName, - localConstructorName, - maskName + localConstructorName ) } else { // Standard constructor call @@ -292,6 +303,11 @@ internal class AdapterGenerator( separator = ",\n" } + if (useDefaultsConstructor) { + // Add the mask and a null instance for the trailing default marker instance + result.addCode(",\n%L,\nnull", maskName) + } + result.addCode("\n»)\n") // Assign properties not present in the constructor. @@ -299,7 +315,7 @@ internal class AdapterGenerator( if (property.hasConstructorParameter) { continue // Property already handled. } - if (property.differentiateAbsentFromNull) { + if (property.hasLocalIsPresentName) { result.addStatement("%1N.%2N = if (%3N) %4N else %1N.%2N", resultName, property.name, property.localIsPresentName, property.localName) } else { diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt index 46d6f20..13f46c4 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt +++ b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/PropertyGenerator.kt @@ -36,18 +36,18 @@ internal class PropertyGenerator( val hasConstructorParameter get() = target.parameterIndex != -1 - /** We prefer to use 'null' to mean absent, but for some properties those are distinct. */ - val differentiateAbsentFromNull get() = delegateKey.nullable && hasDefault - /** * IsPresent is required if the following conditions are met: * - Is not transient - * - Has a default and one of the below - * - Is a constructor property - * - Is a nullable non-constructor property + * - Has a default + * - Is not a constructor parameter (for constructors we use a defaults mask) + * - Is nullable (because we differentiate absent from null) + * + * This is used to indicate that presence should be checked first before possible assigning null + * to an absent value */ - val hasLocalIsPresentName = !isTransient && hasDefault && - (hasConstructorParameter || delegateKey.nullable) + val hasLocalIsPresentName = !isTransient && hasDefault && !hasConstructorParameter && delegateKey.nullable + val hasConstructorDefault = hasDefault && hasConstructorParameter fun allocateNames(nameAllocator: NameAllocator) { localName = nameAllocator.newName(name) @@ -58,7 +58,10 @@ internal class PropertyGenerator( return PropertySpec.builder(localName, target.type.copy(nullable = true)) .mutable(true) .apply { - if (hasLocalIsPresentName) { + if (hasConstructorDefault) { + // We default to the primitive default type, as reflectively invoking the constructor + // without this (even though it's a throwaway) will fail argument type resolution in + // the reflective invocation. initializer(target.type.defaultPrimitiveValue()) } else { initializer("null") diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DefaultConstructorTest.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DefaultConstructorTest.kt index e9e4d37..9f95a36 100644 --- a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DefaultConstructorTest.kt +++ b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DefaultConstructorTest.kt @@ -2,45 +2,11 @@ package com.squareup.moshi.kotlin import com.squareup.moshi.JsonClass import com.squareup.moshi.Moshi -import com.squareup.moshi.internal.Util import org.junit.Test class DefaultConstructorTest { @Test fun minimal() { - val expected = TestClass("requiredClass") - val args = arrayOf("requiredClass", null, 0, null, 0, 0) - val mask = Util.createDefaultValuesParametersMask(true, false, false, false, false, false) - val constructor = Util.lookupDefaultsConstructor(TestClass::class.java) - val instance = Util.invokeDefaultConstructor(TestClass::class.java, constructor, mask, *args) - check(instance == expected) { - "No match:\nActual : $instance\nExpected: $expected" - } - } - - @Test fun allSet() { - val expected = TestClass("requiredClass", "customOptional", 4, "setDynamic", 5, 6) - val args = arrayOf("requiredClass", "customOptional", 4, "setDynamic", 5, 6) - val mask = Util.createDefaultValuesParametersMask(true, true, true, true, true, true) - val constructor = Util.lookupDefaultsConstructor(TestClass::class.java) - val instance = Util.invokeDefaultConstructor(TestClass::class.java, constructor, mask, *args) - check(instance == expected) { - "No match:\nActual : $instance\nExpected: $expected" - } - } - - @Test fun customDynamic() { - val expected = TestClass("requiredClass", "customOptional") - val args = arrayOf("requiredClass", "customOptional", 0, null, 0, 0) - val mask = Util.createDefaultValuesParametersMask(true, true, false, false, false, false) - val constructor = Util.lookupDefaultsConstructor(TestClass::class.java) - val instance = Util.invokeDefaultConstructor(TestClass::class.java, constructor, mask, *args) - check(instance == expected) { - "No match:\nActual : $instance\nExpected: $expected" - } - } - - @Test fun minimal_codeGen() { val expected = TestClass("requiredClass") val json = """{"required":"requiredClass"}""" val instance = Moshi.Builder().build().adapter(TestClass::class.java) @@ -50,7 +16,7 @@ class DefaultConstructorTest { } } - @Test fun allSet_codeGen() { + @Test fun allSet() { val expected = TestClass("requiredClass", "customOptional", 4, "setDynamic", 5, 6) val json = """{"required":"requiredClass","optional":"customOptional","optional2":4,"dynamicSelfReferenceOptional":"setDynamic","dynamicOptional":5,"dynamicInlineOptional":6}""" val instance = Moshi.Builder().build().adapter(TestClass::class.java) @@ -60,7 +26,7 @@ class DefaultConstructorTest { } } - @Test fun customDynamic_codeGen() { + @Test fun customDynamic() { val expected = TestClass("requiredClass", "customOptional") val json = """{"required":"requiredClass","optional":"customOptional"}""" val instance = Moshi.Builder().build().adapter(TestClass::class.java) diff --git a/moshi/src/main/java/com/squareup/moshi/internal/Util.java b/moshi/src/main/java/com/squareup/moshi/internal/Util.java index 574332b..3f1c866 100644 --- a/moshi/src/main/java/com/squareup/moshi/internal/Util.java +++ b/moshi/src/main/java/com/squareup/moshi/internal/Util.java @@ -543,7 +543,6 @@ public final class Util { * @param targetClass the target kotlin class to instantiate. * @param the type of {@code targetClass}. * @return the instantiated {@code targetClass} instance. - * @see #createDefaultValuesParametersMask(boolean...) */ public static Constructor lookupDefaultsConstructor(Class targetClass) { if (DEFAULT_CONSTRUCTOR_MARKER == null) { @@ -555,42 +554,6 @@ public final class Util { return defaultConstructor; } - /** - * Reflectively invokes the defaults constructor of a kotlin class. This allows indicating which - * arguments are "set" or not, and thus recreate the behavior of named a arguments invocation - * dynamically. - * - * @param targetClass the target kotlin class to instantiate. - * @param defaultsConstructor the target class's defaults constructor in kotlin invoke. - * @param mask an int mask indicating which {@code args} are present. - * @param args the constructor arguments, including "unset" values (set to null or the primitive - * default). - * @param the type of {@code targetClass}. - * @return the instantiated {@code targetClass} instance. - * @see #createDefaultValuesParametersMask(boolean...) - */ - public static T invokeDefaultConstructor( - Class targetClass, - Constructor defaultsConstructor, - int mask, - Object... args) { - Object[] finalArgs = Arrays.copyOf(args, args.length + 2); - finalArgs[finalArgs.length - 2] = mask; - finalArgs[finalArgs.length - 1] = null; // DefaultConstructorMarker param - try { - return defaultsConstructor.newInstance(finalArgs); - } catch (InstantiationException e) { - throw new IllegalStateException("Could not instantiate instance of " + targetClass); - } catch (IllegalAccessException e) { - throw new IllegalStateException("Could not access defaults constructor of " + targetClass); - } catch (InvocationTargetException e) { - Throwable cause = e.getCause(); - if (cause instanceof RuntimeException) throw (RuntimeException) cause; - if (cause instanceof Error) throw (Error) cause; - throw new RuntimeException("Could not invoke defaults constructor of " + targetClass, cause); - } - } - private static Constructor findConstructor(Class targetClass) { for (Constructor constructor : targetClass.getDeclaredConstructors()) { Class[] paramTypes = constructor.getParameterTypes(); @@ -604,26 +567,6 @@ public final class Util { throw new IllegalStateException("No defaults constructor found for " + targetClass); } - /** - * Creates an mask with bits set to indicate which indices of a default constructor's parameters - * are set. - * - * @param argPresentValues vararg of all present values (set or unset). Max allowable size is 32. - * @return the created mask. - */ - public static int createDefaultValuesParametersMask(boolean... argPresentValues) { - if (argPresentValues.length > 32) { - throw new IllegalArgumentException("Arg present values exceeds max allowable 32."); - } - int mask = 0; - for (int i = 0; i < argPresentValues.length; ++i) { - if (!argPresentValues[i]) { - mask = mask | (1 << i); - } - } - return mask; - } - public static JsonDataException missingProperty(String property, JsonReader reader) { return jsonDataException(REQUIRED_PROPERTY_TEMPLATE, property, reader); }