From 132d9eab8ba15439ca7257014f5009f88010e8bc Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Fri, 5 Feb 2021 10:40:21 -0800 Subject: [PATCH] Fix an issue where a module referencing a component without a builder would fail. This is done by moving where we filter builder-less components further to when we generate components as oppposed to immediately after finding all of the @DefineComponent classes. RELNOTES=Fix an issue where a module referencing a component without a builder would fail PiperOrigin-RevId: 355880849 --- .../processor/internal/ComponentTree.java | 38 +++++++++++++-- .../definecomponent/DefineComponents.java | 46 ++----------------- .../dagger/hilt/processor/internal/root/BUILD | 1 + .../internal/root/RootGenerator.java | 29 +++++++++++- .../processor/internal/root/RootMetadata.java | 6 +++ 5 files changed, 72 insertions(+), 48 deletions(-) diff --git a/java/dagger/hilt/processor/internal/ComponentTree.java b/java/dagger/hilt/processor/internal/ComponentTree.java index f0ba5a055a5..6d2137a0205 100644 --- a/java/dagger/hilt/processor/internal/ComponentTree.java +++ b/java/dagger/hilt/processor/internal/ComponentTree.java @@ -21,17 +21,36 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.common.graph.GraphBuilder; import com.google.common.graph.Graphs; import com.google.common.graph.ImmutableGraph; +import com.google.common.graph.MutableGraph; import com.squareup.javapoet.ClassName; import java.util.HashMap; import java.util.Map; +import java.util.Set; /** A representation of the full tree of scopes. */ public final class ComponentTree { private final ImmutableGraph graph; + private final ComponentDescriptor root; - public ComponentTree(ImmutableGraph graph) { + /** Creates a new tree from a set of descriptors. */ + public static ComponentTree from(Set descriptors) { + MutableGraph graph = + GraphBuilder.directed().allowsSelfLoops(false).build(); + + descriptors.forEach( + descriptor -> { + graph.addNode(descriptor); + descriptor.parent().ifPresent(parent -> graph.putEdge(parent, descriptor)); + }); + + return new ComponentTree(ImmutableGraph.copyOf(graph)); + } + + private ComponentTree(ImmutableGraph graph) { this.graph = Preconditions.checkNotNull(graph); Preconditions.checkState( !Graphs.hasCycle(graph), @@ -56,14 +75,17 @@ public ComponentTree(ImmutableGraph graph) { descriptors.put(descriptor.component(), descriptor); } - ImmutableList roots = + ImmutableList roots = graph.nodes().stream() .filter(node -> graph.inDegree(node) == 0) - .map(node -> node.component()) .collect(toImmutableList()); Preconditions.checkState( - roots.size() == 1, "Component graph must have exactly 1 root. Found: %s", roots); + roots.size() == 1, + "Component graph must have exactly 1 root. Found: %s", + roots.stream().map(ComponentDescriptor::component).collect(toImmutableList())); + + root = Iterables.getOnlyElement(roots); } public ImmutableSet getComponentDescriptors() { @@ -73,4 +95,12 @@ public ImmutableSet getComponentDescriptors() { public ImmutableSet childrenOf(ComponentDescriptor componentDescriptor) { return ImmutableSet.copyOf(graph.successors(componentDescriptor)); } + + public ImmutableGraph graph() { + return graph; + } + + public ComponentDescriptor root() { + return root; + } } diff --git a/java/dagger/hilt/processor/internal/definecomponent/DefineComponents.java b/java/dagger/hilt/processor/internal/definecomponent/DefineComponents.java index ab22c0cabfb..0b330111ad1 100644 --- a/java/dagger/hilt/processor/internal/definecomponent/DefineComponents.java +++ b/java/dagger/hilt/processor/internal/definecomponent/DefineComponents.java @@ -22,16 +22,9 @@ import com.google.auto.common.MoreElements; import com.google.auto.value.AutoValue; -import com.google.common.base.Preconditions; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; -import com.google.common.graph.Graph; -import com.google.common.graph.GraphBuilder; -import com.google.common.graph.Graphs; -import com.google.common.graph.ImmutableGraph; -import com.google.common.graph.MutableGraph; import com.squareup.javapoet.ClassName; import dagger.hilt.processor.internal.AnnotationValues; import dagger.hilt.processor.internal.ClassNames; @@ -45,7 +38,6 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.Optional; -import java.util.Set; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.AnnotationValue; import javax.lang.model.element.Element; @@ -127,42 +119,10 @@ public ComponentTree getComponentTree(Elements elements) { Map builderMap = new LinkedHashMap<>(); builderMultimap.entries().forEach(e -> builderMap.put(e.getKey(), e.getValue())); - // Build the component tree graph. - Set filteredDescriptors = aggregatedMetadata.components().stream() - .map(componentMetadata -> toComponentDescriptor(componentMetadata, builderMap)) - // Only keep components that have builders (besides the root component) since if - // we didn't find any builder class, then we don't need to generate the component - // since it would be inaccessible. - .filter(descriptor -> descriptor.isRoot() || descriptor.creator().isPresent()) - .collect(toImmutableSet()); - - // The graph may still have nodes that are children of components that don't have builders, - // so we need to find reachable nodes from the root and create a new graph to remove those. - Graph filteredGraph = buildGraph(filteredDescriptors); - Set roots = filteredDescriptors.stream() - .filter(ComponentDescriptor::isRoot) - .collect(toImmutableSet()); - - Preconditions.checkState( - roots.size() == 1, - "Expected exactly 1 root component to be found but found instead %s", - roots); - - return new ComponentTree(ImmutableGraph.copyOf(buildGraph( - Graphs.reachableNodes(filteredGraph, Iterables.getOnlyElement(roots))))); - } - - private static Graph buildGraph(Set descriptors) { - MutableGraph graph = - GraphBuilder.directed().allowsSelfLoops(false).build(); - descriptors.forEach( - descriptor -> { - graph.addNode(descriptor); - descriptor.parent().ifPresent(parent -> graph.putEdge(parent, descriptor)); - }); - - return graph; + return ComponentTree.from(aggregatedMetadata.components().stream() + .map(componentMetadata -> toComponentDescriptor(componentMetadata, builderMap)) + .collect(toImmutableSet())); } private static ComponentDescriptor toComponentDescriptor( diff --git a/java/dagger/hilt/processor/internal/root/BUILD b/java/dagger/hilt/processor/internal/root/BUILD index 923a79e22f6..709eb7f5ffc 100644 --- a/java/dagger/hilt/processor/internal/root/BUILD +++ b/java/dagger/hilt/processor/internal/root/BUILD @@ -51,6 +51,7 @@ java_library( "//java/dagger/internal/codegen/extension", "//java/dagger/internal/guava:base", "//java/dagger/internal/guava:collect", + "//java/dagger/internal/guava:graph", "@google_bazel_common//third_party/java/auto:service", "@google_bazel_common//third_party/java/incap", "@google_bazel_common//third_party/java/javapoet", diff --git a/java/dagger/hilt/processor/internal/root/RootGenerator.java b/java/dagger/hilt/processor/internal/root/RootGenerator.java index bca9b66ae1a..c2c55e6c4c9 100644 --- a/java/dagger/hilt/processor/internal/root/RootGenerator.java +++ b/java/dagger/hilt/processor/internal/root/RootGenerator.java @@ -25,6 +25,9 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.graph.GraphBuilder; +import com.google.common.graph.Graphs; +import com.google.common.graph.MutableGraph; import com.squareup.javapoet.AnnotationSpec; import com.squareup.javapoet.ClassName; import com.squareup.javapoet.JavaFile; @@ -45,7 +48,11 @@ final class RootGenerator { static void generate(RootMetadata metadata, ProcessingEnvironment env) throws IOException { - new RootGenerator(metadata, env).generateComponents(); + new RootGenerator( + RootMetadata.copyWithNewTree( + metadata, + filterDescriptors(metadata.componentTree())), + env).generateComponents(); } private final RootMetadata metadata; @@ -102,6 +109,26 @@ private void generateComponents() throws IOException { env.getFiler()); } + private static ComponentTree filterDescriptors(ComponentTree componentTree) { + MutableGraph graph = + GraphBuilder.from(componentTree.graph()).build(); + + componentTree.graph().nodes().forEach(graph::addNode); + componentTree.graph().edges().forEach(graph::putEdge); + + // Remove components that do not have builders (besides the root component) since if + // we didn't find any builder class, then we don't need to generate the component + // since it would be inaccessible. + componentTree.getComponentDescriptors().stream() + .filter(descriptor -> !descriptor.isRoot() && !descriptor.creator().isPresent()) + .forEach(graph::removeNode); + + // The graph may still have nodes that are children of components that don't have builders, + // so we need to find reachable nodes from the root and create a new graph to remove those. + // We reuse the root from the original tree since it should not have been removed. + return ComponentTree.from(Graphs.reachableNodes(graph, componentTree.root())); + } + private ImmutableMap subcomponentBuilderModules( TypeSpec.Builder componentsWrapper) throws IOException { ImmutableMap.Builder modules = ImmutableMap.builder(); diff --git a/java/dagger/hilt/processor/internal/root/RootMetadata.java b/java/dagger/hilt/processor/internal/root/RootMetadata.java index bf77ced3a33..4c43f1d611a 100644 --- a/java/dagger/hilt/processor/internal/root/RootMetadata.java +++ b/java/dagger/hilt/processor/internal/root/RootMetadata.java @@ -58,6 +58,12 @@ static RootMetadata create( return metadata; } + static RootMetadata copyWithNewTree( + RootMetadata other, + ComponentTree componentTree) { + return create(other.root, componentTree, other.deps, other.env); + } + private final Root root; private final ProcessingEnvironment env; private final Elements elements;