Fix incorrect type variance being applied on generated adapters (#1010)

* Fix: force object type for type arguments

* Add outDeclaration regression test for #1009

* Create mapTypesUtility for reusing recursive type mapping

* Strip typevar variance where appropriate

Resolves #1009

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

@@ -69,7 +69,7 @@ internal class AdapterGenerator(
 
   private val nameAllocator = NameAllocator()
   private val adapterName = "${className.simpleNames.joinToString(separator = "_")}JsonAdapter"
-  private val originalTypeName = target.typeName
+  private val originalTypeName = target.typeName.stripTypeVarVariance()
   private val originalRawTypeName = originalTypeName.rawType()
 
   private val moshiParam = ParameterSpec.builder(
@@ -130,7 +130,7 @@ internal class AdapterGenerator(
     result.superclass(jsonAdapterTypeName)
 
     if (typeVariables.isNotEmpty()) {
-      result.addTypeVariables(typeVariables)
+      result.addTypeVariables(typeVariables.map { it.stripTypeVarVariance() as TypeVariableName })
     }
 
     // TODO make this configurable. Right now it just matches the source model

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

@@ -29,11 +29,15 @@ import com.squareup.kotlinpoet.KModifier
 import com.squareup.kotlinpoet.LONG
 import com.squareup.kotlinpoet.NOTHING
 import com.squareup.kotlinpoet.ParameterizedTypeName
+import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy
 import com.squareup.kotlinpoet.SHORT
+import com.squareup.kotlinpoet.STAR
 import com.squareup.kotlinpoet.TypeName
 import com.squareup.kotlinpoet.TypeVariableName
 import com.squareup.kotlinpoet.UNIT
+import com.squareup.kotlinpoet.WildcardTypeName
 import com.squareup.kotlinpoet.asTypeName
+import kotlin.reflect.KClass
 
 internal fun TypeName.rawType(): ClassName {
   return when (this) {
@@ -95,3 +99,48 @@ internal fun KModifier.checkIsVisibility() {
     "Visibility must be one of ${(0..ordinal).joinToString { KModifier.values()[it].name }}. Is $name"
   }
 }
+
+internal inline fun <reified T: TypeName> TypeName.mapTypes(noinline transform: T.() -> TypeName?): TypeName {
+  return mapTypes(T::class, transform)
+}
+
+@Suppress("UNCHECKED_CAST")
+internal fun <T: TypeName> TypeName.mapTypes(target: KClass<T>, transform: T.() -> TypeName?): TypeName {
+  if (target.java == javaClass) {
+    return (this as T).transform() ?: return this
+  }
+  return when (this) {
+    is ClassName -> this
+    is ParameterizedTypeName -> {
+      (rawType.mapTypes(target, transform) as ClassName).parameterizedBy(typeArguments.map { it.mapTypes(target, transform) })
+          .copy(nullable = isNullable, annotations = annotations)
+    }
+    is TypeVariableName -> {
+      copy(bounds = bounds.map { it.mapTypes(target, transform) })
+    }
+    is WildcardTypeName -> {
+      // TODO Would be nice if KotlinPoet modeled these easier.
+      // Producer type - empty inTypes, single element outTypes
+      // Consumer type - single element inTypes, single ANY element outType.
+      when {
+        this == STAR -> this
+        outTypes.isNotEmpty() && inTypes.isEmpty() -> {
+          WildcardTypeName.producerOf(outTypes[0].mapTypes(target, transform))
+              .copy(nullable = isNullable, annotations = annotations)
+        }
+        inTypes.isNotEmpty() -> {
+          WildcardTypeName.consumerOf(inTypes[0].mapTypes(target, transform))
+              .copy(nullable = isNullable, annotations = annotations)
+        }
+        else -> throw UnsupportedOperationException("Not possible.")
+      }
+    }
+    else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.")
+  }
+}
+
+internal fun TypeName.stripTypeVarVariance(): TypeName {
+  return mapTypes<TypeVariableName> {
+    TypeVariableName(name = name, bounds = bounds.map { it.mapTypes(TypeVariableName::stripTypeVarVariance) }, variance = null)
+  }
+}

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

@@ -48,6 +48,7 @@ import com.squareup.moshi.kotlin.codegen.api.TargetConstructor
 import com.squareup.moshi.kotlin.codegen.api.TargetParameter
 import com.squareup.moshi.kotlin.codegen.api.TargetProperty
 import com.squareup.moshi.kotlin.codegen.api.TargetType
+import com.squareup.moshi.kotlin.codegen.api.mapTypes
 import java.lang.annotation.ElementType
 import java.lang.annotation.Retention
 import java.lang.annotation.RetentionPolicy
@@ -59,6 +60,7 @@ import javax.lang.model.element.TypeElement
 import javax.lang.model.util.Elements
 import javax.lang.model.util.Types
 import javax.tools.Diagnostic
+import kotlin.reflect.KClass
 
 private val JSON_QUALIFIER = JsonQualifier::class.java
 private val JSON = Json::class.asClassName()
@@ -320,46 +322,19 @@ private fun String.escapeDollarSigns(): String {
   return replace("\$", "\${\'\$\'}")
 }
 
-private fun TypeName.unwrapTypeAlias(): TypeName {
-  return when (this) {
-    is ClassName -> {
-      tag<TypeNameAliasTag>()?.type?.let { unwrappedType ->
-        // If any type is nullable, then the whole thing is nullable
-        var isAnyNullable = isNullable
-        // Keep track of all annotations across type levels. Sort them too for consistency.
-        val runningAnnotations = TreeSet<AnnotationSpec>(compareBy { it.toString() }).apply {
-          addAll(annotations)
-        }
-        val nestedUnwrappedType = unwrappedType.unwrapTypeAlias()
-        runningAnnotations.addAll(nestedUnwrappedType.annotations)
-        isAnyNullable = isAnyNullable || nestedUnwrappedType.isNullable
-        nestedUnwrappedType.copy(nullable = isAnyNullable, annotations = runningAnnotations.toList())
-      } ?: this
-    }
-    is ParameterizedTypeName -> {
-      return (rawType.unwrapTypeAlias() as ClassName).parameterizedBy(typeArguments.map { it.unwrapTypeAlias() })
-          .copy(nullable = isNullable, annotations = annotations)
-    }
-    is TypeVariableName -> {
-      return copy(bounds = bounds.map { it.unwrapTypeAlias() })
-    }
-    is WildcardTypeName -> {
-      // TODO Would be nice if KotlinPoet modeled these easier.
-      // Producer type - empty inTypes, single element outTypes
-      // Consumer type - single element inTypes, single ANY element outType.
-      return when {
-        this == STAR -> this
-        outTypes.isNotEmpty() && inTypes.isEmpty() -> {
-          WildcardTypeName.producerOf(outTypes[0].unwrapTypeAlias())
-              .copy(nullable = isNullable, annotations = annotations)
-        }
-        inTypes.isNotEmpty() -> {
-          WildcardTypeName.consumerOf(inTypes[0].unwrapTypeAlias())
-              .copy(nullable = isNullable, annotations = annotations)
-        }
-        else -> throw UnsupportedOperationException("Not possible.")
+internal fun TypeName.unwrapTypeAlias(): TypeName {
+  return mapTypes<ClassName> {
+    tag<TypeNameAliasTag>()?.type?.let { unwrappedType ->
+      // If any type is nullable, then the whole thing is nullable
+      var isAnyNullable = isNullable
+      // Keep track of all annotations across type levels. Sort them too for consistency.
+      val runningAnnotations = TreeSet<AnnotationSpec>(compareBy { it.toString() }).apply {
+        addAll(annotations)
       }
+      val nestedUnwrappedType = unwrappedType.unwrapTypeAlias()
+      runningAnnotations.addAll(nestedUnwrappedType.annotations)
+      isAnyNullable = isAnyNullable || nestedUnwrappedType.isNullable
+      nestedUnwrappedType.copy(nullable = isAnyNullable, annotations = runningAnnotations.toList())
     }
-    else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.")
   }
 }

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

@@ -522,6 +522,24 @@ class DualKotlinTest(useReflection: Boolean) {
       val convolutedMultiNullableShouldBeNullable: NullableB?,
       val deepNestedNullableShouldBeNullable: E
   )
+
+  // Regression test for https://github.com/square/moshi/issues/1009
+  @Test fun outDeclaration() {
+    val adapter = moshi.adapter<OutDeclaration<Int>>()
+
+    @Language("JSON")
+    val testJson = """{"input":3}"""
+
+    val instance = OutDeclaration(3)
+    assertThat(adapter.serializeNulls().toJson(instance))
+        .isEqualTo(testJson)
+
+    val result = adapter.fromJson(testJson)!!
+    assertThat(result).isEqualTo(instance)
+  }
+
+  @JsonClass(generateAdapter = true)
+  data class OutDeclaration<out T>(val input: T)
 }
 
 typealias TypeAlias = Int

kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/reflect/-MoshiKotlinExtensions.kt

@@ -57,12 +57,16 @@ fun <T> Moshi.adapter(ktype: KType): JsonAdapter<T> {
 }
 
 @PublishedApi
-internal fun KType.toType(): Type {
+internal fun KType.toType(allowPrimitives: Boolean = true): Type {
   classifier?.let {
     when (it) {
       is KTypeParameter -> throw IllegalArgumentException("Type parameters are not supported")
       is KClass<*> -> {
-        val javaType = it.java
+        val javaType = if (allowPrimitives) {
+          it.java
+        } else {
+          it.javaObjectType
+        }
         if (javaType.isArray) {
           return Types.arrayOf(javaType.componentType)
         }
@@ -88,7 +92,7 @@ internal fun KType.toType(): Type {
 }
 
 internal fun KTypeProjection.toType(): Type {
-  val javaType = type?.toType() ?: return Any::class.java
+  val javaType = type?.toType(allowPrimitives = false) ?: return Any::class.java
   return when (variance) {
     null -> Any::class.java
     KVariance.INVARIANT -> javaType