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

Upstream KSP implementation #1393

Merged
merged 29 commits into from
Oct 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
203ecbc
Add KSP deps
ZacSweers Sep 7, 2021
a8b6679
Initial code drop
ZacSweers Sep 7, 2021
4302951
Consolidate some shared proguard rules writing logic
ZacSweers Sep 7, 2021
31fe0ba
Kotlin 1.5.30 and use KSP for autoservice
ZacSweers Sep 7, 2021
5a34746
Remove remaining incap refs
ZacSweers Sep 7, 2021
8a9dea6
Configure CI matrix for testing
ZacSweers Sep 10, 2021
497bfbc
Add extra KSP tests
ZacSweers Sep 10, 2021
45e8290
Clean up properties
ZacSweers Sep 10, 2021
66a3933
Remove kotlin toolchain debugging
ZacSweers Sep 10, 2021
5d7c9af
Spotless
ZacSweers Sep 10, 2021
bac1f8e
Simplify KSP test
ZacSweers Sep 10, 2021
637357e
Make kapt happy in unit tests
ZacSweers Sep 10, 2021
8b4192d
Update KCT
ZacSweers Sep 10, 2021
9841567
Ignore weird KSP issue for now
ZacSweers Sep 10, 2021
2f852c5
Misc cleanups
ZacSweers Sep 10, 2021
bd4fd96
Extract and share Options
ZacSweers Sep 10, 2021
7da1e39
Un-ignore tests and force latest KSP impl in tests
ZacSweers Sep 13, 2021
2408e77
Fix double extension in ProguardRules
ZacSweers Sep 13, 2021
7b7b3aa
Kotlin 1.5.31 and dokka 1.5.30
ZacSweers Sep 25, 2021
d8a7685
Add kotlinpoet ksp deps
ZacSweers Sep 25, 2021
b816f98
Update kotlinpoet to 1.10.1
ZacSweers Sep 25, 2021
d0171e7
Switch to KSP extensions + share typealiasunwrapping
ZacSweers Sep 25, 2021
0bc41b2
Work around kapt annoyingness
ZacSweers Sep 25, 2021
ff8c8d4
Fix + enable the complex self-referencing typevar test
ZacSweers Sep 25, 2021
07faa9f
Remove leftover comment
ZacSweers Sep 25, 2021
5d1792a
Switch to shaded utils for experimental KSP APIs
ZacSweers Oct 15, 2021
7e8374e
Another fix
ZacSweers Oct 16, 2021
0eef2a2
Simplify
ZacSweers Oct 16, 2021
d808086
Use toKModifier helper
ZacSweers Oct 16, 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
4 changes: 3 additions & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ on: [push, pull_request]

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

strategy:
fail-fast: false
matrix:
java-version:
- 16
use-ksp: [ true, false ]

steps:
- name: Checkout
Expand All @@ -36,7 +38,7 @@ jobs:
java-version: ${{ matrix.java-version }}

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

- name: Publish (default branch only)
if: github.repository == 'square/moshi' && github.ref == 'refs/heads/master' && matrix.java-version == '16'
Expand Down
32 changes: 15 additions & 17 deletions gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -16,34 +16,32 @@

# For Dokka https://github.com/Kotlin/dokka/issues/1405
org.gradle.jvmargs=-Xmx2048m -Dfile.encoding=UTF-8 \
--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED

# TODO move this to DSL in Kotlin 1.5.30 https://youtrack.jetbrains.com/issue/KT-44266
kotlin.daemon.jvmargs=-Dfile.encoding=UTF-8 \
--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED

# Kapt doesn't read the above jvm args when workers are used
# https://youtrack.jetbrains.com/issue/KT-48402
kapt.use.worker.api=false
kapt.include.compile.classpath=false

GROUP=com.squareup.moshi
Expand Down
21 changes: 12 additions & 9 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,17 @@
[versions]
autoService = "1.0"
gjf = "1.11.0"
incap = "0.3"
jvmTarget = "1.8"
kotlin = "1.5.21"
kotlinCompileTesting = "1.4.3"
kotlinpoet = "1.10.0"
kotlin = "1.5.31"
kotlinCompileTesting = "1.4.4"
kotlinpoet = "1.10.1"
ksp = "1.5.31-1.0.0"
ktlint = "0.41.0"

[plugins]
dokka = { id = "org.jetbrains.dokka", version = "1.5.0" }
dokka = { id = "org.jetbrains.dokka", version = "1.5.30" }
japicmp = { id = "me.champeau.gradle.japicmp", version = "0.2.9" }
ksp = { id = "com.google.devtools.ksp", version.ref = "ksp" }
mavenPublish = { id = "com.vanniktech.maven.publish", version = "0.17.0" }
mavenShadow = { id = "com.github.johnrengelman.shadow", version = "7.0.0" }
spotless = { id = "com.diffplug.spotless", version = "5.14.2" }
Expand All @@ -33,20 +34,22 @@ spotless = { id = "com.diffplug.spotless", version = "5.14.2" }
asm = "org.ow2.asm:asm:9.2"
autoCommon = "com.google.auto:auto-common:1.1"
autoService = { module = "com.google.auto.service:auto-service-annotations", version.ref = "autoService" }
autoService-processor = { module = "com.google.auto.service:auto-service", version.ref = "autoService" }
guava = { module = "com.google.guava:guava", version = "30.1.1-jre" }
incap = { module = "net.ltgt.gradle.incap:incap", version.ref = "incap" }
incap-processor = { module = "net.ltgt.gradle.incap:incap-processor", version.ref = "incap" }
autoService-ksp = "dev.zacsweers.autoservice:auto-service-ksp:1.0.0"
guava = "com.google.guava:guava:30.1.1-jre"
jsr305 = "com.google.code.findbugs:jsr305:3.0.2"
kotlin-compilerEmbeddable = { module = "org.jetbrains.kotlin:kotlin-compiler-embeddable", version.ref = "kotlin" }
kotlin-reflect = { module = "org.jetbrains.kotlin:kotlin-reflect", version.ref = "kotlin" }
kotlinpoet = { module = "com.squareup:kotlinpoet", version.ref = "kotlinpoet" }
kotlinpoet-metadata = { module = "com.squareup:kotlinpoet-metadata", version.ref = "kotlinpoet" }
kotlinpoet-ksp = { module = "com.squareup:kotlinpoet-ksp", version.ref = "kotlinpoet" }
kotlinxMetadata = "org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.3.0"
ksp = { module = "com.google.devtools.ksp:symbol-processing", version.ref = "ksp" }
ksp-api = { module = "com.google.devtools.ksp:symbol-processing-api", version.ref = "ksp" }
okio = "com.squareup.okio:okio:2.10.0"

# Test libs
assertj = "org.assertj:assertj-core:3.11.1"
junit = "junit:junit:4.13.2"
kotlinCompileTesting = { module = "com.github.tschuchortdev:kotlin-compile-testing", version.ref = "kotlinCompileTesting" }
kotlinCompileTesting-ksp = { module = "com.github.tschuchortdev:kotlin-compile-testing-ksp", version.ref ="kotlinCompileTesting" }
truth = "com.google.truth:truth:1.1.3"
38 changes: 25 additions & 13 deletions 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")
kotlin("kapt")
alias(libs.plugins.ksp)
id("com.vanniktech.maven.publish")
alias(libs.plugins.mavenShadow)
}
Expand All @@ -31,12 +31,12 @@ tasks.withType<KotlinCompile>().configureEach {
freeCompilerArgs += listOf(
"-Xopt-in=kotlin.RequiresOptIn",
"-Xopt-in=com.squareup.kotlinpoet.metadata.KotlinPoetMetadataPreview",
"-Xopt-in=com.squareup.kotlinpoet.ksp.KotlinPoetKspPreview",
)
}
}

tasks.withType<Test>().configureEach {
// For kapt to work with kotlin-compile-testing
jvmArgs(
"--add-opens=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
Expand All @@ -47,17 +47,15 @@ tasks.withType<Test>().configureEach {
"--add-opens=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED"
)
}

val shade: Configuration = configurations.maybeCreate("compileShaded")
configurations.getByName("compileOnly").extendsFrom(shade)
dependencies {
// Use `api` because kapt will not resolve `runtime` dependencies without it, only `compile`
// https://youtrack.jetbrains.com/issue/KT-41702
api(project(":moshi"))
api(kotlin("reflect"))
implementation(project(":moshi"))
implementation(kotlin("reflect"))
shade(libs.kotlinxMetadata) {
exclude(group = "org.jetbrains.kotlin", module = "kotlin-stdlib")
}
Expand All @@ -67,16 +65,30 @@ dependencies {
exclude(group = "com.squareup", module = "kotlinpoet")
exclude(group = "com.google.guava")
}
api(libs.guava)
api(libs.asm)
shade(libs.kotlinpoet.ksp) {
exclude(group = "org.jetbrains.kotlin")
exclude(group = "com.squareup", module = "kotlinpoet")
}
implementation(libs.guava)
implementation(libs.asm)

implementation(libs.autoService)
ksp(libs.autoService.ksp)

api(libs.autoService)
kapt(libs.autoService.processor)
api(libs.incap)
kapt(libs.incap.processor)
// KSP deps
compileOnly(libs.ksp)
compileOnly(libs.ksp.api)
compileOnly(libs.kotlin.compilerEmbeddable)
// Always force the latest KSP version to match the one we're compiling against
testImplementation(libs.ksp)
testImplementation(libs.kotlin.compilerEmbeddable)
ZacSweers marked this conversation as resolved.
Show resolved Hide resolved
testImplementation(libs.kotlinCompileTesting.ksp)

// Copy these again as they're not automatically included since they're shaded
testImplementation(project(":moshi"))
testImplementation(kotlin("reflect"))
testImplementation(libs.kotlinpoet.metadata)
testImplementation(libs.kotlinpoet.ksp)
testImplementation(libs.junit)
testImplementation(libs.truth)
testImplementation(libs.kotlinCompileTesting)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (C) 2021 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.moshi.kotlin.codegen.api

import com.squareup.kotlinpoet.ClassName

internal object Options {
/**
* This processing option can be specified to have a `@Generated` annotation
* included in the generated code. It is not encouraged unless you need it for static analysis
* reasons and not enabled by default.
*
* Note that this can only be one of the following values:
* * `"javax.annotation.processing.Generated"` (JRE 9+)
* * `"javax.annotation.Generated"` (JRE <9)
*/
const val OPTION_GENERATED: String = "moshi.generated"
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻


/**
* This boolean processing option can disable proguard rule generation.
* Normally, this is not recommended unless end-users build their own JsonAdapter look-up tool.
* This is enabled by default.
*/
const val OPTION_GENERATE_PROGUARD_RULES: String = "moshi.generateProguardRules"

val POSSIBLE_GENERATED_NAMES = arrayOf(
ClassName("javax.annotation.processing", "Generated"),
ClassName("javax.annotation", "Generated")
).associateBy { it.canonicalName }
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
package com.squareup.moshi.kotlin.codegen.api

import com.squareup.kotlinpoet.ClassName
import javax.annotation.processing.Filer
import javax.lang.model.element.Element
import javax.tools.StandardLocation

/**
* Represents a proguard configuration for a given spec. This covers three main areas:
Expand All @@ -42,16 +39,11 @@ internal data class ProguardConfig(
val targetConstructorParams: List<String>,
val qualifierProperties: Set<QualifierAdapterProperty>
) {
private val outputFile = "META-INF/proguard/moshi-${targetClass.canonicalName}.pro"

/** Writes this to `filer`. */
fun writeTo(filer: Filer, vararg originatingElements: Element) {
filer.createResource(StandardLocation.CLASS_OUTPUT, "", outputFile, *originatingElements)
.openWriter()
.use(::writeTo)
fun outputFilePathWithoutExtension(canonicalName: String): String {
return "META-INF/proguard/moshi-$canonicalName"
}

private fun writeTo(out: Appendable): Unit = out.run {
fun writeTo(out: Appendable): Unit = out.run {
//
// -if class {the target class}
// -keepnames class {the target class}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,12 @@ internal fun List<TypeName>.toTypeVariableResolver(
// replacement later that may add bounds referencing this.
val id = typeVar.name
parametersMap[id] = TypeVariableName(id)
}

for (typeVar in this) {
check(typeVar is TypeVariableName)
// Now replace it with the full version.
parametersMap[id] = typeVar.deepCopy(null) { it.stripTypeVarVariance(resolver) }
parametersMap[typeVar.name] = typeVar.deepCopy(null) { it.stripTypeVarVariance(resolver) }
}

return resolver
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright (C) 2021 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.squareup.moshi.kotlin.codegen.api

import com.squareup.kotlinpoet.AnnotationSpec
import com.squareup.kotlinpoet.ClassName
import com.squareup.kotlinpoet.LambdaTypeName
import com.squareup.kotlinpoet.ParameterizedTypeName
import com.squareup.kotlinpoet.TypeName
import com.squareup.kotlinpoet.TypeVariableName
import com.squareup.kotlinpoet.WildcardTypeName
import com.squareup.kotlinpoet.tag
import com.squareup.kotlinpoet.tags.TypeAliasTag
import java.util.TreeSet

internal fun TypeName.unwrapTypeAliasReal(): TypeName {
ZacSweers marked this conversation as resolved.
Show resolved Hide resolved
return tag<TypeAliasTag>()?.abbreviatedType?.let { unwrappedType ->
// If any type is nullable, then the whole thing is nullable
var isAnyNullable = isNullable
ZacSweers marked this conversation as resolved.
Show resolved Hide resolved
// 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
nestedUnwrappedType.copy(nullable = isAnyNullable, annotations = runningAnnotations.toList())
} ?: this
ZacSweers marked this conversation as resolved.
Show resolved Hide resolved
}

internal fun TypeName.unwrapTypeAlias(): TypeName {
return when (this) {
is ClassName -> unwrapTypeAliasReal()
is ParameterizedTypeName -> {
if (TypeAliasTag::class in tags) {
unwrapTypeAliasReal()
} else {
deepCopy(TypeName::unwrapTypeAlias)
}
}
is TypeVariableName -> {
if (TypeAliasTag::class in tags) {
unwrapTypeAliasReal()
} else {
deepCopy(transform = TypeName::unwrapTypeAlias)
}
}
is WildcardTypeName -> {
if (TypeAliasTag::class in tags) {
unwrapTypeAliasReal()
} else {
deepCopy(TypeName::unwrapTypeAlias)
}
}
is LambdaTypeName -> {
if (TypeAliasTag::class in tags) {
unwrapTypeAliasReal()
} else {
deepCopy(TypeName::unwrapTypeAlias)
}
}
else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t understand who this message is for.

If you’re blaming the caller it should be an IllegalStateException saying that this function doesn’t support its input.

If you’re blaming this function itself for being incomplete then it should be an AssertionError saying that the case is unexpected.

As is if I get this crash I don’t know where blame lies. As a reviewer I don’t even know!

Copy link
Collaborator

Choose a reason for hiding this comment

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

(this feedback is on code that predates this PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is only possible for Dynamic types, which we don't support here. I suppose we could support it in the future, but for now while we're JVM-only it's not even possible to reach this case. I can rename else to Dynamic to make it more explicit, but I don't know necessarily what the right way to rule that out is in future versions. wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made dynamic explicit here b62ec84

}
}
Loading