From 558cc51334d1f8cbcf6ad6cc0940bacc8a568956 Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Fri, 29 Sep 2023 15:52:08 -0700 Subject: [PATCH] Fix Dagger's incremental processing for KSP. This change should be temporary until we can get a more robust fix in KSP (https://github.com/google/ksp/issues/1555). Fixes #4063 Fixes #4054 RELNOTES=N/A PiperOrigin-RevId: 569608460 --- .../validation/InjectBindingRegistryImpl.java | 93 ++++++++--- .../artifacts/dagger/build-tests/build.gradle | 1 + .../main/java/buildtests/GradleModule.java | 5 + .../buildtests/IncrementalProcessingTest.java | 144 ++++++++++++++++++ javatests/artifacts/dagger/build.gradle | 1 + 5 files changed, 226 insertions(+), 18 deletions(-) create mode 100644 javatests/artifacts/dagger/build-tests/src/test/java/buildtests/IncrementalProcessingTest.java diff --git a/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java b/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java index cc2d6e9403d..13bee7f379b 100644 --- a/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java +++ b/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java @@ -17,6 +17,7 @@ package dagger.internal.codegen.validation; import static androidx.room.compiler.processing.XElementKt.isTypeElement; +import static androidx.room.compiler.processing.compat.XConverters.toKS; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; @@ -27,6 +28,7 @@ import static dagger.internal.codegen.binding.SourceFiles.generatedClassNameForBinding; import static dagger.internal.codegen.extension.DaggerCollectors.toOptional; import static dagger.internal.codegen.xprocessing.XElements.asTypeElement; +import static dagger.internal.codegen.xprocessing.XElements.closestEnclosingTypeElement; import static dagger.internal.codegen.xprocessing.XTypes.erasedTypeName; import static dagger.internal.codegen.xprocessing.XTypes.isDeclared; import static dagger.internal.codegen.xprocessing.XTypes.nonObjectSuperclass; @@ -41,6 +43,7 @@ import androidx.room.compiler.processing.XTypeElement; import com.google.common.collect.Maps; import com.google.common.collect.Sets; +import com.google.devtools.ksp.symbol.Origin; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.squareup.javapoet.ClassName; import dagger.Component; @@ -82,7 +85,7 @@ final class InjectBindingRegistryImpl implements InjectBindingRegistry { private final BindingFactory bindingFactory; private final CompilerOptions compilerOptions; - final class BindingsCollection { + private final class BindingsCollection { private final ClassName factoryClass; private final Map bindingsByKey = Maps.newLinkedHashMap(); private final Deque bindingsRequiringGeneration = new ArrayDeque<>(); @@ -115,20 +118,42 @@ B getBinding(Key key) { } /** Caches the binding and generates it if it needs generation. */ - void tryRegisterBinding(B binding, boolean warnIfNotAlreadyGenerated) { + void tryRegisterBinding( + B binding, + boolean warnIfNotAlreadyGenerated, + boolean isCalledFromInjectProcessor) { + if (processingEnv.getBackend() == XProcessingEnv.Backend.KSP) { + Origin origin = + toKS(closestEnclosingTypeElement(binding.bindingElement().get())).getOrigin(); + // If the origin of the element is from a source file in the current compilation unit then + // we're guaranteed that the InjectProcessor should run over the element so only generate + // the Factory/MembersInjector if we're being called from the InjectProcessor. + // + // TODO(bcorso): generally, this isn't something we would need to keep track of manually. + // However, KSP incremental processing has a bug that will overwrite the cache for the + // element if we generate files for it, which can lead to missing generated files from + // other processors. See https://github.com/google/dagger/issues/4063 and + // https://github.com/google/dagger/issues/4054. Remove this once that bug is fixed. + if (!isCalledFromInjectProcessor && (origin == Origin.JAVA || origin == Origin.KOTLIN)) { + return; + } + } tryToCacheBinding(binding); @SuppressWarnings("unchecked") B maybeUnresolved = binding.unresolved().isPresent() ? (B) binding.unresolved().get() : binding; - tryToGenerateBinding(maybeUnresolved, warnIfNotAlreadyGenerated); + tryToGenerateBinding(maybeUnresolved, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); } /** * Tries to generate a binding, not generating if it already is generated. For resolved * bindings, this will try to generate the unresolved version of the binding. */ - void tryToGenerateBinding(B binding, boolean warnIfNotAlreadyGenerated) { + void tryToGenerateBinding( + B binding, + boolean warnIfNotAlreadyGenerated, + boolean isCalledFromInjectProcessor) { if (shouldGenerateBinding(binding)) { bindingsRequiringGeneration.offer(binding); if (compilerOptions.warnIfInjectionFactoryNotGeneratedUpstream() @@ -204,15 +229,22 @@ public void generateSourcesForRequiredBindings( * Registers the binding for generation and later lookup. If the binding is resolved, we also * attempt to register an unresolved version of it. */ - private void registerBinding(ProvisionBinding binding, boolean warnIfNotAlreadyGenerated) { - provisionBindings.tryRegisterBinding(binding, warnIfNotAlreadyGenerated); + private void registerBinding( + ProvisionBinding binding, + boolean warnIfNotAlreadyGenerated, + boolean isCalledFromInjectProcessor) { + provisionBindings.tryRegisterBinding( + binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); } /** * Registers the binding for generation and later lookup. If the binding is resolved, we also * attempt to register an unresolved version of it. */ - private void registerBinding(MembersInjectionBinding binding, boolean warnIfNotAlreadyGenerated) { + private void registerBinding( + MembersInjectionBinding binding, + boolean warnIfNotAlreadyGenerated, + boolean isCalledFromInjectProcessor) { /* * We generate MembersInjector classes for types with @Inject constructors only if they have any * injection sites. @@ -232,20 +264,26 @@ private void registerBinding(MembersInjectionBinding binding, boolean warnIfNotA : binding.hasLocalInjectionSites(); } - membersInjectionBindings.tryRegisterBinding(binding, warnIfNotAlreadyGenerated); + membersInjectionBindings + .tryRegisterBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); } @Override public Optional tryRegisterInjectConstructor( XConstructorElement constructorElement) { - return tryRegisterConstructor(constructorElement, Optional.empty(), false); + return tryRegisterConstructor( + constructorElement, + Optional.empty(), + /* warnIfNotAlreadyGenerated= */ false, + /* isCalledFromInjectProcessor= */ true); } @CanIgnoreReturnValue private Optional tryRegisterConstructor( XConstructorElement constructorElement, Optional resolvedType, - boolean warnIfNotAlreadyGenerated) { + boolean warnIfNotAlreadyGenerated, + boolean isCalledFromInjectProcessor) { XTypeElement typeElement = constructorElement.getEnclosingElement(); // Validating here shouldn't have a performance penalty because the validator caches its reports @@ -263,9 +301,10 @@ private Optional tryRegisterConstructor( } ProvisionBinding binding = bindingFactory.injectionBinding(constructorElement, resolvedType); - registerBinding(binding, warnIfNotAlreadyGenerated); + registerBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); if (!binding.injectionSites().isEmpty()) { - tryRegisterMembersInjectedType(typeElement, resolvedType, warnIfNotAlreadyGenerated); + tryRegisterMembersInjectedType( + typeElement, resolvedType, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); } return Optional.of(binding); } @@ -281,7 +320,10 @@ public Optional tryRegisterInjectField(XFieldElement fi fieldElement); } return tryRegisterMembersInjectedType( - asTypeElement(fieldElement.getEnclosingElement()), Optional.empty(), false); + asTypeElement(fieldElement.getEnclosingElement()), + Optional.empty(), + /* warnIfNotAlreadyGenerated= */ false, + /* isCalledFromInjectProcessor= */ true); } @Override @@ -295,12 +337,18 @@ public Optional tryRegisterInjectMethod(XMethodElement methodElement); } return tryRegisterMembersInjectedType( - asTypeElement(methodElement.getEnclosingElement()), Optional.empty(), false); + asTypeElement(methodElement.getEnclosingElement()), + Optional.empty(), + /* warnIfNotAlreadyGenerated= */ false, + /* isCalledFromInjectProcessor= */ true); } @CanIgnoreReturnValue private Optional tryRegisterMembersInjectedType( - XTypeElement typeElement, Optional resolvedType, boolean warnIfNotAlreadyGenerated) { + XTypeElement typeElement, + Optional resolvedType, + boolean warnIfNotAlreadyGenerated, + boolean isCalledFromInjectProcessor) { // Validating here shouldn't have a performance penalty because the validator caches its reports ValidationReport report = injectValidator.validateForMembersInjection(typeElement); report.printMessagesTo(messager); @@ -316,7 +364,7 @@ private Optional tryRegisterMembersInjectedType( } MembersInjectionBinding binding = bindingFactory.membersInjectionBinding(type, resolvedType); - registerBinding(binding, warnIfNotAlreadyGenerated); + registerBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); for (Optional supertype = nonObjectSuperclass(type); supertype.isPresent(); supertype = nonObjectSuperclass(supertype.get())) { @@ -351,7 +399,13 @@ public Optional getOrFindProvisionBinding(Key key) { assistedInjectedConstructors(element).stream()) // We're guaranteed that there's at most 1 @Inject constructors from above validation. .collect(toOptional()) - .flatMap(constructor -> tryRegisterConstructor(constructor, Optional.of(type), true)); + .flatMap( + constructor -> + tryRegisterConstructor( + constructor, + Optional.of(type), + /* warnIfNotAlreadyGenerated= */ true, + /* isCalledFromInjectProcessor= */ false)); } @CanIgnoreReturnValue @@ -365,7 +419,10 @@ public Optional getOrFindMembersInjectionBinding(Key ke return Optional.of(binding); } return tryRegisterMembersInjectedType( - key.type().xprocessing().getTypeElement(), Optional.of(key.type().xprocessing()), true); + key.type().xprocessing().getTypeElement(), + Optional.of(key.type().xprocessing()), + /* warnIfNotAlreadyGenerated= */ true, + /* isCalledFromInjectProcessor= */ false); } @Override diff --git a/javatests/artifacts/dagger/build-tests/build.gradle b/javatests/artifacts/dagger/build-tests/build.gradle index 2d2696d9e4e..7418b9f01ad 100644 --- a/javatests/artifacts/dagger/build-tests/build.gradle +++ b/javatests/artifacts/dagger/build-tests/build.gradle @@ -23,6 +23,7 @@ plugins { test { systemProperty 'dagger_version', "$dagger_version" systemProperty 'kotlin_version', "$kotlin_version" + systemProperty 'ksp_version', "$ksp_version" } dependencies { diff --git a/javatests/artifacts/dagger/build-tests/src/main/java/buildtests/GradleModule.java b/javatests/artifacts/dagger/build-tests/src/main/java/buildtests/GradleModule.java index d02dd91eaa5..c4d53d61e92 100644 --- a/javatests/artifacts/dagger/build-tests/src/main/java/buildtests/GradleModule.java +++ b/javatests/artifacts/dagger/build-tests/src/main/java/buildtests/GradleModule.java @@ -20,6 +20,7 @@ import java.io.File; import java.io.FileWriter; import java.io.IOException; +import java.nio.file.Path; /** Used to create files for a Gradle module in a particular directory. */ public final class GradleModule { @@ -39,6 +40,10 @@ private GradleModule(File moduleDir) { this.moduleSrcDir = new File(moduleDir, "src/main/java/"); } + public Path getDir() { + return moduleDir.toPath(); + } + public GradleModule addBuildFile(String... content) throws IOException { writeFile(createFile(moduleDir, "build.gradle"), content); return this; diff --git a/javatests/artifacts/dagger/build-tests/src/test/java/buildtests/IncrementalProcessingTest.java b/javatests/artifacts/dagger/build-tests/src/test/java/buildtests/IncrementalProcessingTest.java new file mode 100644 index 00000000000..bb84b832e86 --- /dev/null +++ b/javatests/artifacts/dagger/build-tests/src/test/java/buildtests/IncrementalProcessingTest.java @@ -0,0 +1,144 @@ +/* + * Copyright (C) 2023 The Dagger Authors. + * + * 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 + * + * http://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 buildtests; + +import static com.google.common.truth.Truth.assertThat; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Set; +import java.util.stream.Collectors; +import org.gradle.testkit.runner.BuildResult; +import org.gradle.testkit.runner.GradleRunner; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +// This is a regression test for https://github.com/google/dagger/issues/4054 +@RunWith(JUnit4.class) +public class IncrementalProcessingTest { + @Rule public TemporaryFolder tmpFolder = new TemporaryFolder(); + + @Test + public void testIncrementalProcessing() throws IOException { + File projectDir = tmpFolder.getRoot(); + GradleModule.create(projectDir) + .addSettingsFile("include 'app'") + .addBuildFile( + "buildscript {", + " ext {", + String.format("dagger_version = \"%s\"", System.getProperty("dagger_version")), + String.format("kotlin_version = \"%s\"", System.getProperty("kotlin_version")), + String.format("ksp_version = \"%s\"", System.getProperty("ksp_version")), + " }", + "}", + "", + "allprojects {", + " repositories {", + " mavenCentral()", + " mavenLocal()", + " }", + "}"); + + GradleModule appModule = + GradleModule.create(projectDir, "app") + .addBuildFile( + "plugins {", + " id 'application'", + " id 'org.jetbrains.kotlin.jvm' version \"$kotlin_version\"", + " id 'com.google.devtools.ksp' version \"$ksp_version\"", + "}", + "dependencies {", + " implementation \"org.jetbrains.kotlin:kotlin-stdlib\"", + " implementation \"com.google.dagger:dagger:$dagger_version\"", + " ksp \"com.google.dagger:dagger-compiler:$dagger_version\"", + "}") + // Note: both A and AFactory need to be in the same source file for this to test the + // regression in https://github.com/google/dagger/issues/4054. + .addSrcFile( + "A.kt", + "package app", + "", + "import dagger.assisted.AssistedFactory", + "import dagger.assisted.AssistedInject", + "", + "class A @AssistedInject constructor()", + "", + "@AssistedFactory", + "interface AFactory {", + " fun create(): A", + "}"); + + // We'll be changing the contents of MyComponent between builds, so store it in a variable. + String myComponentContent = + String.join( + "\n", + "package app", + "", + "import dagger.Component", + "", + "@Component", + "interface MyComponent {", + " fun factory(): AFactory", + "}"); + appModule.addSrcFile("MyComponent.kt", myComponentContent); + + // Build #1 + build(projectDir); + assertThat(getAllKspGeneratedFileNames(appModule.getDir())) + .containsExactly( + "A_Factory.java", + "AFactory_Impl.java", + "DaggerMyComponent.java"); + + // Change method name in MyComponent.kt to trigger incremental processing of only MyComponent. + appModule.addSrcFile("MyComponent.kt", myComponentContent.replace("factory()", "factory2()")); + + // Build #2 + build(projectDir); + assertThat(getAllKspGeneratedFileNames(appModule.getDir())) + .containsExactly( + "A_Factory.java", + "AFactory_Impl.java", + "DaggerMyComponent.java"); + } + + private static BuildResult build(File projectDir) { + return GradleRunner.create() + .withArguments("--stacktrace", "build") + .withProjectDir(projectDir) + .build(); + } + + private static Set getAllKspGeneratedFileNames(Path moduleDir) throws IOException { + return getAllFileNames(moduleDir.resolve("build/generated/ksp/main/java/")); + } + + private static Set getAllFileNames(Path dir) throws IOException { + if (!Files.isDirectory(dir)) { + throw new IllegalArgumentException("Expected directory: " + dir); + } + return Files.walk(dir) + .filter(Files::isRegularFile) + .map(file -> file.getFileName().toString()) + .collect(Collectors.toSet()); + } +} diff --git a/javatests/artifacts/dagger/build.gradle b/javatests/artifacts/dagger/build.gradle index 5c8c7d5a5ff..ca05f8ef480 100644 --- a/javatests/artifacts/dagger/build.gradle +++ b/javatests/artifacts/dagger/build.gradle @@ -18,6 +18,7 @@ buildscript { ext { dagger_version = "LOCAL-SNAPSHOT" kotlin_version = "1.9.0" + ksp_version = "$kotlin_version-1.0.12" junit_version = "4.13" truth_version = "1.0.1" }