Upstream KSP implementation #1393

ZacSweers · 2021-09-10T02:17:49Z

This upstreams the moshi-ksp implementation from https://github.com/zacsweers/moshix.

Highlights

KSP lives in a codegen.ksp subpackage alongside the existing apt version
They reuse the same integration tests in :moshi:kotlin:tests. The KSP implementation is functionally compatible!
Bringing along a few KSP-specific regression tests
- ~~One is presented but commented out until next KotlinPoet release as we ported the same fix there - Fix self-referencing type variables in metadata parsing kotlinpoet#1137~~
CI runs a matrix of both kapt and KSP.
- This also made me realize we could possibly use a CI matrix to truly achieve what Fix dualkotlintest #1064 was trying to do (but that's for another PR)
Switches to auto-service-ksp for for AutoService support and removes kapt itself from the codegen project, as I was really tired of dealing with Kapt's strange shenanigans on JDK 16.

Some other things to do:

Consolidate options constants
~~Consolidate processor tests with a parameterized form?~~ Will save this for a later PR after Descriptor wasn't found for declaration CLASS when processing generated code in a later round google/ksp#621 is resolved
??

ZacSweers · 2021-09-10T19:32:51Z

Some of these KSP - KotlinPoet bits definitely have a future in KotlinPoet itself, but that's for another day

kotlin/codegen/build.gradle.kts

.../codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt

So tired of kapt oh my god

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/typeAliasUnwrapping.kt

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt

evant · 2021-10-15T21:43:13Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt

+        addAnnotation(Transient::class)
+      }
+      addAnnotations(
+        this@toPropertySpec.annotations


Don't know if there's an intention to support this, but I noticed a difference from kapt if you qualify the annotation with get/set, ex:

class ClassWithQualiifer(@get:MyQualifier val a: Int)

will see the qualifier in kapt, but ignore it in ksp.

If you do want to look at these, you need to check getter.annotations and/or setter.annotations as well.

Yup we don't really have a clear story there as far as what we check, but officially our stance in other issues has been that only properties are supported and anything else is undefined

I'd like to straighten it out more but will save that for a later PR

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/KspUtil.kt

evant

New annotation methods look much cleaner!

ZacSweers · 2021-10-16T05:52:18Z

Thanks for the review!

swankjesse · 2021-12-05T19:36:40Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/Options.kt

+   *   * `"javax.annotation.processing.Generated"` (JRE 9+)
+   *   * `"javax.annotation.Generated"` (JRE <9)
+   */
+  const val OPTION_GENERATED: String = "moshi.generated"


kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/typeAliasUnwrapping.kt

swankjesse · 2021-12-05T19:45:59Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/typeAliasUnwrapping.kt

+        deepCopy(TypeName::unwrapTypeAlias)
+      }
+    }
+    else -> throw UnsupportedOperationException("Type '${javaClass.simpleName}' is illegal. Only classes, parameterized types, wildcard types, or type variables are allowed.")


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!

(this feedback is on code that predates this PR)

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?

Made dynamic explicit here b62ec84

swankjesse · 2021-12-05T20:43:59Z

...egen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorProvider.kt

+        val adapterGenerator = adapterGenerator(logger, resolver, type) ?: return emptyList()
+        try {
+          val preparedAdapter = adapterGenerator
+            .prepare(generateProguardRules) { spec ->


Would this be simpler as two functions?

the whole process() function or splitting prepare and writing?

swankjesse · 2021-12-05T20:55:56Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/KspUtil.kt

+  }
+}
+
+internal fun KSAnnotation.toAnnotationSpec(resolver: Resolver): AnnotationSpec {


Yeah KotlinPoet wants to eat this

PR for a later day!

swankjesse · 2021-12-05T20:56:18Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/KspUtil.kt

+  else -> CodeBlock.of("%L", value)
+}
+
+internal inline fun KSPLogger.check(condition: Boolean, message: () -> String) {


kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt

swankjesse

LGTM

Lots of feedback; I might send PRs to address some. (Also please feel free to send your own follow ups!)

swankjesse · 2021-12-05T21:08:38Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/TargetTypes.kt

+      parameter = parameter,
+      visibility = property.getVisibility().toKModifier() ?: KModifier.PUBLIC,
+      jsonName = parameter?.jsonName ?: property.jsonName()
+        ?: name.escapeDollarSigns()


This escapeDollarSigns call reads very suspicious to me . .. why doesn’t it escape when it comes from a parameter or property JSON name

https://github.com/square/moshi/pull/1446/files

this is leftover from kapt (which didn't escape them) and I believe is still necessary here too. I can test, basically it shows fine in the annotation string but doesn't play nice in generated code because of Kotlin templating

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt

swankjesse · 2021-12-05T21:12:30Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt

+
+internal fun <T : Annotation> KSAnnotated.getAnnotationsByType(annotationKClass: KClass<T>): Sequence<T> {
+  return this.annotations.filter {
+    it.shortName.getShortName() == annotationKClass.simpleName && it.annotationType.resolve().declaration


any idea what the utility of the short name check is?

Simple early check before doing a more expensive resolve() call in the second check

swankjesse · 2021-12-05T21:13:59Z

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/ksp/shadedUtil.kt

+  getAnnotationsByType(annotationKClass).firstOrNull() != null
+
+@Suppress("UNCHECKED_CAST")
+private fun <T : Annotation> KSAnnotation.toAnnotation(annotationClass: Class<T>): T {


Whoa, I dislike this. What the heck are we doing instantiating an annotation instance from the processed code inside the processor?

Let’s clean this up to definitely not use a dynamic proxy to create an annotation instance; this is a ton of mechanism to just read the properties of @Json and @JsonClass and @Retention

This is what apt is doing too though, that's why KSP added it as a familiar API. Parsing members from the annotations themselves were actually quite messy. We can go back to that, but this is supposed to be the long term idiomatic mechanism

Why is it in our code and not KSP?

swankjesse · 2021-12-05T21:17:49Z

.../codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt

+import kotlin.reflect.full.createType
+
+/** Execute kotlinc to confirm that either files are generated or errors are printed. */
+class JsonClassSymbolProcessorTest {


.../codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt

swankjesse · 2021-12-05T21:20:55Z

.../codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt

+
+    // We're checking here that we only generate one `stringAdapter` that's used for both the
+    // regular string properties as well as the the aliased ones.
+    // TODO loading compiled classes from results not supported in KSP yet


Replace this assertion with confirming how many files were generated?

number of files wouldn't verify anything. This is about the number of adapter properties in a file

swankjesse · 2021-12-05T21:22:06Z

.../codegen/src/test/java/com/squareup/moshi/kotlin/codegen/ksp/JsonClassSymbolProcessorTest.kt

+  }
+
+  @Test
+  fun `Processor should generate comprehensive proguard rules`() {


This is not a good test, but it serves as nice raw material from which good tests could be created. Rather than covering an assortment of features in one scattershot test, we aught to test each of the interesting features each in their own tests.

Let's do a dedicated PR for that, the existing apt tests are the same and it'd be pretty involved

That'd be great! I'm more ranting than anything.

swankjesse · 2021-12-05T21:22:49Z

kotlin/tests/build.gradle.kts

+  alias(libs.plugins.ksp) apply false
+}
+
+val useKsp = hasProperty("useKsp")


kotlin/tests/src/test/kotlin/com/squareup/moshi/kotlin/DualKotlinTest.kt

ZacSweers · 2021-12-05T23:10:20Z

I thiiiiiink I've covered most of them in #1448, and responded to a few others

* 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 <jesse@swank.ca>

ZacSweers marked this pull request as ready for review September 10, 2021 06:21

ZacSweers requested a review from swankjesse September 10, 2021 06:21

ZacSweers mentioned this pull request Sep 12, 2021

Implement basic KSP interop square/kotlinpoet#1146

Merged

1 task

ZacSweers commented Sep 13, 2021

View reviewed changes

ZacSweers mentioned this pull request Sep 20, 2021

Update to JDK 17 #1394

Merged

ZacSweers added 23 commits September 24, 2021 21:01

Add KSP deps

203ecbc

Initial code drop

a8b6679

Consolidate some shared proguard rules writing logic

4302951

Kotlin 1.5.30 and use KSP for autoservice

31fe0ba

So tired of kapt oh my god

Remove remaining incap refs

5a34746

Configure CI matrix for testing

8a9dea6

Add extra KSP tests

497bfbc

Clean up properties

45e8290

Remove kotlin toolchain debugging

66a3933

Spotless

5d7c9af

Simplify KSP test

bac1f8e

Make kapt happy in unit tests

637357e

Update KCT

8b4192d

Ignore weird KSP issue for now

9841567

Misc cleanups

2f852c5

Extract and share Options

bd4fd96

Un-ignore tests and force latest KSP impl in tests

7da1e39

Fix double extension in ProguardRules

2408e77

Kotlin 1.5.31 and dokka 1.5.30

7b7b3aa

Add kotlinpoet ksp deps

d8a7685

Update kotlinpoet to 1.10.1

b816f98

Switch to KSP extensions + share typealiasunwrapping

d0171e7

Work around kapt annoyingness

0bc41b2

ZacSweers force-pushed the z/upstreamKsp branch from 9616b4a to 0bc41b2 Compare September 25, 2021 01:22

Fix + enable the complex self-referencing typevar test

ff8c8d4

ZacSweers commented Sep 25, 2021

View reviewed changes

kotlin/codegen/src/main/java/com/squareup/moshi/kotlin/codegen/api/typeAliasUnwrapping.kt Outdated Show resolved Hide resolved

Remove leftover comment

07faa9f

ZacSweers mentioned this pull request Oct 9, 2021

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

Merged

evant reviewed Oct 15, 2021

View reviewed changes

ZacSweers added 2 commits October 15, 2021 19:48

Switch to shaded utils for experimental KSP APIs

5d1792a

Another fix

7e8374e

ZacSweers mentioned this pull request Oct 16, 2021

Fix enum lookup in annotation proxies google/ksp#685

Merged

ZacSweers added 2 commits October 15, 2021 20:18

Simplify

0eef2a2

Use toKModifier helper

d808086

evant approved these changes Oct 16, 2021

View reviewed changes

ZacSweers merged commit 2db351f into square:master Oct 16, 2021

ZacSweers deleted the z/upstreamKsp branch October 16, 2021 05:52

johan12345 mentioned this pull request Nov 1, 2021

Move away from Databinding to be able to use KSP? ev-map/EVMap#144

Open

mateuszkwiecinski mentioned this pull request Nov 16, 2021

Record requires ASM8 when using 1.13.0-SNAPSHOT #1424

Closed

swankjesse reviewed Dec 5, 2021

View reviewed changes

ZacSweers mentioned this pull request Dec 6, 2021

Add support for KSAnnotation.toAnnotationSpec() square/kotlinpoet#1196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream KSP implementation #1393

Upstream KSP implementation #1393

ZacSweers commented Sep 10, 2021 •

edited

ZacSweers commented Sep 10, 2021

evant Oct 15, 2021 •

edited

ZacSweers Oct 15, 2021

ZacSweers Oct 15, 2021

evant left a comment

ZacSweers commented Oct 16, 2021

swankjesse Dec 5, 2021

swankjesse Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 5, 2021

swankjesse left a comment

swankjesse Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 6, 2021

swankjesse Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 5, 2021

ZacSweers Dec 5, 2021

swankjesse Dec 6, 2021

swankjesse Dec 5, 2021

ZacSweers commented Dec 5, 2021

Upstream KSP implementation #1393

Upstream KSP implementation #1393

Conversation

ZacSweers commented Sep 10, 2021 • edited

ZacSweers commented Sep 10, 2021

evant Oct 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evant left a comment

Choose a reason for hiding this comment

ZacSweers commented Oct 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Dec 5, 2021

ZacSweers commented Sep 10, 2021 •

edited

evant Oct 15, 2021 •

edited