Skip to content

Commit

Permalink
Fix bug when reporting cycles if some components have no entry points…
Browse files Browse the repository at this point in the history 
… that depend on the cycle.

RELNOTES=Fix bug when reporting cycles if some components have no entry points that depend on the cycle.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=205674036
  • Loading branch information
@netdpb @ronshapiro
netdpb authored and ronshapiro committed Aug 1, 2018
1 parent dd03682 commit c55bec3
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 31 deletions.
39 changes: 18 additions & 21 deletions java/dagger/internal/codegen/BindingCycleValidation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import static dagger.internal.codegen.DaggerStreams.toImmutableSet;
import static dagger.internal.codegen.RequestKinds.extractKeyType;
import static dagger.internal.codegen.RequestKinds.getRequestKind;
import static java.util.Comparator.comparingInt;
import static javax.tools.Diagnostic.Kind.ERROR;

import com.google.auto.value.AutoValue;
Expand All @@ -40,6 +39,7 @@
import com.google.common.graph.NetworkBuilder;
import dagger.model.BindingGraph;
import dagger.model.BindingGraph.BindingNode;
import dagger.model.BindingGraph.ComponentNode;
import dagger.model.BindingGraph.DependencyEdge;
import dagger.model.BindingGraph.Node;
import dagger.model.BindingKind;
Expand Down Expand Up @@ -106,6 +106,15 @@ private Optional<Cycle<Node>> cycleContainingEndpointPair(

/**
* Reports a dependency cycle at the dependency into the cycle that is closest to an entry point.
*
* <p>Looks for the shortest path from the component that contains the cycle (all bindings in a
* cycle must be in the same component; see below) to some binding in the cycle. Then looks for
* the last dependency in that path that is not in the cycle; that is the dependency that will be
* reported, so that the dependency trace will end just before the cycle.
*
* <p>Proof (by counterexample) that all bindings in a cycle must be in the same component: Assume
* one binding in the cycle is in a parent component. Bindings cannot depend on bindings in child
* components, so that binding cannot depend on the next binding in the cycle.
*/
private void reportCycle(
Cycle<Node> cycle, BindingGraph bindingGraph, DiagnosticReporter diagnosticReporter) {
Expand All @@ -121,18 +130,11 @@ private void reportCycle(
private ImmutableList<Node> shortestPathToCycleFromAnEntryPoint(
Cycle<Node> cycle, BindingGraph bindingGraph) {
Node someCycleNode = cycle.nodes().asList().get(0);
return bindingGraph
.componentNodes()
.stream()
.map(componentNode -> shortestPath(bindingGraph, componentNode, someCycleNode))
// Ignore paths that go through subcomponents by requiring all nodes after the first to be
// BindingNodes. We can't just use nonCycleBreakingDependencyGraph because that filters out
// edges that might break a cycle, but those edges might still be part of the shortest path
// TO a cycle.
.filter(path -> path.stream().skip(1).allMatch(node -> node instanceof BindingNode))
.map(path -> subpathToCycle(path, cycle))
.min(comparingInt(ImmutableList::size))
.get();
ComponentNode componentContainingCycle =
bindingGraph.componentNode(someCycleNode.componentPath()).get();
ImmutableList<Node> pathToCycle =
shortestPath(bindingGraph, componentContainingCycle, someCycleNode);
return subpathToCycle(pathToCycle, cycle);
}

/**
Expand Down Expand Up @@ -232,13 +234,8 @@ private DependencyEdge chooseDependencyEdgeConnecting(
private ImmutableNetwork<Node, DependencyEdge> nonCycleBreakingDependencyGraph(
BindingGraph bindingGraph) {
MutableNetwork<Node, DependencyEdge> dependencyNetwork =
NetworkBuilder.directed()
.allowsParallelEdges(true)
.allowsSelfLoops(true)
.nodeOrder(bindingGraph.nodeOrder())
.edgeOrder(bindingGraph.edgeOrder())
.expectedNodeCount(
bindingGraph.bindingNodes().size() + bindingGraph.componentNodes().size())
NetworkBuilder.from(bindingGraph)
.expectedNodeCount(bindingGraph.nodes().size())
.expectedEdgeCount(bindingGraph.dependencyEdges().size())
.build();
bindingGraph
Expand Down Expand Up @@ -298,7 +295,7 @@ Cycle<N> shift(N startNode) {
}

@Override
public String toString() {
public final String toString() {
return endpointPairs().toString();
}

Expand Down
112 changes: 102 additions & 10 deletions javatests/dagger/internal/codegen/GraphValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ public void cyclicDependencyInSubcomponents() {
"",
"@Component",
"interface Parent {",
" Child child();",
" Child.Builder child();",
"}");
JavaFileObject child =
JavaFileObjects.forSourceLines(
Expand All @@ -625,9 +625,14 @@ public void cyclicDependencyInSubcomponents() {
"",
"import dagger.Subcomponent;",
"",
"@Subcomponent(modules = ChildModule.class)",
"@Subcomponent(modules = CycleModule.class)",
"interface Child {",
" Grandchild grandchild();",
" Grandchild.Builder grandchild();",
"",
" @Subcomponent.Builder",
" interface Builder {",
" Child build();",
" }",
"}");
JavaFileObject grandchild =
JavaFileObjects.forSourceLines(
Expand All @@ -639,17 +644,22 @@ public void cyclicDependencyInSubcomponents() {
"@Subcomponent",
"interface Grandchild {",
" String entry();",
"",
" @Subcomponent.Builder",
" interface Builder {",
" Grandchild build();",
" }",
"}");
JavaFileObject childModule =
JavaFileObject cycleModule =
JavaFileObjects.forSourceLines(
"test.ChildModule",
"test.CycleModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"",
"@Module",
"abstract class ChildModule {",
"abstract class CycleModule {",
" @Provides static Object object(String string) {",
" return string;",
" }",
Expand All @@ -659,20 +669,102 @@ public void cyclicDependencyInSubcomponents() {
" }",
"}");

Compilation compilation = daggerCompiler().compile(parent, child, grandchild, childModule);
Compilation compilation = daggerCompiler().compile(parent, child, grandchild, cycleModule);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining(
message(
"[test.Grandchild.entry()] Found a dependency cycle:",
"java.lang.String is injected at",
" test.ChildModule.object(string)",
" test.CycleModule.object(string)",
"java.lang.Object is injected at",
" test.ChildModule.string(object)",
" test.CycleModule.string(object)",
"java.lang.String is provided at",
" test.Grandchild.entry()"))
.inFile(parent)
.onLineContaining("interface Parent {");
.onLineContaining("interface Parent");
}

@Test
public void cyclicDependencyInSubcomponentsWithChildren() {
JavaFileObject parent =
JavaFileObjects.forSourceLines(
"test.Parent",
"package test;",
"",
"import dagger.Component;",
"",
"@Component",
"interface Parent {",
" Child.Builder child();",
"}");
JavaFileObject child =
JavaFileObjects.forSourceLines(
"test.Child",
"package test;",
"",
"import dagger.Subcomponent;",
"",
"@Subcomponent(modules = CycleModule.class)",
"interface Child {",
" String entry();",
"",
" Grandchild grandchild();",
"",
" @Subcomponent.Builder",
" interface Builder {",
" Child build();",
" }",
"}");
// Grandchild has no entry point that depends on the cycle. http://b/111317986
JavaFileObject grandchild =
JavaFileObjects.forSourceLines(
"test.Grandchild",
"package test;",
"",
"import dagger.Subcomponent;",
"",
"@Subcomponent",
"interface Grandchild {",
"",
" @Subcomponent.Builder",
" interface Builder {",
" Grandchild build();",
" }",
"}");
JavaFileObject cycleModule =
JavaFileObjects.forSourceLines(
"test.CycleModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"",
"@Module",
"abstract class CycleModule {",
" @Provides static Object object(String string) {",
" return string;",
" }",
"",
" @Provides static String string(Object object) {",
" return object.toString();",
" }",
"}");

Compilation compilation = daggerCompiler().compile(parent, child, grandchild, cycleModule);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining(
message(
"[test.Child.entry()] Found a dependency cycle:",
"java.lang.String is injected at",
" test.CycleModule.object(string)",
"java.lang.Object is injected at",
" test.CycleModule.string(object)",
"java.lang.String is provided at",
" test.Child.entry()"))
.inFile(parent)
.onLineContaining("interface Parent");
}

@Test
Expand Down

0 comments on commit c55bec3

Please sign in to comment.