From a4138e7f0a100cb8fed56c90366bb7e97cac85bc Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Tue, 12 Jul 2022 08:50:29 -0700 Subject: [PATCH] Fixes mismatched parameter names in subcomponent factory methods. Fixes #3401 This issue occurs when the original factory method is in a separate build unit from the component that implements it. This issue happens when building with Gradle (couldn't repro with Bazel) and when the original factory method is a kotlin library. This CL changes `createSubcomponentFactoryMethod()` to use the "overriding" factory method's parameters instead of the parameter names from the original factory method. RELNOTES=N/A PiperOrigin-RevId: 460473493 --- .../writing/ComponentImplementation.java | 13 +- .../artifacts/dagger/build-tests/build.gradle | 1 + .../TransitiveSubcomponentTest.java | 135 ++++++++++++++++++ 3 files changed, 141 insertions(+), 8 deletions(-) create mode 100644 javatests/artifacts/dagger/build-tests/src/test/java/buildtests/TransitiveSubcomponentTest.java diff --git a/java/dagger/internal/codegen/writing/ComponentImplementation.java b/java/dagger/internal/codegen/writing/ComponentImplementation.java index b1c166ec8e2..30df2889551 100644 --- a/java/dagger/internal/codegen/writing/ComponentImplementation.java +++ b/java/dagger/internal/codegen/writing/ComponentImplementation.java @@ -56,7 +56,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.Sets; import com.squareup.javapoet.ClassName; @@ -84,13 +83,11 @@ import dagger.internal.codegen.javapoet.TypeNames; import dagger.internal.codegen.javapoet.TypeSpecs; import dagger.internal.codegen.langmodel.Accessibility; -import dagger.internal.codegen.xprocessing.JavaPoetExt; import dagger.internal.codegen.xprocessing.XTypeElements; import dagger.spi.model.BindingGraph.Node; import dagger.spi.model.Key; import dagger.spi.model.RequestKind; import java.util.ArrayList; -import java.util.Collection; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -845,12 +842,12 @@ private boolean canInstantiateAllRequirements() { private void createSubcomponentFactoryMethod(XMethodElement factoryMethod) { checkState(parent.isPresent()); - Collection params = - Maps.transformValues(graph.factoryMethodParameters(), JavaPoetExt::getParameterSpec) - .values(); XType parentType = parent.get().graph().componentTypeElement().getType(); MethodSpec.Builder method = overriding(factoryMethod, parentType); - params.forEach( + // Use the parameter names from the overriding method, which may be different from the + // parameter names at the declaration site if it is pulled in as a class dependency from a + // separate build unit (see https://github.com/google/dagger/issues/3401). + method.parameters.forEach( param -> method.addStatement("$T.checkNotNull($N)", Preconditions.class, param)); method.addStatement( "return new $T($L)", @@ -861,7 +858,7 @@ private void createSubcomponentFactoryMethod(XMethodElement factoryMethod) { creatorComponentFields().stream() .map(field -> ParameterSpec.builder(field.type, field.name).build()) .collect(toImmutableList())) - .addAll(params) + .addAll(method.parameters) .build())); parent.get().getComponentShard().addMethod(COMPONENT_METHOD, method.build()); diff --git a/javatests/artifacts/dagger/build-tests/build.gradle b/javatests/artifacts/dagger/build-tests/build.gradle index 407c7ef0f78..2d2696d9e4e 100644 --- a/javatests/artifacts/dagger/build-tests/build.gradle +++ b/javatests/artifacts/dagger/build-tests/build.gradle @@ -22,6 +22,7 @@ plugins { // Set the versions in a system property so that tests can access it. test { systemProperty 'dagger_version', "$dagger_version" + systemProperty 'kotlin_version', "$kotlin_version" } dependencies { diff --git a/javatests/artifacts/dagger/build-tests/src/test/java/buildtests/TransitiveSubcomponentTest.java b/javatests/artifacts/dagger/build-tests/src/test/java/buildtests/TransitiveSubcomponentTest.java new file mode 100644 index 00000000000..1ce08af8f83 --- /dev/null +++ b/javatests/artifacts/dagger/build-tests/src/test/java/buildtests/TransitiveSubcomponentTest.java @@ -0,0 +1,135 @@ +/* + * Copyright (C) 2022 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 static org.gradle.testkit.runner.TaskOutcome.SUCCESS; + +import java.io.File; +import java.io.IOException; +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/3401 +// This issues occurs specifically when the subcomponent factory method is defined in a separate +// kotlin library from the component that implements the subcomponent factory method. +@RunWith(JUnit4.class) +public class TransitiveSubcomponentTest { + @Rule public TemporaryFolder folder = new TemporaryFolder(); + + @Test + public void testBuild() throws IOException { + BuildResult result = setupRunner().build(); + assertThat(result.task(":app:assemble").getOutcome()).isEqualTo(SUCCESS); + } + + private GradleRunner setupRunner() throws IOException { + File projectDir = folder.getRoot(); + GradleModule.create(projectDir) + .addSettingsFile( + "include 'app'", + "include 'library1'") + .addBuildFile( + "buildscript {", + " ext {", + String.format("dagger_version = \"%s\"", System.getProperty("dagger_version")), + String.format("kotlin_version = \"%s\"", System.getProperty("kotlin_version")), + " }", + "}", + "", + "allprojects {", + " repositories {", + " mavenCentral()", + " mavenLocal()", + " }", + "}"); + + GradleModule.create(projectDir, "app") + .addBuildFile( + "plugins {", + " id 'java'", + " id 'application'", + "}", + "dependencies {", + " implementation project(':library1')", + " implementation \"com.google.dagger:dagger:$dagger_version\"", + " annotationProcessor \"com.google.dagger:dagger-compiler:$dagger_version\"", + "}") + .addSrcFile( + "MyComponent.java", + "package app;", + "", + "import dagger.Component;", + "import library1.MySubcomponent1;", + "", + "@Component", + "interface MyComponent {", + " MySubcomponent1 subcomponent1();", + "}"); + + GradleModule.create(projectDir, "library1") + .addBuildFile( + "plugins {", + " id 'org.jetbrains.kotlin.jvm' version \"$kotlin_version\"", + " id 'org.jetbrains.kotlin.kapt' version \"$kotlin_version\"", + "}", + "dependencies {", + " implementation \"com.google.dagger:dagger:$dagger_version\"", + " annotationProcessor \"com.google.dagger:dagger-compiler:$dagger_version\"", + "}") + .addSrcFile( + "MyModule.kt", + "package library1", + "", + "import dagger.Module", + "import dagger.Provides", + "", + "@Module", + "public class MyModule(private val int: Int) {", + " @Provides public fun provideInt(): Int = int", + "}") + .addSrcFile( + "MySubcomponent1.kt", + "package library1", + "", + "import dagger.Subcomponent", + "", + "@Subcomponent", + "public interface MySubcomponent1 {", + " public fun subcomponent2(myModule: MyModule): MySubcomponent2", + "}") + .addSrcFile( + "MySubcomponent2.kt", + "package library1", + "", + "import dagger.Subcomponent", + "", + "@Subcomponent(modules = [MyModule::class])", + "public interface MySubcomponent2 {", + " public fun integer(): Int", + "}"); + + return GradleRunner.create() + .withArguments("--stacktrace", "build") + .withProjectDir(projectDir); + } +}