From fb5dd0816866d3be94dd735f92b72bcf2a3e128a Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sun, 5 Dec 2021 20:34:37 -0500 Subject: [PATCH] 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 --- .../kotlin/codegen/api/typeAliasUnwrapping.kt | 38 +++------ .../moshi/kotlin/codegen/apt/AppliedType.kt | 23 ++++-- .../moshi/kotlin/codegen/apt/metadata.kt | 26 +++--- .../moshi/kotlin/codegen/ksp/AppliedType.kt | 6 +- .../ksp/JsonClassSymbolProcessorProvider.kt | 79 +++++++++---------- .../moshi/kotlin/codegen/ksp/TargetTypes.kt | 10 +-- .../ksp/JsonClassSymbolProcessorTest.kt | 3 +- .../squareup/moshi/kotlin/DualKotlinTest.kt | 2 +- 8 files changed, 85 insertions(+), 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 index 87dd398..1e6b598 100644 --- 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()?.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(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 index 2d09ff5..e619229 100644 --- 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` with enough information to know how to resolve its type * variables. @@ -27,8 +33,9 @@ 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 = LinkedHashSet() ): LinkedHashSet { @@ -36,8 +43,14 @@ internal class AppliedType private constructor( 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 } @@ -45,7 +58,7 @@ internal class AppliedType private constructor( 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 index 5deb7c4..5666041 100644 --- 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() val resolvedTypes = mutableListOf() - 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 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 index 1038bae..f820bb4 100644 --- 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 { val result: LinkedHashSet = 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/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt b/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt index 9776957..f324f6f 100644 --- a/kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt +++ b/kotlin/codegen/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 { 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() ?: 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() ?: 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 index daf8de1..a6561fe 100644 --- 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() 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/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt b/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt index c7cccb3..17732d7 100644 --- a/kotlin/codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt +++ b/kotlin/codegen/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 index 19295c0..a615000 100644 --- 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 = // """{}"""