Inline mask generation and constructor invocation (#908)

* Differentiate local naming from properties with constructor defaults

* Remove constructor invocation and mask creation methods

We're inlining all this

* Add explanatory comment for why we use the default primitive value

* Inline mask generation and constructor invocation to generated adapters

* Remove unused argument

Co-Authored-By: Jake Wharton <jakew@google.com>

* Compute masks directly during code gen

* Opportunistic: remove extraneous double space from control flow

* Just mask and invert directly

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 ->
+    // We track property index and mask index separately, because mask index is based on _all_
-      if (property.hasLocalIsPresentName) {
+    // constructor arguments, while property index is only based on the index passed into
-        result.beginControlFlow("%L -> ", index)
+    // 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,
+          localConstructorName
-          originalTypeName,
-          localConstructorName,
-          maskName
       )
     } 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 {

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
+   * - Has a default
-   *   - Is a constructor property
+   * - Is not a constructor parameter (for constructors we use a defaults mask)
-   *   - Is a nullable non-constructor property
+   * - 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 &&
+  val hasLocalIsPresentName = !isTransient && hasDefault && !hasConstructorParameter && delegateKey.nullable
-      (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")

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>(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>(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>(TestClass::class.java)

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 <T> the type of {@code targetClass}.
    * @return the instantiated {@code targetClass} instance.
-   * @see #createDefaultValuesParametersMask(boolean...)
    */
   public static <T> Constructor<T> lookupDefaultsConstructor(Class<T> 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 <T> the type of {@code targetClass}.
-   * @return the instantiated {@code targetClass} instance.
-   * @see #createDefaultValuesParametersMask(boolean...)
-   */
-  public static <T> T invokeDefaultConstructor(
-      Class<T> targetClass,
-      Constructor<T> 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 <T> Constructor<T> findConstructor(Class<T> 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);
   }