Make FrameworkInstanceBindingRepresentation do not use `DirectInsta…

…nceBindingRepresentation`.

RELNOTES=n/a
PiperOrigin-RevId: 408544948

java/dagger/internal/codegen/binding/BindingGraph.java

@@ -18,6 +18,7 @@
 
 import static com.google.common.collect.Iterables.transform;
 import static dagger.internal.codegen.extension.DaggerCollectors.toOptional;
+import static dagger.internal.codegen.extension.DaggerStreams.instancesOf;
 import static dagger.internal.codegen.extension.DaggerStreams.presentValues;
 import static dagger.internal.codegen.extension.DaggerStreams.stream;
 import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList;
@@ -40,15 +41,20 @@
 import dagger.internal.codegen.base.TarjanSCCs;
 import dagger.spi.model.BindingGraph.ChildFactoryMethodEdge;
 import dagger.spi.model.BindingGraph.ComponentNode;
+import dagger.spi.model.BindingGraph.DependencyEdge;
 import dagger.spi.model.BindingGraph.Edge;
 import dagger.spi.model.BindingGraph.Node;
+import dagger.spi.model.BindingKind;
 import dagger.spi.model.ComponentPath;
 import dagger.spi.model.DaggerTypeElement;
+import dagger.spi.model.DependencyRequest;
 import dagger.spi.model.Key;
 import java.util.Comparator;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Stream;
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.TypeElement;
@@ -90,11 +96,14 @@ static TopLevelBindingGraph create(
       // AutoValue to prevent exposing this data outside of the class.
       topLevelBindingGraph.componentNodes = componentNodes;
       topLevelBindingGraph.subcomponentNodes = subcomponentNodesBuilder.build();
+      topLevelBindingGraph.frameworkTypeBindings =
+          frameworkRequestBindingSet(network, topLevelBindingGraph.bindings());
       return topLevelBindingGraph;
     }
 
     private ImmutableMap<ComponentPath, ComponentNode> componentNodes;
     private ImmutableSetMultimap<ComponentNode, ComponentNode> subcomponentNodes;
+    private ImmutableSet<Binding> frameworkTypeBindings;
 
     TopLevelBindingGraph() {}
 
@@ -148,6 +157,63 @@ public ImmutableSet<ImmutableSet<Node>> stronglyConnectedNodes() {
           node ->
               network().successors(node).stream().sorted(nodeOrder()).collect(toImmutableList()));
     }
+
+    public boolean hasframeworkRequest(Binding binding) {
+      return frameworkTypeBindings.contains(binding);
+    }
+
+    private static ImmutableSet<Binding> frameworkRequestBindingSet(
+        ImmutableNetwork<Node, Edge> network, ImmutableSet<dagger.spi.model.Binding> bindings) {
+      Set<Binding> frameworkRequestBindings = new HashSet<>();
+      for (dagger.spi.model.Binding binding : bindings) {
+        // When a delegator binding received an instance request, it will manually create an
+        // instance request for its delegated binding in direct instance binding representation. It
+        // is possible a provider.get() expression will be returned to satisfy the request for the
+        // delegated binding. In this case, the returned expression should have a type cast, because
+        // the returned expression's type can be Object. The type cast is handled by
+        // DelegateRequestRepresentation. If we change to use framework instance binding
+        // representation to handle the delegate bindings, then we will be missing the type cast.
+        // Because in this case, when requesting an instance for the delegator binding, framework
+        // instance binding representation will manually create a provider request for delegated
+        // binding first, then use DerivedFromFrameworkInstanceRequestRepresentaion to wrap that
+        // provider expression. Then we will still have a provider.get(), but it is generated with
+        // two different request representation, so the type cast step is skipped. As the result, we
+        // can't directly switch the delegation binding to always use the framework instance if a
+        // framework request already exists. So I'm adding an temporary exemption for delegate
+        // binding here to make it still use the old generation logic. We might be able to remove
+        // the exemption when we handle the type cast differently.
+        // TODO(wanyingd): fix the type cast problem and remove the exemption for delegate binding.
+        if (binding.kind().equals(BindingKind.DELEGATE)
+            // In fast init mode, for Assisted injection binding, since we manually create a direct
+            // instance when the request type is a Provider, then there can't really be any
+            // framework requests for the binding.
+            // TODO(wanyingd): inline assisted injection binding expression in assisted factory.
+            || binding.kind().equals(BindingKind.ASSISTED_INJECTION)) {
+          continue;
+        }
+        ImmutableList<DependencyEdge> edges =
+            network.inEdges(binding).stream()
+                .flatMap(instancesOf(DependencyEdge.class))
+                .collect(toImmutableList());
+        for (DependencyEdge edge : edges) {
+          DependencyRequest request = edge.dependencyRequest();
+          switch (request.kind()) {
+            case INSTANCE:
+            case FUTURE:
+              continue;
+            case PRODUCED:
+            case PRODUCER:
+            case MEMBERS_INJECTION:
+            case PROVIDER_OF_LAZY:
+            case LAZY:
+            case PROVIDER:
+              frameworkRequestBindings.add(((BindingNode) binding).delegate());
+              break;
+          }
+        }
+      }
+      return ImmutableSet.copyOf(frameworkRequestBindings);
+    }
   }
 
   static BindingGraph create(

java/dagger/internal/codegen/writing/FrameworkFieldInitializer.java

@@ -111,13 +111,13 @@ private void initializeField() {
 
       case INITIALIZING:
         fieldSpec = getOrCreateField();
-        // If this is an ASSISTED_FACTORY binding in fastInit, then we don't have to worry about
-        // cycles since the creation of the factory itself doesn't actually take any dependencies.
-        // TODO(wanyingd): all switching providers do not need the delegation
-        if (isFastInit && binding.kind().equals(BindingKind.ASSISTED_FACTORY)) {
+        // We were recursively invoked, so create a delegate factory instead to break the loop.
+        // However, because SwitchingProvider takes no dependencies, even if they are recursively
+        // invoked, we don't need to delegate it since there is no dependency cycle.
+        if (ProvisionBindingRepresentation.usesSwitchingProvider(binding, isFastInit)) {
           break;
         }
-        // We were recursively invoked, so create a delegate factory instead
+
         fieldInitializationState = InitializationState.DELEGATED;
         shardImplementation.addInitialization(
             CodeBlock.of("this.$N = new $T<>();", fieldSpec, delegateType()));

java/dagger/internal/codegen/writing/ProvisionBindingRepresentation.java

@@ -25,6 +25,7 @@
 import dagger.assisted.AssistedInject;
 import dagger.internal.codegen.binding.BindingGraph;
 import dagger.internal.codegen.binding.BindingRequest;
+import dagger.internal.codegen.binding.ContributionBinding;
 import dagger.internal.codegen.binding.ProvisionBinding;
 import dagger.internal.codegen.compileroption.CompilerOptions;
 import dagger.internal.codegen.langmodel.DaggerTypes;
@@ -59,10 +60,8 @@ final class ProvisionBindingRepresentation implements BindingRepresentation {
     this.directInstanceBindingRepresentation =
         directInstanceBindingRepresentationFactory.create(binding);
     FrameworkInstanceSupplier frameworkInstanceSupplier = null;
-    if (usesSwitchingProvider()) {
-      frameworkInstanceSupplier =
-          switchingProviderInstanceSupplierFactory.create(
-              binding, directInstanceBindingRepresentation);
+    if (usesSwitchingProvider(binding, isFastInit)) {
+      frameworkInstanceSupplier = switchingProviderInstanceSupplierFactory.create(binding);
     } else if (usesStaticFactoryCreation(binding, isFastInit)) {
       frameworkInstanceSupplier = staticFactoryInstanceSupplierFactory.create(binding);
     } else {
@@ -74,16 +73,27 @@ final class ProvisionBindingRepresentation implements BindingRepresentation {
 
   @Override
   public RequestRepresentation getRequestRepresentation(BindingRequest request) {
-    return usesDirectInstanceExpression(request.requestKind(), binding, graph, isFastInit)
+    return usesDirectInstanceExpression(request.requestKind())
         ? directInstanceBindingRepresentation.getRequestRepresentation(request)
         : frameworkInstanceBindingRepresentation.getRequestRepresentation(request);
   }
 
-  static boolean usesDirectInstanceExpression(
-      RequestKind requestKind, ProvisionBinding binding, BindingGraph graph, boolean isFastInit) {
+  private boolean usesDirectInstanceExpression(RequestKind requestKind) {
     if (requestKind != RequestKind.INSTANCE && requestKind != RequestKind.FUTURE) {
       return false;
     }
+
+    // In fast init mode, we can avoid generating direct instance expressions if a framework
+    // instance expression already exists in the graph. Default mode has more edge cases, so can not
+    // be handled with simple pre-check in the graph. For example, a provider for a subcomponent
+    // builder is backed with its direct instance, returning framework instance for both cases will
+    // form a loop. There are also difficulties introduced by manually created framework requests.
+    // TODO(wanyingd): refactor framework instance so that we don't need to generate both direct
+    // instance and framework instance representation for the same binding.
+    if (isFastInit && graph.topLevelBindingGraph().hasframeworkRequest(binding)) {
+      return false;
+    }
+
     switch (binding.kind()) {
       case MEMBERS_INJECTOR:
         // Currently, we always use a framework instance for MembersInjectors, e.g.
@@ -107,7 +117,7 @@ static boolean usesDirectInstanceExpression(
     }
   }
 
-  private boolean usesSwitchingProvider() {
+  public static boolean usesSwitchingProvider(ContributionBinding binding, boolean isFastInit) {
     if (!isFastInit) {
       return false;
     }

java/dagger/internal/codegen/writing/SwitchingProviderInstanceSupplier.java

@@ -16,22 +16,18 @@
 
 package dagger.internal.codegen.writing;
 
-import static dagger.internal.codegen.binding.BindingRequest.bindingRequest;
 import static dagger.internal.codegen.javapoet.TypeNames.DOUBLE_CHECK;
 import static dagger.internal.codegen.javapoet.TypeNames.SINGLE_CHECK;
-import static dagger.internal.codegen.writing.ProvisionBindingRepresentation.usesDirectInstanceExpression;
 
 import com.squareup.javapoet.CodeBlock;
 import dagger.assisted.Assisted;
 import dagger.assisted.AssistedFactory;
 import dagger.assisted.AssistedInject;
 import dagger.internal.codegen.binding.Binding;
 import dagger.internal.codegen.binding.BindingGraph;
-import dagger.internal.codegen.binding.BindingRequest;
 import dagger.internal.codegen.binding.ProvisionBinding;
 import dagger.internal.codegen.writing.FrameworkFieldInitializer.FrameworkInstanceCreationExpression;
 import dagger.spi.model.BindingKind;
-import dagger.spi.model.RequestKind;
 
 /**
  * An object that initializes a framework-type component field for a binding using instances created
@@ -43,26 +39,14 @@ final class SwitchingProviderInstanceSupplier implements FrameworkInstanceSuppli
   @AssistedInject
   SwitchingProviderInstanceSupplier(
       @Assisted ProvisionBinding binding,
-      @Assisted DirectInstanceBindingRepresentation directInstanceBindingRepresentation,
       SwitchingProviders switchingProviders,
       BindingGraph graph,
       ComponentImplementation componentImplementation,
       UnscopedDirectInstanceRequestRepresentationFactory
           unscopedDirectInstanceRequestRepresentationFactory) {
-    BindingRequest instanceRequest = bindingRequest(binding.key(), RequestKind.INSTANCE);
     FrameworkInstanceCreationExpression frameworkInstanceCreationExpression =
         switchingProviders.newFrameworkInstanceCreationExpression(
-            binding,
-            // Use the directInstanceBindingRepresentation if possible, that way we share a private
-            // method implementation if one already exists. Otherwise, we use the
-            // unscopedDirectInstanceRequestRepresentation and, since we're guaranteed this is the
-            // only place that will be using the expression in this case, there is no need to wrap
-            // the expression in a private method.
-            // Note: we can't use ComponentBindingRepresentation.getRequestRepresentation(
-            // instanceRequest) here, since that would return fooProvider.get() and cause a cycle.
-            usesDirectInstanceExpression(RequestKind.INSTANCE, binding, graph, true)
-                ? directInstanceBindingRepresentation.getRequestRepresentation(instanceRequest)
-                : unscopedDirectInstanceRequestRepresentationFactory.create(binding));
+            binding, unscopedDirectInstanceRequestRepresentationFactory.create(binding));
     this.frameworkInstanceSupplier =
         new FrameworkFieldInitializer(
             componentImplementation, binding, scope(binding, frameworkInstanceCreationExpression));
@@ -91,8 +75,6 @@ private FrameworkInstanceCreationExpression scope(
 
   @AssistedFactory
   static interface Factory {
-    SwitchingProviderInstanceSupplier create(
-        ProvisionBinding binding,
-        DirectInstanceBindingRepresentation directInstanceBindingRepresentation);
+    SwitchingProviderInstanceSupplier create(ProvisionBinding binding);
   }
 }

javatests/dagger/internal/codegen/AssistedFactoryTest.java

@@ -125,7 +125,7 @@ public void testAssistedFactory() {
                 "    @Override",
                 "    public T get() {",
                 "      switch (id) {",
-                "        case 0: // test.FooFactory ",
+                "        case 0:",
                 "        return (T) new FooFactory() {",
                 "          @Override",
                 "          public Foo create(String str) {",
@@ -249,7 +249,7 @@ public void testAssistedFactoryCycle() {
                 "    @Override",
                 "    public T get() {",
                 "      switch (id) {",
-                "        case 0: // test.FooFactory ",
+                "        case 0:",
                 "        return (T) new FooFactory() {",
                 "          @Override",
                 "          public Foo create(String str) {",
@@ -375,7 +375,7 @@ public void testInjectParamDuplicateNames() {
                 "    @Override",
                 "    public T get() {",
                 "      switch (id) {",
-                "        case 0: // test.FooFactory ",
+                "        case 0:",
                 "        return (T) new FooFactory() {",
                 "          @Override",
                 "          public Foo create(Integer arg) {",
@@ -522,7 +522,7 @@ public void testAssistParamDuplicateNames() {
                 "    @Override",
                 "    public T get() {",
                 "      switch (id) {",
-                "        case 0: // test.FooFactory ",
+                "        case 0:",
                 "        return (T) new FooFactory() {",
                 "          @Override",
                 "          public Foo create(Integer arg) {",
@@ -756,8 +756,7 @@ public void testParameterizedAssistParam() {
                 "    @Override",
                 "    public T get() {",
                 "      switch (id) {",
-                "        case 0: // test.FooFactory<java.lang.String> ",
-                "        return (T) new FooFactory<String>() {",
+                "        case 0: return (T) new FooFactory<String>() {",
                 "          @Override",
                 "          public Foo<String> create(String arg) {",
                 "            return testComponent.fooOfString(arg);",