Combine BindingGraphFactory.Resolver's caching logic into a single location.

bcorso · Dagger Team · commit 86fcef595888 · 2024-08-09T12:38:56.000-07:00
Currently, there are two places where we try to cache a binding:

  1. `Resolver#resolve(Key)` checks if there's a previously resolved binding for the given key in an ancestor resolver.
  2. `Resolver#lookUpBindings(Key)` checks if a resolved binding should be owned by an ancestor component (e.g. because its scoped).

Note that these two checks are slightly different. Case 1 only depends on the `Key` and is iteration-order dependent since it depends on whether the key has already been resolved in a ancestor binding. Case 2 depends on the binding rather than just the key (which is why it occurs at the end of `lookUpBindings`) and tries to determine if a binding could be resolved in a parent component (e.g. based on the scope or which component installs the module). Thus, case 2 may cache bindings that case 1 doesn't in cases where we know a binding is resolvable in an ancestor but hasn't yet been resolved (e.g. due to order or just because nothing actually requests it).

In any case, this CL consolidates that logic into a single place. This should make it easier to extract the caching logic later from the intermediate graph creation.

RELNOTES=N/A
PiperOrigin-RevId: 661363402
diff --git 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<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);
+                  })
               .collect(toImmutableSet()));
     }
 
@@ -443,38 +452,32 @@ private ContributionBinding createDelegateBinding(DelegateDeclaration delegateDe
     }
 
     /**
-     * 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)}.
+     * 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<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));
+        }
       }
+      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<Resolver> 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<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) {
@@ -647,36 +650,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 +686,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
@@ -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<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));
+  }
 }
