diff --git a/java/dagger/internal/codegen/binding/BindingGraphFactory.java b/java/dagger/internal/codegen/binding/BindingGraphFactory.java index 84640db875d..1a70870d059 100644 --- a/java/dagger/internal/codegen/binding/BindingGraphFactory.java +++ b/java/dagger/internal/codegen/binding/BindingGraphFactory.java @@ -335,13 +335,22 @@ && isAssistedFactoryType(requestKey.type().xprocessing().getTypeElement())) { requestKey, bindings.stream() .map( - binding -> - bindingNodeFactory.forContributionBindings( - getOwningComponentPath(requestKey, binding), - binding, - multibindingDeclarations, - optionalBindingDeclarations, - subcomponentDeclarations)) + binding -> { + Optional bindingNodeOwnedByAncestor = + getBindingNodeOwnedByAncestor(requestKey, binding); + // If a binding is owned by an ancestor we use the corresponding BindingNode + // instance directly rather than creating a new instance to avoid accidentally + // including additional multi/optional/subcomponent declarations that don't + // exist in the ancestor's BindingNode instance. + return bindingNodeOwnedByAncestor.isPresent() + ? bindingNodeOwnedByAncestor.get() + : bindingNodeFactory.forContributionBindings( + componentPath, + binding, + multibindingDeclarations, + optionalBindingDeclarations, + subcomponentDeclarations); + }) .collect(toImmutableSet())); } @@ -443,38 +452,34 @@ private ContributionBinding createDelegateBinding(DelegateDeclaration delegateDe } /** - * Returns the component that should contain the framework field for {@code binding}. - * - *

If {@code binding} is either not bound in an ancestor component or depends transitively on - * bindings in this component, returns this component. - * - *

Otherwise, resolves {@code request} in this component's parent in order to resolve any - * multibinding contributions in the parent, and returns the parent-resolved {@link - * ResolvedBindings#owningComponent(ContributionBinding)}. + * Returns a {@link BindingNode} for the given binding that is owned by an ancestor component, + * if one exists. Otherwise returns {@link Optional#empty()}. */ - private ComponentPath getOwningComponentPath(Key requestKey, ContributionBinding binding) { - if (isResolvedInParent(requestKey, binding) && !requiresResolution(binding)) { - ResolvedBindings parentResolvedBindings = - parentResolver.get().resolvedContributionBindings.get(requestKey); - return parentResolvedBindings.forBinding(binding).componentPath(); - } else { - return componentPath; + private Optional getBindingNodeOwnedByAncestor( + Key requestKey, ContributionBinding binding) { + if (canBeResolvedInParent(requestKey, binding)) { + // Resolve in the parent to make sure we have the most recent multi/optional contributions. + parentResolver.get().resolve(requestKey); + if (!requiresResolution(binding)) { + return Optional.of(getPreviouslyResolvedBindings(requestKey).get().forBinding(binding)); + } } + return Optional.empty(); } - /** - * Returns {@code true} if {@code binding} is owned by an ancestor. If so, {@linkplain #resolve - * resolves} the {@link Key} in this component's parent. Don't resolve directly in the owning - * component in case it depends on multibindings in any of its descendants. - */ - private boolean isResolvedInParent(Key requestKey, ContributionBinding binding) { - Optional owningResolver = getOwningResolver(binding); - if (owningResolver.isPresent() && !owningResolver.get().equals(this)) { - parentResolver.get().resolve(requestKey); - return true; - } else { + private boolean canBeResolvedInParent(Key requestKey, ContributionBinding binding) { + if (parentResolver.isEmpty()) { return false; } + Optional owningResolver = getOwningResolver(binding); + if (owningResolver.isPresent()) { + return !owningResolver.get().equals(this); + } + return !Keys.isComponentOrCreator(requestKey) + // TODO(b/305748522): Allow caching for assisted injection bindings. + && binding.kind() != BindingKind.ASSISTED_INJECTION + && getPreviouslyResolvedBindings(requestKey).isPresent() + && getPreviouslyResolvedBindings(requestKey).get().bindings().contains(binding); } private Optional getOwningResolver(ContributionBinding binding) { @@ -509,6 +514,9 @@ private Optional getOwningResolver(ContributionBinding binding) { return Optional.empty(); } + // TODO(b/359893922): we currently iterate from child to parent to find an owning resolver, + // but we probably want to iterate from parent to child to catch missing bindings in + // misconfigured repeated modules. for (Resolver requestResolver : getResolverLineage().reverse()) { if (requestResolver.containsExplicitBinding(binding)) { return Optional.of(requestResolver); @@ -647,36 +655,6 @@ void resolve(Key key) { return; } - /* - * If the binding was previously resolved in an ancestor component, then we may be able to - * avoid resolving it here and just depend on the ancestor component resolution. - * - * 1. If it depends transitively on multibinding contributions or optional bindings with - * bindings from this subcomponent, then we have to resolve it in this subcomponent so - * that it sees the local bindings. - * - * 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() && !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); - ResolvedBindings previouslyResolvedBindings = getPreviouslyResolvedBindings(key).get(); - // TODO(b/305748522): Allow caching for assisted injection bindings. - boolean isAssistedInjectionBinding = - previouslyResolvedBindings.bindings().stream() - .anyMatch(binding -> binding.kind() == BindingKind.ASSISTED_INJECTION); - if (!isAssistedInjectionBinding - && !requiresResolution(key) - && !hasLocalExplicitBindings(key)) { - /* Cache the inherited parent component's bindings in case resolving at the parent found - * bindings in some component between this one and the previously-resolved one. */ - resolvedContributionBindings.put(key, previouslyResolvedBindings); - return; - } - } - cycleStack.push(key); try { ResolvedBindings bindings = lookUpBindings(key); @@ -713,10 +691,6 @@ private ResolvedBindings getResolvedMembersInjectionBindings(Key key) { return resolvedMembersInjectionBindings.get(key); } - private boolean requiresResolution(Key key) { - return new LegacyRequiresResolutionChecker().requiresResolution(key); - } - private boolean requiresResolution(Binding binding) { return new LegacyRequiresResolutionChecker().requiresResolution(binding); } diff --git a/javatests/dagger/internal/codegen/OptionalBindingTest.java b/javatests/dagger/internal/codegen/OptionalBindingTest.java index bcec73ff632..76a0592dbde 100644 --- a/javatests/dagger/internal/codegen/OptionalBindingTest.java +++ b/javatests/dagger/internal/codegen/OptionalBindingTest.java @@ -105,4 +105,74 @@ public void provideExplicitOptionalInParent_AndBindsOptionalOfInChild() { .onLineContaining("interface Parent"); }); } + + // Note: This is a regression test for an issue we ran into in CL/644086367, where an optional + // binding owned by a parent component is also requested by a child component which declares an + // additional @BindsOptionalOf declaration. In this case, we just want to make sure that the setup + // builds successfully. + @Test + public void cachedInParent_succeeds() { + Source parent = + CompilerTests.javaSource( + "test.Parent", + "package test;", + "", + "import dagger.Component;", + "import java.util.Optional;", + "", + "@Component(modules = ParentModule.class)", + "interface Parent {", + " Optional optionalString();", + " Child child();", + "}"); + Source parentModule = + CompilerTests.javaSource( + "test.ParentModule", + "package test;", + "", + "import dagger.BindsOptionalOf;", + "import dagger.Module;", + "import dagger.Provides;", + "import java.util.Optional;", + "", + "@Module", + "interface ParentModule {", + " @BindsOptionalOf", + " String optionalParentString();", + "", + " @Provides", + " static String provideString() {", + " return \"\";", + " }", + "}"); + Source child = + CompilerTests.javaSource( + "test.Child", + "package test;", + "", + "import dagger.Subcomponent;", + "import java.util.Optional;", + "", + "@Subcomponent(modules = ChildModule.class)", + "interface Child {", + " Optional optionalString();", + "}"); + Source childModule = + CompilerTests.javaSource( + "test.ChildModule", + "package test;", + "", + "import dagger.BindsOptionalOf;", + "import dagger.Module;", + "", + "@Module", + "interface ChildModule {", + " @BindsOptionalOf", + " String optionalChildString();", + "}"); + + CompilerTests.daggerCompiler(parent, parentModule, child, childModule) + .withProcessingOptions(compilerMode.processorOptions()) + .compile(subject -> subject.hasErrorCount(0)); + } }