Start KSP dual implementation #722

ZacSweers · 2023-07-07T19:34:18Z

This implements dual support for anvil code gen and KSP code gen using ContributesBinding and AnvilAnnotationDetectorCheck.

The general design is like this

object {Feature}CodeGen {

  <impl>

  @AutoService(CodeGenerator::class)
  class EmbeddedGenerator { ... }

  class KspGenerator {
    <impl>
     
    @AutoService(SymbolProcessorProvider::class)
    class Provider { ... }
  }
}

Where the common code gen implementation lives in the parent object and frontend-specific impls live in nested classes. The impl is agnostic to KSP and Anvil+PSI.

To support this, I've also added new testing support to anvil compilation tests for KSP. This primarily runs through a new AnvilCompilationMode API, which supports two modes: Embedded (the traditional anvil way) and Ksp. This PR's a little larger due to this ground work, but future PRs should be simpler as we continue to replicate some of these common utilities and infra.

ZacSweers · 2023-07-29T22:02:17Z

...utils/src/testFixtures/java/com/squareup/anvil/compiler/internal/testing/AnvilCompilation.kt

+            )
+            addAll(mode.symbolProcessorProviders)
+          }
+          kspWithCompilation = true


This is how we force KSP to compile in-process during the kotlinc invocation, just for testing.

This is how we force KSP to compile in-process during the kotlinc invocation, just for testing.

Maybe worth adding this as a code comment? Unless that's already documented on the KCT side

It's on the kdoc iirc

Added one in d51fc90 (#722)

ZacSweers · 2023-07-29T22:03:37Z

...utils/src/testFixtures/java/com/squareup/anvil/compiler/internal/testing/AnvilCompilation.kt

+          pluginOptions +=
+            listOf(
+              PluginOption(
+                pluginId = anvilCommandLineProcessor.pluginId,
+                optionName = "src-gen-dir",
+                optionValue = File(workingDir, "build/anvil").absolutePath
+              ),
+              PluginOption(
+                pluginId = anvilCommandLineProcessor.pluginId,
+                optionName = "generate-dagger-factories",
+                optionValue = generateDaggerFactories.toString()
+              ),
+              PluginOption(
+                pluginId = anvilCommandLineProcessor.pluginId,
+                optionName = "generate-dagger-factories-only",
+                optionValue = generateDaggerFactoriesOnly.toString()
+              )
+            )
+        }


I gated these off now that they may not be necessary in all compilations

ZacSweers · 2023-07-29T22:06:03Z

compiler/src/main/java/com/squareup/anvil/compiler/AnvilCommandLineProcessor.kt

@@ -61,6 +65,13 @@ class AnvilCommandLineProcessor : CommandLineProcessor {
        "@MergeComponent or @MergeSubcomponent.",
      required = false,
      allowMultipleOccurrences = false
+    ),
+    CliOption(


I added this option so we could correctly gate the modes, but have not wired it up in the gradle plugin yet. I figure we can wait on that until everything's fully implemented

ZacSweers · 2023-07-31T20:48:30Z

compiler-api/src/main/java/com/squareup/anvil/compiler/api/AnvilApplicabilityChecker.kt

@@ -0,0 +1,9 @@
+package com.squareup.anvil.compiler.api
+
+public interface AnvilApplicabilityChecker {


The goal with this is to be able to hoist up the applicability checks to shared locations

JoelWilcox

High level looks great so far!

I wasn't quite able to make it through the whole review yet, leaving some initial questions and comments for now.

JoelWilcox · 2023-08-01T00:06:29Z

compiler/src/main/java/com/squareup/anvil/compiler/AnvilCommandLineProcessor.kt

@@ -34,7 +38,7 @@ class AnvilCommandLineProcessor : CommandLineProcessor {
      optionName = srcGenDirName,
      valueDescription = "<file-path>",
      description = "Path to directory in which Anvil specific code should be generated",
-      required = true,
+      required = false,


Why is this optional now? Do we lose the ability to configure this with KSP?

We don't need it at all in KSP

JoelWilcox · 2023-08-01T00:10:42Z

compiler/src/main/java/com/squareup/anvil/compiler/AnvilCommandLineProcessor.kt

+    ),
+    CliOption(
+      optionName = compilationModeName,
+      valueDescription = "<embedded|ksp>",


Could you add constants for both of these values? Looks like we could probably utilize the existing types you created in AnvilCompilationMode or at least house the values there if we don't want them directly tied to the Type names. Just wanna make sure these can all be updated in one place if they change since they're already referenced in a couple places (CommandLineOptions, AnvilComponentRegistrar, and based on the enum name in AnvilCompilation)

Extracted in 4ed03de

JoelWilcox · 2023-08-01T00:17:28Z

compiler/src/main/java/com/squareup/anvil/compiler/AnvilCommandLineProcessor.kt

@@ -77,6 +88,8 @@ class AnvilCommandLineProcessor : CommandLineProcessor {
        configuration.put(generateDaggerFactoriesOnlyKey, value.toBoolean())
      disableComponentMergingName ->
        configuration.put(disableComponentMergingKey, value.toBoolean())
+      compilationModeName ->
+        configuration.put(compilationModeKey, value)


Do you know if CommandLineProcessor automatically validates inputs based on the provided value description? I didn't see anything from a quick search, but if it's not handled automatically I think we should add a quick check that the input matches one of the expected values.

It does not validate

Added some + a stronger typed parsing in 4ed03de

JoelWilcox · 2023-08-01T00:47:42Z

...utils/src/testFixtures/java/com/squareup/anvil/compiler/internal/testing/AnvilCompilation.kt

+            )
+            addAll(mode.symbolProcessorProviders)
+          }
+          kspWithCompilation = true


This is how we force KSP to compile in-process during the kotlinc invocation, just for testing.

Maybe worth adding this as a code comment? Unless that's already documented on the KCT side

JoelWilcox · 2023-08-01T00:49:22Z

...utils/src/testFixtures/java/com/squareup/anvil/compiler/internal/testing/AnvilCompilation.kt

+          }
+          kspWithCompilation = true
+          kspArgs["generate-dagger-factories"] = generateDaggerFactories.toString()
+          kspArgs["generate-dagger-factories-only"] = generateDaggerFactoriesOnly.toString()


Why do we not use PluginOption for these still / why is there this API difference for KSP?

KSP uses its own API yeah. Modeled after how annotation processing options are

JoelWilcox · 2023-08-01T00:56:21Z

...utils/src/testFixtures/java/com/squareup/anvil/compiler/internal/testing/AnvilCompilation.kt

-  block: Result.() -> Unit = { },
-): Result {
+  block: KotlinCompilation.Result.() -> Unit = { },
+): KotlinCompilation.Result {


Nit: Any reason to make these Result usages more qualified? I see we're still importing com.tschuchort.compiletesting.KotlinCompilation.Result and there are other usages left unqualified. I'm good with either style but I think it's worth keeping them consistent

Because kotlin.Result is an implicit import and they cause issues. This is why I changed the name (and updated in the KCT update PR)

JoelWilcox · 2023-08-01T01:20:33Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/ContributesBindingCodeGen.kt

+  }
+
+  @AutoService(CodeGenerator::class)
+  internal class AnvilGenerator : CodeGenerator {


What do you think about matching this up with the compilation mode naming?

Suggested change

internal class AnvilGenerator : CodeGenerator {

internal class EmbeddedGenerator : CodeGenerator {

Sounds good to me 👍

Done in 074d5cb (#722)

JoelWilcox · 2023-08-01T01:51:09Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/ContributesBindingCodeGen.kt

+
+    override fun processChecked(resolver: Resolver): List<KSAnnotated> {
+      resolver
+        .getSymbolsWithAnnotation(contributesBindingFqName.asString())


Starting to get familiar with the KSP apis as part of this review and I see there's also Resolver#getAllFiles() used in some of the samples, which looks very similar to our previous logic around iterating over all project files. Does getSymbolsWithAnnotation handle checking all files as well as inner classes that may be annotated? It seems that way but just want to clarify

Yeah this API is the preferred one and handles all that logic for us in a heavily cached and optimized way

JoelWilcox · 2023-08-01T01:56:48Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/ContributesBindingCodeGen.kt

+              env.logger.error(
+                "Only classes can be annotated with @ContributesBinding.", annotated
+              )


JoelWilcox · 2023-08-01T02:17:34Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/ContributesBindingCodeGen.kt

+          spec.writeTo(
+            env.codeGenerator,
+            aggregating = false,
+            originatingKSFiles = listOf(clazz.containingFile!!)


It's awesome that this is already built into the API (for both KSP and KotlinPoet) 🙌

Yup one of the first things I implemented for KP's KSP support 😁

JoelWilcox · 2023-08-01T20:34:43Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/ksp/KSAnnotationExtensions.kt

+import com.squareup.anvil.compiler.qualifierFqName
+import org.jetbrains.kotlin.name.FqName
+
+internal fun <T : KSAnnotation> List<T>.checkNoDuplicateScopeAndBoundType(


Part of me is wondering if we should add some light layer of abstraction to share the larger utility extensions between ksp and embedded/reference impls, but I think this is fine for now and we can do some combining later if it there ends up being enough duplication to be worth it.

JoelWilcox · 2023-08-01T20:47:30Z

settings.gradle

@@ -13,6 +13,10 @@ dependencyResolutionManagement {
      String keyString = key.toString()
      if (keyString.startsWith("override_")) {
        overrides.put(keyString, value.toString())
+        if (keyString.endsWith("_kotlin")) {
+          // TODO hardcoded to match what's in libs.versions.toml, but kinda ugly


No need to change in this PR but in a followup could you add more detail around why we need to do this / when it will no longer be necessary? Is this to work around Kotlin 1.8 support?

ZacSweers added 8 commits July 28, 2023 23:31

Add deps

d7e63ad

Invert ContributesBindingCodeGen

e9e7b99

KSP impl

2337b75

Add compile-testing ksp deps

b7ee29f

Fix rebase dupe

cad1b65

Set up exception handling to log errors

e6ac598

Implement checkNoDuplicateScopeAndBoundType check

a030d15

Add compilation mode support + test infrastructure

6e11193

ZacSweers force-pushed the z/refactorCodeGen branch from f6181b4 to 6e11193 Compare July 29, 2023 20:05

Finish getting tests working 🎉

c6d5775

ZacSweers changed the title ~~Demo KSP dual implementation~~ Start KSP dual implementation w/ ContributesBinding code gen. Jul 29, 2023

ZacSweers commented Jul 29, 2023

View reviewed changes

ZacSweers added 3 commits July 29, 2023 14:26

Force older ksp version in CI as needed

14cf287

Convert AnvilAnnotationDetectorCheck

78815d3

Some small simplifications

c513b73

ZacSweers changed the title ~~Start KSP dual implementation w/ ContributesBinding code gen.~~ Start KSP dual implementation Jul 30, 2023

ZacSweers commented Jul 31, 2023

View reviewed changes

JoelWilcox reviewed Aug 1, 2023

View reviewed changes

ZacSweers added 6 commits July 31, 2023 22:56

Merge branch 'main' into z/refactorCodeGen

6e223db

Extract AnvilBackend enum reference

4ed03de

Add clarifying comment

d51fc90

Rename to EmbeddedGenerator

074d5cb

More renames for backend

31ae236

Fixes

1eb9653

JoelWilcox approved these changes Aug 1, 2023

View reviewed changes

JoelWilcox merged commit 2545677 into square:main Aug 1, 2023
19 checks passed

ZacSweers mentioned this pull request Sep 27, 2023

Support KSP in factory generation #751

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start KSP dual implementation #722

Start KSP dual implementation #722

ZacSweers commented Jul 7, 2023 •

edited

Loading

ZacSweers Jul 29, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

ZacSweers Aug 1, 2023

ZacSweers Jul 29, 2023

ZacSweers Jul 29, 2023

ZacSweers Jul 31, 2023

JoelWilcox left a comment

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

JoelWilcox Aug 1, 2023

ZacSweers Aug 1, 2023

JoelWilcox Aug 1, 2023

JoelWilcox Aug 1, 2023

		@@ -0,0 +1,9 @@
		package com.squareup.anvil.compiler.api

		public interface AnvilApplicabilityChecker {

	internal class AnvilGenerator : CodeGenerator {
	internal class EmbeddedGenerator : CodeGenerator {

Start KSP dual implementation #722

Start KSP dual implementation #722

Conversation

ZacSweers commented Jul 7, 2023 • edited Loading

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

JoelWilcox 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

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 Jul 7, 2023 •

edited

Loading