KSP followups from #1393 (#1448)

* Rename to unwrapTypeAliasInternal + simplify * Move down isAnyNullable * Make dynamic explicit * Clean up supertypes doc and filtering * Switch to invoke extensions * Just best guess the annotation * Clean up redundant sequence and use a regular loop * element -> type * supertypes -> superclasses * Spotless * Fix copyright * Add multiple messages check * Link issue Co-authored-by: Jesse Wilson <jesse@swank.ca>
square · Dec 6, 2021 · fb5dd08 · fb5dd08
1 parent f57d720
commit fb5dd08
Show file tree

Hide file tree

Showing 8 changed files with 85 additions and 102 deletions.
diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/typeAliasUnwrapping.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/typeAliasUnwrapping.kt
@@ -17,6 +17,7 @@ package com.squareup.moshi.kotlin.codegen.api
 
 import com.squareup.kotlinpoet.AnnotationSpec
 import com.squareup.kotlinpoet.ClassName
+import com.squareup.kotlinpoet.Dynamic
 import com.squareup.kotlinpoet.LambdaTypeName
 import com.squareup.kotlinpoet.ParameterizedTypeName
 import com.squareup.kotlinpoet.TypeName
@@ -26,52 +27,35 @@ import com.squareup.kotlinpoet.tag
 import com.squareup.kotlinpoet.tags.TypeAliasTag
 import java.util.TreeSet
 
-internal fun TypeName.unwrapTypeAliasReal(): TypeName {
+private fun TypeName.unwrapTypeAliasInternal(): TypeName? {
   return tag<TypeAliasTag>()?.abbreviatedType?.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
+    // If any type is nullable, then the whole thing is nullable
+    val isAnyNullable = isNullable || nestedUnwrappedType.isNullable
     nestedUnwrappedType.copy(nullable = isAnyNullable, annotations = runningAnnotations.toList())
-  } ?: this
+  }
 }
 
 internal fun TypeName.unwrapTypeAlias(): TypeName {
   return when (this) {
-    is ClassName -> unwrapTypeAliasReal()
+    is ClassName -> unwrapTypeAliasInternal() ?: this
     is ParameterizedTypeName -> {
-      if (TypeAliasTag::class in tags) {
-        unwrapTypeAliasReal()
-      } else {
-        deepCopy(TypeName::unwrapTypeAlias)
-      }
+      unwrapTypeAliasInternal() ?: deepCopy(TypeName::unwrapTypeAlias)
     }
     is TypeVariableName -> {
-      if (TypeAliasTag::class in tags) {
-        unwrapTypeAliasReal()
-      } else {
-        deepCopy(transform = TypeName::unwrapTypeAlias)
-      }
+      unwrapTypeAliasInternal() ?: deepCopy(transform = TypeName::unwrapTypeAlias)
     }
     is WildcardTypeName -> {
-      if (TypeAliasTag::class in tags) {
-        unwrapTypeAliasReal()
-      } else {
-        deepCopy(TypeName::unwrapTypeAlias)
-      }
+      unwrapTypeAliasInternal() ?: deepCopy(TypeName::unwrapTypeAlias)
     }
     is LambdaTypeName -> {
-      if (TypeAliasTag::class in tags) {
-        unwrapTypeAliasReal()
-      } else {
-        deepCopy(TypeName::unwrapTypeAlias)
-      }
+      unwrapTypeAliasInternal() ?: deepCopy(TypeName::unwrapTypeAlias)
     }
-    else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.")
+    Dynamic -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.")
   }
 }
diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/apt/AppliedType.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/apt/AppliedType.kt
@@ -15,10 +15,16 @@
  */
 package com.squareup.moshi.kotlin.codegen.apt
 
+import com.squareup.kotlinpoet.ClassName
+import com.squareup.kotlinpoet.DelicateKotlinPoetApi
+import com.squareup.kotlinpoet.asClassName
+import javax.lang.model.element.ElementKind.CLASS
 import javax.lang.model.element.TypeElement
 import javax.lang.model.type.DeclaredType
 import javax.lang.model.util.Types
 
+private val OBJECT_CLASS = ClassName("java.lang", "Object")
+
 /**
  * A concrete type like `List<String>` with enough information to know how to resolve its type
  * variables.
@@ -27,25 +33,32 @@ internal class AppliedType private constructor(
   val element: TypeElement,
   private val mirror: DeclaredType
 ) {
-  /** Returns all supertypes of this, recursively. Includes both interface and class supertypes. */
-  fun supertypes(
+  /** Returns all supertypes of this, recursively. Only [CLASS] is used as we can't really use other types. */
+  @OptIn(DelicateKotlinPoetApi::class)
+  fun superclasses(
     types: Types,
     result: LinkedHashSet<AppliedType> = LinkedHashSet()
   ): LinkedHashSet<AppliedType> {
     result.add(this)
     for (supertype in types.directSupertypes(mirror)) {
       val supertypeDeclaredType = supertype as DeclaredType
       val supertypeElement = supertypeDeclaredType.asElement() as TypeElement
-      val appliedSupertype = AppliedType(supertypeElement, supertypeDeclaredType)
-      appliedSupertype.supertypes(types, result)
+      if (supertypeElement.kind != CLASS) {
+        continue
+      } else if (supertypeElement.asClassName() == OBJECT_CLASS) {
+        // Don't load properties for java.lang.Object.
+        continue
+      }
+      val appliedSuperclass = AppliedType(supertypeElement, supertypeDeclaredType)
+      appliedSuperclass.superclasses(types, result)
     }
     return result
   }
 
   override fun toString() = mirror.toString()
 
   companion object {
-    fun get(typeElement: TypeElement): AppliedType {
+    operator fun invoke(typeElement: TypeElement): AppliedType {
       return AppliedType(typeElement, typeElement.asType() as DeclaredType)
     }
   }

diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/apt/metadata.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/apt/metadata.kt
@@ -52,7 +52,6 @@ import java.lang.annotation.RetentionPolicy
 import javax.annotation.processing.Messager
 import javax.lang.model.element.AnnotationMirror
 import javax.lang.model.element.Element
-import javax.lang.model.element.ElementKind
 import javax.lang.model.element.TypeElement
 import javax.lang.model.type.DeclaredType
 import javax.lang.model.util.Elements
@@ -63,7 +62,6 @@ import javax.tools.Diagnostic.Kind.WARNING
 private val JSON_QUALIFIER = JsonQualifier::class.java
 private val JSON = Json::class.asClassName()
 private val TRANSIENT = Transient::class.asClassName()
-private val OBJECT_CLASS = ClassName("java.lang", "Object")
 private val VISIBILITY_MODIFIERS = setOf(
   KModifier.INTERNAL,
   KModifier.PRIVATE,
@@ -206,7 +204,7 @@ internal fun targetType(
 
   val kotlinApi = cachedClassInspector.toTypeSpec(kmClass)
   val typeVariables = kotlinApi.typeVariables
-  val appliedType = AppliedType.get(element)
+  val appliedType = AppliedType(element)
 
   val constructor = primaryConstructor(element, kotlinApi, elements, messager)
   if (constructor == null) {
@@ -230,28 +228,24 @@ internal fun targetType(
   val properties = mutableMapOf<String, TargetProperty>()
 
   val resolvedTypes = mutableListOf<ResolvedTypeMapping>()
-  val superTypes = appliedType.supertypes(types)
-    .filterNot { supertype ->
-      supertype.element.asClassName() == OBJECT_CLASS || // Don't load properties for java.lang.Object.
-        supertype.element.kind != ElementKind.CLASS // Don't load properties for interface types.
-    }
-    .onEach { supertype ->
-      if (supertype.element.getAnnotation(Metadata::class.java) == null) {
+  val superclass = appliedType.superclasses(types)
+    .onEach { superclass ->
+      if (superclass.element.getAnnotation(Metadata::class.java) == null) {
         messager.printMessage(
           ERROR,
-          "@JsonClass can't be applied to $element: supertype $supertype is not a Kotlin type",
+          "@JsonClass can't be applied to $element: supertype $superclass is not a Kotlin type",
           element
         )
         return null
       }
     }
-    .associateWithTo(LinkedHashMap()) { supertype ->
+    .associateWithTo(LinkedHashMap()) { superclass ->
       // Load the kotlin API cache into memory eagerly so we can reuse the parsed APIs
-      val api = if (supertype.element == element) {
+      val api = if (superclass.element == element) {
         // We've already parsed this api above, reuse it
         kotlinApi
       } else {
-        cachedClassInspector.toTypeSpec(supertype.element)
+        cachedClassInspector.toTypeSpec(superclass.element)
       }
 
       val apiSuperClass = api.superclass
@@ -268,7 +262,7 @@ internal fun targetType(
         // Then when we look at Bar<T> later, we'll look up to the descendent Foo and extract its
         // materialized type from there.
         //
-        val superSuperClass = supertype.element.superclass as DeclaredType
+        val superSuperClass = superclass.element.superclass as DeclaredType
 
         // Convert to an element and back to wipe the typed generics off of this
         val untyped = superSuperClass.asElement().asType().asTypeName() as ParameterizedTypeName
@@ -285,7 +279,7 @@ internal fun targetType(
       return@associateWithTo api
     }
 
-  for ((localAppliedType, supertypeApi) in superTypes.entries) {
+  for ((localAppliedType, supertypeApi) in superclass.entries) {
     val appliedClassName = localAppliedType.element.asClassName()
     val supertypeProperties = declaredProperties(
       constructor = constructor,

diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/AppliedType.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/AppliedType.kt
@@ -35,8 +35,8 @@ internal class AppliedType private constructor(
   val typeName: TypeName = type.toClassName()
 ) {
 
-  /** Returns all supertypes of this, recursively. Includes both interface and class supertypes. */
-  fun supertypes(
+  /** Returns all super classes of this, recursively. Only [CLASS] is used as we can't really use other types. */
+  fun superclasses(
     resolver: Resolver,
   ): LinkedHashSet<AppliedType> {
     val result: LinkedHashSet<AppliedType> = LinkedHashSet()
@@ -63,7 +63,7 @@ internal class AppliedType private constructor(
   override fun toString() = type.qualifiedName!!.asString()
 
   companion object {
-    fun get(type: KSClassDeclaration): AppliedType {
+    operator fun invoke(type: KSClassDeclaration): AppliedType {
       return AppliedType(type)
     }
   }

diff --git a/...n/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt b/...n/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt
@@ -27,8 +27,8 @@ import com.google.devtools.ksp.symbol.KSAnnotated
 import com.google.devtools.ksp.symbol.KSDeclaration
 import com.google.devtools.ksp.symbol.KSFile
 import com.squareup.kotlinpoet.AnnotationSpec
+import com.squareup.kotlinpoet.ClassName
 import com.squareup.kotlinpoet.ksp.addOriginatingKSFile
-import com.squareup.kotlinpoet.ksp.toClassName
 import com.squareup.kotlinpoet.ksp.writeTo
 import com.squareup.moshi.JsonClass
 import com.squareup.moshi.kotlin.codegen.api.AdapterGenerator
@@ -66,54 +66,47 @@ private class JsonClassSymbolProcessor(
 
   override fun process(resolver: Resolver): List<KSAnnotated> {
     val generatedAnnotation = generatedOption?.let {
-      val annotationType = resolver.getClassDeclarationByName(resolver.getKSNameFromString(it))
-        ?: run {
-          logger.error("Generated annotation type doesn't exist: $it")
-          return emptyList()
-        }
-      AnnotationSpec.builder(annotationType.toClassName())
+      AnnotationSpec.builder(ClassName.bestGuess(it))
         .addMember("value = [%S]", JsonClassSymbolProcessor::class.java.canonicalName)
         .addMember("comments = %S", "https://github.com/square/moshi")
         .build()
     }
 
-    resolver.getSymbolsWithAnnotation(JSON_CLASS_NAME)
-      .asSequence()
-      .forEach { type ->
-        // For the smart cast
-        if (type !is KSDeclaration) {
-          logger.error("@JsonClass can't be applied to $type: must be a Kotlin class", type)
-          return@forEach
-        }
-
-        val jsonClassAnnotation = type.findAnnotationWithType<JsonClass>() ?: return@forEach
-
-        val generator = jsonClassAnnotation.generator
-
-        if (generator.isNotEmpty()) return@forEach
-
-        if (!jsonClassAnnotation.generateAdapter) return@forEach
-
-        val originatingFile = type.containingFile!!
-        val adapterGenerator = adapterGenerator(logger, resolver, type) ?: return emptyList()
-        try {
-          val preparedAdapter = adapterGenerator
-            .prepare(generateProguardRules) { spec ->
-              spec.toBuilder()
-                .apply {
-                  generatedAnnotation?.let(::addAnnotation)
-                }
-                .addOriginatingKSFile(originatingFile)
-                .build()
-            }
-          preparedAdapter.spec.writeTo(codeGenerator, aggregating = false)
-          preparedAdapter.proguardConfig?.writeTo(codeGenerator, originatingFile)
-        } catch (e: Exception) {
-          logger.error(
-            "Error preparing ${type.simpleName.asString()}: ${e.stackTrace.joinToString("\n")}"
-          )
-        }
+    for (type in resolver.getSymbolsWithAnnotation(JSON_CLASS_NAME)) {
+      // For the smart cast
+      if (type !is KSDeclaration) {
+        logger.error("@JsonClass can't be applied to $type: must be a Kotlin class", type)
+        continue
       }
+
+      val jsonClassAnnotation = type.findAnnotationWithType<JsonClass>() ?: continue
+
+      val generator = jsonClassAnnotation.generator
+
+      if (generator.isNotEmpty()) continue
+
+      if (!jsonClassAnnotation.generateAdapter) continue
+
+      val originatingFile = type.containingFile!!
+      val adapterGenerator = adapterGenerator(logger, resolver, type) ?: return emptyList()
+      try {
+        val preparedAdapter = adapterGenerator
+          .prepare(generateProguardRules) { spec ->
+            spec.toBuilder()
+              .apply {
+                generatedAnnotation?.let(::addAnnotation)
+              }
+              .addOriginatingKSFile(originatingFile)
+              .build()
+          }
+        preparedAdapter.spec.writeTo(codeGenerator, aggregating = false)
+        preparedAdapter.proguardConfig?.writeTo(codeGenerator, originatingFile)
+      } catch (e: Exception) {
+        logger.error(
+          "Error preparing ${type.simpleName.asString()}: ${e.stackTrace.joinToString("\n")}"
+        )
+      }
+    }
     return emptyList()
   }
 

diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt
@@ -53,7 +53,7 @@ import com.squareup.moshi.kotlin.codegen.api.TargetProperty
 import com.squareup.moshi.kotlin.codegen.api.TargetType
 import com.squareup.moshi.kotlin.codegen.api.unwrapTypeAlias
 
-/** Returns a target type for `element`, or null if it cannot be used with code gen. */
+/** Returns a target type for [type] or null if it cannot be used with code gen. */
 internal fun targetType(
   type: KSDeclaration,
   resolver: Resolver,
@@ -89,7 +89,7 @@ internal fun targetType(
     sourceTypeHint = type.qualifiedName!!.asString()
   )
   val typeVariables = type.typeParameters.map { it.toTypeVariableName(classTypeParamsResolver) }
-  val appliedType = AppliedType.get(type)
+  val appliedType = AppliedType(type)
 
   val constructor = primaryConstructor(resolver, type, classTypeParamsResolver, logger)
     ?: run {
@@ -108,12 +108,12 @@ internal fun targetType(
   val properties = mutableMapOf<String, TargetProperty>()
 
   val originalType = appliedType.type
-  for (supertype in appliedType.supertypes(resolver)) {
-    val classDecl = supertype.type
+  for (superclass in appliedType.superclasses(resolver)) {
+    val classDecl = superclass.type
     if (!classDecl.isKotlinClass()) {
       logger.error(
         """
-        @JsonClass can't be applied to $type: supertype $supertype is not a Kotlin type.
+        @JsonClass can't be applied to $type: supertype $superclass is not a Kotlin type.
         Origin=${classDecl.origin}
         Annotations=${classDecl.annotations.joinToString(prefix = "[", postfix = "]") { it.shortName.getShortName() }}
         """.trimIndent(),

diff --git a/...degen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt b/...degen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt
@@ -430,8 +430,7 @@ class JsonClassSymbolProcessorTest {
     )
     assertThat(result.exitCode).isEqualTo(KotlinCompilation.ExitCode.COMPILATION_ERROR)
     assertThat(result.messages).contains("property a is not visible")
-    // TODO we throw eagerly currently and don't collect
-//    assertThat(result.messages).contains("property c is not visible")
+    assertThat(result.messages).contains("property c is not visible")
   }
 
   @Test

diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt
@@ -257,7 +257,7 @@ class DualKotlinTest {
     val result = adapter.fromJson(testJson)!!
     assertThat(result.i).isEqualTo(6)
 
-    // TODO doesn't work yet.
+    // TODO doesn't work yet. https://github.com/square/moshi/issues/1170
     //  need to invoke the constructor_impl$default static method, invoke constructor with result
 //    val testEmptyJson =
 //      """{}"""