From c2d097fa4e88f55a8c0a1f1c84d19c9538cb7364 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Tue, 2 Mar 2021 14:20:19 -0800 Subject: [PATCH] Fix an issue where subcomponent builder bindings could be accidentally cached from the parent component. RELNOTES=Fix an issue with cached subcomponent builder bindings PiperOrigin-RevId: 360513850 --- .../codegen/base/ComponentAnnotation.java | 25 ++ java/dagger/internal/codegen/base/Keys.java | 14 + .../codegen/binding/BindingGraphFactory.java | 3 +- .../SubcomponentBuilderMultibindingsTest.java | 265 ++++++++++++++++++ 4 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 javatests/dagger/functional/subcomponent/SubcomponentBuilderMultibindingsTest.java diff --git a/java/dagger/internal/codegen/base/ComponentAnnotation.java b/java/dagger/internal/codegen/base/ComponentAnnotation.java index c9b3580c1c9..b70ef54bbd6 100644 --- a/java/dagger/internal/codegen/base/ComponentAnnotation.java +++ b/java/dagger/internal/codegen/base/ComponentAnnotation.java @@ -57,6 +57,19 @@ public abstract class ComponentAnnotation { private static final ImmutableSet> SUBCOMPONENT_ANNOTATIONS = ImmutableSet.of(Subcomponent.class, ProductionSubcomponent.class); + // TODO(erichang): Move ComponentCreatorAnnotation into /base and use that here? + /** The component/subcomponent creator annotation types. */ + private static final ImmutableSet> CREATOR_ANNOTATIONS = + ImmutableSet.of( + Component.Builder.class, + Component.Factory.class, + ProductionComponent.Builder.class, + ProductionComponent.Factory.class, + Subcomponent.Builder.class, + Subcomponent.Factory.class, + ProductionSubcomponent.Builder.class, + ProductionSubcomponent.Factory.class); + /** All component annotation types. */ private static final ImmutableSet> ALL_COMPONENT_ANNOTATIONS = ImmutableSet.>builder() @@ -64,6 +77,13 @@ public abstract class ComponentAnnotation { .addAll(SUBCOMPONENT_ANNOTATIONS) .build(); + /** All component and creator annotation types. */ + private static final ImmutableSet> + ALL_COMPONENT_AND_CREATOR_ANNOTATIONS = ImmutableSet.>builder() + .addAll(ALL_COMPONENT_ANNOTATIONS) + .addAll(CREATOR_ANNOTATIONS) + .build(); + /** The annotation itself. */ public abstract AnnotationMirror annotation(); @@ -206,6 +226,11 @@ public static ImmutableSet> allComponentAnnotations( return ALL_COMPONENT_ANNOTATIONS; } + /** All component and creator annotation types. */ + public static ImmutableSet> allComponentAndCreatorAnnotations() { + return ALL_COMPONENT_AND_CREATOR_ANNOTATIONS; + } + /** * An actual component annotation. * diff --git a/java/dagger/internal/codegen/base/Keys.java b/java/dagger/internal/codegen/base/Keys.java index a25f9963fef..4d139266d31 100644 --- a/java/dagger/internal/codegen/base/Keys.java +++ b/java/dagger/internal/codegen/base/Keys.java @@ -16,6 +16,10 @@ package dagger.internal.codegen.base; +import static com.google.auto.common.MoreTypes.asTypeElement; +import static dagger.internal.codegen.base.ComponentAnnotation.allComponentAndCreatorAnnotations; +import static dagger.internal.codegen.langmodel.DaggerElements.isAnyAnnotationPresent; + import com.google.auto.common.MoreElements; import com.google.auto.common.MoreTypes; import dagger.internal.codegen.langmodel.DaggerTypes; @@ -88,4 +92,14 @@ public Boolean visitDeclared(DeclaredType type, Void ignored) { }, null); } + + /** + * Returns {@code true} if the given key is for a component/subcomponent or a creator of a + * component/subcomponent. + */ + public static boolean isComponentOrCreator(Key key) { + return !key.qualifier().isPresent() + && key.type().getKind() == TypeKind.DECLARED + && isAnyAnnotationPresent(asTypeElement(key.type()), allComponentAndCreatorAnnotations()); + } } diff --git a/java/dagger/internal/codegen/binding/BindingGraphFactory.java b/java/dagger/internal/codegen/binding/BindingGraphFactory.java index 2c15e26252c..a94f6b01e58 100644 --- a/java/dagger/internal/codegen/binding/BindingGraphFactory.java +++ b/java/dagger/internal/codegen/binding/BindingGraphFactory.java @@ -47,6 +47,7 @@ import dagger.Reusable; import dagger.internal.codegen.base.ClearableCache; import dagger.internal.codegen.base.ContributionType; +import dagger.internal.codegen.base.Keys; import dagger.internal.codegen.base.MapType; import dagger.internal.codegen.base.OptionalType; import dagger.internal.codegen.compileroption.CompilerOptions; @@ -796,7 +797,7 @@ void resolve(Key key) { * 2. If there are any explicit bindings in this component, they may conflict with those in * the ancestor component, so resolve them here so that conflicts can be caught. */ - if (getPreviouslyResolvedBindings(key).isPresent()) { + if (getPreviouslyResolvedBindings(key).isPresent() && !Keys.isComponentOrCreator(key)) { /* Resolve in the parent in case there are multibinding contributions or conflicts in some * component between this one and the previously-resolved one. */ parentResolver.get().resolve(key); diff --git a/javatests/dagger/functional/subcomponent/SubcomponentBuilderMultibindingsTest.java b/javatests/dagger/functional/subcomponent/SubcomponentBuilderMultibindingsTest.java new file mode 100644 index 00000000000..7065acc208c --- /dev/null +++ b/javatests/dagger/functional/subcomponent/SubcomponentBuilderMultibindingsTest.java @@ -0,0 +1,265 @@ +/* + * Copyright (C) 2015 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.subcomponent; + +import static com.google.common.truth.Truth.assertThat; + +import dagger.Component; +import dagger.Module; +import dagger.Provides; +import dagger.Subcomponent; +import dagger.multibindings.IntoSet; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.util.Set; +import javax.inject.Inject; +import javax.inject.Qualifier; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Regression tests for an issue where subcomponent builder bindings were incorrectly reused from + * a parent even if the subcomponent were redeclared on the child component. This manifested via + * multibindings, especially since subcomponent builder bindings are special in that we cannot + * traverse them to see if they depend on local multibinding contributions. + */ +@RunWith(JUnit4.class) +public final class SubcomponentBuilderMultibindingsTest { + + @Retention(RetentionPolicy.RUNTIME) + @Qualifier + public @interface ParentFoo{} + + @Retention(RetentionPolicy.RUNTIME) + @Qualifier + public @interface ChildFoo{} + + public static final class Foo { + final Set multi; + @Inject Foo(Set multi) { + this.multi = multi; + } + } + + // This tests the case where a subcomponent is installed in both the parent and child component. + // In this case, we expect two subcomponents to be generated with the child one including the + // child multibinding contribution. + public static final class ChildInstallsFloating { + @Component(modules = ParentModule.class) + public interface Parent { + @ParentFoo Foo getParentFoo(); + + Child getChild(); + } + + @Subcomponent(modules = ChildModule.class) + public interface Child { + @ChildFoo Foo getChildFoo(); + } + + @Subcomponent + public interface FloatingSub { + Foo getFoo(); + + @Subcomponent.Builder + public interface Builder { + FloatingSub build(); + } + } + + @Module(subcomponents = FloatingSub.class) + public interface ParentModule { + @Provides + @IntoSet + static String provideStringMulti() { + return "parent"; + } + + @Provides + @ParentFoo + static Foo provideParentFoo(FloatingSub.Builder builder) { + return builder.build().getFoo(); + } + } + + // The subcomponent installation of FloatingSub here is the key difference + @Module(subcomponents = FloatingSub.class) + public interface ChildModule { + @Provides + @IntoSet + static String provideStringMulti() { + return "child"; + } + + @Provides + @ChildFoo + static Foo provideChildFoo(FloatingSub.Builder builder) { + return builder.build().getFoo(); + } + } + + private ChildInstallsFloating() {} + } + + // This is the same as the above, except this time the child does not install the subcomponent + // builder. Here, we expect the child to reuse the parent subcomponent binding (we want to avoid + // any mistakes that might implicitly create a new subcomponent relationship) and so therefore + // we expect only one subcomponent to be generated in the parent resulting in the child not seeing + // the child multibinding contribution. + public static final class ChildDoesNotInstallFloating { + @Component(modules = ParentModule.class) + public interface Parent { + @ParentFoo Foo getParentFoo(); + + Child getChild(); + } + + @Subcomponent(modules = ChildModule.class) + public interface Child { + @ChildFoo Foo getChildFoo(); + } + + @Subcomponent + public interface FloatingSub { + Foo getFoo(); + + @Subcomponent.Builder + public interface Builder { + FloatingSub build(); + } + } + + @Module(subcomponents = FloatingSub.class) + public interface ParentModule { + @Provides + @IntoSet + static String provideStringMulti() { + return "parent"; + } + + @Provides + @ParentFoo + static Foo provideParentFoo(FloatingSub.Builder builder) { + return builder.build().getFoo(); + } + } + + // The lack of a subcomponent installation of FloatingSub here is the key difference + @Module + public interface ChildModule { + @Provides + @IntoSet + static String provideStringMulti() { + return "child"; + } + + @Provides + @ChildFoo + static Foo provideChildFoo(FloatingSub.Builder builder) { + return builder.build().getFoo(); + } + } + + private ChildDoesNotInstallFloating() {} + } + + // This is similar to the first, except this time the components installs the subcomponent via + // factory methods. Here, we expect the child to get a new subcomponent and so should see its + // multibinding contribution. + public static final class ChildInstallsFloatingFactoryMethod { + @Component(modules = ParentModule.class) + public interface Parent { + @ParentFoo Foo getParentFoo(); + + Child getChild(); + + FloatingSub getFloatingSub(); + } + + @Subcomponent(modules = ChildModule.class) + public interface Child { + @ChildFoo Foo getChildFoo(); + + FloatingSub getFloatingSub(); + } + + @Subcomponent + public interface FloatingSub { + Foo getFoo(); + } + + @Module + public interface ParentModule { + @Provides + @IntoSet + static String provideStringMulti() { + return "parent"; + } + + @Provides + @ParentFoo + static Foo provideParentFoo(Parent componentSelf) { + return componentSelf.getFloatingSub().getFoo(); + } + } + + @Module + public interface ChildModule { + @Provides + @IntoSet + static String provideStringMulti() { + return "child"; + } + + @Provides + @ChildFoo + static Foo provideChildFoo(Child componentSelf) { + return componentSelf.getFloatingSub().getFoo(); + } + } + + private ChildInstallsFloatingFactoryMethod() {} + } + + @Test + public void testChildInstallsFloating() { + ChildInstallsFloating.Parent parentComponent = + DaggerSubcomponentBuilderMultibindingsTest_ChildInstallsFloating_Parent.create(); + assertThat(parentComponent.getParentFoo().multi).containsExactly("parent"); + assertThat(parentComponent.getChild().getChildFoo().multi).containsExactly("parent", "child"); + } + + @Test + public void testChildDoesNotInstallFloating() { + ChildDoesNotInstallFloating.Parent parentComponent = + DaggerSubcomponentBuilderMultibindingsTest_ChildDoesNotInstallFloating_Parent.create(); + assertThat(parentComponent.getParentFoo().multi).containsExactly("parent"); + // Don't expect the child contribution because the child didn't redeclare the subcomponent + // dependency, meaning it intends to just use the subcomponent relationship from the parent + // component. + assertThat(parentComponent.getChild().getChildFoo().multi).containsExactly("parent"); + } + + @Test + public void testChildInstallsFloatingFactoryMethod() { + ChildInstallsFloatingFactoryMethod.Parent parentComponent = + DaggerSubcomponentBuilderMultibindingsTest_ChildInstallsFloatingFactoryMethod_Parent.create(); + assertThat(parentComponent.getParentFoo().multi).containsExactly("parent"); + assertThat(parentComponent.getChild().getChildFoo().multi).containsExactly("parent", "child"); + } +}