Repository: square/anvil (kotlin)

Issue:
Excluded modules and contributed bindings should have their "replaces" ignored.
Given this example
```kotlin
@ContributesTo(AppScope::class, replaces = [DaggerModule2::class])
@Module
abstract class DaggerModule1

@ContributesTo(AppScope::class)
@Module
abstract class DaggerModule2 

@MergeComponent(
    scope = AppScope::class,
    exclude = [DaggerModule1::class]
)
interface ComponentInterface
```

One would expect `DaggerModule2` to be included in the generated component. However, it is not. Similar issues arise if the excluded module, contributed binding, or contributed multibinding specifies that it replaces another module or binding.

I'm working on a fix so that excluded modules and bindings that specify `replaces` have no effect.

Pull request:
Fixes #301. Modules and bindings are no longer replaced if the replacement is declared by an excluded module.

Open to better ways of accomplishing this as I found the fq and psi class work a little tricky.

Patch:
diff --git a/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMerger.kt b/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMerger.kt
index acdf5c919..d5180bef8 100644
--- a/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMerger.kt
+++ b/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMerger.kt
@@ -118,7 +118,47 @@ internal class ModuleMerger(
       // for replaced classes and the final result.
       .toList()
 
+    val excludedModules = (mergeAnnotation.getAnnotationValue("exclude") as? ArrayValue)
+      ?.value
+      ?.map {
+        val argumentType = it.argumentType(module)
+        val classDescriptorForExclusion = argumentType.classDescriptorForType()
+
+        val contributesToAnnotation = classDescriptorForExclusion
+          .annotationOrNull(contributesToFqName)
+        val contributesBindingAnnotation = classDescriptorForExclusion
+          .annotationOrNull(contributesBindingFqName)
+        val contributesMultibindingAnnotation = classDescriptorForExclusion
+          .annotationOrNull(contributesMultibindingFqName)
+
+        // Verify that the the replaced classes use the same scope.
+        val scopeOfExclusion = contributesToAnnotation?.scope(module)
+          ?: contributesBindingAnnotation?.scope(module)
+          ?: contributesMultibindingAnnotation?.scope(module)
+          ?: throw AnvilCompilationException(
+            codegen.descriptor,
+            "Could not determine the scope of the excluded class " +
+              "${classDescriptorForExclusion.fqNameSafe}."
+          )
+
+        if (scopeOfExclusion.fqNameSafe != scopeFqName) {
+          throw AnvilCompilationException(
+            codegen.descriptor,
+            "${codegen.descriptor.fqNameSafe} with scope $scopeFqName wants to exclude " +
+              "${classDescriptorForExclusion.fqNameSafe} with scope " +
+              "${scopeOfExclusion.fqNameSafe}. The exclusion must use the same scope."
+          )
+        }
+
+        argumentType.asmType(codegen.typeMapper)
+      }
+      ?: emptyList()
+
     val replacedModules = modules
+      .filter { (classDescriptor, _) ->
+        // Ignore replaced modules or bindings specified by excluded modules.
+        classDescriptor.defaultType.asmType(codegen.typeMapper) !in excludedModules
+      }
       .flatMap { (classDescriptor, contributeAnnotation) ->
         contributeAnnotation.replaces(module)
           .map { classDescriptorForReplacement ->
@@ -182,42 +222,6 @@ internal class ModuleMerger(
       hintPackagePrefix = HINT_MULTIBINDING_PACKAGE_PREFIX
     )
 
-    val excludedModules = (mergeAnnotation.getAnnotationValue("exclude") as? ArrayValue)
-      ?.value
-      ?.map {
-        val argumentType = it.argumentType(module)
-        val classDescriptorForExclusion = argumentType.classDescriptorForType()
-
-        val contributesToAnnotation = classDescriptorForExclusion
-          .annotationOrNull(contributesToFqName)
-        val contributesBindingAnnotation = classDescriptorForExclusion
-          .annotationOrNull(contributesBindingFqName)
-        val contributesMultibindingAnnotation = classDescriptorForExclusion
-          .annotationOrNull(contributesMultibindingFqName)
-
-        // Verify that the the replaced classes use the same scope.
-        val scopeOfExclusion = contributesToAnnotation?.scope(module)
-          ?: contributesBindingAnnotation?.scope(module)
-          ?: contributesMultibindingAnnotation?.scope(module)
-          ?: throw AnvilCompilationException(
-            codegen.descriptor,
-            "Could not determine the scope of the excluded class " +
-              "${classDescriptorForExclusion.fqNameSafe}."
-          )
-
-        if (scopeOfExclusion.fqNameSafe != scopeFqName) {
-          throw AnvilCompilationException(
-            codegen.descriptor,
-            "${codegen.descriptor.fqNameSafe} with scope $scopeFqName wants to exclude " +
-              "${classDescriptorForExclusion.fqNameSafe} with scope " +
-              "${scopeOfExclusion.fqNameSafe}. The exclusion must use the same scope."
-          )
-        }
-
-        argumentType.asmType(codegen.typeMapper)
-      }
-      ?: emptyList()
-
     if (predefinedModules != null) {
       val intersect = predefinedModules.intersect(excludedModules)
       if (intersect.isNotEmpty()) {
diff --git a/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMergerIr.kt b/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMergerIr.kt
index 761d3c2d4..f1f35e1be 100644
--- a/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMergerIr.kt
+++ b/compiler/src/main/java/com/squareup/anvil/compiler/ModuleMergerIr.kt
@@ -117,7 +117,38 @@ internal class ModuleMergerIr(
       // for replaced classes and the final result.
       .toList()
 
+    val excludedModules = annotationConstructorCall.exclude()
+      .onEach { excludedClass ->
+        val contributesToAnnotation = excludedClass
+          .annotationOrNull(contributesToFqName)
+        val contributesBindingAnnotation = excludedClass
+          .annotationOrNull(contributesBindingFqName)
+        val contributesMultibindingAnnotation = excludedClass
+          .annotationOrNull(contributesMultibindingFqName)
+
+        // Verify that the the replaced classes use the same scope.
+        val scopeOfExclusion = contributesToAnnotation?.scope()
+          ?: contributesBindingAnnotation?.scope()
+          ?: contributesMultibindingAnnotation?.scope()
+          ?: throw AnvilCompilationException(
+            message = "Could not determine the scope of the excluded class " +
+              "${excludedClass.fqName}.",
+            element = declaration
+          )
+
+        if (scopeOfExclusion != scope) {
+          throw AnvilCompilationException(
+            message = "${declaration.fqName} with scope $scope wants to exclude " +
+              "${excludedClass.fqName} with scope $scopeOfExclusion. The exclusion must " +
+              "use the same scope.",
+            element = declaration
+          )
+        }
+      }
+
     val replacedModules = modules
+      // Ignore replaced modules or bindings specified by excluded modules.
+      .filter { (classSymbol, _) -> classSymbol.owner !in excludedModules }
       .flatMap { (classSymbol, contributesAnnotation) ->
         contributesAnnotation.replaces()
           .onEach { irClassForReplacement ->
@@ -174,35 +205,6 @@ internal class ModuleMergerIr(
       hintPackagePrefix = HINT_MULTIBINDING_PACKAGE_PREFIX
     )
 
-    val excludedModules = annotationConstructorCall.exclude()
-      .onEach { excludedClass ->
-        val contributesToAnnotation = excludedClass
-          .annotationOrNull(contributesToFqName)
-        val contributesBindingAnnotation = excludedClass
-          .annotationOrNull(contributesBindingFqName)
-        val contributesMultibindingAnnotation = excludedClass
-          .annotationOrNull(contributesMultibindingFqName)
-
-        // Verify that the the replaced classes use the same scope.
-        val scopeOfExclusion = contributesToAnnotation?.scope()
-          ?: contributesBindingAnnotation?.scope()
-          ?: contributesMultibindingAnnotation?.scope()
-          ?: throw AnvilCompilationException(
-            message = "Could not determine the scope of the excluded class " +
-              "${excludedClass.fqName}.",
-            element = declaration
-          )
-
-        if (scopeOfExclusion != scope) {
-          throw AnvilCompilationException(
-            message = "${declaration.fqName} with scope $scope wants to exclude " +
-              "${excludedClass.fqName} with scope $scopeOfExclusion. The exclusion must " +
-              "use the same scope.",
-            element = declaration
-          )
-        }
-      }
-
     if (predefinedModules.isNotEmpty()) {
       val intersect = predefinedModules.intersect(excludedModules)
       if (intersect.isNotEmpty()) {
diff --git a/compiler/src/main/java/com/squareup/anvil/compiler/codegen/BindingModuleGenerator.kt b/compiler/src/main/java/com/squareup/anvil/compiler/codegen/BindingModuleGenerator.kt
index 0511591fe..64fd9c29b 100644
--- a/compiler/src/main/java/com/squareup/anvil/compiler/codegen/BindingModuleGenerator.kt
+++ b/compiler/src/main/java/com/squareup/anvil/compiler/codegen/BindingModuleGenerator.kt
@@ -173,6 +173,9 @@ internal class BindingModuleGenerator(
     module: ModuleDescriptor
   ): Collection<GeneratedFile> {
     return mergedScopes.flatMap { (scope, daggerModuleFiles) ->
+      // Precompute this list once per scope since it expensive.
+      val excludedNames = excludedTypesForScope[scope].orEmpty().map { it.fqNameSafe }
+
       // Contributed Dagger modules can replace other Dagger modules but also contributed bindings.
       // If a binding is replaced, then we must not generate the binding method.
       //
@@ -181,6 +184,8 @@ internal class BindingModuleGenerator(
       val bindingsReplacedInDaggerModules = contributedModuleClasses
         .asSequence()
         .filter { it.scope(contributesToFqName, module) == scope }
+        // Ignore replaced bindings specified by excluded modules for this scope.
+        .filter { it.fqName !in excludedNames }
         .flatMap { it.replaces(contributesToFqName, module) }
         .plus(
           classScanner
@@ -191,6 +196,8 @@ internal class BindingModuleGenerator(
               scope = scope
             )
             .filter { it.annotationOrNull(daggerModuleFqName) != null }
+            // Ignore replaced bindings specified by excluded modules for this scope.
+            .filter { it !in excludedTypesForScope[scope].orEmpty() }
             .flatMap {
               it.annotationOrNull(contributesToFqName, scope)
                 ?.replaces(module)


Test patch:
diff --git a/compiler/src/test/java/com/squareup/anvil/compiler/ModuleMergerTest.kt b/compiler/src/test/java/com/squareup/anvil/compiler/ModuleMergerTest.kt
index 983a32e14..290b75286 100644
--- a/compiler/src/test/java/com/squareup/anvil/compiler/ModuleMergerTest.kt
+++ b/compiler/src/test/java/com/squareup/anvil/compiler/ModuleMergerTest.kt
@@ -675,6 +675,36 @@ class ModuleMergerTest(
     }
   }
 
+  @Test fun `contributed modules cannot be replaced by excluded modules`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import com.squareup.anvil.annotations.ContributesTo
+      $import
+
+      @ContributesTo(Any::class, replaces = [DaggerModule2::class])
+      @dagger.Module
+      abstract class DaggerModule1
+
+      @ContributesTo(Any::class)
+      @dagger.Module
+      abstract class DaggerModule2 
+
+      $annotation(
+          scope = Any::class,
+          exclude = [
+            DaggerModule1::class
+          ]
+      )
+      interface ComponentInterface
+      """
+    ) {
+      val component = componentInterface.anyDaggerComponent
+      assertThat(component.modules.withoutAnvilModule()).containsExactly(daggerModule2.kotlin)
+    }
+  }
+
   @Test fun `contributed bindings can be excluded`() {
     compile(
       """
diff --git a/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleGeneratorTest.kt b/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleGeneratorTest.kt
index 341ea6017..0e61f2abf 100644
--- a/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleGeneratorTest.kt
+++ b/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleGeneratorTest.kt
@@ -392,6 +392,50 @@ class BindingModuleGeneratorTest(
     }
   }
 
+  @Test fun `the contributed binding is not replaced by excluded modules`() {
+    compile(
+      """
+      package com.squareup.test
+      
+      import com.squareup.anvil.annotations.ContributesBinding
+      import com.squareup.anvil.annotations.ContributesTo
+      $import
+
+      interface ParentInterface
+
+      @ContributesBinding(Any::class)
+      interface ContributingInterface : ParentInterface
+      
+      @ContributesTo(
+          Any::class,
+          replaces = [ContributingInterface::class]
+      )
+      @dagger.Module
+      abstract class DaggerModule1
+      
+      $annotation(Any::class, exclude = [DaggerModule1::class])
+      interface ComponentInterface
+      """
+    ) {
+      val modules = if (annotationClass == MergeModules::class) {
+        componentInterface.daggerModule.includes.toList()
+      } else {
+        componentInterface.anyDaggerComponent.modules
+      }
+      assertThat(modules).containsExactly(componentInterfaceAnvilModule.kotlin)
+
+      val methods = modules.first().java.declaredMethods
+      assertThat(methods).hasLength(1)
+
+      with(methods[0]) {
+        assertThat(returnType).isEqualTo(parentInterface)
+        assertThat(parameterTypes.toList()).containsExactly(contributingInterface)
+        assertThat(isAbstract).isTrue()
+        assertThat(isAnnotationPresent(Binds::class.java)).isTrue()
+      }
+    }
+  }
+
   private val Class<*>.anyDaggerComponent: AnyDaggerComponent
     get() = anyDaggerComponent(annotationClass)
 }
diff --git a/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleMultibindingSetTest.kt b/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleMultibindingSetTest.kt
index 733b327f3..b0d7aed22 100644
--- a/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleMultibindingSetTest.kt
+++ b/compiler/src/test/java/com/squareup/anvil/compiler/codegen/BindingModuleMultibindingSetTest.kt
@@ -305,6 +305,51 @@ class BindingModuleMultibindingSetTest(
     }
   }
 
+  @Test fun `the contributed multibinding is not replaced by excluded modules`() {
+    compile(
+      """
+      package com.squareup.test
+      
+      import com.squareup.anvil.annotations.ContributesMultibinding
+      import com.squareup.anvil.annotations.ContributesTo
+      $import
+
+      interface ParentInterface
+
+      @ContributesMultibinding(Any::class)
+      interface ContributingInterface : ParentInterface
+      
+      @ContributesTo(
+          Any::class,
+          replaces = [ContributingInterface::class]
+      )
+      @dagger.Module
+      abstract class DaggerModule1
+      
+      $annotation(Any::class, exclude = [DaggerModule1::class])
+      interface ComponentInterface
+      """
+    ) {
+      val modules = if (annotationClass == MergeModules::class) {
+        componentInterface.daggerModule.includes.toList()
+      } else {
+        componentInterface.anyDaggerComponent.modules
+      }
+      assertThat(modules).containsExactly(componentInterfaceAnvilModule.kotlin)
+
+      val methods = modules.first().java.declaredMethods
+      assertThat(methods).hasLength(1)
+
+      with(methods[0]) {
+        assertThat(returnType).isEqualTo(parentInterface)
+        assertThat(parameterTypes.toList()).containsExactly(contributingInterface)
+        assertThat(isAbstract).isTrue()
+        assertThat(isAnnotationPresent(Binds::class.java)).isTrue()
+        assertThat(isAnnotationPresent(IntoSet::class.java)).isTrue()
+      }
+    }
+  }
+
   private val Class<*>.anyDaggerComponent: AnyDaggerComponent
     get() = anyDaggerComponent(annotationClass)
 }