From 5a8880a12137615a856b50835ec387db32c06268 Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Sun, 21 Nov 2021 19:46:32 -0800 Subject: [PATCH] Use the javac element to get the @BindsInstance parameter name. This CL fixes an issue where the parameter name for a ComponentRequirement causes a cache-miss for that requirement. This occurs because one ComponentRequirement uses the name from JavacMethodParameter.getName() which pulls the original parameter name from the KotlinMetadata, and the other uses VariableElement.getSimpleName() which may match the original parameter in some cases but may be "arg0" if the name comes from a precompiled class. This difference in name causes the cache miss. I'll need to look into if this is something we should fix on the XProcessing side, e.g. have the user explicitly choose if they want to use the name from the kotlin metadata. Fixes #2997 Fixes #3032 RELNOTES=N/A PiperOrigin-RevId: 411454511 --- .../binding/ComponentCreatorDescriptor.java | 9 ++-- .../codegen/binding/ComponentRequirement.java | 8 ++-- .../dagger/simpleKotlin/app/build.gradle | 45 +++++++++++++++++++ .../dagger/simpleKotlin/AssistedInjects.kt | 0 .../dagger/simpleKotlin/SimpleApplication.kt | 3 ++ .../ComponentProcessorBuildTest.kt | 0 .../dagger/simpleKotlin/build.gradle | 37 +++------------ .../simpleKotlin/kotlin-library/build.gradle | 38 ++++++++++++++++ .../dagger/simpleKotlin/MySubcomponent.kt | 35 +++++++++++++++ .../dagger/simpleKotlin/settings.gradle | 3 ++ 10 files changed, 139 insertions(+), 39 deletions(-) create mode 100644 javatests/artifacts/dagger/simpleKotlin/app/build.gradle rename javatests/artifacts/dagger/simpleKotlin/{ => app}/src/main/java/dagger/simpleKotlin/AssistedInjects.kt (100%) rename javatests/artifacts/dagger/simpleKotlin/{ => app}/src/main/java/dagger/simpleKotlin/SimpleApplication.kt (90%) rename javatests/artifacts/dagger/simpleKotlin/{ => app}/src/test/java/dagger/simpleKotlin/ComponentProcessorBuildTest.kt (100%) create mode 100644 javatests/artifacts/dagger/simpleKotlin/kotlin-library/build.gradle create mode 100644 javatests/artifacts/dagger/simpleKotlin/kotlin-library/src/main/java/dagger/simpleKotlin/MySubcomponent.kt create mode 100644 javatests/artifacts/dagger/simpleKotlin/settings.gradle diff --git a/java/dagger/internal/codegen/binding/ComponentCreatorDescriptor.java b/java/dagger/internal/codegen/binding/ComponentCreatorDescriptor.java index 258ed6bc4f5..9c626cecf56 100644 --- a/java/dagger/internal/codegen/binding/ComponentCreatorDescriptor.java +++ b/java/dagger/internal/codegen/binding/ComponentCreatorDescriptor.java @@ -165,8 +165,7 @@ public static ComponentCreatorDescriptor create( XExecutableParameterElement parameter = getOnlyElement(method.getParameters()); XType parameterType = getOnlyElement(resolvedMethodType.getParameterTypes()); setterMethods.put( - requirement( - method, parameter, parameterType, dependencyRequestFactory, method.getName()), + requirement(method, parameter, parameterType, dependencyRequestFactory, method), method); } } @@ -187,7 +186,7 @@ public static ComponentCreatorDescriptor create( parameter, parameterType, dependencyRequestFactory, - parameter.getName()), + parameter), parameter); } // Validation should have ensured exactly one creator annotation is present on the type. @@ -201,13 +200,13 @@ private static ComponentRequirement requirement( XExecutableParameterElement parameter, XType parameterType, DependencyRequestFactory dependencyRequestFactory, - String variableName) { + XElement elementForVariableName) { if (method.hasAnnotation(TypeNames.BINDS_INSTANCE) || parameter.hasAnnotation(TypeNames.BINDS_INSTANCE)) { DependencyRequest request = dependencyRequestFactory.forRequiredResolvedVariable(parameter, parameterType); return ComponentRequirement.forBoundInstance( - request.key(), request.isNullable(), variableName); + request.key(), request.isNullable(), elementForVariableName); } return moduleAnnotation(parameterType.getTypeElement()).isPresent() diff --git a/java/dagger/internal/codegen/binding/ComponentRequirement.java b/java/dagger/internal/codegen/binding/ComponentRequirement.java index 5a07f1311a7..fb466d09f71 100644 --- a/java/dagger/internal/codegen/binding/ComponentRequirement.java +++ b/java/dagger/internal/codegen/binding/ComponentRequirement.java @@ -26,6 +26,7 @@ import static javax.lang.model.element.Modifier.PRIVATE; import static javax.lang.model.element.Modifier.STATIC; +import androidx.room.compiler.processing.XElement; import androidx.room.compiler.processing.XType; import androidx.room.compiler.processing.XTypeElement; import com.google.auto.common.MoreElements; @@ -222,13 +223,14 @@ public static ComponentRequirement forModule(TypeMirror type) { simpleVariableName(MoreTypes.asTypeElement(type))); } - static ComponentRequirement forBoundInstance(Key key, boolean nullable, String variableName) { + static ComponentRequirement forBoundInstance( + Key key, boolean nullable, XElement elementForVariableName) { return new AutoValue_ComponentRequirement( Kind.BOUND_INSTANCE, MoreTypes.equivalence().wrap(key.type().java()), nullable ? Optional.of(NullPolicy.ALLOW) : Optional.empty(), Optional.of(key), - variableName); + toJavac(elementForVariableName).getSimpleName().toString()); } public static ComponentRequirement forBoundInstance(ContributionBinding binding) { @@ -236,7 +238,7 @@ public static ComponentRequirement forBoundInstance(ContributionBinding binding) return forBoundInstance( binding.key(), binding.nullableType().isPresent(), - toJavac(binding.bindingElement().get()).getSimpleName().toString()); + binding.bindingElement().get()); } /** diff --git a/javatests/artifacts/dagger/simpleKotlin/app/build.gradle b/javatests/artifacts/dagger/simpleKotlin/app/build.gradle new file mode 100644 index 00000000000..613d9c2c770 --- /dev/null +++ b/javatests/artifacts/dagger/simpleKotlin/app/build.gradle @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2021 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. + */ + +plugins { + id 'application' + id 'org.jetbrains.kotlin.jvm' version "$kotlin_version" + id 'org.jetbrains.kotlin.kapt' version "$kotlin_version" +} + +java { + // Make sure the generated source is compatible with Java 8. + sourceCompatibility = JavaVersion.VERSION_1_8 +} + +dependencies { + implementation project(path: ':kotlin-library') + + implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" + implementation 'com.google.dagger:dagger:LOCAL-SNAPSHOT' + kapt 'com.google.dagger:dagger-compiler:LOCAL-SNAPSHOT' + + // This is testImplementation rather than kaptTest because we're actually + // testing the reference to ComponentProcessor. + // See https://github.com/google/dagger/issues/2765 + testImplementation 'com.google.dagger:dagger-compiler:LOCAL-SNAPSHOT' + testImplementation 'com.google.truth:truth:1.0.1' + testImplementation 'junit:junit:4.13' +} + +application { + mainClass = 'dagger.simple.SimpleApplicationKt' +} \ No newline at end of file diff --git a/javatests/artifacts/dagger/simpleKotlin/src/main/java/dagger/simpleKotlin/AssistedInjects.kt b/javatests/artifacts/dagger/simpleKotlin/app/src/main/java/dagger/simpleKotlin/AssistedInjects.kt similarity index 100% rename from javatests/artifacts/dagger/simpleKotlin/src/main/java/dagger/simpleKotlin/AssistedInjects.kt rename to javatests/artifacts/dagger/simpleKotlin/app/src/main/java/dagger/simpleKotlin/AssistedInjects.kt diff --git a/javatests/artifacts/dagger/simpleKotlin/src/main/java/dagger/simpleKotlin/SimpleApplication.kt b/javatests/artifacts/dagger/simpleKotlin/app/src/main/java/dagger/simpleKotlin/SimpleApplication.kt similarity index 90% rename from javatests/artifacts/dagger/simpleKotlin/src/main/java/dagger/simpleKotlin/SimpleApplication.kt rename to javatests/artifacts/dagger/simpleKotlin/app/src/main/java/dagger/simpleKotlin/SimpleApplication.kt index fb211083a4e..24b174a23b7 100644 --- a/javatests/artifacts/dagger/simpleKotlin/src/main/java/dagger/simpleKotlin/SimpleApplication.kt +++ b/javatests/artifacts/dagger/simpleKotlin/app/src/main/java/dagger/simpleKotlin/SimpleApplication.kt @@ -38,6 +38,9 @@ class SimpleApplication { @Component(modules = [SimpleModule::class]) interface SimpleComponent { fun foo(): Foo + + // Reproduces a regression in https://github.com/google/dagger/issues/2997. + fun mySubcomponentFactory(): MySubcomponent.Factory } companion object { diff --git a/javatests/artifacts/dagger/simpleKotlin/src/test/java/dagger/simpleKotlin/ComponentProcessorBuildTest.kt b/javatests/artifacts/dagger/simpleKotlin/app/src/test/java/dagger/simpleKotlin/ComponentProcessorBuildTest.kt similarity index 100% rename from javatests/artifacts/dagger/simpleKotlin/src/test/java/dagger/simpleKotlin/ComponentProcessorBuildTest.kt rename to javatests/artifacts/dagger/simpleKotlin/app/src/test/java/dagger/simpleKotlin/ComponentProcessorBuildTest.kt diff --git a/javatests/artifacts/dagger/simpleKotlin/build.gradle b/javatests/artifacts/dagger/simpleKotlin/build.gradle index a7013134691..24babe5167e 100644 --- a/javatests/artifacts/dagger/simpleKotlin/build.gradle +++ b/javatests/artifacts/dagger/simpleKotlin/build.gradle @@ -18,35 +18,10 @@ buildscript { ext.kotlin_version = "1.5.0" } -plugins { - id 'application' - id 'org.jetbrains.kotlin.jvm' version "$kotlin_version" - id 'org.jetbrains.kotlin.kapt' version "$kotlin_version" +allprojects { + repositories { + jcenter() + mavenCentral() + mavenLocal() + } } - -repositories { - mavenCentral() - mavenLocal() -} - -java { - // Make sure the generated source is compatible with Java 7. - sourceCompatibility = JavaVersion.VERSION_1_8 -} - -dependencies { - implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" - implementation 'com.google.dagger:dagger:LOCAL-SNAPSHOT' - kapt 'com.google.dagger:dagger-compiler:LOCAL-SNAPSHOT' - - // This is testImplementation rather than kaptTest because we're actually - // testing the reference to ComponentProcessor. - // See https://github.com/google/dagger/issues/2765 - testImplementation 'com.google.dagger:dagger-compiler:LOCAL-SNAPSHOT' - testImplementation 'com.google.truth:truth:1.0.1' - testImplementation 'junit:junit:4.13' -} - -application { - mainClass = 'dagger.simple.SimpleApplicationKt' -} \ No newline at end of file diff --git a/javatests/artifacts/dagger/simpleKotlin/kotlin-library/build.gradle b/javatests/artifacts/dagger/simpleKotlin/kotlin-library/build.gradle new file mode 100644 index 00000000000..e9d653f7584 --- /dev/null +++ b/javatests/artifacts/dagger/simpleKotlin/kotlin-library/build.gradle @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2021 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. + */ + +plugins { + id 'org.jetbrains.kotlin.jvm' version "$kotlin_version" + id 'org.jetbrains.kotlin.kapt' version "$kotlin_version" +} + +java { + // Make sure the generated source is compatible with Java 8. + sourceCompatibility = JavaVersion.VERSION_1_8 +} + +dependencies { + implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" + implementation 'com.google.dagger:dagger:LOCAL-SNAPSHOT' + kapt 'com.google.dagger:dagger-compiler:LOCAL-SNAPSHOT' + + // This is testImplementation rather than kaptTest because we're actually + // testing the reference to ComponentProcessor. + // See https://github.com/google/dagger/issues/2765 + testImplementation 'com.google.dagger:dagger-compiler:LOCAL-SNAPSHOT' + testImplementation 'com.google.truth:truth:1.0.1' + testImplementation 'junit:junit:4.13' +} diff --git a/javatests/artifacts/dagger/simpleKotlin/kotlin-library/src/main/java/dagger/simpleKotlin/MySubcomponent.kt b/javatests/artifacts/dagger/simpleKotlin/kotlin-library/src/main/java/dagger/simpleKotlin/MySubcomponent.kt new file mode 100644 index 00000000000..f1176595aa8 --- /dev/null +++ b/javatests/artifacts/dagger/simpleKotlin/kotlin-library/src/main/java/dagger/simpleKotlin/MySubcomponent.kt @@ -0,0 +1,35 @@ +/* + * Copyright (C) 2021 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 dagger.simpleKotlin + +import dagger.BindsInstance +import dagger.Subcomponent + +/** + * This subcomponent reproduces a regression in https://github.com/google/dagger/issues/2997. + */ +@Subcomponent +abstract class MySubcomponent { + abstract fun instance(): InstanceType + + @Subcomponent.Factory + interface Factory { + fun create(@BindsInstance instance: InstanceType): MySubcomponent + } +} + +class InstanceType diff --git a/javatests/artifacts/dagger/simpleKotlin/settings.gradle b/javatests/artifacts/dagger/simpleKotlin/settings.gradle new file mode 100644 index 00000000000..3c34d88c459 --- /dev/null +++ b/javatests/artifacts/dagger/simpleKotlin/settings.gradle @@ -0,0 +1,3 @@ +rootProject.name='Simple Kotlin Dagger' +include ':app' +include ':kotlin-library'