Skip to content

Commit

Permalink
Allow component dependencies to have multiple scoped dependencies. Wh…
Browse files Browse the repository at this point in the history 
…ile enforcing this can help prevent cases where people mistakenly do this and leak objects beyond their intended lifetime, that is already something that could be done with a single scoped dependency and there are valid use cases for wanting to create a component that is an intersection of two lifetimes.

Fixes #1414

RELNOTES=Allow multiple scoped component dependencies

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=300138087
  • Loading branch information
@BraisGabin @netdpb
BraisGabin authored and netdpb committed Mar 11, 2020
1 parent 2300b53 commit 715bdd7
Show file tree
Hide file tree
Showing 2 changed files with 213 additions and 32 deletions.
32 changes: 11 additions & 21 deletions java/dagger/internal/codegen/validation/ComponentDescriptorValidator.java
View file
Expand Up @@ -20,7 +20,6 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Predicates.in;
import static com.google.common.collect.Collections2.transform;
import static com.google.common.collect.Iterables.getOnlyElement;
import static dagger.internal.codegen.base.ComponentAnnotation.rootComponentAnnotation;
import static dagger.internal.codegen.base.DiagnosticFormatting.stripCommonTypePrefixes;
import static dagger.internal.codegen.base.Formatter.INDENT;
Expand Down Expand Up @@ -197,8 +196,8 @@ private void validateComponentDependencyHierarchy(
}

/**
* Validates that among the dependencies are at most one scoped dependency, that there are no
* cycles within the scoping chain, and that singleton components have no scoped dependencies.
* Validates that among the dependencies there are no cycles within the scoping chain, and that
* singleton components have no scoped dependencies.
*/
private void validateDependencyScopes(ComponentDescriptor component) {
ImmutableSet<Scope> scopes = component.scopes();
Expand Down Expand Up @@ -226,17 +225,6 @@ private void validateDependencyScopes(ComponentDescriptor component) {
component,
message.toString());
}
} else if (scopedDependencies.size() > 1) {
// Scoped components may depend on at most one scoped component.
StringBuilder message = new StringBuilder();
for (Scope scope : scopes) {
message.append(getReadableSource(scope)).append(' ');
}
message
.append(component.typeElement().getQualifiedName())
.append(" depends on more than one scoped component:\n");
appendIndentedComponentsList(message, scopedDependencies);
reportComponentError(component, message.toString());
} else {
// Dagger 1.x scope compatibility requires this be suppress-able.
if (!compilerOptions.scopeCycleValidationType().equals(ValidationType.NONE)) {
Expand Down Expand Up @@ -448,15 +436,17 @@ private void validateDependencyScopeHierarchy(
componentAnnotation -> {
ImmutableSet<TypeElement> scopedDependencies =
scopedTypesIn(componentAnnotation.dependencies());
if (scopedDependencies.size() == 1) {
// empty can be ignored (base-case), and > 1 is a separately-reported error.
if (!scopedDependencies.isEmpty()) {
// empty can be ignored (base-case)
scopeStack.push(scopes);
scopedDependencyStack.push(dependency);
validateDependencyScopeHierarchy(
component,
getOnlyElement(scopedDependencies),
scopeStack,
scopedDependencyStack);
for (TypeElement scopedDependency : scopedDependencies) {
validateDependencyScopeHierarchy(
component,
scopedDependency,
scopeStack,
scopedDependencyStack);
}
scopedDependencyStack.pop();
scopeStack.pop();
}
Expand Down
213 changes: 202 additions & 11 deletions javatests/dagger/internal/codegen/ScopingValidationTest.java
View file
Expand Up @@ -384,9 +384,8 @@ public void fullBindingGraphValidationDoesNotReportInjectBindings() {
}

@Test
public void componentWithScopeMayDependOnOnlyOneScopedComponent() {
// If a scoped component will have dependencies, they must only include, at most, a single
// scoped component
public void componentWithScopeCanDependOnMultipleScopedComponents() {
// If a scoped component will have dependencies, they can include multiple scoped component
JavaFileObject type =
JavaFileObjects.forSourceLines(
"test.SimpleType",
Expand Down Expand Up @@ -461,14 +460,114 @@ public void componentWithScopeMayDependOnOnlyOneScopedComponent() {
daggerCompiler()
.compile(
type, simpleScope, simpleScoped, singletonScopedA, singletonScopedB, scopeless);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining(
message(
"@test.SimpleScope test.SimpleScopedComponent depends on more than one scoped "
+ "component:",
" @Singleton test.SingletonComponentA",
" @Singleton test.SingletonComponentB"));
assertThat(compilation).succeededWithoutWarnings();
}

// Tests the following component hierarchy:
//
// @ScopeA
// ComponentA
// [SimpleType getSimpleType()]
// / \
// / \
// @ScopeB @ScopeB
// ComponentB1 ComponentB2
// \ [SimpleType getSimpleType()]
// \ /
// \ /
// @ScopeC
// ComponentC
// [SimpleType getSimpleType()]
@Test
public void componentWithScopeCanDependOnMultipleScopedComponentsEvenDoingADiamond() {
JavaFileObject type =
JavaFileObjects.forSourceLines(
"test.SimpleType",
"package test;",
"",
"import javax.inject.Inject;",
"",
"class SimpleType {",
" @Inject SimpleType() {}",
"}");
JavaFileObject simpleScope =
JavaFileObjects.forSourceLines(
"test.SimpleScope",
"package test;",
"",
"import javax.inject.Scope;",
"",
"@Scope @interface SimpleScope {}");
JavaFileObject scopeA =
JavaFileObjects.forSourceLines(
"test.ScopeA",
"package test;",
"",
"import javax.inject.Scope;",
"",
"@Scope @interface ScopeA {}");
JavaFileObject scopeB =
JavaFileObjects.forSourceLines(
"test.ScopeB",
"package test;",
"",
"import javax.inject.Scope;",
"",
"@Scope @interface ScopeB {}");
JavaFileObject componentA =
JavaFileObjects.forSourceLines(
"test.ComponentA",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeA",
"@Component",
"interface ComponentA {",
" SimpleType type();",
"}");
JavaFileObject componentB1 =
JavaFileObjects.forSourceLines(
"test.ComponentB1",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeB",
"@Component(dependencies = ComponentA.class)",
"interface ComponentB1 {",
" SimpleType type();",
"}");
JavaFileObject componentB2 =
JavaFileObjects.forSourceLines(
"test.ComponentB2",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeB",
"@Component(dependencies = ComponentA.class)",
"interface ComponentB2 {",
"}");
JavaFileObject componentC =
JavaFileObjects.forSourceLines(
"test.ComponentC",
"package test;",
"",
"import dagger.Component;",
"",
"@SimpleScope",
"@Component(dependencies = {ComponentB1.class, ComponentB2.class})",
"interface ComponentC {",
" SimpleType type();",
"}");

Compilation compilation =
daggerCompiler()
.compile(
type, simpleScope, scopeA, scopeB,
componentA, componentB1, componentB2, componentC);
assertThat(compilation).succeededWithoutWarnings();
}

@Test
Expand Down Expand Up @@ -575,6 +674,98 @@ public void componentWithSingletonScopeMayNotDependOnOtherScope() {
" @test.SimpleScope test.SimpleScopedComponent"));
}

@Test
public void componentScopeWithMultipleScopedDependenciesMustNotCycle() {
JavaFileObject type =
JavaFileObjects.forSourceLines(
"test.SimpleType",
"package test;",
"",
"import javax.inject.Inject;",
"",
"class SimpleType {",
" @Inject SimpleType() {}",
"}");
JavaFileObject scopeA =
JavaFileObjects.forSourceLines(
"test.ScopeA",
"package test;",
"",
"import javax.inject.Scope;",
"",
"@Scope @interface ScopeA {}");
JavaFileObject scopeB =
JavaFileObjects.forSourceLines(
"test.ScopeB",
"package test;",
"",
"import javax.inject.Scope;",
"",
"@Scope @interface ScopeB {}");
JavaFileObject longLifetime =
JavaFileObjects.forSourceLines(
"test.ComponentLong",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeA",
"@Component",
"interface ComponentLong {",
" SimpleType type();",
"}");
JavaFileObject mediumLifetime1 =
JavaFileObjects.forSourceLines(
"test.ComponentMedium1",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeB",
"@Component(dependencies = ComponentLong.class)",
"interface ComponentMedium1 {",
" SimpleType type();",
"}");
JavaFileObject mediumLifetime2 =
JavaFileObjects.forSourceLines(
"test.ComponentMedium2",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeB",
"@Component",
"interface ComponentMedium2 {",
"}");
JavaFileObject shortLifetime =
JavaFileObjects.forSourceLines(
"test.ComponentShort",
"package test;",
"",
"import dagger.Component;",
"",
"@ScopeA",
"@Component(dependencies = {ComponentMedium1.class, ComponentMedium2.class})",
"interface ComponentShort {",
" SimpleType type();",
"}");

Compilation compilation =
daggerCompiler()
.compile(
type, scopeA, scopeB,
longLifetime, mediumLifetime1, mediumLifetime2, shortLifetime);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining(
message(
"test.ComponentShort depends on scoped components in a non-hierarchical scope "
+ "ordering:",
" @test.ScopeA test.ComponentLong",
" @test.ScopeB test.ComponentMedium1",
" @test.ScopeA test.ComponentShort"));
}

@Test
public void componentScopeAncestryMustNotCycle() {
// The dependency relationship of components is necessarily from shorter lifetimes to
Expand Down

0 comments on commit 715bdd7

Please sign in to comment.