Skip to content

Commit

Permalink
Do full binding graph validation for components and subcomponents.
Browse files Browse the repository at this point in the history 
RELNOTES=Do full binding graph validation for components and subcomponents, not just modules.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=244185059
  • Loading branch information
@netdpb @ronshapiro
netdpb authored and ronshapiro committed Apr 18, 2019
1 parent 5d6fa23 commit 15d20f8
Show file tree
Hide file tree
Showing 8 changed files with 171 additions and 83 deletions.
8 changes: 2 additions & 6 deletions java/dagger/internal/codegen/BindingGraphConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,11 @@

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

private final BindingDeclarationFormatter bindingDeclarationFormatter;
private final CompilerOptions compilerOptions;

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

/**
Expand Down Expand Up @@ -89,7 +85,7 @@ private final class Traverser extends ComponentTreeTraverser {
private ComponentNode currentComponent;

Traverser(BindingGraph graph) {
super(graph, compilerOptions);
super(graph);
}

@Override
Expand Down
31 changes: 24 additions & 7 deletions java/dagger/internal/codegen/ComponentProcessingStep.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static dagger.internal.codegen.ComponentCreatorAnnotation.allCreatorAnnotations;
import static dagger.internal.codegen.ComponentCreatorAnnotation.rootComponentCreatorAnnotations;
import static dagger.internal.codegen.ComponentCreatorAnnotation.subcomponentCreatorAnnotations;
import static dagger.internal.codegen.ValidationType.NONE;
import static java.util.Collections.disjoint;

import com.google.auto.common.BasicAnnotationProcessor.ProcessingStep;
Expand Down Expand Up @@ -134,14 +135,18 @@ private void processRootComponent(TypeElement component) {
if (!isValid(componentDescriptor)) {
return;
}
if (!isFullBindingGraphValid(componentDescriptor)) {
return;
}
BindingGraph bindingGraph = bindingGraphFactory.create(componentDescriptor, false);
if (isValid(bindingGraph)) {
generateComponent(bindingGraph);
}
}

private void processSubcomponent(TypeElement subcomponent) {
if (!compilerOptions.aheadOfTimeSubcomponents()) {
if (!compilerOptions.aheadOfTimeSubcomponents()
&& compilerOptions.moduleBindingValidationType(subcomponent).equals(NONE)) {
return;
}
if (!isSubcomponentValid(subcomponent)) {
Expand All @@ -150,9 +155,14 @@ private void processSubcomponent(TypeElement subcomponent) {
ComponentDescriptor subcomponentDescriptor =
componentDescriptorFactory.subcomponentDescriptor(subcomponent);
// TODO(dpb): ComponentDescriptorValidator for subcomponents, as we do for root components.
BindingGraph bindingGraph = bindingGraphFactory.create(subcomponentDescriptor, false);
if (isValid(bindingGraph)) {
generateComponent(bindingGraph);
if (!isFullBindingGraphValid(subcomponentDescriptor)) {
return;
}
if (compilerOptions.aheadOfTimeSubcomponents()) {
BindingGraph bindingGraph = bindingGraphFactory.create(subcomponentDescriptor, false);
if (isValid(bindingGraph)) {
generateComponent(bindingGraph);
}
}
}

Expand Down Expand Up @@ -220,10 +230,17 @@ private boolean isSubcomponentValid(Element subcomponentElement) {
return false;
}
ValidationReport<?> subcomponentReport = reportsBySubcomponent.get(subcomponentElement);
if (subcomponentReport != null && !subcomponentReport.isClean()) {
return false;
return subcomponentReport == null || subcomponentReport.isClean();
}

private boolean isFullBindingGraphValid(ComponentDescriptor componentDescriptor) {
if (compilerOptions
.moduleBindingValidationType(componentDescriptor.typeElement())
.equals(NONE)) {
return true;
}
return true;
BindingGraph fullBindingGraph = bindingGraphFactory.create(componentDescriptor, true);
return isValid(fullBindingGraph);
}

private boolean isValid(ComponentDescriptor componentDescriptor) {
Expand Down
8 changes: 1 addition & 7 deletions java/dagger/internal/codegen/ComponentTreeTraverser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package dagger.internal.codegen;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Verify.verify;
import static dagger.internal.codegen.DaggerStreams.toImmutableList;
Expand Down Expand Up @@ -47,12 +46,7 @@ public class ComponentTreeTraverser {
private final Deque<ComponentPath> componentPaths = new ArrayDeque<>();

/** Constructs a traverser for a root (component, not subcomponent) binding graph. */
public ComponentTreeTraverser(BindingGraph rootGraph, CompilerOptions compilerOptions) {
checkArgument(
!rootGraph.componentDescriptor().isSubcomponent()
|| compilerOptions.aheadOfTimeSubcomponents(),
"only root graphs can be traversed, not %s",
rootGraph.componentTypeElement().getQualifiedName());
public ComponentTreeTraverser(BindingGraph rootGraph) {
bindingGraphPath.add(rootGraph);
componentPaths.add(ComponentPath.create(ImmutableList.of(rootGraph.componentTypeElement())));
}
Expand Down
18 changes: 9 additions & 9 deletions java/dagger/internal/codegen/DiagnosticReporterFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ void printMessage(
Diagnostic.Kind diagnosticKind,
CharSequence message,
@NullableDecl Element elementToReport) {
if (graph.isModuleBindingGraph()) {
if (graph.isFullBindingGraph()) {
ValidationType validationType = compilerOptions.moduleBindingValidationType(rootComponent);
if (validationType.equals(NONE)) {
return;
Expand Down Expand Up @@ -297,12 +297,12 @@ private final class DiagnosticInfo {
@Override
public String toString() {
StringBuilder message =
graph.isModuleBindingGraph()
graph.isFullBindingGraph()
? new StringBuilder()
: new StringBuilder(dependencyTrace.size() * 100 /* a guess heuristic */);

// Print the dependency trace unless it's a module binding graph
if (!graph.isModuleBindingGraph()) {
// Print the dependency trace unless it's a full binding graph
if (!graph.isFullBindingGraph()) {
dependencyTrace.forEach(
edge ->
dependencyRequestFormatter.appendFormatLine(message, edge.dependencyRequest()));
Expand All @@ -317,22 +317,22 @@ public String toString() {
// if printing entry points, skip entry points and the traced request
.filter(
request ->
graph.isModuleBindingGraph()
graph.isFullBindingGraph()
|| (!request.isEntryPoint() && !isTracedRequest(request)))
.map(request -> request.dependencyRequest().requestElement())
.flatMap(presentValues())
.collect(toImmutableSet());
if (!requestsToPrint.isEmpty()) {
message
.append("\nIt is")
.append(graph.isModuleBindingGraph() ? " " : " also ")
.append(graph.isFullBindingGraph() ? " " : " also ")
.append("requested at:");
elementFormatter().formatIndentedList(message, requestsToPrint, 1);
}

// Print the remaining entry points, showing which component they're in, unless we're in a
// module binding graph
if (!graph.isModuleBindingGraph() && entryPoints.size() > 1) {
// Print the remaining entry points, showing which component they're in, unless it's a full
// binding graph
if (!graph.isFullBindingGraph() && entryPoints.size() > 1) {
message.append("\nThe following other entry points also depend on it:");
entryPointFormatter.formatIndentedList(
message,
Expand Down
8 changes: 5 additions & 3 deletions java/dagger/internal/codegen/IncompatiblyScopedBindingsValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,11 @@ public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter diagnosticR
ComponentNode componentNode =
bindingGraph.componentNode(binding.componentPath()).get();
if (!componentNode.scopes().contains(scope)) {
// @Inject bindings in full binding graphs will appear at the properly scoped
// ancestor component, so ignore them here.
if (binding.kind().equals(INJECTION) && bindingGraph.isFullBindingGraph()) {
// @Inject bindings in module or subcomponent binding graphs will appear at the
// properly scoped ancestor component, so ignore them here.
if (binding.kind().equals(INJECTION)
&& (bindingGraph.rootComponentNode().isSubcomponent()
|| !bindingGraph.rootComponentNode().isRealComponent())) {
return;
}
incompatibleBindings.put(componentNode, binding);
Expand Down
2 changes: 1 addition & 1 deletion java/dagger/internal/codegen/MissingBindingValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public String pluginName() {

@Override
public void visitGraph(BindingGraph graph, DiagnosticReporter diagnosticReporter) {
// Don't report missing bindings when validating a full graph or a graph built from a
// Don't report missing bindings when validating a full binding graph or a graph built from a
// subcomponent.
if (graph.isFullBindingGraph() || graph.rootComponentNode().isSubcomponent()) {
return;
Expand Down
122 changes: 74 additions & 48 deletions javatests/dagger/internal/codegen/DependencyCycleValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,56 +18,59 @@

import static com.google.testing.compile.CompilationSubject.assertThat;
import static dagger.internal.codegen.Compilers.daggerCompiler;
import static dagger.internal.codegen.TestUtils.endsWithMessage;
import static dagger.internal.codegen.TestUtils.message;

import com.google.testing.compile.Compilation;
import com.google.testing.compile.JavaFileObjects;
import java.util.regex.Pattern;
import javax.tools.JavaFileObject;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class DependencyCycleValidationTest {
@Test public void cyclicDependency() {
JavaFileObject component =
JavaFileObjects.forSourceLines(
"test.Outer",
"package test;",
"",
"import dagger.Binds;",
"import dagger.Component;",
"import dagger.Module;",
"import dagger.Provides;",
"import javax.inject.Inject;",
"",
"final class Outer {",
" static class A {",
" @Inject A(C cParam) {}",
" }",
"",
" static class B {",
" @Inject B(A aParam) {}",
" }",
"",
" static class C {",
" @Inject C(B bParam) {}",
" }",
"",
" @Module",
" interface MModule {",
" @Binds Object object(C c);",
" }",
"",
" @Component",
" interface CComponent {",
" C getC();",
" }",
"}");
private static final JavaFileObject SIMPLE_CYCLIC_DEPENDENCY =
JavaFileObjects.forSourceLines(
"test.Outer",
"package test;",
"",
"import dagger.Binds;",
"import dagger.Component;",
"import dagger.Module;",
"import dagger.Provides;",
"import javax.inject.Inject;",
"",
"final class Outer {",
" static class A {",
" @Inject A(C cParam) {}",
" }",
"",
" static class B {",
" @Inject B(A aParam) {}",
" }",
"",
" static class C {",
" @Inject C(B bParam) {}",
" }",
"",
" @Module",
" interface MModule {",
" @Binds Object object(C c);",
" }",
"",
" @Component",
" interface CComponent {",
" C getC();",
" }",
"}");

Compilation compilation =
daggerCompiler().withOptions("-Adagger.moduleBindingValidation=ERROR").compile(component);
@Test
public void cyclicDependency() {
Compilation compilation = daggerCompiler().compile(SIMPLE_CYCLIC_DEPENDENCY);
assertThat(compilation).failed();

assertThat(compilation)
.hadErrorContaining(
message(
Expand All @@ -80,21 +83,44 @@ public class DependencyCycleValidationTest {
" test.Outer.C(bParam)",
" test.Outer.C is provided at",
" test.Outer.CComponent.getC()"))
.inFile(component)
.inFile(SIMPLE_CYCLIC_DEPENDENCY)
.onLineContaining("interface CComponent");

assertThat(compilation).hadErrorCount(1);
}

@Test
public void cyclicDependencyWithModuleBindingValidation() {
// Cycle errors should not show a dependency trace to an entry point when doing full binding
// graph validation. So ensure that the message doesn't end with "test.Outer.C is provided at
// test.Outer.CComponent.getC()", as the previous test's message does.
Pattern moduleBindingValidationError =
endsWithMessage(
"Found a dependency cycle:",
" test.Outer.C is injected at",
" test.Outer.A(cParam)",
" test.Outer.A is injected at",
" test.Outer.B(aParam)",
" test.Outer.B is injected at",
" test.Outer.C(bParam)");

Compilation compilation =
daggerCompiler()
.withOptions("-Adagger.moduleBindingValidation=ERROR")
.compile(SIMPLE_CYCLIC_DEPENDENCY);
assertThat(compilation).failed();

assertThat(compilation)
.hadErrorContaining(
message(
"Found a dependency cycle:",
" test.Outer.C is injected at",
" test.Outer.A(cParam)",
" test.Outer.A is injected at",
" test.Outer.B(aParam)",
" test.Outer.B is injected at",
" test.Outer.C(bParam)"))
.inFile(component)
.hadErrorContainingMatch(moduleBindingValidationError)
.inFile(SIMPLE_CYCLIC_DEPENDENCY)
.onLineContaining("interface MModule");

assertThat(compilation)
.hadErrorContainingMatch(moduleBindingValidationError)
.inFile(SIMPLE_CYCLIC_DEPENDENCY)
.onLineContaining("interface CComponent");

assertThat(compilation).hadErrorCount(2);
}

@Test public void cyclicDependencyNotIncludingEntryPoint() {
Expand Down
Loading

0 comments on commit 15d20f8

Please sign in to comment.