diff --git a/java/dagger/internal/codegen/binding/BindingGraphFactory.java b/java/dagger/internal/codegen/binding/BindingGraphFactory.java index 1c7a53e3daf..da7b1a3f8e1 100644 --- a/java/dagger/internal/codegen/binding/BindingGraphFactory.java +++ b/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 resolvedContributionBindings = new LinkedHashMap<>(); final Map resolvedMembersInjectionBindings = new LinkedHashMap<>(); final Deque cycleStack = new ArrayDeque<>(); - final Map keyDependsOnLocalBindingsCache = new HashMap<>(); - final Map bindingDependsOnLocalBindingsCache = - new HashMap<>(); + final Map keyDependsOnLocalBindingsCache = new HashMap<>(); + final Map bindingDependsOnLocalBindingsCache = new HashMap<>(); final Queue subcomponentsToResolve = new ArrayDeque<>(); Resolver( @@ -877,7 +870,7 @@ ImmutableMap getResolvedMembersInjectionBindings() { } private final class LocalDependencyChecker { - private final Deque localCycleStack = new ArrayDeque<>(); + private final Set 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; } /** diff --git a/javatests/dagger/functional/multibindings/BUILD b/javatests/dagger/functional/multibindings/BUILD index 51ebb594fb8..63a7bfd2728 100644 --- a/javatests/dagger/functional/multibindings/BUILD +++ b/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", diff --git a/javatests/dagger/functional/multibindings/MultibindingResolutionTest.java b/javatests/dagger/functional/multibindings/MultibindingResolutionTest.java deleted file mode 100644 index 5ee546e43db..00000000000 --- a/javatests/dagger/functional/multibindings/MultibindingResolutionTest.java +++ /dev/null @@ -1,138 +0,0 @@ -/* - * Copyright (C) 2023 The Dagger Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package dagger.functional.multibindings; - -import static com.google.common.truth.Truth.assertThat; - -import dagger.Component; -import dagger.Module; -import dagger.Provides; -import dagger.Subcomponent; -import dagger.multibindings.IntoSet; -import java.util.Set; -import javax.inject.Inject; -import javax.inject.Provider; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -// This is a regression test for b/290683637. -@RunWith(JUnit4.class) -public final class MultibindingResolutionTest { - @Component(modules = ParentModule.class) - interface ParentComponent { - A getA(); - - Child1Component getChild1Component(); - - Child2Component getChild2Component(); - } - - @Module - interface ParentModule { - @Provides - @IntoSet - static X provideX() { - return new ParentX(); - } - } - - @Subcomponent(modules = ChildModule.class) - interface Child1Component { - B getB(); - A getA(); - } - - // Technically, the original bug only occurred when the entry points were declared in the order - // B then A, as in Child1Component. However, since that depends on the order we resolved entry - // points (an implementation detail that can change) we create another component with the opposite - // order. - @Subcomponent(modules = ChildModule.class) - interface Child2Component { - A getA(); - B getB(); - } - - @Module - interface ChildModule { - @Provides - @IntoSet - static X provideX() { - return new ChildX(); - } - } - - interface X {} - - static final class ParentX implements X {} - - static final class ChildX implements X {} - - static final class A { - final B b; - final C c; - - @Inject - A(B b, C c) { - this.b = b; - this.c = c; - } - } - - static final class B { - final Provider aProvider; - - @Inject - B(Provider aProvider) { - this.aProvider = aProvider; - } - } - - static final class C { - final Set multibindingContributions; - - @Inject - C(Set multibindingContributions) { - this.multibindingContributions = multibindingContributions; - } - } - - private ParentComponent parentComponent; - - @Before - public void setup() { - parentComponent = DaggerMultibindingResolutionTest_ParentComponent.create(); - } - - @Test - public void testParent() { - assertThat(parentComponent.getA().c.multibindingContributions).hasSize(1); - } - - @Test - public void testChild1() { - Child1Component child1Component = parentComponent.getChild1Component(); - assertThat(child1Component.getB().aProvider.get().c.multibindingContributions).hasSize(2); - } - - @Test - public void testChild2() { - Child2Component child2Component = parentComponent.getChild2Component(); - assertThat(child2Component.getB().aProvider.get().c.multibindingContributions).hasSize(2); - } -}