From 378535e5a49d07f3caa43d380eb655bc249c5c59 Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Mon, 2 Oct 2023 12:07:26 -0700 Subject: [PATCH] Refactor ResolvedBindings and LegacyBindingGraph to include the ComponentPath. Including the ComponentPath in ResolvedBindings avoids having to create new objects of type ResolvedBindingsWithPath to satisfy our Map key. In addition, passing the ComponentPath while creating LegacyBindingGraph avoids having to recreate it from scratch during BindingGraphConverter phase. RELNOTES=N/A PiperOrigin-RevId: 570137117 --- .../binding/BindingGraphConverter.java | 93 +++++-------------- .../codegen/binding/BindingGraphFactory.java | 17 +++- .../codegen/binding/LegacyBindingGraph.java | 13 +++ .../codegen/binding/ResolvedBindings.java | 11 ++- 4 files changed, 62 insertions(+), 72 deletions(-) diff --git a/java/dagger/internal/codegen/binding/BindingGraphConverter.java b/java/dagger/internal/codegen/binding/BindingGraphConverter.java index 5928b8fb09c..8e76e09e248 100644 --- a/java/dagger/internal/codegen/binding/BindingGraphConverter.java +++ b/java/dagger/internal/codegen/binding/BindingGraphConverter.java @@ -19,7 +19,6 @@ import static com.google.common.base.Verify.verify; import static dagger.internal.codegen.binding.BindingRequest.bindingRequest; import static dagger.internal.codegen.extension.DaggerGraphs.unreachableNodes; -import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList; import static dagger.internal.codegen.model.BindingKind.SUBCOMPONENT_CREATOR; import androidx.room.compiler.processing.XMethodElement; @@ -29,11 +28,9 @@ import com.google.auto.value.extension.memoized.Memoized; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; import com.google.common.graph.ImmutableNetwork; import com.google.common.graph.MutableNetwork; -import com.google.common.graph.Network; import com.google.common.graph.NetworkBuilder; import dagger.internal.codegen.binding.BindingGraph.TopLevelBindingGraph; import dagger.internal.codegen.binding.ComponentDescriptor.ComponentMethodDescriptor; @@ -70,7 +67,7 @@ final class BindingGraphConverter { */ BindingGraph convert(LegacyBindingGraph legacyBindingGraph, boolean isFullBindingGraph) { MutableNetwork network = asNetwork(legacyBindingGraph); - ComponentNode rootNode = rootComponentNode(network); + ComponentNode rootNode = legacyBindingGraph.componentNode(); // When bindings are copied down into child graphs because they transitively depend on local // multibindings or optional bindings, the parent-owned binding is still there. If that @@ -92,47 +89,19 @@ private MutableNetwork asNetwork(LegacyBindingGraph graph) { return converter.network; } - // TODO(dpb): Example of BindingGraph logic applied to derived networks. - private ComponentNode rootComponentNode(Network network) { - return (ComponentNode) - Iterables.find( - network.nodes(), - node -> node instanceof ComponentNode && node.componentPath().atRoot()); - } - - /** - * Used as a cache key to make sure resolved bindings are cached per component path. - * This is required so that binding nodes are not reused across different branches of the - * graph since the ResolvedBindings class only contains the component and not the path. - */ - @AutoValue - abstract static class ResolvedBindingsWithPath { - abstract ResolvedBindings resolvedBindings(); - abstract ComponentPath componentPath(); - - static ResolvedBindingsWithPath create( - ResolvedBindings resolvedBindings, ComponentPath componentPath) { - return new AutoValue_BindingGraphConverter_ResolvedBindingsWithPath( - resolvedBindings, componentPath); - } - } - private final class Converter { /** The path from the root graph to the currently visited graph. */ private final Deque bindingGraphPath = new ArrayDeque<>(); - /** The {@link ComponentPath} for each component in {@link #bindingGraphPath}. */ - private final Deque componentPaths = new ArrayDeque<>(); - private final MutableNetwork network = NetworkBuilder.directed().allowsParallelEdges(true).allowsSelfLoops(true).build(); private final Set bindings = new HashSet<>(); - private final Map> resolvedBindingsMap = + private final Map> resolvedBindingsMap = new HashMap<>(); private void visitRootComponent(LegacyBindingGraph graph) { - visitComponent(graph, null); + visitComponent(graph); } /** @@ -145,30 +114,20 @@ private void visitRootComponent(LegacyBindingGraph graph) { * {@link #visitSubcomponentFactoryMethod(ComponentNode, ComponentNode, XMethodElement)}. *
  • For each entry point in the component, calls {@link #visitEntryPoint(ComponentNode, * DependencyRequest)}. - *
  • For each child component, calls {@link #visitComponent(LegacyBindingGraph, - * ComponentNode)}, updating the traversal state. + *
  • For each child component, calls {@link #visitComponent(LegacyBindingGraph)}, + * updating the traversal state. * * * @param graph the currently visited graph */ - private void visitComponent(LegacyBindingGraph graph, ComponentNode parentComponent) { + private void visitComponent(LegacyBindingGraph graph) { bindingGraphPath.addLast(graph); - ComponentPath graphPath = - ComponentPath.create( - bindingGraphPath.stream() - .map(LegacyBindingGraph::componentDescriptor) - .map(ComponentDescriptor::typeElement) - .map(DaggerTypeElement::from) - .collect(toImmutableList())); - componentPaths.addLast(graphPath); - ComponentNode currentComponent = - ComponentNodeImpl.create(componentPath(), graph.componentDescriptor()); - - network.addNode(currentComponent); + + network.addNode(graph.componentNode()); for (ComponentMethodDescriptor entryPointMethod : graph.componentDescriptor().entryPointMethods()) { - visitEntryPoint(currentComponent, entryPointMethod.dependencyRequest().get()); + visitEntryPoint(graph.componentNode(), entryPointMethod.dependencyRequest().get()); } for (ResolvedBindings resolvedBindings : graph.resolvedBindings()) { @@ -180,7 +139,7 @@ private void visitComponent(LegacyBindingGraph graph, ComponentNode parentCompon } } if (binding.kind().equals(SUBCOMPONENT_CREATOR) - && binding.componentPath().equals(currentComponent.componentPath())) { + && binding.componentPath().equals(graph.componentPath())) { network.addEdge( binding, subcomponentNode(binding.key().type().xprocessing(), graph), @@ -191,22 +150,23 @@ private void visitComponent(LegacyBindingGraph graph, ComponentNode parentCompon } if (bindingGraphPath.size() > 1) { - LegacyBindingGraph parent = Iterators.get(bindingGraphPath.descendingIterator(), 1); - parent + LegacyBindingGraph parentGraph = Iterators.get(bindingGraphPath.descendingIterator(), 1); + parentGraph .componentDescriptor() .getFactoryMethodForChildComponent(graph.componentDescriptor()) .ifPresent( childFactoryMethod -> visitSubcomponentFactoryMethod( - parentComponent, currentComponent, childFactoryMethod.methodElement())); + parentGraph.componentNode(), + graph.componentNode(), + childFactoryMethod.methodElement())); } for (LegacyBindingGraph child : graph.subgraphs()) { - visitComponent(child, currentComponent); + visitComponent(child); } verify(bindingGraphPath.removeLast().equals(graph)); - verify(componentPaths.removeLast().equals(graphPath)); } /** @@ -242,7 +202,7 @@ private void visitSubcomponentFactoryMethod( * component. */ private ComponentPath componentPath() { - return componentPaths.getLast(); + return bindingGraphPath.getLast().componentPath(); } /** @@ -250,9 +210,9 @@ private ComponentPath componentPath() { * component. */ private ComponentPath pathFromRootToAncestor(XTypeElement ancestor) { - for (ComponentPath componentPath : componentPaths) { - if (componentPath.currentComponent().xprocessing().equals(ancestor)) { - return componentPath; + for (LegacyBindingGraph graph : bindingGraphPath) { + if (graph.componentDescriptor().typeElement().equals(ancestor)) { + return graph.componentPath(); } } throw new IllegalArgumentException( @@ -325,23 +285,18 @@ private ResolvedBindings resolvedDependencies( } private ImmutableSet bindingNodes(ResolvedBindings resolvedBindings) { - ResolvedBindingsWithPath resolvedBindingsWithPath = - ResolvedBindingsWithPath.create(resolvedBindings, componentPath()); - return resolvedBindingsMap.computeIfAbsent( - resolvedBindingsWithPath, this::uncachedBindingNodes); + return resolvedBindingsMap.computeIfAbsent(resolvedBindings, this::uncachedBindingNodes); } - private ImmutableSet uncachedBindingNodes( - ResolvedBindingsWithPath resolvedBindingsWithPath) { + private ImmutableSet uncachedBindingNodes(ResolvedBindings resolvedBindings) { ImmutableSet.Builder bindingNodes = ImmutableSet.builder(); - resolvedBindingsWithPath.resolvedBindings() + resolvedBindings .allBindings() .asMap() .forEach( (component, bindings) -> { for (Binding binding : bindings) { - bindingNodes.add( - bindingNode(resolvedBindingsWithPath.resolvedBindings(), binding, component)); + bindingNodes.add(bindingNode(resolvedBindings, binding, component)); } }); return bindingNodes.build(); diff --git a/java/dagger/internal/codegen/binding/BindingGraphFactory.java b/java/dagger/internal/codegen/binding/BindingGraphFactory.java index ba1157d50bd..37d7c499e23 100644 --- a/java/dagger/internal/codegen/binding/BindingGraphFactory.java +++ b/java/dagger/internal/codegen/binding/BindingGraphFactory.java @@ -51,6 +51,8 @@ import dagger.internal.codegen.base.OptionalType; import dagger.internal.codegen.compileroption.CompilerOptions; import dagger.internal.codegen.javapoet.TypeNames; +import dagger.internal.codegen.model.ComponentPath; +import dagger.internal.codegen.model.DaggerTypeElement; import dagger.internal.codegen.model.DependencyRequest; import dagger.internal.codegen.model.Key; import dagger.internal.codegen.model.Scope; @@ -190,8 +192,14 @@ private LegacyBindingGraph createLegacyBindingGraph( optionalsBuilder.addAll(moduleDescriptor.optionalDeclarations()); } + DaggerTypeElement component = DaggerTypeElement.from(componentDescriptor.typeElement()); + ComponentPath componentPath = + parentResolver.isPresent() + ? parentResolver.get().componentPath.childPath(component) + : ComponentPath.create(ImmutableList.of(component)); final Resolver requestResolver = new Resolver( + componentPath, parentResolver, componentDescriptor, indexBindingDeclarationsByKey(explicitBindingsBuilder.build()), @@ -237,6 +245,7 @@ private LegacyBindingGraph createLegacyBindingGraph( } return new LegacyBindingGraph( + componentPath, componentDescriptor, ImmutableMap.copyOf(requestResolver.getResolvedContributionBindings()), ImmutableMap.copyOf(requestResolver.getResolvedMembersInjectionBindings()), @@ -300,6 +309,7 @@ public void clearCache() { } private final class Resolver { + final ComponentPath componentPath; final Optional parentResolver; final ComponentDescriptor componentDescriptor; final ImmutableSetMultimap explicitBindings; @@ -318,6 +328,7 @@ private final class Resolver { final Queue subcomponentsToResolve = new ArrayDeque<>(); Resolver( + ComponentPath componentPath, Optional parentResolver, ComponentDescriptor componentDescriptor, ImmutableSetMultimap explicitBindings, @@ -325,6 +336,7 @@ private final class Resolver { ImmutableSetMultimap subcomponentDeclarations, ImmutableSetMultimap delegateDeclarations, ImmutableSetMultimap optionalBindingDeclarations) { + this.componentPath = componentPath; this.parentResolver = parentResolver; this.componentDescriptor = checkNotNull(componentDescriptor); this.explicitBindings = checkNotNull(explicitBindings); @@ -428,6 +440,7 @@ && isAssistedFactoryType(requestKey.type().xprocessing().getTypeElement())) { } return ResolvedBindings.forContributionBindings( + componentPath, requestKey, Multimaps.index(bindings, binding -> getOwningComponent(requestKey, binding)), multibindingDeclarations, @@ -466,8 +479,8 @@ ResolvedBindings lookUpMembersInjectionBinding(Key requestKey) { injectBindingRegistry.getOrFindMembersInjectionBinding(requestKey); return binding.isPresent() ? ResolvedBindings.forMembersInjectionBinding( - requestKey, componentDescriptor, binding.get()) - : ResolvedBindings.noBindings(requestKey); + componentPath, requestKey, componentDescriptor, binding.get()) + : ResolvedBindings.noBindings(componentPath, requestKey); } /** diff --git a/java/dagger/internal/codegen/binding/LegacyBindingGraph.java b/java/dagger/internal/codegen/binding/LegacyBindingGraph.java index b38697e8d7e..be8e33f8878 100644 --- a/java/dagger/internal/codegen/binding/LegacyBindingGraph.java +++ b/java/dagger/internal/codegen/binding/LegacyBindingGraph.java @@ -22,6 +22,8 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.collect.Multimaps; +import dagger.internal.codegen.model.BindingGraph.ComponentNode; +import dagger.internal.codegen.model.ComponentPath; import dagger.internal.codegen.model.Key; import dagger.internal.codegen.model.RequestKind; import java.util.Collection; @@ -30,22 +32,33 @@ // TODO(bcorso): Remove the LegacyBindingGraph after we've migrated to the new BindingGraph. /** The canonical representation of a full-resolved graph. */ final class LegacyBindingGraph { + private final ComponentNode componentNode; private final ComponentDescriptor componentDescriptor; private final ImmutableMap contributionBindings; private final ImmutableMap membersInjectionBindings; private final ImmutableList subgraphs; LegacyBindingGraph( + ComponentPath componentPath, ComponentDescriptor componentDescriptor, ImmutableMap contributionBindings, ImmutableMap membersInjectionBindings, ImmutableList subgraphs) { + this.componentNode = ComponentNodeImpl.create(componentPath, componentDescriptor); this.componentDescriptor = componentDescriptor; this.contributionBindings = contributionBindings; this.membersInjectionBindings = membersInjectionBindings; this.subgraphs = checkForDuplicates(subgraphs); } + ComponentNode componentNode() { + return componentNode; + } + + ComponentPath componentPath() { + return componentNode.componentPath(); + } + ComponentDescriptor componentDescriptor() { return componentDescriptor; } diff --git a/java/dagger/internal/codegen/binding/ResolvedBindings.java b/java/dagger/internal/codegen/binding/ResolvedBindings.java index 8a354429aed..406ae41f585 100644 --- a/java/dagger/internal/codegen/binding/ResolvedBindings.java +++ b/java/dagger/internal/codegen/binding/ResolvedBindings.java @@ -27,6 +27,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Multimap; +import dagger.internal.codegen.model.ComponentPath; import dagger.internal.codegen.model.Key; /** @@ -40,6 +41,9 @@ */ @AutoValue abstract class ResolvedBindings { + /** The component path for the resolved bindings. */ + abstract ComponentPath componentPath(); + /** The binding key for which the {@link #bindings()} have been resolved. */ abstract Key key(); @@ -127,12 +131,14 @@ final XTypeElement owningComponent(ContributionBinding binding) { /** Creates a {@link ResolvedBindings} for contribution bindings. */ static ResolvedBindings forContributionBindings( + ComponentPath componentPath, Key key, Multimap contributionBindings, Iterable multibindings, Iterable subcomponentDeclarations, Iterable optionalBindingDeclarations) { return new AutoValue_ResolvedBindings( + componentPath, key, ImmutableSetMultimap.copyOf(contributionBindings), ImmutableMap.of(), @@ -145,10 +151,12 @@ static ResolvedBindings forContributionBindings( * Creates a {@link ResolvedBindings} for members injection bindings. */ static ResolvedBindings forMembersInjectionBinding( + ComponentPath componentPath, Key key, ComponentDescriptor owningComponent, MembersInjectionBinding ownedMembersInjectionBinding) { return new AutoValue_ResolvedBindings( + componentPath, key, ImmutableSetMultimap.of(), ImmutableMap.of(owningComponent.typeElement(), ownedMembersInjectionBinding), @@ -160,8 +168,9 @@ static ResolvedBindings forMembersInjectionBinding( /** * Creates a {@link ResolvedBindings} appropriate for when there are no bindings for the key. */ - static ResolvedBindings noBindings(Key key) { + static ResolvedBindings noBindings(ComponentPath componentPath, Key key) { return new AutoValue_ResolvedBindings( + componentPath, key, ImmutableSetMultimap.of(), ImmutableMap.of(),