Skip to content

Commit

Permalink
Fix an issue where a scoped @BINDS used in a cycle would cause an NPE…
Browse files Browse the repository at this point in the history
… on component creation.

RELNOTES=Fix an issue where a scoped @BINDS used in a cycle would cause an NPE on component creation.
PiperOrigin-RevId: 507636194
  • Loading branch information
Chang-Eric authored and Dagger Team committed Feb 7, 2023
1 parent 85a2cb2 commit fae46c7
Show file tree
Hide file tree
Showing 23 changed files with 1,466 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ public final class ShardImplementation implements GeneratedImplementation {

private ShardImplementation(ClassName name) {
this.name = name;
this.switchingProviders = new SwitchingProviders(this);
this.switchingProviders = new SwitchingProviders(this, processingEnv);
this.experimentalSwitchingProviders =
new ExperimentalSwitchingProviders(this, componentRequestRepresentationsProvider);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import dagger.internal.codegen.binding.FrameworkField;
import dagger.internal.codegen.javapoet.AnnotationSpecs;
import dagger.internal.codegen.javapoet.TypeNames;
import dagger.internal.codegen.writing.ComponentImplementation.CompilerMode;
import dagger.internal.codegen.writing.ComponentImplementation.ShardImplementation;
import dagger.spi.model.BindingKind;
import java.util.Optional;
Expand Down Expand Up @@ -65,7 +64,6 @@ default Optional<ClassName> alternativeFrameworkClass() {
private final ShardImplementation shardImplementation;
private final ContributionBinding binding;
private final FrameworkInstanceCreationExpression frameworkInstanceCreationExpression;
private final CompilerMode compilerMode;
private FieldSpec fieldSpec;
private InitializationState fieldInitializationState = InitializationState.UNINITIALIZED;

Expand All @@ -75,7 +73,6 @@ default Optional<ClassName> alternativeFrameworkClass() {
FrameworkInstanceCreationExpression frameworkInstanceCreationExpression) {
this.binding = checkNotNull(binding);
this.shardImplementation = checkNotNull(componentImplementation).shardImplementation(binding);
this.compilerMode = componentImplementation.compilerMode();
this.frameworkInstanceCreationExpression = checkNotNull(frameworkInstanceCreationExpression);
}

Expand Down Expand Up @@ -113,12 +110,13 @@ private void initializeField() {
case INITIALIZING:
fieldSpec = getOrCreateField();
// We were recursively invoked, so create a delegate factory instead to break the loop.
// However, because SwitchingProvider takes no dependencies, even if they are recursively
// invoked, we don't need to delegate it since there is no dependency cycle.
if (FrameworkInstanceKind.from(binding, compilerMode)
.equals(FrameworkInstanceKind.SWITCHING_PROVIDER)) {
break;
}

// TODO(erichang): For the most part SwitchingProvider takes no dependencies so even if they
// are recursively invoked, we don't need to delegate it since there is no dependency cycle.
// However, there is a case with a scoped @Binds where we reference the impl binding when
// passing it into DoubleCheck. For this case, we do need to delegate it. There might be
// a way to only do delegates in this situation, but we'd need to keep track of what other
// bindings use this.

fieldInitializationState = InitializationState.DELEGATED;
shardImplementation.addInitialization(
Expand Down
10 changes: 8 additions & 2 deletions java/dagger/internal/codegen/writing/SwitchingProviders.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static javax.lang.model.element.Modifier.PUBLIC;
import static javax.lang.model.element.Modifier.STATIC;

import androidx.room.compiler.processing.XProcessingEnv;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.squareup.javapoet.ClassName;
Expand All @@ -42,6 +43,7 @@
import dagger.internal.codegen.javapoet.CodeBlocks;
import dagger.internal.codegen.writing.ComponentImplementation.ShardImplementation;
import dagger.internal.codegen.writing.FrameworkFieldInitializer.FrameworkInstanceCreationExpression;
import dagger.internal.codegen.xprocessing.XProcessingEnvs;
import dagger.spi.model.BindingKind;
import dagger.spi.model.Key;
import java.util.HashMap;
Expand Down Expand Up @@ -77,9 +79,11 @@ final class SwitchingProviders {
new LinkedHashMap<>();

private final ShardImplementation shardImplementation;
private final XProcessingEnv processingEnv;

SwitchingProviders(ShardImplementation shardImplementation) {
SwitchingProviders(ShardImplementation shardImplementation, XProcessingEnv processingEnv) {
this.shardImplementation = checkNotNull(shardImplementation);
this.processingEnv = checkNotNull(processingEnv);
}

/** Returns the framework instance creation expression for an inner switching provider class. */
Expand Down Expand Up @@ -133,7 +137,9 @@ private CodeBlock getNewInstanceCodeBlock(
// Add the type parameter explicitly when the binding is scoped because Java can't resolve
// the type when wrapped. For example, the following will error:
// fooProvider = DoubleCheck.provider(new SwitchingProvider<>(1));
(binding.scope().isPresent() || binding.kind().equals(BindingKind.ASSISTED_FACTORY))
(binding.scope().isPresent()
|| binding.kind().equals(BindingKind.ASSISTED_FACTORY)
|| XProcessingEnvs.isPreJava8SourceVersion(processingEnv))
? CodeBlock.of(
"$T", shardImplementation.accessibleTypeName(binding.contributedType()))
: "",
Expand Down
53 changes: 46 additions & 7 deletions javatests/dagger/functional/binds/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,59 @@ load("//:test_defs.bzl", "GenJavaTests")
package(default_visibility = ["//:src"])

GenJavaTests(
name = "binds",
srcs = glob(["*.java"]),
name = "BindsTest",
srcs = [
"AccessesExposedComponent.java",
"BindsTest.java",
"Foo.java",
"FooOfObjects.java",
"FooOfStrings.java",
"InterfaceModule.java",
"NeedsFactory.java",
"SimpleBindingModule.java",
"SomeQualifier.java",
"TestComponent.java",
],
gen_library_deps = [
"//javatests/dagger/functional/binds/subpackage",
],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/auto:factory",
"//third_party/java/guava/base",
"//third_party/java/guava/collect",
"//third_party/java/guava/util/concurrent",
"//third_party/java/jsr305_annotations",
"//third_party/java/jsr330_inject",
"//third_party/java/junit",
"//third_party/java/truth",
],
)

GenJavaTests(
name = "BindsCollectionsWithoutMultibindingsTest",
srcs = ["BindsCollectionsWithoutMultibindingsTest.java"],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/junit",
"//third_party/java/truth",
],
)

GenJavaTests(
name = "ScopedBindsTest",
srcs = ["ScopedBindsTest.java"],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/junit",
"//third_party/java/truth",
],
)

GenJavaTests(
name = "RecursiveBindsTest",
srcs = ["RecursiveBindsTest.java"],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/junit",
"//third_party/java/truth",
],
Expand Down
67 changes: 67 additions & 0 deletions javatests/dagger/functional/binds/RecursiveBindsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* 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 dagger.functional.binds;

import static com.google.common.truth.Truth.assertThat;

import dagger.Binds;
import dagger.Component;
import dagger.Module;
import javax.inject.Inject;
import javax.inject.Provider;
import javax.inject.Singleton;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

// This is a regression test for b/267223822 where a scoped @Binds used in a cycle caused problems
// in fastInit mode.
@RunWith(JUnit4.class)
public class RecursiveBindsTest {

public interface Foo {}

public static final class FooImpl implements Foo {
@Inject FooImpl(@SuppressWarnings("unused") Provider<Foo> provider) {}
}

@Module
public interface FooModule {
// This binding must be scoped to create the cycle. Otherwise without a scope, the generated
// code just doesn't have a field for this @Binds because we can directly use FooImpl's
// provider as they are equivalent.
@Binds
@Singleton
Foo bindFoo(FooImpl impl);
}

@Component(modules = FooModule.class)
@Singleton
public interface TestSingletonComponent {
// We get the impl here to create a cycle where the impl factory needs to be delegated.
// That way the scoped binds (which does something like DoubleCheck.provider(implFactory)) is
// the one that would fail if it wasn't delegated properly.
Provider<FooImpl> getFooImplProvider();
}

@Test
public void test() {
// Technically the NPE would happen when just initializing the component.
assertThat(DaggerRecursiveBindsTest_TestSingletonComponent.create().getFooImplProvider().get())
.isNotNull();
}
}
42 changes: 35 additions & 7 deletions javatests/dagger/functional/kotlinsrc/binds/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,48 @@ load("//:test_defs.bzl", "GenKtTests")
package(default_visibility = ["//:src"])

GenKtTests(
name = "binds",
srcs = glob(["*.kt"]),
name = "BindsTest",
srcs = [
"AccessesExposedComponent.kt",
"BindsTest.kt",
"Foo.kt",
"FooOfObjects.kt",
"FooOfStrings.kt",
"InterfaceModule.kt",
"NeedsFactory.kt",
"SimpleBindingModule.kt",
"SomeQualifier.kt",
"TestComponent.kt",
],
gen_library_deps = [
"//javatests/dagger/functional/kotlinsrc/binds/subpackage",
],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/auto:factory",
"//third_party/java/guava/base",
"//third_party/java/guava/collect",
"//third_party/java/guava/util/concurrent",
"//third_party/java/jsr305_annotations",
"//third_party/java/jsr330_inject",
"//third_party/java/junit",
"//third_party/java/truth",
],
)

GenKtTests(
name = "BindsCollectionsWithoutMultibindingsTest",
srcs = ["BindsCollectionsWithoutMultibindingsTest.kt"],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/junit",
"//third_party/java/truth",
],
)

GenKtTests(
name = "ScopedBindsTest",
srcs = ["ScopedBindsTest.kt"],
javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
deps = [
"//:dagger_with_compiler",
"//third_party/java/junit",
"//third_party/java/truth",
],
Expand Down
63 changes: 63 additions & 0 deletions javatests/dagger/functional/kotlinsrc/binds/RecursiveBindsTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* 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 dagger.functional.kotlinsrc.binds

import com.google.common.truth.Truth.assertThat
import dagger.Binds
import dagger.Component
import dagger.Module
import javax.inject.Inject
import javax.inject.Provider
import javax.inject.Singleton
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

// This is a regression test for b/267223822 where a scoped @Binds used in a cycle caused problems
// in fastInit mode.
@RunWith(JUnit4::class)
class RecursiveBindsTest {

interface Foo {}

class FooImpl internal @Inject constructor(@SuppressWarnings("unused") provider: Provider<Foo>) :
Foo {}

@Module
interface FooModule {
// This binding must be scoped to create the cycle. Otherwise without a scope, the generated
// code just doesn't have a field for this @Binds because we can directly use FooImpl's
// provider as they are equivalent.
@Binds @Singleton fun bindFoo(impl: FooImpl): Foo
}

@Component(modules = FooModule::class)
@Singleton
interface TestSingletonComponent {
// We get the impl here to create a cycle where the impl factory needs to be delegated.
// That way the scoped binds (which does something like DoubleCheck.provider(implFactory)) is
// the one that would fail if it wasn't delegated properly.
fun getFooImplProvider(): Provider<FooImpl>
}

@Test
fun test() {
// Technically the NPE would happen when just initializing the component.
assertThat(DaggerRecursiveBindsTest_TestSingletonComponent.create().getFooImplProvider().get())
.isNotNull()
}
}
Loading

0 comments on commit fae46c7

Please sign in to comment.