Add better error message for @Module-annotated Kotlin object with…

… invalid instance methods inherited from superclass. See details in b/264618194. RELNOTES=N/A PiperOrigin-RevId: 506327206
google · Feb 1, 2023 · 757d709 · 757d709
1 parent ebf8e9e
commit 757d709
Show file tree

Hide file tree

Showing 3 changed files with 140 additions and 1 deletion.
diff --git a/java/dagger/internal/codegen/validation/ModuleValidator.java b/java/dagger/internal/codegen/validation/ModuleValidator.java
@@ -176,6 +176,7 @@ private ValidationReport validateUncached(XTypeElement module, Set<XTypeElement>
       }
     }
 
+    validateKotlinObjectDoesNotInheritInstanceBindingMethods(module, moduleKind, builder);
     validateDaggerAndroidProcessorRequirements(module, builder);
 
     if (bindingMethods.stream()
@@ -239,6 +240,29 @@ private void validateDaggerAndroidProcessorRequirements(
                         .build()));
   }
 
+  private void validateKotlinObjectDoesNotInheritInstanceBindingMethods(
+      XTypeElement module, ModuleKind moduleKind, ValidationReport.Builder builder) {
+    if (!module.isKotlinObject()) {
+      return;
+    }
+    XTypeElement currentClass = module;
+    while (!currentClass.getSuperType().getTypeName().equals(TypeName.OBJECT)) {
+      currentClass = currentClass.getSuperType().getTypeElement();
+      currentClass.getDeclaredMethods().stream()
+          .filter(anyBindingMethodValidator::isBindingMethod)
+          .filter(method -> ModuleMethodKind.ofMethod(method) == INSTANCE_BINDING)
+          .forEach(
+              method ->
+                  // TODO(b/264618194): Consider allowing this use case.
+                  builder.addError(
+                      String.format(
+                          "@%s-annotated Kotlin object cannot inherit instance (i.e. non-abstract, "
+                              + "non-JVM static) binding method: %s",
+                          moduleKind.annotation().simpleName(),
+                          methodSignatureFormatter.format(method))));
+    }
+  }
+
   private void validateReferencedSubcomponents(
       XTypeElement subject, ModuleKind moduleKind, ValidationReport.Builder builder) {
     XAnnotation moduleAnnotation = moduleKind.getModuleAnnotation(subject);

diff --git a/java/dagger/testing/compile/CompilerTests.java b/java/dagger/testing/compile/CompilerTests.java
@@ -70,7 +70,8 @@ public static XProcessingEnv.Backend backend(CompilationResultSubject subject) {
     String output = subject.getCompilationResult().toString();
     if (output.startsWith("CompilationResult (with ksp)")) {
       return XProcessingEnv.Backend.KSP;
-    } else if (output.startsWith("CompilationResult (with javac)")) {
+    } else if (output.startsWith("CompilationResult (with javac)")
+                   || output.startsWith("CompilationResult (with kapt)")) {
       return XProcessingEnv.Backend.JAVAC;
     }
     throw new AssertionError("Unexpected backend for subject.");

diff --git a/javatests/dagger/internal/codegen/ModuleValidationTest.java b/javatests/dagger/internal/codegen/ModuleValidationTest.java
@@ -18,6 +18,7 @@
 
 import static dagger.internal.codegen.DaggerModuleMethodSubject.Factory.assertThatModuleMethod;
 
+import androidx.room.compiler.processing.XProcessingEnv;
 import androidx.room.compiler.processing.util.Source;
 import dagger.Module;
 import dagger.producers.ProducerModule;
@@ -463,4 +464,117 @@ public void moduleIncludesSelfCycle() {
               }
             });
   }
+
+  // Regression test for b/264618194.
+  @Test
+  public void objectModuleInheritsInstanceBindingFails() {
+    Source objectModule =
+        CompilerTests.kotlinSource(
+            "test.ObjectModule.kt",
+            "package test",
+            "",
+            "import dagger.Module",
+            "import dagger.Provides",
+            "",
+            "@Module",
+            "object ObjectModule : ClassModule() {",
+            "  @Provides fun provideString(): String = \"\"",
+            "}");
+    Source classModule =
+        CompilerTests.kotlinSource(
+            "test.ClassModule.kt",
+            "package test",
+            "",
+            "import dagger.Module",
+            "import dagger.Provides",
+            "",
+            "@Module",
+            "abstract class ClassModule {",
+            "  @Provides fun provideInt(): Int = 1",
+            "}");
+    Source component =
+        CompilerTests.kotlinSource(
+            "test.TestComponent.kt",
+            "package test",
+            "",
+            "import dagger.Component",
+            "",
+            "@Component(modules = [ObjectModule::class])",
+            "interface TestComponent {",
+            "  fun getInt(): Int",
+            "  fun getString(): String",
+            "}");
+
+    CompilerTests.daggerCompiler(component, objectModule, classModule)
+        .compile(
+            subject -> {
+              subject.hasErrorCount(2);
+              subject.hasErrorContaining("test.ObjectModule has errors")
+                  .onSource(component)
+                  .onLineContaining("ObjectModule::class");
+              subject.hasErrorContaining(
+                      "@Module-annotated Kotlin object cannot inherit instance "
+                          + "(i.e. non-abstract, non-JVM static) binding method: "
+                          + "@Provides int test.ClassModule.provideInt()")
+                  .onSource(objectModule)
+                  .onLineContaining(
+                      // TODO(b/267223703): KAPT incorrectly reports the error on the annotation.
+                      CompilerTests.backend(subject) == XProcessingEnv.Backend.JAVAC
+                          ? "@Module"
+                          : "object ObjectModule");
+            });
+  }
+
+  // Regression test for b/264618194.
+  @Test
+  public void objectModuleInheritsNonInstanceBindingSucceeds() {
+    Source objectModule =
+        CompilerTests.kotlinSource(
+            "test.ObjectModule.kt",
+            "package test",
+            "",
+            "import dagger.Module",
+            "import dagger.Provides",
+            "",
+            "@Module",
+            "object ObjectModule : ClassModule() {",
+            "  @Provides fun provideString(): String = \"\"",
+            "}");
+    Source classModule =
+        CompilerTests.javaSource(
+            "test.ClassModule",
+            "package test;",
+            "",
+            "import dagger.Binds;",
+            "import dagger.Module;",
+            "import dagger.Provides;",
+            "",
+            "@Module",
+            "public abstract class ClassModule {",
+            "  // A non-binding instance method is okay.",
+            "  public int nonBindingMethod() {",
+            "    return 1;",
+            "  }",
+            "",
+            "  // A static binding method is also okay.",
+            "  @Provides",
+            "  public static int provideInt() {",
+            "    return 1;",
+            "  }",
+            "}");
+    Source component =
+        CompilerTests.kotlinSource(
+            "test.TestComponent.kt",
+            "package test",
+            "",
+            "import dagger.Component",
+            "",
+            "@Component(modules = [ObjectModule::class])",
+            "interface TestComponent {",
+            "  fun getInt(): Int",
+            "  fun getString(): String",
+            "}");
+    CompilerTests.daggerCompiler(component, objectModule, classModule)
+        .compile(subject -> subject.hasErrorCount(0));
+  }
 }