Rollback fix for LocalDependencyChecker#dependsOnLocalBindings and …

…a flag to enable it.

RELNOTES=N/A
PiperOrigin-RevId: 576615427

java/dagger/internal/codegen/binding/BindingGraphFactory.java

@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static dagger.internal.codegen.base.RequestKinds.getRequestKind;
+import static dagger.internal.codegen.base.Util.reentrantComputeIfAbsent;
 import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedFactoryType;
 import static dagger.internal.codegen.binding.SourceFiles.generatedMonitoringModuleName;
 import static dagger.internal.codegen.model.BindingKind.ASSISTED_INJECTION;
@@ -74,13 +75,6 @@
 /** A factory for {@link BindingGraph} objects. */
 @Singleton
 public final class BindingGraphFactory implements ClearableCache {
-  private enum DependsOnLocalBindingsResult {
-    TRUE,
-    FALSE,
-    // A result is temporarily unknown if we run into a cycle since whether or not a binding in a
-    // cycle depends on a local binding depends on all other bindings in the cycle.
-    UNKNOWN
-  }
 
   private final XProcessingEnv processingEnv;
   private final InjectBindingRegistry injectBindingRegistry;
@@ -330,9 +324,8 @@ private final class Resolver {
     final Map<Key, ResolvedBindings> resolvedContributionBindings = new LinkedHashMap<>();
     final Map<Key, ResolvedBindings> resolvedMembersInjectionBindings = new LinkedHashMap<>();
     final Deque<Key> cycleStack = new ArrayDeque<>();
-    final Map<Key, DependsOnLocalBindingsResult> keyDependsOnLocalBindingsCache = new HashMap<>();
-    final Map<Binding, DependsOnLocalBindingsResult> bindingDependsOnLocalBindingsCache =
-        new HashMap<>();
+    final Map<Key, Boolean> keyDependsOnLocalBindingsCache = new HashMap<>();
+    final Map<Binding, Boolean> bindingDependsOnLocalBindingsCache = new HashMap<>();
     final Queue<ComponentDescriptor> subcomponentsToResolve = new ArrayDeque<>();
 
     Resolver(
@@ -877,7 +870,7 @@ ImmutableMap<Key, ResolvedBindings> getResolvedMembersInjectionBindings() {
     }
 
     private final class LocalDependencyChecker {
-      private final Deque<Object> localCycleStack = new ArrayDeque<>();
+      private final Set<Object> cycleChecker = new HashSet<>();
 
       /**
        * Returns {@code true} if any of the bindings resolved for {@code key} are multibindings with
@@ -891,13 +884,13 @@ private final class LocalDependencyChecker {
        * @throws IllegalArgumentException if {@link #getPreviouslyResolvedBindings(Key)} is empty
        */
       private boolean dependsOnLocalBindings(Key key) {
-        if (dependsOnLocalBindingsInternal(key) == DependsOnLocalBindingsResult.UNKNOWN) {
-          // If the result is UNKNOWN then it means that we've iterated through all dependencies
-          // in the cycle and none of them had a local binding (otherwise the result would be TRUE)
-          // so just return FALSE now.
-          keyDependsOnLocalBindingsCache.put(key, DependsOnLocalBindingsResult.FALSE);
+        // Don't recur infinitely if there are valid cycles in the dependency graph.
+        // http://b/23032377
+        if (!cycleChecker.add(key)) {
+          return false;
         }
-        return keyDependsOnLocalBindingsCache.get(key) == DependsOnLocalBindingsResult.TRUE;
+        return reentrantComputeIfAbsent(
+            keyDependsOnLocalBindingsCache, key, this::dependsOnLocalBindingsUncached);
       }
 
       /**
@@ -910,52 +903,14 @@ private boolean dependsOnLocalBindings(Key key) {
        * multibindings with contributions from subcomponents.
        */
       private boolean dependsOnLocalBindings(Binding binding) {
-        if (dependsOnLocalBindingsInternal(binding) == DependsOnLocalBindingsResult.UNKNOWN) {
-          // If the result is UNKNOWN then it means that we've iterated through all dependencies
-          // in the cycle and none of them had a local binding (otherwise the result would be TRUE)
-          // so just return FALSE now.
-          bindingDependsOnLocalBindingsCache.put(binding, DependsOnLocalBindingsResult.FALSE);
-        }
-        return bindingDependsOnLocalBindingsCache.get(binding) == DependsOnLocalBindingsResult.TRUE;
-      }
-
-      private DependsOnLocalBindingsResult dependsOnLocalBindingsInternal(Key key) {
-        if (localCycleStack.contains(key)) {
-          // Don't recur infinitely if there are valid cycles in the dependency graph (b/23032377).
-          return DependsOnLocalBindingsResult.UNKNOWN;
-        }
-        try {
-          localCycleStack.push(key);
-          DependsOnLocalBindingsResult result = dependsOnLocalBindingsUncached(key);
-          // If the result is UNKNOWN, don't cache so that it can be tried again later.
-          if (result != DependsOnLocalBindingsResult.UNKNOWN) {
-            keyDependsOnLocalBindingsCache.put(key, result);
-          }
-          return result;
-        } finally {
-          localCycleStack.pop();
-        }
-      }
-
-      private DependsOnLocalBindingsResult dependsOnLocalBindingsInternal(Binding binding) {
-        if (localCycleStack.contains(binding)) {
-          // Don't recur infinitely if there are valid cycles in the dependency graph (b/23032377).
-          return DependsOnLocalBindingsResult.UNKNOWN;
-        }
-        try {
-          localCycleStack.push(binding);
-          DependsOnLocalBindingsResult result = dependsOnLocalBindingsUncached(binding);
-          // If the result is UNKNOWN, don't cache so that it can be tried again later.
-          if (result != DependsOnLocalBindingsResult.UNKNOWN) {
-            bindingDependsOnLocalBindingsCache.put(binding, result);
-          }
-          return result;
-        } finally {
-          localCycleStack.pop();
+        if (!cycleChecker.add(binding)) {
+          return false;
         }
+        return reentrantComputeIfAbsent(
+            bindingDependsOnLocalBindingsCache, binding, this::dependsOnLocalBindingsUncached);
       }
 
-      private DependsOnLocalBindingsResult dependsOnLocalBindingsUncached(Key key) {
+      private boolean dependsOnLocalBindingsUncached(Key key) {
         checkArgument(
             getPreviouslyResolvedBindings(key).isPresent(),
             "no previously resolved bindings in %s for %s",
@@ -964,45 +919,28 @@ private DependsOnLocalBindingsResult dependsOnLocalBindingsUncached(Key key) {
         ResolvedBindings previouslyResolvedBindings = getPreviouslyResolvedBindings(key).get();
         if (hasLocalMultibindingContributions(key)
             || hasLocalOptionalBindingContribution(previouslyResolvedBindings)) {
-          return DependsOnLocalBindingsResult.TRUE;
+          return true;
         }
-        boolean hasUnknown = false;
+
         for (Binding binding : previouslyResolvedBindings.bindings()) {
-          switch (dependsOnLocalBindingsInternal(binding)) {
-            case TRUE:
-              return DependsOnLocalBindingsResult.TRUE;
-            case UNKNOWN:
-              hasUnknown = true;
-              break;
-            case FALSE:
-              continue;
+          if (dependsOnLocalBindings(binding)) {
+            return true;
           }
         }
-        return hasUnknown
-            ? DependsOnLocalBindingsResult.UNKNOWN
-            : DependsOnLocalBindingsResult.FALSE;
+        return false;
       }
 
-      private DependsOnLocalBindingsResult dependsOnLocalBindingsUncached(Binding binding) {
-        boolean hasUnknown = false;
+      private boolean dependsOnLocalBindingsUncached(Binding binding) {
         if ((!binding.scope().isPresent() || binding.scope().get().isReusable())
             // TODO(beder): Figure out what happens with production subcomponents.
             && !binding.bindingType().equals(BindingType.PRODUCTION)) {
           for (DependencyRequest dependency : binding.dependencies()) {
-            switch (dependsOnLocalBindingsInternal(dependency.key())) {
-              case TRUE:
-                return DependsOnLocalBindingsResult.TRUE;
-              case UNKNOWN:
-                hasUnknown = true;
-                break;
-              case FALSE:
-                continue;
+            if (dependsOnLocalBindings(dependency.key())) {
+              return true;
             }
           }
         }
-        return hasUnknown
-            ? DependsOnLocalBindingsResult.UNKNOWN
-            : DependsOnLocalBindingsResult.FALSE;
+        return false;
       }
 
       /**

javatests/dagger/functional/multibindings/BUILD

@@ -30,9 +30,7 @@ GenJavaTests(
     gen_library_deps = [
         "//javatests/dagger/functional/multibindings/subpackage",
     ],
-    javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES + [
-        "-Adagger.useDependsOnLocalBindingsFix=ENABLED",
-    ],
+    javacopts = DOCLINT_HTML_AND_SYNTAX + DOCLINT_REFERENCES,
     deps = [
         "//:dagger_with_compiler",
         "//third_party/java/auto:value",