Don't allow abstract modules with instance @Provides/@produces methods

Fixes #621 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=165177293
google · Aug 14, 2017 · e2dc678 · e2dc678
1 parent a97deb4
commit e2dc678
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 3 deletions.
diff --git a/WORKSPACE b/WORKSPACE
@@ -115,8 +115,8 @@ maven_jar(
 
 maven_jar(
     name = "com_google_testing_compile_compile_testing",
-    artifact = "com.google.testing.compile:compile-testing:0.10",
-    sha1 = "51e6189be9d2861d1eb22b4009c8f3430319490c",
+    artifact = "com.google.testing.compile:compile-testing:0.11",
+    sha1 = "bff5d5aa61e6384b9dd4f5f7bb97a921081f4e1c",
 )
 
 maven_jar(

diff --git a/java/dagger/internal/codegen/BindingGraphValidator.java b/java/dagger/internal/codegen/BindingGraphValidator.java
@@ -52,6 +52,7 @@
 import static dagger.internal.codegen.ErrorMessages.REQUIRES_AT_INJECT_CONSTRUCTOR_OR_PROVIDER_OR_PRODUCER_FORMAT;
 import static dagger.internal.codegen.ErrorMessages.REQUIRES_PROVIDER_FORMAT;
 import static dagger.internal.codegen.ErrorMessages.REQUIRES_PROVIDER_OR_PRODUCER_FORMAT;
+import static dagger.internal.codegen.ErrorMessages.abstractModuleHasInstanceBindingMethods;
 import static dagger.internal.codegen.ErrorMessages.duplicateMapKeysError;
 import static dagger.internal.codegen.ErrorMessages.inconsistentMapKeyAnnotationsError;
 import static dagger.internal.codegen.ErrorMessages.nullableToNonNullable;
@@ -112,6 +113,7 @@
 import javax.inject.Provider;
 import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.type.ArrayType;
 import javax.lang.model.type.DeclaredType;
@@ -198,6 +200,7 @@ private ValidationReport.Builder<TypeElement> report(BindingGraph graph) {
     protected void visitComponent(BindingGraph graph) {
       validateDependencyScopes(graph);
       validateComponentDependencyHierarchy(graph);
+      validateModules(graph);
       validateBuilders(graph);
       super.visitComponent(graph);
       checkScopedBindings(graph);
@@ -344,6 +347,19 @@ private void validateDependencyScopes(BindingGraph graph) {
       }
     }
 
+    private void validateModules(BindingGraph graph) {
+      for (ModuleDescriptor module : graph.componentDescriptor().transitiveModules()) {
+        if (module.moduleElement().getModifiers().contains(Modifier.ABSTRACT)) {
+          for (ContributionBinding binding : module.bindings()) {
+            if (binding.requiresModuleInstance()) {
+              report(graph).addError(abstractModuleHasInstanceBindingMethods(module));
+              break;
+            }
+          }
+        }
+      }
+    }
+
     private void validateBuilders(BindingGraph graph) {
       ComponentDescriptor componentDesc = graph.componentDescriptor();
       if (!componentDesc.builderSpec().isPresent()) {

diff --git a/java/dagger/internal/codegen/ErrorMessages.java b/java/dagger/internal/codegen/ErrorMessages.java
@@ -416,6 +416,24 @@ static String tooManyBindingMethodAnnotations(
         methodAnnotations.stream().map(Class::getCanonicalName).collect(joining(", ")));
   }
 
+  static String abstractModuleHasInstanceBindingMethods(ModuleDescriptor module) {
+    String methodAnnotations;
+    switch (module.kind()) {
+      case MODULE:
+        methodAnnotations = "@Provides";
+        break;
+      case PRODUCER_MODULE:
+        methodAnnotations = "@Provides or @Produces";
+        break;
+      default:
+        throw new AssertionError(module.kind());
+    }
+    return String.format(
+        "%s is abstract and has instance %s methods. Consider making the methods static or "
+            + "including a non-abstract subclass of the module instead.",
+        module.moduleElement(), methodAnnotations);
+  }
+
   static class ComponentBuilderMessages {
     static final ComponentBuilderMessages INSTANCE = new ComponentBuilderMessages();
 

diff --git a/java/dagger/internal/codegen/ModuleDescriptor.java b/java/dagger/internal/codegen/ModuleDescriptor.java
@@ -78,6 +78,8 @@ abstract class ModuleDescriptor {
   /** The {@link BindsOptionalOf} method declarations that define optional bindings. */
   abstract ImmutableSet<OptionalBindingDeclaration> optionalDeclarations();
 
+  abstract Kind kind();
+
   enum Kind {
     MODULE(Module.class, Provides.class),
     PRODUCER_MODULE(ProducerModule.class, Produces.class);
@@ -196,7 +198,8 @@ ModuleDescriptor create(TypeElement moduleElement) {
           multibindingDeclarations.build(),
           subcomponentDeclarationFactory.forModule(moduleElement),
           delegates.build(),
-          optionalDeclarations.build());
+          optionalDeclarations.build(),
+          Kind.forAnnotatedElement(moduleElement).get());
     }
 
     @CanIgnoreReturnValue

diff --git a/javatests/dagger/internal/codegen/GraphValidationTest.java b/javatests/dagger/internal/codegen/GraphValidationTest.java
@@ -17,13 +17,16 @@
 package dagger.internal.codegen;
 
 import static com.google.common.truth.Truth.assertAbout;
+import static com.google.testing.compile.CompilationSubject.assertThat;
 import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource;
 import static com.google.testing.compile.JavaSourcesSubject.assertThat;
 import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources;
+import static dagger.internal.codegen.Compilers.daggerCompiler;
 import static dagger.internal.codegen.ErrorMessages.nullableToNonNullable;
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
+import com.google.testing.compile.Compilation;
 import com.google.testing.compile.JavaFileObjects;
 import java.util.Arrays;
 import javax.tools.JavaFileObject;
@@ -2527,6 +2530,77 @@ public void releasableReferenceManagerConflict() {
         .onLine(19);
   }
 
+  @Test
+  public void abstractModuleWithInstanceMethod() {
+    JavaFileObject module =
+        JavaFileObjects.forSourceLines(
+            "test.TestModule",
+            "package test;",
+            "",
+            "import dagger.Module;",
+            "import dagger.Provides;",
+            "",
+            "@Module",
+            "abstract class TestModule {",
+            "  @Provides int i() { return 1; }",
+            "}");
+    JavaFileObject component =
+        JavaFileObjects.forSourceLines(
+            "test.TestComponent",
+            "package test;",
+            "",
+            "import dagger.Component;",
+            "",
+            "@Component(modules = TestModule.class)",
+            "interface TestComponent {",
+            "  int i();",
+            "}");
+    Compilation compilation = daggerCompiler().compile(module, component);
+    assertThat(compilation).failed();
+    assertThat(compilation)
+        .hadErrorContaining("TestModule is abstract and has instance @Provides methods")
+        .inFile(component)
+        .onLineContaining("interface TestComponent");
+  }
+
+  @Test
+  public void abstractModuleWithInstanceMethod_subclassedIsAllowed() {
+    JavaFileObject abstractModule =
+        JavaFileObjects.forSourceLines(
+            "test.AbstractModule",
+            "package test;",
+            "",
+            "import dagger.Module;",
+            "import dagger.Provides;",
+            "",
+            "@Module",
+            "abstract class AbstractModule {",
+            "  @Provides int i() { return 1; }",
+            "}");
+    JavaFileObject subclassedModule =
+        JavaFileObjects.forSourceLines(
+            "test.SubclassedModule",
+            "package test;",
+            "",
+            "import dagger.Module;",
+            "",
+            "@Module",
+            "class SubclassedModule extends AbstractModule {}");
+    JavaFileObject component =
+        JavaFileObjects.forSourceLines(
+            "test.TestComponent",
+            "package test;",
+            "",
+            "import dagger.Component;",
+            "",
+            "@Component(modules = SubclassedModule.class)",
+            "interface TestComponent {",
+            "  int i();",
+            "}");
+    Compilation compilation = daggerCompiler().compile(abstractModule, subclassedModule, component);
+    assertThat(compilation).succeeded();
+  }
+
   private String error(String... lines) {
     return Joiner.on("\n      ").join(lines);
   }