Skip to content

Commit

Permalink
Fix an issue where a module referencing a component without a builder…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@Chang-Eric
Chang-Eric authored and Dagger Team committed Feb 5, 2021
1 parent 5dfd484 commit 132d9ea
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 48 deletions.
38 changes: 34 additions & 4 deletions java/dagger/hilt/processor/internal/ComponentTree.java
View file
Expand Up @@ -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<ComponentDescriptor> graph;
private final ComponentDescriptor root;

public ComponentTree(ImmutableGraph<ComponentDescriptor> graph) {
/** Creates a new tree from a set of descriptors. */
public static ComponentTree from(Set<ComponentDescriptor> descriptors) {
MutableGraph<ComponentDescriptor> 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<ComponentDescriptor> graph) {
this.graph = Preconditions.checkNotNull(graph);
Preconditions.checkState(
!Graphs.hasCycle(graph),
Expand All @@ -56,14 +75,17 @@ public ComponentTree(ImmutableGraph<ComponentDescriptor> graph) {
descriptors.put(descriptor.component(), descriptor);
}

ImmutableList<ClassName> roots =
ImmutableList<ComponentDescriptor> 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<ComponentDescriptor> getComponentDescriptors() {
Expand All @@ -73,4 +95,12 @@ public ImmutableSet<ComponentDescriptor> getComponentDescriptors() {
public ImmutableSet<ComponentDescriptor> childrenOf(ComponentDescriptor componentDescriptor) {
return ImmutableSet.copyOf(graph.successors(componentDescriptor));
}

public ImmutableGraph<ComponentDescriptor> graph() {
return graph;
}

public ComponentDescriptor root() {
return root;
}
}
46 changes: 3 additions & 43 deletions java/dagger/hilt/processor/internal/definecomponent/DefineComponents.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -127,42 +119,10 @@ public ComponentTree getComponentTree(Elements elements) {
Map<DefineComponentMetadata, DefineComponentBuilderMetadata> builderMap = new LinkedHashMap<>();
builderMultimap.entries().forEach(e -> builderMap.put(e.getKey(), e.getValue()));

// Build the component tree graph.
Set<ComponentDescriptor> 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<ComponentDescriptor> filteredGraph = buildGraph(filteredDescriptors);
Set<ComponentDescriptor> 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<ComponentDescriptor> buildGraph(Set<ComponentDescriptor> descriptors) {
MutableGraph<ComponentDescriptor> 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(
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/processor/internal/root/BUILD
View file
Expand Up @@ -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",
Expand Down
29 changes: 28 additions & 1 deletion java/dagger/hilt/processor/internal/root/RootGenerator.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -102,6 +109,26 @@ private void generateComponents() throws IOException {
env.getFiler());
}

private static ComponentTree filterDescriptors(ComponentTree componentTree) {
MutableGraph<ComponentDescriptor> 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<ComponentDescriptor, ClassName> subcomponentBuilderModules(
TypeSpec.Builder componentsWrapper) throws IOException {
ImmutableMap.Builder<ComponentDescriptor, ClassName> modules = ImmutableMap.builder();
Expand Down
6 changes: 6 additions & 0 deletions java/dagger/hilt/processor/internal/root/RootMetadata.java
View file
Expand Up @@ -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;
Expand Down

0 comments on commit 132d9ea

Please sign in to comment.