Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support direct annotation instantiation in code gen on Kotlin 1.6 #1390

Merged
merged 26 commits into from
Oct 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
18b1a92
Set language version to 1.6
ZacSweers Sep 4, 2021
c1144dc
Prototype direct annotation instantiation
ZacSweers Sep 4, 2021
3093f78
Gate generation on language version
ZacSweers Sep 4, 2021
b4b52f5
Don't require FIELD access
ZacSweers Sep 4, 2021
3abf7ee
Add a toe-hold for language version testing
ZacSweers Sep 4, 2021
a103ef7
Support language version in CI
ZacSweers Sep 4, 2021
b9ea6de
(test) Try with 1.6 M1 from kotlin-dev repo
ZacSweers Sep 13, 2021
dcbb0b0
1.6.0-M1
ZacSweers Sep 24, 2021
0801da4
Rewire CI for 1.6 milestone
ZacSweers Sep 24, 2021
70c1639
Fix CI versions
ZacSweers Sep 24, 2021
3b23543
One more fix
ZacSweers Sep 24, 2021
163f7ff
Update to 1.6.0-RC
ZacSweers Oct 12, 2021
54c0f73
Add option and wire into both
ZacSweers Oct 16, 2021
2cd0baa
Add Kotlin version to build title
ZacSweers Oct 16, 2021
34089ec
Fixup snapshots
ZacSweers Oct 16, 2021
5986666
Don't check FIELD when instantiating annotations in KSP
ZacSweers Oct 16, 2021
22265c7
Add CI support to match ksp and kotlin RCs
ZacSweers Oct 16, 2021
bca3683
Fix plugin applications now that it's on the buildscript classpath
ZacSweers Oct 16, 2021
a4a51ea
Parameterize tests to cover different behaviors
ZacSweers Oct 16, 2021
38f2bbf
Cover behaviors in JsonClassSymbolProcessorTest
ZacSweers Oct 16, 2021
684ab6a
Make the test all work safely on diff versions
ZacSweers Oct 16, 2021
17fb7a8
Why is this so fickle
ZacSweers Oct 16, 2021
f386977
Merge branch 'master' into z/prototypeAnnotationInstantiation
ZacSweers Oct 22, 2021
f4b33d2
Update KotlinPoet to 1.10.2
ZacSweers Oct 22, 2021
9e77200
Add explanatory comment
ZacSweers Oct 22, 2021
b80a7f4
Use arrayOf in KSP too
ZacSweers Oct 22, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,24 @@ on: [push, pull_request]

jobs:
build:
name: 'Java ${{ matrix.java-version }} | KSP ${{ matrix.use-ksp }}'
name: 'Java ${{ matrix.java-version }} | Kotlin ${{ matrix.kotlin-version }} | KSP ${{ matrix.use-ksp }}'
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
use-ksp: [ true, false ]
kotlin-version: [ '1.5.31', '1.6.0-RC' ]
ksp-version: [ '1.5.31-1.0.0', '1.6.0-RC-1.0.0' ]
exclude:
- kotlin-version: '1.5.31'
ksp-version: '1.6.0-RC-1.0.0'
- kotlin-version: '1.6.0-RC'
ksp-version: '1.5.31-1.0.0'

env:
MOSHI_KOTLIN_VERSION: '${{ matrix.kotlin-version }}'
MOSHI_KSP_VERSION: '${{ matrix.ksp-version }}'

steps:
- name: Checkout
Expand All @@ -36,8 +47,8 @@ jobs:
java-version: '17'

- name: Test
run: ./gradlew build check --stacktrace -PuseKsp=${{ matrix.use-ksp }}
run: ./gradlew build check --stacktrace -PuseKsp=${{ matrix.use-ksp }} -PkotlinVersion=${{ matrix.kotlin-version }}

- name: Publish (default branch only)
if: github.repository == 'square/moshi' && github.ref == 'refs/heads/master'
if: github.repository == 'square/moshi' && github.ref == 'refs/heads/master' && matrix.kotlin-version == '1.5.31' && matrix.use-ksp == 'false'
run: ./gradlew publish -PmavenCentralUsername=${{ secrets.SONATYPE_NEXUS_USERNAME }} -PmavenCentralPassword=${{ secrets.SONATYPE_NEXUS_PASSWORD }} --stacktrace
12 changes: 9 additions & 3 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ import java.net.URL

buildscript {
dependencies {
classpath(kotlin("gradle-plugin", version = libs.versions.kotlin.get()))
val kotlinVersion = System.getenv("MOSHI_KOTLIN_VERSION")
?: libs.versions.kotlin.get()
val kspVersion = System.getenv("MOSHI_KSP_VERSION")
?: libs.versions.ksp.get()
classpath(kotlin("gradle-plugin", version = kotlinVersion))
classpath("com.google.devtools.ksp:symbol-processing-gradle-plugin:$kspVersion")
// https://github.com/melix/japicmp-gradle-plugin/issues/36
classpath("com.google.guava:guava:28.2-jre")
}
Expand Down Expand Up @@ -125,8 +130,9 @@ subprojects {
pluginManager.withPlugin("org.jetbrains.kotlin.jvm") {
tasks.withType<KotlinCompile>().configureEach {
kotlinOptions {
@Suppress("SuspiciousCollectionReassignment")
freeCompilerArgs += listOf("-progressive")
// TODO re-enable when no longer supporting multiple kotlin versions
// @Suppress("SuspiciousCollectionReassignment")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete if not needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, will remove before merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually going to leave it but with a comment explaining it's just temporary while we support dual kotlin versions

// freeCompilerArgs += listOf("-progressive")
jvmTarget = libs.versions.jvmTarget.get()
}
}
Expand Down
2 changes: 1 addition & 1 deletion gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ gjf = "1.11.0"
jvmTarget = "1.8"
kotlin = "1.5.31"
kotlinCompileTesting = "1.4.4"
kotlinpoet = "1.10.1"
kotlinpoet = "1.10.2"
ksp = "1.5.31-1.0.0"
ktlint = "0.41.0"

Expand Down
2 changes: 1 addition & 1 deletion kotlin/codegen/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
kotlin("jvm")
alias(libs.plugins.ksp)
id("com.google.devtools.ksp")
id("com.vanniktech.maven.publish")
alias(libs.plugins.mavenShadow)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,26 +185,31 @@ internal class AdapterGenerator(
result.addAnnotation(COMMON_SUPPRESS)
result.addType(generatedAdapter)
val proguardConfig = if (generateProguardRules) {
generatedAdapter.createProguardRule()
generatedAdapter.createProguardRule(target.instantiateAnnotations)
} else {
null
}
return PreparedAdapter(result.build(), proguardConfig)
}

private fun TypeSpec.createProguardRule(): ProguardConfig {
val adapterProperties = propertySpecs
.asSequence()
.filter { prop ->
prop.type.rawType() == JsonAdapter::class.asClassName()
}
.filter { prop -> prop.annotations.isNotEmpty() }
.mapTo(mutableSetOf()) { prop ->
QualifierAdapterProperty(
name = prop.name,
qualifiers = prop.annotations.mapTo(mutableSetOf()) { it.typeName.rawType() }
)
}
private fun TypeSpec.createProguardRule(instantiateAnnotations: Boolean): ProguardConfig {
val adapterProperties = if (instantiateAnnotations) {
// Don't need to do anything special if we instantiate them directly!
emptySet()
} else {
propertySpecs
.asSequence()
.filter { prop ->
prop.type.rawType() == JsonAdapter::class.asClassName()
}
.filter { prop -> prop.annotations.isNotEmpty() }
.mapTo(mutableSetOf()) { prop ->
QualifierAdapterProperty(
name = prop.name,
qualifiers = prop.annotations.mapTo(mutableSetOf()) { it.typeName.rawType() }
)
}
}

val adapterConstructorParams = when (requireNotNull(primaryConstructor).parameters.size) {
1 -> listOf(CN_MOSHI.reflectionName())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.CodeBlock
import com.squareup.kotlinpoet.KModifier
import com.squareup.kotlinpoet.MemberName
import com.squareup.kotlinpoet.NameAllocator
Expand All @@ -29,14 +30,16 @@ import com.squareup.kotlinpoet.TypeVariableName
import com.squareup.kotlinpoet.WildcardTypeName
import com.squareup.kotlinpoet.asClassName
import com.squareup.kotlinpoet.asTypeName
import com.squareup.kotlinpoet.joinToCode
import com.squareup.moshi.JsonAdapter
import com.squareup.moshi.Types
import java.util.Locale

/** A JsonAdapter that can be used to encode and decode a particular field. */
internal data class DelegateKey(
private val type: TypeName,
private val jsonQualifiers: List<AnnotationSpec>
private val jsonQualifiers: List<AnnotationSpec>,
private val instantiateAnnotations: Boolean
) {
val nullable get() = type.isNullable

Expand All @@ -60,8 +63,12 @@ internal data class DelegateKey(
moshiParameter,
typeRenderer.render(type)
)

val (initializerString, args) = when {
jsonQualifiers.isEmpty() -> ", %M()" to arrayOf(MemberName("kotlin.collections", "emptySet"))
instantiateAnnotations -> {
", setOf(%L)" to arrayOf(jsonQualifiers.map { it.asInstantiationExpression() }.joinToCode())
}
else -> {
", %T.getFieldJsonQualifierAnnotations(javaClass, " +
"%S)" to arrayOf(Types::class.asTypeName(), adapterName)
Expand All @@ -70,12 +77,25 @@ internal data class DelegateKey(
val finalArgs = arrayOf(*standardArgs, *args, propertyName)

return PropertySpec.builder(adapterName, adapterTypeName, KModifier.PRIVATE)
.addAnnotations(jsonQualifiers)
.apply {
if (!instantiateAnnotations) {
addAnnotations(jsonQualifiers)
}
}
.initializer("%N.adapter(%L$initializerString, %S)", *finalArgs)
.build()
}
}

private fun AnnotationSpec.asInstantiationExpression(): CodeBlock {
// <Type>(args)
return CodeBlock.of(
"%T(%L)",
typeName,
members.joinToCode()
)
}

/**
* Returns a suggested variable name derived from a list of type names. This just concatenates,
* yielding types like MapOfStringLong.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ internal object Options {
*/
const val OPTION_GENERATE_PROGUARD_RULES: String = "moshi.generateProguardRules"

/**
* This boolean processing option controls whether or not Moshi will directly instantiate
* JsonQualifier annotations in Kotlin 1.6+. Note that this is enabled by default in Kotlin 1.6
* but can be disabled to restore the legacy behavior of storing annotations on generated adapter
* fields and looking them up reflectively.
*/
const val OPTION_INSTANTIATE_ANNOTATIONS: String = "moshi.instantiateAnnotations"

val POSSIBLE_GENERATED_NAMES = arrayOf(
ClassName("javax.annotation.processing", "Generated"),
ClassName("javax.annotation", "Generated")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ internal data class TargetType(
val properties: Map<String, TargetProperty>,
val typeVariables: List<TypeVariableName>,
val isDataClass: Boolean,
val visibility: KModifier
val visibility: KModifier,
val instantiateAnnotations: Boolean
) {

init {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import com.squareup.moshi.JsonClass
import com.squareup.moshi.kotlin.codegen.api.AdapterGenerator
import com.squareup.moshi.kotlin.codegen.api.Options.OPTION_GENERATED
import com.squareup.moshi.kotlin.codegen.api.Options.OPTION_GENERATE_PROGUARD_RULES
import com.squareup.moshi.kotlin.codegen.api.Options.OPTION_INSTANTIATE_ANNOTATIONS
import com.squareup.moshi.kotlin.codegen.api.Options.POSSIBLE_GENERATED_NAMES
import com.squareup.moshi.kotlin.codegen.api.ProguardConfig
import com.squareup.moshi.kotlin.codegen.api.PropertyGenerator
Expand Down Expand Up @@ -59,6 +60,7 @@ public class JsonClassCodegenProcessor : AbstractProcessor() {
private val annotation = JsonClass::class.java
private var generatedType: ClassName? = null
private var generateProguardRules: Boolean = true
private var instantiateAnnotations: Boolean = true

override fun getSupportedAnnotationTypes(): Set<String> = setOf(annotation.canonicalName)

Expand All @@ -76,6 +78,7 @@ public class JsonClassCodegenProcessor : AbstractProcessor() {
}

generateProguardRules = processingEnv.options[OPTION_GENERATE_PROGUARD_RULES]?.toBooleanStrictOrNull() ?: true
instantiateAnnotations = processingEnv.options[OPTION_INSTANTIATE_ANNOTATIONS]?.toBooleanStrictOrNull() ?: true

this.types = processingEnv.typeUtils
this.elements = processingEnv.elementUtils
Expand Down Expand Up @@ -135,11 +138,18 @@ public class JsonClassCodegenProcessor : AbstractProcessor() {
element: TypeElement,
cachedClassInspector: MoshiCachedClassInspector
): AdapterGenerator? {
val type = targetType(messager, elements, types, element, cachedClassInspector) ?: return null
val type = targetType(
messager,
elements,
types,
element,
cachedClassInspector,
instantiateAnnotations
) ?: return null

val properties = mutableMapOf<String, PropertyGenerator>()
for (property in type.properties.values) {
val generator = property.generator(messager, element, elements)
val generator = property.generator(messager, element, elements, type.instantiateAnnotations)
if (generator != null) {
properties[property.name] = generator
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ private val VISIBILITY_MODIFIERS = setOf(
KModifier.PROTECTED,
KModifier.PUBLIC
)
private val ANNOTATION_INSTANTIATION_MIN_VERSION = KotlinVersion(1, 6, 0)

private fun Collection<KModifier>.visibility(): KModifier {
return find { it in VISIBILITY_MODIFIERS } ?: KModifier.PUBLIC
Expand Down Expand Up @@ -122,7 +123,8 @@ internal fun targetType(
elements: Elements,
types: Types,
element: TypeElement,
cachedClassInspector: MoshiCachedClassInspector
cachedClassInspector: MoshiCachedClassInspector,
instantiateAnnotationsEnabled: Boolean
): TargetType? {
val typeMetadata = element.getAnnotation(Metadata::class.java)
if (typeMetadata == null) {
Expand Down Expand Up @@ -204,6 +206,11 @@ internal fun targetType(
}
}

val instantiateAnnotations = instantiateAnnotationsEnabled && run {
val (major, minor, patch) = typeMetadata.metadataVersion
val languageVersion = KotlinVersion(major, minor, patch)
languageVersion >= ANNOTATION_INSTANTIATION_MIN_VERSION
}
val kotlinApi = cachedClassInspector.toTypeSpec(kmClass)
val typeVariables = kotlinApi.typeVariables
val appliedType = AppliedType.get(element)
Expand Down Expand Up @@ -319,7 +326,8 @@ internal fun targetType(
properties = properties,
typeVariables = typeVariables,
isDataClass = KModifier.DATA in kotlinApi.modifiers,
visibility = resolvedVisibility
visibility = resolvedVisibility,
instantiateAnnotations = instantiateAnnotations
)
}

Expand Down Expand Up @@ -418,7 +426,8 @@ private val TargetProperty.isVisible: Boolean
internal fun TargetProperty.generator(
messager: Messager,
sourceElement: TypeElement,
elements: Elements
elements: Elements,
instantiateAnnotations: Boolean
): PropertyGenerator? {
if (isTransient) {
if (!hasDefault) {
Expand All @@ -429,7 +438,7 @@ internal fun TargetProperty.generator(
)
return null
}
return PropertyGenerator(this, DelegateKey(type, emptyList()), true)
return PropertyGenerator(this, DelegateKey(type, emptyList(), instantiateAnnotations), true)
}

if (!isVisible) {
Expand All @@ -452,6 +461,7 @@ internal fun TargetProperty.generator(
// Check Java types since that covers both Java and Kotlin annotations.
val annotationElement = elements.getTypeElement(qualifierRawType.canonicalName)
?: continue

annotationElement.getAnnotation(Retention::class.java)?.let {
if (it.value != RetentionPolicy.RUNTIME) {
messager.printMessage(
Expand All @@ -460,12 +470,14 @@ internal fun TargetProperty.generator(
)
}
}
annotationElement.getAnnotation(Target::class.java)?.let {
if (ElementType.FIELD !in it.value) {
messager.printMessage(
ERROR,
"JsonQualifier @${qualifierRawType.simpleName} must support FIELD target"
)
if (!instantiateAnnotations) {
annotationElement.getAnnotation(Target::class.java)?.let {
if (ElementType.FIELD !in it.value) {
messager.printMessage(
ERROR,
"JsonQualifier @${qualifierRawType.simpleName} must support FIELD target"
)
}
}
}
}
Expand All @@ -478,7 +490,7 @@ internal fun TargetProperty.generator(

return PropertyGenerator(
this,
DelegateKey(type, jsonQualifierSpecs)
DelegateKey(type, jsonQualifierSpecs, instantiateAnnotations)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import com.squareup.moshi.JsonClass
import com.squareup.moshi.kotlin.codegen.api.AdapterGenerator
import com.squareup.moshi.kotlin.codegen.api.Options.OPTION_GENERATED
import com.squareup.moshi.kotlin.codegen.api.Options.OPTION_GENERATE_PROGUARD_RULES
import com.squareup.moshi.kotlin.codegen.api.Options.OPTION_INSTANTIATE_ANNOTATIONS
import com.squareup.moshi.kotlin.codegen.api.Options.POSSIBLE_GENERATED_NAMES
import com.squareup.moshi.kotlin.codegen.api.ProguardConfig
import com.squareup.moshi.kotlin.codegen.api.PropertyGenerator
Expand Down Expand Up @@ -63,6 +64,8 @@ private class JsonClassSymbolProcessor(
}
}
private val generateProguardRules = environment.options[OPTION_GENERATE_PROGUARD_RULES]?.toBooleanStrictOrNull() ?: true
private val instantiateAnnotations = (environment.options[OPTION_INSTANTIATE_ANNOTATIONS]?.toBooleanStrictOrNull() ?: true) &&
environment.kotlinVersion.isAtLeast(1, 6)

override fun process(resolver: Resolver): List<KSAnnotated> {
val generatedAnnotation = generatedOption?.let {
Expand Down Expand Up @@ -122,11 +125,11 @@ private class JsonClassSymbolProcessor(
resolver: Resolver,
originalType: KSDeclaration,
): AdapterGenerator? {
val type = targetType(originalType, resolver, logger) ?: return null
val type = targetType(originalType, resolver, logger, instantiateAnnotations) ?: return null

val properties = mutableMapOf<String, PropertyGenerator>()
for (property in type.properties.values) {
val generator = property.generator(logger, resolver, originalType)
val generator = property.generator(logger, resolver, originalType, type.instantiateAnnotations)
if (generator != null) {
properties[property.name] = generator
}
Expand Down
Loading