Repository: square/anvil (kotlin)

Issue:
incorrect factory generation when the injected class has both constructor and member injection
Anvil 2.2.1
Dagger 2.34

I have a reproducer here: https://github.com/RBusarow/AnvilReproducer/tree/factory_generation_member_injection.

Dagger has additional logic in its factories if the class is member-injected.  Anvil does not.  This causes two different but related issues.

example source:
``` Kotlin
import javax.inject.Inject
import javax.inject.Provider

class InjectClass @Inject constructor(
  val string: String
) {
      
  @Inject lateinit var stringProvider: Provider<String>
}
```

What Anvil generates:
``` Kotlin
public class InjectClass_Factory(
  private val param0: Provider<String>
) : Factory<InjectClass> {
  public override fun `get`(): InjectClass = newInstance(param0.get())

  public companion object {
    @JvmStatic
    public fun create(param0: Provider<String>): InjectClass_Factory = InjectClass_Factory(param0)

    @JvmStatic
    public fun newInstance(param0: String): InjectClass = InjectClass(param0)
  }
}
```
What Dagger generates:
``` Java
public final class InjectClass_Factory implements Factory<InjectClass> {
  private final Provider<String> stringProvider;

  private final Provider<String> stringProvider2;

  public InjectClass_Factory(Provider<String> stringProvider, Provider<String> stringProvider2) {
    this.stringProvider = stringProvider;
    this.stringProvider2 = stringProvider2;
  }

  @Override
  public InjectClass get() {
    InjectClass instance = newInstance(stringProvider.get());
    InjectClass_MembersInjector.injectStringProvider(instance, stringProvider2);
    return instance;
  }

  public static InjectClass_Factory create(Provider<String> stringProvider,
      Provider<String> stringProvider2) {
    return new InjectClass_Factory(stringProvider, stringProvider2);
  }

  public static InjectClass newInstance(String string) {
    return new InjectClass(string);
  }
}
```
____
### Issue 1 - `create(...)` functions are missing parameters
In the reproducer, just run `./gradlew build` to see it.

In Dagger-generated factories, the static `create()` function (and the constructor) include Providers for any member-injected parameters.  These extra providers are missing from the Anvil version.

This isn't always a problem, as it depends upon what Dagger generates in the Component.  This `create(...)` function is only called by the Component when it needs to create a `Provider<T>`, `Lazy<T>`, or singleton instance for that type.  In these cases, the signature mismatch fails the build in the Component's module (not the injected class's module).

example error:
```
error: method create in class InjectClass_Factory cannot be applied to given types;
    this.injectClassProvider = InjectClass_Factory.create(stringProvider, stringProvider);
                                                  ^
  required: Provider<String>
  found:    Provider<String>,Provider<String>
  reason: actual and formal argument lists differ in length
```
____
### Issue 2 - `get()` doesn't inject members

The Dagger-generated factory's `get()` performs member injection on a new instance before returning it.  The Anvil version does not perform member injection.

This is only a behavioral difference so it doesn't actually break the build.

In the reproducer, after the build issue is fixed, run the main function [here](https://github.com/RBusarow/AnvilReproducer/blob/factory_generation_member_injection/lib2/src/main/java/com/reproducer/MyComponent.kt) to see an `UninitializedPropertyAccessException`.

Anvil version:
``` Kotlin
  public override fun `get`(): InjectClass = newInstance(param0.get())
```
Dagger version:
``` Java
  @Override
  public InjectClass get() {
    InjectClass instance = newInstance(stringProvider.get());
    InjectClass_MembersInjector.injectStringProvider(instance, stringProvider2);
    return instance;
  }
```

Pull request:
fixes #246

Patch:
diff --git a/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/InjectConstructorFactoryGenerator.kt b/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/InjectConstructorFactoryGenerator.kt
index fc483a906..8446d0a93 100644
--- a/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/InjectConstructorFactoryGenerator.kt
+++ b/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/InjectConstructorFactoryGenerator.kt
@@ -8,8 +8,10 @@ import com.squareup.anvil.compiler.codegen.buildFile
 import com.squareup.anvil.compiler.codegen.classesAndInnerClasses
 import com.squareup.anvil.compiler.codegen.createGeneratedFile
 import com.squareup.anvil.compiler.codegen.fqNameOrNull
+import com.squareup.anvil.compiler.codegen.hasAnnotation
 import com.squareup.anvil.compiler.codegen.injectConstructor
 import com.squareup.anvil.compiler.codegen.mapToParameter
+import com.squareup.anvil.compiler.daggerDoubleCheckFqNameString
 import com.squareup.anvil.compiler.generateClassName
 import com.squareup.anvil.compiler.injectFqName
 import com.squareup.anvil.compiler.safePackageString
@@ -26,10 +28,14 @@ import com.squareup.kotlinpoet.asClassName
 import com.squareup.kotlinpoet.jvm.jvmStatic
 import dagger.internal.Factory
 import org.jetbrains.kotlin.descriptors.ModuleDescriptor
+import org.jetbrains.kotlin.lexer.KtTokens
+import org.jetbrains.kotlin.psi.KtClassBody
 import org.jetbrains.kotlin.psi.KtClassOrObject
 import org.jetbrains.kotlin.psi.KtConstructor
 import org.jetbrains.kotlin.psi.KtFile
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierTypeOrDefault
 import java.io.File
+import java.util.Locale.US
 
 internal class InjectConstructorFactoryGenerator : PrivateCodeGenerator() {
 
@@ -58,7 +64,22 @@ internal class InjectConstructorFactoryGenerator : PrivateCodeGenerator() {
     val packageName = clazz.containingKtFile.packageFqName.safePackageString()
     val className = "${clazz.generateClassName()}_Factory"
 
-    val parameters = constructor.valueParameters.mapToParameter(module)
+    val memberInjectProperties = clazz.children
+      .asSequence()
+      .filterIsInstance<KtClassBody>()
+      .flatMap { it.properties.asSequence() }
+      .filterNot { it.visibilityModifierTypeOrDefault() == KtTokens.PRIVATE_KEYWORD }
+      .filter { it.hasAnnotation(injectFqName) }
+      .toList()
+
+    val parameters = constructor.valueParameters
+      .plus(memberInjectProperties)
+      .mapToParameter(module)
+
+    val constructorSize = constructor.valueParameters.size
+
+    val constructorInjectParameters = parameters.take(constructorSize)
+    val memberInjectParameters = parameters.drop(constructorSize)
 
     val typeParameters = clazz.typeParameterList
       ?.parameters
@@ -118,12 +139,36 @@ internal class InjectConstructorFactoryGenerator : PrivateCodeGenerator() {
             .addModifiers(OVERRIDE)
             .returns(classType)
             .apply {
-              val argumentList = parameters.asArgumentList(
+              val newInstanceArgumentList = constructorInjectParameters.asArgumentList(
                 asProvider = true,
                 includeModule = false
               )
 
-              addStatement("return newInstance($argumentList)")
+              if (memberInjectParameters.isEmpty()) {
+                addStatement("return newInstance($newInstanceArgumentList)")
+              } else {
+                addStatement("val instance = newInstance($newInstanceArgumentList)")
+
+                val memberInjectorClassName = "${clazz.generateClassName()}_MembersInjector"
+                val memberInjectorClass = ClassName(packageName, memberInjectorClassName)
+
+                memberInjectParameters.forEachIndexed { index, parameter ->
+
+                  val property = memberInjectProperties[index]
+                  val propertyName = property.nameAsSafeName.asString()
+                  val functionName = "inject${propertyName.capitalize(US)}"
+
+                  val param = when {
+                    parameter.isWrappedInProvider -> parameter.name
+                    parameter.isWrappedInLazy ->
+                      "$daggerDoubleCheckFqNameString.lazy(${parameter.name})"
+                    else -> parameter.name + ".get()"
+                  }
+
+                  addStatement("%T.$functionName(instance, $param)", memberInjectorClass)
+                }
+                addStatement("return instance")
+              }
             }
             .build()
         )
@@ -165,13 +210,13 @@ internal class InjectConstructorFactoryGenerator : PrivateCodeGenerator() {
                   if (typeParameters.isNotEmpty()) {
                     addTypeVariables(typeParameters)
                   }
-                  parameters.forEach { parameter ->
+                  constructorInjectParameters.forEach { parameter ->
                     addParameter(
                       name = parameter.name,
                       type = parameter.originalTypeName
                     )
                   }
-                  val argumentsWithoutModule = parameters.joinToString { it.name }
+                  val argumentsWithoutModule = constructorInjectParameters.joinToString { it.name }
 
                   addStatement("return %T($argumentsWithoutModule)", classType)
                 }


Test patch:
diff --git a/compiler/src/test/java/com/squareup/anvil/compiler/dagger/InjectConstructorFactoryGeneratorTest.kt b/compiler/src/test/java/com/squareup/anvil/compiler/dagger/InjectConstructorFactoryGeneratorTest.kt
index 20deea440..4aa6c74d9 100644
--- a/compiler/src/test/java/com/squareup/anvil/compiler/dagger/InjectConstructorFactoryGeneratorTest.kt
+++ b/compiler/src/test/java/com/squareup/anvil/compiler/dagger/InjectConstructorFactoryGeneratorTest.kt
@@ -685,6 +685,204 @@ public final class InjectClass_Factory implements Factory<InjectClass> {
     }
   }
 
+  @Test
+  fun `a factory class is generated for a class with constructor and member injection`() {
+    /*
+import dagger.`internal`.Factory
+import javax.inject.Provider
+import kotlin.String
+import kotlin.Suppress
+import kotlin.jvm.JvmStatic
+import kotlin.jvm.JvmSuppressWildcards
+
+public class InjectClass_Factory(
+  private val param0: Provider<String>,
+  private val param1: Provider<@JvmSuppressWildcards Int>
+) : Factory<InjectClass> {
+  public override fun `get`(): InjectClass {
+    val instance = newInstance(param0.get())
+    InjectClass_MembersInjector.injectIntProvider(instance, param1)
+    return instance
+  }
+
+  public companion object {
+    @JvmStatic
+    public fun create(param0: Provider<String>, param1: Provider<@JvmSuppressWildcards Int>):
+        InjectClass_Factory = InjectClass_Factory(param0, param1)
+
+    @JvmStatic
+    public fun newInstance(param0: String): InjectClass = InjectClass(param0)
+  }
+}
+     */
+
+    compile(
+      """
+      package com.squareup.test
+      
+      import javax.inject.Inject
+      import javax.inject.Provider
+
+      class InjectClass @Inject constructor(
+        val string: String
+      ) {
+      
+        @Inject lateinit var intProvider: Provider<Int>
+        
+        override fun equals(other: Any?): Boolean {
+          if (this === other) return true
+          if (other !is InjectClass) return false
+      
+          if (string != other.string) return false
+          if (intProvider.get() != other.intProvider.get()) return false
+      
+          return true
+        }
+      
+        override fun hashCode(): Int {
+          var result = string.hashCode()
+          result = 31 * result + intProvider.get().hashCode()
+          return result
+        }
+      }
+      """
+    ) {
+
+      val factoryClass = injectClass.factoryClass()
+
+      val constructor = factoryClass.declaredConstructors.single()
+      assertThat(constructor.parameterTypes.toList())
+        .containsExactly(Provider::class.java, Provider::class.java)
+
+      val staticMethods = factoryClass.declaredMethods.filter { it.isStatic }
+
+      val factoryInstance = staticMethods.single { it.name == "create" }
+        .invoke(null, Provider { "a" }, Provider { 1 })
+      assertThat(factoryInstance::class.java).isEqualTo(factoryClass)
+
+      val newInstance = staticMethods.single { it.name == "newInstance" }
+        .invoke(null, "a")
+      val getInstance = (factoryInstance as Factory<*>).get()
+
+      assertThat(newInstance).isNotNull()
+      assertThat(getInstance).isNotNull()
+
+      assertThat(newInstance).isNotSameInstanceAs(getInstance)
+    }
+  }
+
+  @Test
+  fun `a factory class is generated which injects members when calling get`() {
+    /*
+import dagger.`internal`.Factory
+import javax.inject.Provider
+import kotlin.Int
+import kotlin.String
+import kotlin.Suppress
+import kotlin.jvm.JvmStatic
+import kotlin.jvm.JvmSuppressWildcards
+
+public class InjectClass_Factory(
+  private val param0: Provider<String>,
+  private val param1: Provider<@JvmSuppressWildcards Int>,
+  private val param2: Provider<String>,
+  private val param3: Provider<String>
+) : Factory<InjectClass> {
+  public override fun `get`(): InjectClass {
+    val instance = newInstance(param0.get())
+    InjectClass_MembersInjector.injectIntProvider(instance, param1)
+    InjectClass_MembersInjector.injectStringLazy(instance, dagger.internal.DoubleCheck.lazy(param2))
+    InjectClass_MembersInjector.injectOtherString(instance, param3.get())
+    return instance
+  }
+
+  public companion object {
+    @JvmStatic
+    public fun create(
+      param0: Provider<String>,
+      param1: Provider<@JvmSuppressWildcards Int>,
+      param2: Provider<String>,
+      param3: Provider<String>
+    ): InjectClass_Factory = InjectClass_Factory(param0, param1, param2, param3)
+
+    @JvmStatic
+    public fun newInstance(param0: String): InjectClass = InjectClass(param0)
+  }
+}
+     */
+
+    compile(
+      """
+      package com.squareup.test
+      
+      import javax.inject.Inject
+      import javax.inject.Provider
+      
+      class InjectClass @Inject constructor(
+        val string: String
+      ) {
+      
+        @Inject lateinit var intProvider: Provider<Int>
+        @Inject lateinit var stringLazy: dagger.Lazy<String>
+        @Inject lateinit var otherString: String
+        
+        override fun equals(other: Any?): Boolean {
+          if (this === other) return true
+          if (other !is InjectClass) return false
+      
+          if (string != other.string) return false
+          if (intProvider.get() != other.intProvider.get()) return false
+          if (stringLazy.get() != other.stringLazy.get()) return false
+          if (otherString != other.otherString) return false
+      
+          return true
+        }
+      
+        override fun hashCode(): Int {
+          var result = string.hashCode()
+          result = 31 * result + intProvider.get().hashCode()
+          result = 31 * result + stringLazy.get().hashCode()
+          result = 31 * result + otherString.hashCode()
+          return result
+        }
+      }
+      """
+    ) {
+
+      val factoryClass = injectClass.factoryClass()
+
+      val constructor = factoryClass.declaredConstructors.single()
+      assertThat(constructor.parameterTypes.toList())
+        .containsExactly(
+          Provider::class.java,
+          Provider::class.java,
+          Provider::class.java,
+          Provider::class.java
+        )
+
+      val staticMethods = factoryClass.declaredMethods.filter { it.isStatic }
+
+      val factoryInstance = staticMethods.single { it.name == "create" }
+        .invoke(
+          null,
+          Provider { "a" },
+          Provider { 1 },
+          Provider { "stringLazy" },
+          Provider { "otherString" }
+        )
+      assertThat(factoryInstance::class.java).isEqualTo(factoryClass)
+
+      val newInstance = staticMethods.single { it.name == "newInstance" }
+        .invoke(null, "a")
+      val getInstance = (factoryInstance as Factory<*>).get()
+
+      assertThat(newInstance).isNotNull()
+      assertThat(getInstance).isNotNull()
+
+      assertThat(newInstance).isNotSameInstanceAs(getInstance)
+    }
+  }
+
   @Test
   fun `a factory class is generated for a class injecting a class starting with a lowercase character`() {
     /*