Skip to content

Commit

Permalink
Fix Dagger's incremental processing for KSP.
Browse files Browse the repository at this point in the history
This change should be temporary until we can get a more robust fix in KSP (google/ksp#1555).

Fixes #4063
Fixes #4054

RELNOTES=N/A
PiperOrigin-RevId: 568629041
  • Loading branch information
bcorso authored and Dagger Team committed Sep 29, 2023
1 parent 5cc209c commit 81c7c61
Show file tree
Hide file tree
Showing 5 changed files with 226 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -82,7 +85,7 @@ final class InjectBindingRegistryImpl implements InjectBindingRegistry {
private final BindingFactory bindingFactory;
private final CompilerOptions compilerOptions;

final class BindingsCollection<B extends Binding> {
private final class BindingsCollection<B extends Binding> {
private final ClassName factoryClass;
private final Map<Key, B> bindingsByKey = Maps.newLinkedHashMap();
private final Deque<B> bindingsRequiringGeneration = new ArrayDeque<>();
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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.
Expand All @@ -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<ProvisionBinding> tryRegisterInjectConstructor(
XConstructorElement constructorElement) {
return tryRegisterConstructor(constructorElement, Optional.empty(), false);
return tryRegisterConstructor(
constructorElement,
Optional.empty(),
/* warnIfNotAlreadyGenerated= */ false,
/* isCalledFromInjectProcessor= */ true);
}

@CanIgnoreReturnValue
private Optional<ProvisionBinding> tryRegisterConstructor(
XConstructorElement constructorElement,
Optional<XType> 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
Expand All @@ -263,9 +301,10 @@ private Optional<ProvisionBinding> 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);
}
Expand All @@ -281,7 +320,10 @@ public Optional<MembersInjectionBinding> tryRegisterInjectField(XFieldElement fi
fieldElement);
}
return tryRegisterMembersInjectedType(
asTypeElement(fieldElement.getEnclosingElement()), Optional.empty(), false);
asTypeElement(fieldElement.getEnclosingElement()),
Optional.empty(),
/* warnIfNotAlreadyGenerated= */ false,
/* isCalledFromInjectProcessor= */ true);
}

@Override
Expand All @@ -295,12 +337,18 @@ public Optional<MembersInjectionBinding> tryRegisterInjectMethod(XMethodElement
methodElement);
}
return tryRegisterMembersInjectedType(
asTypeElement(methodElement.getEnclosingElement()), Optional.empty(), false);
asTypeElement(methodElement.getEnclosingElement()),
Optional.empty(),
/* warnIfNotAlreadyGenerated= */ false,
/* isCalledFromInjectProcessor= */ true);
}

@CanIgnoreReturnValue
private Optional<MembersInjectionBinding> tryRegisterMembersInjectedType(
XTypeElement typeElement, Optional<XType> resolvedType, boolean warnIfNotAlreadyGenerated) {
XTypeElement typeElement,
Optional<XType> 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);
Expand All @@ -316,7 +364,7 @@ private Optional<MembersInjectionBinding> tryRegisterMembersInjectedType(
}

MembersInjectionBinding binding = bindingFactory.membersInjectionBinding(type, resolvedType);
registerBinding(binding, warnIfNotAlreadyGenerated);
registerBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor);
for (Optional<XType> supertype = nonObjectSuperclass(type);
supertype.isPresent();
supertype = nonObjectSuperclass(supertype.get())) {
Expand Down Expand Up @@ -351,7 +399,13 @@ public Optional<ProvisionBinding> 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
Expand All @@ -365,7 +419,10 @@ public Optional<MembersInjectionBinding> 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
Expand Down
1 change: 1 addition & 0 deletions javatests/artifacts/dagger/build-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ plugins {
test {
systemProperty 'dagger_version', "$dagger_version"
systemProperty 'kotlin_version', "$kotlin_version"
systemProperty 'ksp_version', "$ksp_version"
}

dependencies {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String> getAllKspGeneratedFileNames(Path moduleDir) throws IOException {
return getAllFileNames(moduleDir.resolve("build/generated/ksp/main/java/"));
}

private static Set<String> 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());
}
}
1 change: 1 addition & 0 deletions javatests/artifacts/dagger/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down

0 comments on commit 81c7c61

Please sign in to comment.