Skip to content

Commit

Permalink
Rollback of: Combine BindingGraphFactory.Resolver's caching logic i…
Browse files Browse the repository at this point in the history 
…nto a single location.

PiperOrigin-RevId: 661446058
  • Loading branch information
@bcorso
bcorso authored and Dagger Team committed Aug 9, 2024
1 parent 86fcef5 commit f397f6b
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 106 deletions.
103 changes: 67 additions & 36 deletions java/dagger/internal/codegen/binding/BindingGraphFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,22 +335,13 @@ && isAssistedFactoryType(requestKey.type().xprocessing().getTypeElement())) {
requestKey,
bindings.stream()
.map(
binding -> {
Optional<BindingNode> 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);
})
binding ->
bindingNodeFactory.forContributionBindings(
getOwningComponentPath(requestKey, binding),
binding,
multibindingDeclarations,
optionalBindingDeclarations,
subcomponentDeclarations))
.collect(toImmutableSet()));
}

Expand Down Expand Up @@ -452,32 +443,38 @@ private ContributionBinding createDelegateBinding(DelegateDeclaration delegateDe
}

/**
* Returns a {@link BindingNode} for the given binding that is owned by an ancestor component,
* if one exists. Otherwise returns {@link Optional#empty()}.
* Returns the component that should contain the framework field for {@code binding}.
*
* <p>If {@code binding} is either not bound in an ancestor component or depends transitively on
* bindings in this component, returns this component.
*
* <p>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)}.
*/
private Optional<BindingNode> 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));
}
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;
}
return Optional.empty();
}

private boolean canBeResolvedInParent(Key requestKey, ContributionBinding binding) {
if (parentResolver.isEmpty()) {
/**
* 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<Resolver> owningResolver = getOwningResolver(binding);
if (owningResolver.isPresent() && !owningResolver.get().equals(this)) {
parentResolver.get().resolve(requestKey);
return true;
} else {
return false;
}
Optional<Resolver> owningResolver = getOwningResolver(binding);
return (owningResolver.isPresent() && !owningResolver.get().equals(this))
|| (!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<Resolver> getOwningResolver(ContributionBinding binding) {
Expand Down Expand Up @@ -650,6 +647,36 @@ 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);
Expand Down Expand Up @@ -686,6 +713,10 @@ 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);
}
Expand Down
70 changes: 0 additions & 70 deletions javatests/dagger/internal/codegen/OptionalBindingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,74 +105,4 @@ 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<String> 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<String> 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));
}
}

0 comments on commit f397f6b

Please sign in to comment.