Skip to content

Commit

Permalink
Fix broken assertion in `RequiresResolutionChecker#visitUncachedDepen…
Browse files Browse the repository at this point in the history
…dencies()`

Within `visitUncachedDependencies()` we assert that each uncached key is actually uncached before caching it. However, this assertion can fail because during the process of caching, we call `getLocalExplicitBindings()` and `getLocalMultibindingContributions()` which can trigger its own call to `visitUncachedDependencies()` and cache it first.

This CL fixes this issue by avoiding the calls to `getLocalExplicitBindings()` and `getLocalMultibindingContributions()`. In these cases we don't actually need the bindings themselves since we just need to know if the declarations exist or not, so we can just use the declarations directly. This saves us a bit of work because creating the binding requires us to not only resolve the binding itself, but also bindings for dependencies (possibly even transitive dependencies) in the case `@Binds` delegate declarations. It's also nice to keep the assertion because it gaurantees some simplicity to the code.

RELNOTES=N/A
PiperOrigin-RevId: 653633501
  • Loading branch information
bcorso authored and Dagger Team committed Jul 18, 2024
1 parent 982dab4 commit 216a9d2
Showing 1 changed file with 16 additions and 6 deletions.
22 changes: 16 additions & 6 deletions java/dagger/internal/codegen/binding/BindingGraphFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ void resolve(Key key) {
.anyMatch(binding -> binding.kind() == BindingKind.ASSISTED_INJECTION);
if (!isAssistedInjectionBinding
&& !requiresResolution(key)
&& getLocalExplicitBindings(key).isEmpty()) {
&& !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);
Expand Down Expand Up @@ -867,9 +867,12 @@ private boolean hasLocalBindings(ResolvedBindings resolvedBindings) {
* this component's modules that matches the key.
*/
private boolean hasLocalMultibindingContributions(Key requestKey) {
return keysMatchingRequest(requestKey)
.stream()
.anyMatch(key -> !getLocalMultibindingContributions(key).isEmpty());
return keysMatchingRequest(requestKey).stream()
.anyMatch(
multibindingKey ->
!declarations.multibindingContributions(multibindingKey).isEmpty()
|| !declarations.delegateMultibindingContributions(
keyFactory.unwrapMapValueType(multibindingKey)).isEmpty());
}

/**
Expand All @@ -886,14 +889,21 @@ private boolean hasLocalOptionalBindingContribution(
if (previouslyResolvedBindings.stream()
.map(Binding::kind)
.anyMatch(isEqual(OPTIONAL))) {
return !getLocalExplicitBindings(keyFactory.unwrapOptional(key).get())
.isEmpty();
return hasLocalExplicitBindings(keyFactory.unwrapOptional(key).get());
} else {
// If a parent contributes a @Provides Optional<Foo> binding and a child has a
// @BindsOptionalOf Foo method, the two should conflict, even if there is no binding for
// Foo on its own
return !getOptionalBindingDeclarations(key).isEmpty();
}
}

/**
* Returns {@code true} if there is at least one explicit binding that matches the given key.
*/
private boolean hasLocalExplicitBindings(Key requestKey) {
return !declarations.bindings(requestKey).isEmpty()
|| !declarations.delegates(keyFactory.unwrapMapValueType(requestKey)).isEmpty();
}
}
}

0 comments on commit 216a9d2

Please sign in to comment.