Skip to content

Commit

Permalink
Remove a binding's associated declarations from ResolvedBindings an…
Browse files Browse the repository at this point in the history 
…d use `BindingNode` directly instead.

This CL refactors the code to remove the associated declarations from `ResolvedBindings` and have the class use `BindingNode` (which includes the associated declarations itself) instead of `Binding`. This change is subtle, but it allows us to reuse the `BindingNode` instance from an ancestor component rather than trying to recreate it with the exact same declarations in a child component (see follow-up CL/644086367).

Overall, I think this refactor is also an improvement to maintainability because it cuts out the `ResolvedBindings#associatedDeclarations()` being used as a middle-man for then creating a `BindingNode` and just holds the `BindingNode`s directly.

RELNOTES=N/A
PiperOrigin-RevId: 649129006
  • Loading branch information
@bcorso
bcorso authored and Dagger Team committed Jul 3, 2024
1 parent d456afc commit fb8bd41
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 229 deletions.
76 changes: 11 additions & 65 deletions java/dagger/internal/codegen/binding/BindingGraphConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.graph.ImmutableNetwork;
import com.google.common.graph.MutableNetwork;
import com.google.common.graph.NetworkBuilder;
Expand All @@ -44,27 +43,22 @@
import dagger.internal.codegen.model.Key;
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import javax.inject.Inject;

/** Converts {@link BindingGraph}s to {@link dagger.internal.codegen.model.BindingGraph}s. */
final class BindingGraphConverter {
private final BindingDeclarationFormatter bindingDeclarationFormatter;

@Inject
BindingGraphConverter(BindingDeclarationFormatter bindingDeclarationFormatter) {
this.bindingDeclarationFormatter = bindingDeclarationFormatter;
}
BindingGraphConverter() {}

/**
* Creates the external {@link dagger.internal.codegen.model.BindingGraph} representing the given
* internal {@link BindingGraph}.
*/
BindingGraph convert(LegacyBindingGraph legacyBindingGraph, boolean isFullBindingGraph) {
MutableNetwork<Node, Edge> network = asNetwork(legacyBindingGraph);
MutableNetwork<Node, Edge> network = Converter.convertToNetwork(legacyBindingGraph);
ComponentNode rootNode = legacyBindingGraph.componentNode();

// When bindings are copied down into child graphs because they transitively depend on local
Expand All @@ -81,23 +75,20 @@ BindingGraph convert(LegacyBindingGraph legacyBindingGraph, boolean isFullBindin
return BindingGraph.create(rootNode, topLevelBindingGraph);
}

private MutableNetwork<Node, Edge> asNetwork(LegacyBindingGraph graph) {
Converter converter = new Converter();
converter.visitRootComponent(graph);
return converter.network;
}
private static final class Converter {
static MutableNetwork<Node, Edge> convertToNetwork(LegacyBindingGraph graph) {
Converter converter = new Converter();
converter.visitRootComponent(graph);
return converter.network;
}

private final class Converter {
/** The path from the root graph to the currently visited graph. */
private final Deque<LegacyBindingGraph> bindingGraphPath = new ArrayDeque<>();

private final MutableNetwork<Node, Edge> network =
NetworkBuilder.directed().allowsParallelEdges(true).allowsSelfLoops(true).build();
private final Set<BindingNode> bindings = new HashSet<>();

private final Map<ResolvedBindings, ImmutableSet<BindingNode>> resolvedBindingsMap =
new HashMap<>();

private void visitRootComponent(LegacyBindingGraph graph) {
visitComponent(graph);
}
Expand Down Expand Up @@ -128,7 +119,7 @@ private void visitComponent(LegacyBindingGraph graph) {
}

for (ResolvedBindings resolvedBindings : graph.resolvedBindings()) {
for (BindingNode binding : bindingNodes(resolvedBindings)) {
for (BindingNode binding : resolvedBindings.bindingNodes()) {
if (bindings.add(binding)) {
network.addNode(binding);
for (DependencyRequest dependencyRequest : binding.dependencies()) {
Expand All @@ -140,8 +131,7 @@ private void visitComponent(LegacyBindingGraph graph) {
network.addEdge(
binding,
subcomponentNode(binding.key().type().xprocessing(), graph),
new SubcomponentCreatorBindingEdgeImpl(
resolvedBindings.subcomponentDeclarations()));
new SubcomponentCreatorBindingEdgeImpl(binding.subcomponentDeclarations()));
}
}
}
Expand Down Expand Up @@ -170,21 +160,6 @@ private ComponentPath componentPath() {
return bindingGraphPath.getLast().componentPath();
}

/**
* Returns the subpath from the root component to the matching {@code ancestor} of the current
* component.
*/
private ComponentPath pathFromRootToAncestor(XTypeElement ancestor) {
for (LegacyBindingGraph graph : bindingGraphPath) {
if (graph.componentDescriptor().typeElement().equals(ancestor)) {
return graph.componentPath();
}
}
throw new IllegalArgumentException(
String.format(
"%s is not in the current path: %s", ancestor.getQualifiedName(), componentPath()));
}

/**
* Returns the LegacyBindingGraph for {@code ancestor}, where {@code ancestor} is in the
* component path of the current traversal.
Expand All @@ -209,7 +184,7 @@ private void addDependencyEdges(Node source, DependencyRequest dependencyRequest
if (dependencies.isEmpty()) {
addDependencyEdge(source, dependencyRequest, missingBindingNode(dependencies));
} else {
for (BindingNode dependency : bindingNodes(dependencies)) {
for (BindingNode dependency : dependencies.bindingNodes()) {
addDependencyEdge(source, dependencyRequest, dependency);
}
}
Expand Down Expand Up @@ -249,35 +224,6 @@ private ResolvedBindings resolvedDependencies(
.resolvedBindings(bindingRequest(dependencyRequest));
}

private ImmutableSet<BindingNode> bindingNodes(ResolvedBindings resolvedBindings) {
return resolvedBindingsMap.computeIfAbsent(resolvedBindings, this::uncachedBindingNodes);
}

private ImmutableSet<BindingNode> uncachedBindingNodes(ResolvedBindings resolvedBindings) {
ImmutableSet.Builder<BindingNode> bindingNodes = ImmutableSet.builder();
resolvedBindings
.allBindings()
.asMap()
.forEach(
(component, bindings) -> {
for (Binding binding : bindings) {
bindingNodes.add(bindingNode(resolvedBindings, binding, component));
}
});
return bindingNodes.build();
}

private BindingNode bindingNode(
ResolvedBindings resolvedBindings, Binding binding, XTypeElement owningComponent) {
return BindingNode.create(
pathFromRootToAncestor(owningComponent),
binding,
resolvedBindings.multibindingDeclarations(),
resolvedBindings.optionalBindingDeclarations(),
resolvedBindings.subcomponentDeclarations(),
bindingDeclarationFormatter);
}

private MissingBinding missingBindingNode(ResolvedBindings dependencies) {
// Put all missing binding nodes in the root component. This simplifies the binding graph
// and produces better error messages for users since all dependents point to the same node.
Expand Down
52 changes: 30 additions & 22 deletions java/dagger/internal/codegen/binding/BindingGraphFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
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.extension.DaggerStreams.toImmutableSet;
import static dagger.internal.codegen.model.BindingKind.ASSISTED_INJECTION;
import static dagger.internal.codegen.model.BindingKind.DELEGATE;
import static dagger.internal.codegen.model.BindingKind.INJECTION;
Expand All @@ -35,7 +36,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Multimaps;
import dagger.Reusable;
import dagger.internal.codegen.base.ClearableCache;
import dagger.internal.codegen.base.ContributionType;
Expand Down Expand Up @@ -73,6 +73,7 @@ public final class BindingGraphFactory implements ClearableCache {
private final InjectBindingRegistry injectBindingRegistry;
private final KeyFactory keyFactory;
private final BindingFactory bindingFactory;
private final BindingNode.Factory bindingNodeFactory;
private final ComponentDeclarations.Factory componentDeclarationsFactory;
private final BindingGraphConverter bindingGraphConverter;
private final Map<Key, ImmutableSet<Key>> keysMatchingRequestCache = new HashMap<>();
Expand All @@ -83,12 +84,14 @@ public final class BindingGraphFactory implements ClearableCache {
InjectBindingRegistry injectBindingRegistry,
KeyFactory keyFactory,
BindingFactory bindingFactory,
BindingNode.Factory bindingNodeFactory,
ComponentDeclarations.Factory componentDeclarationsFactory,
BindingGraphConverter bindingGraphConverter,
CompilerOptions compilerOptions) {
this.injectBindingRegistry = injectBindingRegistry;
this.keyFactory = keyFactory;
this.bindingFactory = bindingFactory;
this.bindingNodeFactory = bindingNodeFactory;
this.componentDeclarationsFactory = componentDeclarationsFactory;
this.bindingGraphConverter = bindingGraphConverter;
this.compilerOptions = compilerOptions;
Expand Down Expand Up @@ -341,13 +344,18 @@ && isAssistedFactoryType(requestKey.type().xprocessing().getTypeElement())) {
.ifPresent(bindings::add);
}

return ResolvedBindings.forContributionBindings(
componentPath,
return ResolvedBindings.create(
requestKey,
Multimaps.index(bindings, binding -> getOwningComponent(requestKey, binding)),
multibindingDeclarations,
subcomponentDeclarations,
optionalBindingDeclarations);
bindings.stream()
.map(
binding ->
bindingNodeFactory.forContributionBindings(
getOwningComponentPath(requestKey, binding),
binding,
multibindingDeclarations,
optionalBindingDeclarations,
subcomponentDeclarations))
.collect(toImmutableSet()));
}

/**
Expand Down Expand Up @@ -380,9 +388,10 @@ ResolvedBindings lookUpMembersInjectionBinding(Key requestKey) {
Optional<MembersInjectionBinding> binding =
injectBindingRegistry.getOrFindMembersInjectionBinding(requestKey);
return binding.isPresent()
? ResolvedBindings.forMembersInjectionBinding(
componentPath, requestKey, componentDescriptor, binding.get())
: ResolvedBindings.noBindings(componentPath, requestKey);
? ResolvedBindings.create(
requestKey,
bindingNodeFactory.forMembersInjectionBinding(componentPath, binding.get()))
: ResolvedBindings.create(requestKey);
}

/**
Expand Down Expand Up @@ -462,7 +471,7 @@ private ContributionBinding createDelegateBinding(DelegateDeclaration delegateDe
} finally {
cycleStack.pop();
}
if (resolvedDelegate.contributionBindings().isEmpty()) {
if (resolvedDelegate.bindings().isEmpty()) {
// This is guaranteed to result in a missing binding error, so it doesn't matter if the
// binding is a Provision or Production, except if it is a @IntoMap method, in which
// case the key will be of type Map<K, Provider<V>>, which will be "upgraded" into a
Expand All @@ -477,7 +486,7 @@ private ContributionBinding createDelegateBinding(DelegateDeclaration delegateDe
// It doesn't matter which of these is selected, since they will later on produce a
// duplicate binding error.
ContributionBinding explicitDelegate =
resolvedDelegate.contributionBindings().iterator().next();
(ContributionBinding) resolvedDelegate.bindings().iterator().next();
return bindingFactory.delegateBinding(delegateDeclaration, explicitDelegate);
}

Expand All @@ -491,13 +500,13 @@ private ContributionBinding createDelegateBinding(DelegateDeclaration delegateDe
* multibinding contributions in the parent, and returns the parent-resolved {@link
* ResolvedBindings#owningComponent(ContributionBinding)}.
*/
private XTypeElement getOwningComponent(Key requestKey, ContributionBinding binding) {
private ComponentPath getOwningComponentPath(Key requestKey, ContributionBinding binding) {
if (isResolvedInParent(requestKey, binding) && !requiresResolution(binding)) {
ResolvedBindings parentResolvedBindings =
parentResolver.get().resolvedContributionBindings.get(requestKey);
return parentResolvedBindings.owningComponent(binding);
return parentResolvedBindings.forBinding(binding).componentPath();
} else {
return componentDescriptor.typeElement();
return componentPath;
}
}

Expand Down Expand Up @@ -540,8 +549,7 @@ private Optional<Resolver> getOwningResolver(ContributionBinding binding) {
// If a @Reusable binding was resolved in an ancestor, use that component.
ResolvedBindings resolvedBindings =
requestResolver.resolvedContributionBindings.get(binding.key());
if (resolvedBindings != null
&& resolvedBindings.contributionBindings().contains(binding)) {
if (resolvedBindings != null && resolvedBindings.bindings().contains(binding)) {
return Optional.of(requestResolver);
}
}
Expand Down Expand Up @@ -740,7 +748,7 @@ && getLocalExplicitBindings(key).isEmpty()) {
* component.
*/
private void resolveDependencies(ResolvedBindings resolvedBindings) {
for (Binding binding : resolvedBindings.bindingsOwnedBy(componentDescriptor)) {
for (BindingNode binding : resolvedBindings.bindingNodesOwnedBy(componentPath)) {
for (DependencyRequest dependency : binding.dependencies()) {
resolve(dependency.key());
}
Expand Down Expand Up @@ -870,13 +878,13 @@ private boolean hasLocalMultibindingContributions(Key requestKey) {
*/
private boolean hasLocalOptionalBindingContribution(ResolvedBindings resolvedBindings) {
return hasLocalOptionalBindingContribution(
resolvedBindings.key(), resolvedBindings.contributionBindings());
resolvedBindings.key(), resolvedBindings.bindings());
}

private boolean hasLocalOptionalBindingContribution(
Key key, ImmutableSet<ContributionBinding> previousContributionBindings) {
if (previousContributionBindings.stream()
.map(ContributionBinding::kind)
Key key, ImmutableSet<? extends Binding> previouslyResolvedBindings) {
if (previouslyResolvedBindings.stream()
.map(Binding::kind)
.anyMatch(isEqual(OPTIONAL))) {
return !getLocalExplicitBindings(keyFactory.unwrapOptional(key).get())
.isEmpty();
Expand Down
Loading

0 comments on commit fb8bd41

Please sign in to comment.