Handle non-property parameters in kotlin code gen (#982)

* Add parameter-only param in multi-masks test

* Add type to target parameters for later reference

* Ensure parameters are ordered

* Reword AdapterGenerator fromJson logic to use sealed FromJsonComponent

This allows us to handle parameter-only types separately (needed for mask index calculation). Resolves #979

* Add more tests

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/AdapterGenerator.kt

@@ -28,6 +28,7 @@ import com.squareup.kotlinpoet.ParameterSpec
 import com.squareup.kotlinpoet.ParameterizedTypeName
 import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy
 import com.squareup.kotlinpoet.PropertySpec
+import com.squareup.kotlinpoet.TypeName
 import com.squareup.kotlinpoet.TypeSpec
 import com.squareup.kotlinpoet.TypeVariableName
 import com.squareup.kotlinpoet.asClassName
@@ -38,6 +39,9 @@ import com.squareup.moshi.JsonReader
 import com.squareup.moshi.JsonWriter
 import com.squareup.moshi.Moshi
 import com.squareup.moshi.internal.Util
+import com.squareup.moshi.kotlin.codegen.api.FromJsonComponent.ParameterOnly
+import com.squareup.moshi.kotlin.codegen.api.FromJsonComponent.ParameterProperty
+import com.squareup.moshi.kotlin.codegen.api.FromJsonComponent.PropertyOnly
 import java.lang.reflect.Constructor
 import java.lang.reflect.Type
 
@@ -60,6 +64,8 @@ internal class AdapterGenerator(
   private val className = target.typeName.rawType()
   private val visibility = target.visibility
   private val typeVariables = target.typeVariables
+  private val targetConstructorParams = target.constructor.parameters
+      .mapKeys { (_, param) -> param.index }
 
   private val nameAllocator = NameAllocator()
   private val adapterName = "${className.simpleNames.joinToString(separator = "_")}JsonAdapter"
@@ -196,8 +202,34 @@ internal class AdapterGenerator(
       }
     }
 
+    val propertiesByIndex = propertyList.asSequence()
+        .filter { it.hasConstructorParameter }
+        .associateBy { it.target.parameterIndex }
+    val components = mutableListOf<FromJsonComponent>()
+
+    // Add parameters (± properties) first, their index matters
+    for ((index, parameter) in targetConstructorParams) {
+      val property = propertiesByIndex[index]
+      if (property == null) {
+        components += ParameterOnly(parameter)
+      } else {
+        components += ParameterProperty(parameter, property)
+      }
+    }
+
+    // Now add the remaining properties that aren't parameters
+    for (property in propertyList) {
+      if (property.target.parameterIndex in targetConstructorParams) {
+        continue // Already handled
+      }
+      if (property.isTransient) {
+        continue // We don't care about these outside of constructor parameters
+      }
+      components += PropertyOnly(property)
+    }
+
     // Calculate how many masks we'll need. Round up if it's not evenly divisible by 32
-    val propertyCount = propertyList.count { it.hasConstructorParameter }
+    val propertyCount = targetConstructorParams.size
     val maskCount = if (propertyCount == 0) {
       0
     } else {
@@ -207,8 +239,9 @@ internal class AdapterGenerator(
     val maskNames = Array(maskCount) { index ->
       nameAllocator.newName("mask$index")
     }
-    val useConstructorDefaults = nonTransientProperties.any { it.hasConstructorDefault }
+    val useDefaultsConstructor = components.filterIsInstance<ParameterComponent>()
-    if (useConstructorDefaults) {
+        .any { it.parameter.hasDefault }
+    if (useDefaultsConstructor) {
       // Initialize all our masks, defaulting to fully unset (-1)
       for (maskName in maskNames) {
         result.addStatement("var %L = -1", maskName)
@@ -243,14 +276,21 @@ internal class AdapterGenerator(
         maskNameIndex++
       }
     }
-    for (property in propertyList) {
+
-      if (property.isTransient) {
+    for (input in components) {
-        if (property.hasConstructorParameter) {
+      if (input is ParameterOnly ||
-          updateMaskIndexes()
+          (input is ParameterProperty && input.property.isTransient)) {
-          constructorPropertyTypes += property.target.type.asTypeBlock()
+        updateMaskIndexes()
-        }
+        constructorPropertyTypes += input.type.asTypeBlock()
+        continue
+      } else if (input is PropertyOnly && input.property.isTransient) {
         continue
       }
+
+      // We've removed all parameter-only types by this point
+      val property = (input as PropertyComponent).property
+
+      // Proceed as usual
       if (property.hasLocalIsPresentName || property.hasConstructorDefault) {
         result.beginControlFlow("%L ->", propertyIndex)
         if (property.delegateKey.nullable) {
@@ -299,8 +339,6 @@ internal class AdapterGenerator(
     result.addStatement("%N.endObject()", readerParam)
 
     var separator = "\n"
-    val parameterProperties = propertyList.filter { it.hasConstructorParameter }
-    val useDefaultsConstructor = parameterProperties.any { it.hasDefault }
 
     val resultName = nameAllocator.newName("result")
     val hasNonConstructorProperties = nonTransientProperties.any { !it.hasConstructorParameter }
@@ -352,25 +390,29 @@ internal class AdapterGenerator(
       result.addCode("«%L%T(", returnOrResultAssignment, originalTypeName)
     }
 
-    for (property in parameterProperties) {
+    for (input in components.filterIsInstance<ParameterComponent>()) {
       result.addCode(separator)
       if (useDefaultsConstructor) {
-        if (property.isTransient) {
+        if (input is ParameterOnly || (input is ParameterProperty && input.property.isTransient)) {
           // 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
           // the transient type may be a primitive type.
-          result.addCode(property.target.type.rawType().defaultPrimitiveValue())
+          result.addCode(input.type.rawType().defaultPrimitiveValue())
         } else {
-          result.addCode("%N", property.localName)
+          result.addCode("%N", (input as ParameterProperty).property.localName)
         }
-      } else {
+      } else if (input !is ParameterOnly) {
+        val property = (input as ParameterProperty).property
         result.addCode("%N = %N", property.name, property.localName)
       }
-      if (!property.isTransient && property.isRequired) {
+      if (input is PropertyComponent) {
-        val missingPropertyBlock =
+        val property = input.property
-            CodeBlock.of("%T.missingProperty(%S, %S, %N)",
+        if (!property.isTransient && property.isRequired) {
-                MOSHI_UTIL, property.localName, property.jsonName, readerParam)
+          val missingPropertyBlock =
-        result.addCode(" ?: throw·%L", missingPropertyBlock)
+              CodeBlock.of("%T.missingProperty(%S, %S, %N)",
+                  MOSHI_UTIL, property.localName, property.jsonName, readerParam)
+          result.addCode(" ?: throw·%L", missingPropertyBlock)
+        }
       }
       separator = ",\n"
     }
@@ -430,3 +472,40 @@ internal class AdapterGenerator(
     return result.build()
   }
 }
+
+private interface PropertyComponent {
+  val property: PropertyGenerator
+  val type: TypeName
+}
+private interface ParameterComponent {
+  val parameter: TargetParameter
+  val type: TypeName
+}
+
+/**
+ * Type hierarchy for describing fromJson() components. Specifically - parameters, properties, and
+ * parameter properties. All three of these scenarios participate in fromJson() parsing.
+ */
+private sealed class FromJsonComponent {
+
+  abstract val type: TypeName
+
+  data class ParameterOnly(
+      override val parameter: TargetParameter
+  ) : FromJsonComponent(), ParameterComponent {
+    override val type: TypeName = parameter.type
+  }
+
+  data class PropertyOnly(
+      override val property: PropertyGenerator
+  ) : FromJsonComponent(), PropertyComponent {
+    override val type: TypeName = property.target.type
+  }
+
+  data class ParameterProperty(
+    override val parameter: TargetParameter,
+    override val property: PropertyGenerator
+  ) : FromJsonComponent(), ParameterComponent, PropertyComponent {
+    override val type: TypeName = parameter.type
+  }
+}

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/TargetConstructor.kt

@@ -19,7 +19,7 @@ import com.squareup.kotlinpoet.KModifier
 
 /** A constructor in user code that should be called by generated code. */
 internal data class TargetConstructor(
-  val parameters: Map<String, TargetParameter>,
+  val parameters: LinkedHashMap<String, TargetParameter>,
   val visibility: KModifier
 ) {
   init {

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/TargetParameter.kt

@@ -16,11 +16,13 @@
 package com.squareup.moshi.kotlin.codegen.api
 
 import com.squareup.kotlinpoet.AnnotationSpec
+import com.squareup.kotlinpoet.TypeName
 
 /** A parameter in user code that should be populated by generated code. */
 internal data class TargetParameter(
   val name: String,
   val index: Int,
+  val type: TypeName,
   val hasDefault: Boolean,
   val jsonName: String? = null,
   val qualifiers: Set<AnnotationSpec>? = null

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt

@@ -72,12 +72,13 @@ private fun Collection<KModifier>.visibility(): KModifier {
 internal fun primaryConstructor(kotlinApi: TypeSpec, elements: Elements): TargetConstructor? {
   val primaryConstructor = kotlinApi.primaryConstructor ?: return null
 
-  val parameters = mutableMapOf<String, TargetParameter>()
+  val parameters = LinkedHashMap<String, TargetParameter>()
   for ((index, parameter) in primaryConstructor.parameters.withIndex()) {
     val name = parameter.name
     parameters[name] = TargetParameter(
         name = name,
         index = index,
+        type = parameter.type,
         hasDefault = parameter.defaultValue != null,
         qualifiers = parameter.annotations.qualifiers(elements),
         jsonName = parameter.annotations.jsonName()

kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt

@@ -327,6 +327,7 @@ class DualKotlinTest(useReflection: Boolean) {
   @Test fun multipleConstructors() {
     val adapter = moshi.adapter<MultipleConstructorsB>()
 
+    //language=JSON
     assertThat(adapter.toJson(MultipleConstructorsB(6))).isEqualTo("""{"f":{"f":6},"b":6}""")
 
     @Language("JSON")
@@ -342,6 +343,59 @@ class DualKotlinTest(useReflection: Boolean) {
   class MultipleConstructorsB(val f: MultipleConstructorsA = MultipleConstructorsA(5), val b: Int) {
     constructor(f: Int, b: Int = 6): this(MultipleConstructorsA(f), b)
   }
+
+  @Test fun `multiple non-property parameters`() {
+    val adapter = moshi.adapter<MultipleNonPropertyParameters>()
+
+    @Language("JSON")
+    val testJson = """{"prop":7}"""
+
+    assertThat(adapter.toJson(MultipleNonPropertyParameters(7))).isEqualTo(testJson)
+
+    val result = adapter.fromJson(testJson)!!
+    assertThat(result.prop).isEqualTo(7)
+  }
+
+  @JsonClass(generateAdapter = true)
+  class MultipleNonPropertyParameters(
+      val prop: Int,
+      param1: Int = 1,
+      param2: Int = 2
+  ) {
+    init {
+      // Ensure the params always uses their default value
+      require(param1 == 1)
+      require(param2 == 2)
+    }
+  }
+
+  // Tests the case of multiple parameters with no parameter properties.
+  @Test fun `only multiple non-property parameters`() {
+    val adapter = moshi.adapter<OnlyMultipleNonPropertyParameters>()
+
+    @Language("JSON")
+    val testJson = """{"prop":7}"""
+
+    assertThat(adapter.toJson(OnlyMultipleNonPropertyParameters().apply { prop = 7 }))
+        .isEqualTo(testJson)
+
+    val result = adapter.fromJson(testJson)!!
+    assertThat(result.prop).isEqualTo(7)
+  }
+
+  @JsonClass(generateAdapter = true)
+  class OnlyMultipleNonPropertyParameters(
+      param1: Int = 1,
+      param2: Int = 2
+  ) {
+    init {
+      // Ensure the params always uses their default value
+      require(param1 == 1)
+      require(param2 == 2)
+    }
+
+    var prop: Int = 0
+  }
 }
 
 // Has to be outside since inline classes are only allowed on top level

kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/MultipleMasksTest.kt

@@ -75,7 +75,7 @@ class MultipleMasks(
     val arg17: Long = 17,
     val arg18: Long = 18,
     val arg19: Long = 19,
-    val arg20: Long = 20,
+    @Suppress("UNUSED_PARAMETER") arg20: Long = 20,
     val arg21: Long = 21,
     val arg22: Long = 22,
     val arg23: Long = 23,