Handle enclosing visibility + cache metadata parsing (#1026)

* Only allow public or internal classes

* Ensure consistent ordering of supertypes

* Eagerly load supertype typespecs for reuse

* Add regression test for https://github.com/square/moshi/issues/1022

* Add MoshiCachedClassInspector, wire in

* Check visibility of enclosed types too

Resolves #1022

* Fix comments

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

@@ -30,8 +30,8 @@ internal class AppliedType private constructor(
   /** Returns all supertypes of this, recursively. Includes both interface and class supertypes. */
   fun supertypes(
     types: Types,
-    result: MutableSet<AppliedType> = mutableSetOf()
+    result: LinkedHashSet<AppliedType> = LinkedHashSet()
-  ): Set<AppliedType> {
+  ): LinkedHashSet<AppliedType> {
     result.add(this)
     for (supertype in types.directSupertypes(mirror)) {
       val supertypeDeclaredType = supertype as DeclaredType

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

@@ -18,6 +18,7 @@ package com.squareup.moshi.kotlin.codegen
 import com.google.auto.service.AutoService
 import com.squareup.kotlinpoet.AnnotationSpec
 import com.squareup.kotlinpoet.asClassName
+import com.squareup.kotlinpoet.classinspector.elements.ElementsClassInspector
 import com.squareup.kotlinpoet.metadata.KotlinPoetMetadataPreview
 import com.squareup.moshi.JsonClass
 import com.squareup.moshi.kotlin.codegen.api.AdapterGenerator
@@ -95,6 +96,8 @@ class JsonClassCodegenProcessor : AbstractProcessor() {
   }
 
   override fun process(annotations: Set<TypeElement>, roundEnv: RoundEnvironment): Boolean {
+    val classInspector = ElementsClassInspector.create(elements, types)
+    val cachedClassInspector = MoshiCachedClassInspector(classInspector)
     for (type in roundEnv.getElementsAnnotatedWith(annotation)) {
       if (type !is TypeElement) {
         messager.printMessage(
@@ -104,7 +107,7 @@ class JsonClassCodegenProcessor : AbstractProcessor() {
       }
       val jsonClass = type.getAnnotation(annotation)
       if (jsonClass.generateAdapter && jsonClass.generator.isEmpty()) {
-        val generator = adapterGenerator(type) ?: continue
+        val generator = adapterGenerator(type, cachedClassInspector) ?: continue
         generator
             .generateFile {
               it.toBuilder()
@@ -129,8 +132,8 @@ class JsonClassCodegenProcessor : AbstractProcessor() {
     return false
   }
 
-  private fun adapterGenerator(element: TypeElement): AdapterGenerator? {
+  private fun adapterGenerator(element: TypeElement, cachedClassInspector: MoshiCachedClassInspector): AdapterGenerator? {
-    val type = targetType(messager, elements, types, element) ?: return null
+    val type = targetType(messager, elements, types, element, cachedClassInspector) ?: return null
 
     val properties = mutableMapOf<String, PropertyGenerator>()
     for (property in type.properties.values) {

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

@@ -0,0 +1,37 @@
+package com.squareup.moshi.kotlin.codegen
+
+import com.squareup.kotlinpoet.TypeSpec
+import com.squareup.kotlinpoet.metadata.ImmutableKmClass
+import com.squareup.kotlinpoet.metadata.specs.ClassInspector
+import com.squareup.kotlinpoet.metadata.specs.toTypeSpec
+import com.squareup.kotlinpoet.metadata.toImmutableKmClass
+import javax.lang.model.element.TypeElement
+
+/**
+ * This cached API over [ClassInspector] that caches certain lookups Moshi does potentially multiple
+ * times. This is useful mostly because it avoids duplicate reloads in cases like common base
+ * classes, common enclosing types, etc.
+ */
+internal class MoshiCachedClassInspector(private val classInspector: ClassInspector) {
+  private val elementToSpecCache = mutableMapOf<TypeElement, TypeSpec>()
+  private val kmClassToSpecCache = mutableMapOf<ImmutableKmClass, TypeSpec>()
+  private val metadataToKmClassCache = mutableMapOf<Metadata, ImmutableKmClass>()
+
+  fun toImmutableKmClass(metadata: Metadata): ImmutableKmClass {
+    return metadataToKmClassCache.getOrPut(metadata) {
+      metadata.toImmutableKmClass()
+    }
+  }
+
+  fun toTypeSpec(kmClass: ImmutableKmClass): TypeSpec {
+    return kmClassToSpecCache.getOrPut(kmClass) {
+      kmClass.toTypeSpec(classInspector)
+    }
+  }
+
+  fun toTypeSpec(element: TypeElement): TypeSpec {
+    return elementToSpecCache.getOrPut(element) {
+      toTypeSpec(toImmutableKmClass(element.metadata))
+    }
+  }
+}

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

@@ -18,27 +18,20 @@ package com.squareup.moshi.kotlin.codegen
 import com.squareup.kotlinpoet.AnnotationSpec
 import com.squareup.kotlinpoet.ClassName
 import com.squareup.kotlinpoet.KModifier
-import com.squareup.kotlinpoet.ParameterizedTypeName
-import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy
-import com.squareup.kotlinpoet.STAR
 import com.squareup.kotlinpoet.TypeName
 import com.squareup.kotlinpoet.TypeSpec
-import com.squareup.kotlinpoet.TypeVariableName
-import com.squareup.kotlinpoet.WildcardTypeName
 import com.squareup.kotlinpoet.asClassName
 import com.squareup.kotlinpoet.asTypeName
-import com.squareup.kotlinpoet.classinspector.elements.ElementsClassInspector
 import com.squareup.kotlinpoet.metadata.KotlinPoetMetadataPreview
 import com.squareup.kotlinpoet.metadata.isAbstract
 import com.squareup.kotlinpoet.metadata.isClass
 import com.squareup.kotlinpoet.metadata.isEnum
 import com.squareup.kotlinpoet.metadata.isInner
+import com.squareup.kotlinpoet.metadata.isInternal
 import com.squareup.kotlinpoet.metadata.isLocal
+import com.squareup.kotlinpoet.metadata.isPublic
 import com.squareup.kotlinpoet.metadata.isSealed
-import com.squareup.kotlinpoet.metadata.specs.ClassInspector
 import com.squareup.kotlinpoet.metadata.specs.TypeNameAliasTag
-import com.squareup.kotlinpoet.metadata.specs.toTypeSpec
-import com.squareup.kotlinpoet.metadata.toImmutableKmClass
 import com.squareup.kotlinpoet.tag
 import com.squareup.moshi.Json
 import com.squareup.moshi.JsonQualifier
@@ -55,12 +48,12 @@ import java.lang.annotation.RetentionPolicy
 import java.lang.annotation.Target
 import java.util.TreeSet
 import javax.annotation.processing.Messager
+import javax.lang.model.element.Element
 import javax.lang.model.element.ElementKind
 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()
@@ -101,7 +94,9 @@ internal fun primaryConstructor(kotlinApi: TypeSpec, elements: Elements): Target
 internal fun targetType(messager: Messager,
     elements: Elements,
     types: Types,
-    element: TypeElement): TargetType? {
+    element: TypeElement,
+    cachedClassInspector: MoshiCachedClassInspector
+): TargetType? {
   val typeMetadata = element.getAnnotation(Metadata::class.java)
   if (typeMetadata == null) {
     messager.printMessage(
@@ -111,7 +106,7 @@ internal fun targetType(messager: Messager,
   }
 
   val kmClass = try {
-    typeMetadata.toImmutableKmClass()
+    cachedClassInspector.toImmutableKmClass(typeMetadata)
   } catch (e: UnsupportedOperationException) {
     messager.printMessage(
         Diagnostic.Kind.ERROR, "@JsonClass can't be applied to $element: must be a Class type",
@@ -141,7 +136,8 @@ internal fun targetType(messager: Messager,
     }
     kmClass.isSealed -> {
       messager.printMessage(
-          Diagnostic.Kind.ERROR, "@JsonClass can't be applied to $element: must not be sealed", element)
+          Diagnostic.Kind.ERROR, "@JsonClass can't be applied to $element: must not be sealed",
+          element)
       return null
     }
     kmClass.isAbstract -> {
@@ -156,10 +152,16 @@ internal fun targetType(messager: Messager,
           element)
       return null
     }
+    !kmClass.isPublic && !kmClass.isInternal -> {
+      messager.printMessage(
+          Diagnostic.Kind.ERROR,
+          "@JsonClass can't be applied to $element: must be internal or public",
+          element)
+      return null
+    }
   }
 
-  val elementHandler = ElementsClassInspector.create(elements, types)
+  val kotlinApi = cachedClassInspector.toTypeSpec(kmClass)
-  val kotlinApi = kmClass.toTypeSpec(elementHandler)
   val typeVariables = kotlinApi.typeVariables
   val appliedType = AppliedType.get(element)
 
@@ -176,46 +178,62 @@ internal fun targetType(messager: Messager,
   }
 
   val properties = mutableMapOf<String, TargetProperty>()
-  for (supertype in appliedType.supertypes(types)) {
+  val superTypes = appliedType.supertypes(types)
-    if (supertype.element.asClassName() == OBJECT_CLASS) {
+      .filterNot { supertype ->
-      continue // Don't load properties for java.lang.Object.
+        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.
-    if (supertype.element.kind != ElementKind.CLASS) {
-      continue // Don't load properties for interface types.
       }
+      .onEach { supertype ->
         if (supertype.element.getAnnotation(Metadata::class.java) == null) {
           messager.printMessage(Diagnostic.Kind.ERROR,
               "@JsonClass can't be applied to $element: supertype $supertype is not a Kotlin type",
               element)
           return null
         }
-    val supertypeProperties = if (supertype.element == element) {
-      // We've already parsed this api above, reuse it
-      declaredProperties(supertype.element, constructor, elementHandler, kotlinApi)
-    } else {
-      declaredProperties(
-          supertype.element, constructor, elementHandler)
       }
+      .associateWithTo(LinkedHashMap()) { supertype ->
+        // Load the kotlin API cache into memory eagerly so we can reuse the parsed APIs
+        if (supertype.element == element) {
+          // We've already parsed this api above, reuse it
+          kotlinApi
+        } else {
+          cachedClassInspector.toTypeSpec(supertype.element)
+        }
+      }
+  for (supertypeApi in superTypes.values) {
+    val supertypeProperties = declaredProperties(constructor, supertypeApi)
     for ((name, property) in supertypeProperties) {
       properties.putIfAbsent(name, property)
     }
   }
+  val visibility = kotlinApi.modifiers.visibility()
+  // If any class in the enclosing class hierarchy is internal, they must all have internal
+  // generated adapters.
+  val resolvedVisibility = if (visibility == KModifier.INTERNAL) {
+    // Our nested type is already internal, no need to search
+    visibility
+  } else {
+    // Implicitly public, so now look up the hierarchy
+    val forceInternal = generateSequence<Element>(element) { it.enclosingElement }
+        .filterIsInstance<TypeElement>()
+        .map { cachedClassInspector.toImmutableKmClass(it.metadata) }
+        .any { it.isInternal }
+    if (forceInternal) KModifier.INTERNAL else visibility
+  }
   return TargetType(
       typeName = element.asType().asTypeName(),
       constructor = constructor,
       properties = properties,
       typeVariables = typeVariables,
       isDataClass = KModifier.DATA in kotlinApi.modifiers,
-      visibility = kotlinApi.modifiers.visibility())
+      visibility = resolvedVisibility)
 }
 
 /** Returns the properties declared by `typeElement`. */
 @KotlinPoetMetadataPreview
 private fun declaredProperties(
-    typeElement: TypeElement,
     constructor: TargetConstructor,
-    elementHandler: ClassInspector,
+    kotlinApi: TypeSpec
-    kotlinApi: TypeSpec = typeElement.toTypeSpec(elementHandler)
 ): Map<String, TargetProperty> {
 
   val result = mutableMapOf<String, TargetProperty>()
@@ -338,3 +356,9 @@ internal fun TypeName.unwrapTypeAlias(): TypeName {
     }
   }
 }
+
+internal val TypeElement.metadata: Metadata
+  get() {
+    return getAnnotation(Metadata::class.java)
+        ?: throw IllegalStateException("Not a kotlin type! $this")
+  }

kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessorTest.kt

@@ -182,6 +182,20 @@ class JsonClassCodegenProcessorTest {
         "error: @JsonClass can't be applied to LocalClass: must not be local")
   }
 
+  @Test fun privateClassesNotSupported() {
+    val result = compile(kotlin("source.kt",
+        """
+          import com.squareup.moshi.JsonClass
+          
+          @JsonClass(generateAdapter = true)
+          private class PrivateClass(val a: Int)
+          """
+    ))
+    assertThat(result.exitCode).isEqualTo(KotlinCompilation.ExitCode.COMPILATION_ERROR)
+    assertThat(result.messages).contains(
+        "error: @JsonClass can't be applied to PrivateClass: must be internal or public")
+  }
+
   @Test fun objectDeclarationsNotSupported() {
     val result = compile(kotlin("source.kt",
         """

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

@@ -1171,6 +1171,20 @@ class GeneratedAdaptersTest {
   }
 }
 
+// Regression test for https://github.com/square/moshi/issues/1022
+// Compile-only test
+@JsonClass(generateAdapter = true)
+internal data class MismatchParentAndNestedClassVisibility(
+    val type: Int,
+    val name: String? = null
+) {
+
+  @JsonClass(generateAdapter = true)
+  data class NestedClass(
+      val nestedProperty: String
+  )
+}
+
 // Has to be outside to avoid Types seeing an owning class
 @JsonClass(generateAdapter = true)
 data class NullableTypeParams<T>(