diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/AppliedType.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/AppliedType.kt index 91d9de2..9821476 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/AppliedType.kt +++ b/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 = mutableSetOf() - ): Set { + result: LinkedHashSet = LinkedHashSet() + ): LinkedHashSet { result.add(this) for (supertype in types.directSupertypes(mirror)) { val supertypeDeclaredType = supertype as DeclaredType diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessor.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessor.kt index a5e19f8..bfc2f72 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessor.kt +++ b/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, 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? { - val type = targetType(messager, elements, types, element) ?: return null + private fun adapterGenerator(element: TypeElement, cachedClassInspector: MoshiCachedClassInspector): AdapterGenerator? { + val type = targetType(messager, elements, types, element, cachedClassInspector) ?: return null val properties = mutableMapOf() for (property in type.properties.values) { diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/MoshiCachedClassInspector.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/MoshiCachedClassInspector.kt new file mode 100644 index 0000000..2a8b446 --- /dev/null +++ b/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() + private val kmClassToSpecCache = mutableMapOf() + private val metadataToKmClassCache = mutableMapOf() + + 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)) + } + } +} \ No newline at end of file diff --git a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt index 21d6fe6..e2cfe52 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/metadata.kt +++ b/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 = kmClass.toTypeSpec(elementHandler) + val kotlinApi = cachedClassInspector.toTypeSpec(kmClass) val typeVariables = kotlinApi.typeVariables val appliedType = AppliedType.get(element) @@ -176,46 +178,62 @@ internal fun targetType(messager: Messager, } val properties = mutableMapOf() - for (supertype in appliedType.supertypes(types)) { - if (supertype.element.asClassName() == OBJECT_CLASS) { - continue // Don't load properties for java.lang.Object. - } - if (supertype.element.kind != ElementKind.CLASS) { - continue // Don't load properties for interface types. - } - 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) - } + 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) { + messager.printMessage(Diagnostic.Kind.ERROR, + "@JsonClass can't be applied to $element: supertype $supertype is not a Kotlin type", + element) + return null + } + } + .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) { it.enclosingElement } + .filterIsInstance() + .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 = typeElement.toTypeSpec(elementHandler) + kotlinApi: TypeSpec ): Map { val result = mutableMapOf() @@ -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") + } diff --git a/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessorTest.kt b/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessorTest.kt index 410d812..03cb650 100644 --- a/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/JsonClassCodegenProcessorTest.kt +++ b/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", """ diff --git a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/GeneratedAdaptersTest.kt b/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/GeneratedAdaptersTest.kt index a583841..00a6266 100644 --- a/kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/codegen/GeneratedAdaptersTest.kt +++ b/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(