Skip to content

Commit

Permalink
Use the javac element to get the @BindsInstance parameter name.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bcorso authored and Dagger Team committed Nov 22, 2021
1 parent 0c52e85 commit 826582b
Show file tree
Hide file tree
Showing 10 changed files with 139 additions and 39 deletions.
Expand Up @@ -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);
}
}
Expand All @@ -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.
Expand All @@ -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()
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -222,21 +223,22 @@ 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) {
checkArgument(binding.kind().equals(BindingKind.BOUND_INSTANCE));
return forBoundInstance(
binding.key(),
binding.nullableType().isPresent(),
toJavac(binding.bindingElement().get()).getSimpleName().toString());
binding.bindingElement().get());
}

/**
Expand Down
45 changes: 45 additions & 0 deletions 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'
}
Expand Up @@ -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 {
Expand Down
37 changes: 6 additions & 31 deletions javatests/artifacts/dagger/simpleKotlin/build.gradle
Expand Up @@ -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'
}
@@ -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'
}
@@ -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
3 changes: 3 additions & 0 deletions javatests/artifacts/dagger/simpleKotlin/settings.gradle
@@ -0,0 +1,3 @@
rootProject.name='Simple Kotlin Dagger'
include ':app'
include ':kotlin-library'

0 comments on commit 826582b

Please sign in to comment.