Skip to content

Commit

Permalink
Check if user added methods have same method signature as generated f…
Browse files Browse the repository at this point in the history 
…actory methods.

Fixes #2575

RELNOTES=Check if user added methods have same method signature as generated factory methods.
PiperOrigin-RevId: 382587936
  • Loading branch information
@java-team-github-bot
java-team-github-bot authored and Dagger Team committed Jul 1, 2021
1 parent 30fa0f3 commit 6971d00
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 27 deletions.
26 changes: 24 additions & 2 deletions java/dagger/internal/codegen/writing/ComponentImplementation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@
import static javax.lang.model.element.Modifier.PRIVATE;
import static javax.lang.model.element.Modifier.PUBLIC;
import static javax.lang.model.element.Modifier.STATIC;
import static javax.tools.Diagnostic.Kind.ERROR;

import com.google.auto.common.MoreElements;
import com.google.common.base.Function;
import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -90,6 +92,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import javax.annotation.processing.Messager;
import javax.inject.Inject;
import javax.inject.Provider;
import javax.lang.model.element.ExecutableElement;
Expand Down Expand Up @@ -252,6 +255,7 @@ private static ImmutableList<ImmutableList<Binding>> bindingPartitions(
private final DaggerTypes types;
private final KotlinMetadataUtil metadataUtil;
private final ImmutableMap<ComponentImplementation, FieldSpec> componentFieldsByImplementation;
private final Messager messager;

@Inject
ComponentImplementation(
Expand All @@ -265,7 +269,8 @@ private static ImmutableList<ImmutableList<Binding>> bindingPartitions(
CompilerOptions compilerOptions,
DaggerElements elements,
DaggerTypes types,
KotlinMetadataUtil metadataUtil) {
KotlinMetadataUtil metadataUtil,
Messager messager) {
this.parent = parent;
this.childComponentImplementationFactory = childComponentImplementationFactory;
this.bindingExpressionsProvider = bindingExpressionsProvider;
Expand All @@ -291,6 +296,7 @@ private static ImmutableList<ImmutableList<Binding>> bindingPartitions(
// Create and claim the fields for this and all ancestor components stored as fields.
this.componentFieldsByImplementation =
createComponentFieldsByImplementation(this, compilerOptions);
this.messager = messager;
}

/**
Expand Down Expand Up @@ -699,7 +705,7 @@ private void createRootComponentFactoryMethod() {
factoryMethodName = "build";
noArgFactoryMethod = true;
}

validateMethodNameDoesNotOverrideGeneratedCreator(creatorKind.methodName());
MethodSpec creatorFactoryMethod =
methodBuilder(creatorKind.methodName())
.addModifiers(PUBLIC, STATIC)
Expand All @@ -708,6 +714,7 @@ private void createRootComponentFactoryMethod() {
.build();
addMethod(MethodSpecKind.BUILDER_METHOD, creatorFactoryMethod);
if (noArgFactoryMethod && canInstantiateAllRequirements()) {
validateMethodNameDoesNotOverrideGeneratedCreator("create");
addMethod(
MethodSpecKind.BUILDER_METHOD,
methodBuilder("create")
Expand All @@ -718,6 +725,21 @@ private void createRootComponentFactoryMethod() {
}
}

private void validateMethodNameDoesNotOverrideGeneratedCreator(String creatorName) {
// Check if there is any client added method has the same signature as generated creatorName.
MoreElements.getAllMethods(graph.componentTypeElement(), types, elements).stream()
.filter(method -> method.getSimpleName().contentEquals(creatorName))
.filter(method -> method.getParameters().isEmpty())
.filter(method -> !method.getModifiers().contains(Modifier.STATIC))
.forEach(
(ExecutableElement method) ->
messager.printMessage(
ERROR,
String.format(
"Cannot override generated method: %s.%s()",
method.getEnclosingElement().getSimpleName(), method.getSimpleName())));
}

/** {@code true} if all of the graph's required dependencies can be automatically constructed */
private boolean canInstantiateAllRequirements() {
return !Iterables.any(
Expand Down
2 changes: 1 addition & 1 deletion javatests/dagger/internal/codegen/ComponentCreatorTestHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ String process(String... lines) {
line ->
line.replace("Builder", "Factory")
.replace("builder", "factory")
.replace("build", "create"));
.replace("build", "createComponent"));
}
return stream.collect(joining("\n"));
}
Expand Down
112 changes: 112 additions & 0 deletions javatests/dagger/internal/codegen/ComponentValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,118 @@ public void componentOnConcreteClass() {
assertThat(compilation).hadErrorContaining("interface");
}

@Test
public void componentOnOverridingBuilder_failsWhenMethodNameConflictsWithStaticCreatorName() {
JavaFileObject componentFile =
JavaFileObjects.forSourceLines(
"test.TestComponent",
"package test;",
"",
"import dagger.Component;",
"",
"@Component(modules=TestModule.class)",
"interface TestComponent {",
" String builder();",
"}");
JavaFileObject moduleFile =
JavaFileObjects.forSourceLines(
"test.TestModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"",
"@Module",
"interface TestModule {",
" @Provides",
" static String provideString() { return \"test\"; }",
"}");

Compilation compilation = daggerCompiler().compile(componentFile, moduleFile);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining("Cannot override generated method: TestComponent.builder()");
}

@Test
public void componentOnOverridingCreate_failsWhenGeneratedCreateMethod() {
JavaFileObject componentFile =
JavaFileObjects.forSourceLines(
"test.TestComponent",
"package test;",
"",
"import dagger.Component;",
"",
"@Component(modules=TestModule.class)",
"interface TestComponent {",
" String create();",
"}");
JavaFileObject moduleFile =
JavaFileObjects.forSourceLines(
"test.TestModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"",
"@Module",
"interface TestModule {",
" @Provides",
" static String provideString() { return \"test\"; }",
"}");

Compilation compilation = daggerCompiler().compile(componentFile, moduleFile);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining("Cannot override generated method: TestComponent.create()");
}

@Test
public void subcomponentMethodNameBuilder_succeeds() {
JavaFileObject componentFile =
JavaFileObjects.forSourceLines(
"test.TestComponent",
"package test;",
"",
"import dagger.Component;",
"",
"@Component",
"interface TestComponent {",
" TestSubcomponent.Builder subcomponent();",
"}");
JavaFileObject subcomponentFile =
JavaFileObjects.forSourceLines(
"test.TestSubcomponent",
"package test;",
"",
"import dagger.Subcomponent;",
"",
"@Subcomponent(modules=TestModule.class)",
"interface TestSubcomponent {",
" String builder();",
" @Subcomponent.Builder",
" interface Builder {",
" TestSubcomponent build();",
" }",
"}");
JavaFileObject moduleFile =
JavaFileObjects.forSourceLines(
"test.TestModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"",
"@Module",
"interface TestModule {",
" @Provides",
" static String provideString() { return \"test\"; }",
"}");

Compilation compilation = daggerCompiler().compile(componentFile, subcomponentFile, moduleFile);
assertThat(compilation).succeeded();
}

@Test public void componentOnEnum() {
JavaFileObject componentFile = JavaFileObjects.forSourceLines("test.NotAComponent",
"package test;",
Expand Down
51 changes: 27 additions & 24 deletions javatests/dagger/internal/codegen/SubcomponentCreatorValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,19 @@ public SubcomponentCreatorValidationTest(ComponentCreatorAnnotation componentCre

@Test
public void testRefSubcomponentAndSubCreatorFails() {
JavaFileObject componentFile = preprocessedJavaFile("test.ParentComponent",
"package test;",
"",
"import dagger.Component;",
"import javax.inject.Provider;",
"",
"@Component",
"interface ParentComponent {",
" ChildComponent child();",
" ChildComponent.Builder builder();",
"}");
JavaFileObject componentFile =
preprocessedJavaFile(
"test.ParentComponent",
"package test;",
"",
"import dagger.Component;",
"import javax.inject.Provider;",
"",
"@Component",
"interface ParentComponent {",
" ChildComponent child();",
" ChildComponent.Builder childComponentBuilder();",
"}");
JavaFileObject childComponentFile = preprocessedJavaFile("test.ChildComponent",
"package test;",
"",
Expand All @@ -82,7 +84,7 @@ public void testRefSubcomponentAndSubCreatorFails() {
String.format(
moreThanOneRefToSubcomponent(),
"test.ChildComponent",
process("[child(), builder()]")))
process("[child(), childComponentBuilder()]")))
.inFile(componentFile);
}

Expand Down Expand Up @@ -244,16 +246,18 @@ public void testCreatorNotInComponentFails() {

@Test
public void testCreatorMissingFactoryMethodFails() {
JavaFileObject componentFile = preprocessedJavaFile("test.ParentComponent",
"package test;",
"",
"import dagger.Component;",
"import javax.inject.Provider;",
"",
"@Component",
"interface ParentComponent {",
" ChildComponent.Builder builder();",
"}");
JavaFileObject componentFile =
preprocessedJavaFile(
"test.ParentComponent",
"package test;",
"",
"import dagger.Component;",
"import javax.inject.Provider;",
"",
"@Component",
"interface ParentComponent {",
" ChildComponent.Builder childComponentBuilder();",
"}");
JavaFileObject childComponentFile = preprocessedJavaFile("test.ChildComponent",
"package test;",
"",
Expand Down Expand Up @@ -726,8 +730,7 @@ public void testExtraSettersFails() {
" interface Factory {",
" ChildComponent create(String s, Integer i);",
" }")
.addLines( //
"}")
.addLines("}")
.build();
Compilation compilation = compile(componentFile, childComponentFile);
assertThat(compilation).failed();
Expand Down

0 comments on commit 6971d00

Please sign in to comment.