Repository: square/AssistedInject
Pull request #164 (2021-03-20 03:24:25)

Avoid wrapping a factory in a provider
In the case of our own `@AssistedInject.Factory` or Dagger's `@AssistedFactory`, when used as a dependency request in another factory do not wrap it in a `Provider`. The factory is already stateless, so wrapping it in a stateless provider is pointless and wasteful. Moreover, Dagger's assisted injection implementation explicitly forbids this with an error.


Patch:
diff --git a/assisted-inject-processor/build.gradle b/assisted-inject-processor/build.gradle
index 7fe68da..742c210 100644
--- a/assisted-inject-processor/build.gradle
+++ b/assisted-inject-processor/build.gradle
@@ -24,6 +24,7 @@ dependencies {
   testImplementation deps.truth
   testImplementation deps.compileTesting
   testImplementation deps.inject
+  testImplementation deps.dagger
   if (!Jvm.current().javaVersion.isJava9Compatible()) {
     testImplementation files(Jvm.current().getToolsJar())
   }
diff --git a/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/AssistedInjection.kt b/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/AssistedInjection.kt
index 23f5834..2d64b12 100644
--- a/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/AssistedInjection.kt
+++ b/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/AssistedInjection.kt
@@ -17,7 +17,7 @@ import javax.lang.model.element.Modifier.PRIVATE
 import javax.lang.model.element.Modifier.PUBLIC
 
 private val JAVAX_INJECT = ClassName.get("javax.inject", "Inject")
-private val JAVAX_PROVIDER = ClassName.get("javax.inject", "Provider")
+internal val JAVAX_PROVIDER = ClassName.get("javax.inject", "Provider")
 
 /** The structure of an assisted injection factory. */
 data class AssistedInjection(
@@ -98,15 +98,12 @@ data class AssistedInjection(
   }
 }
 
-/** True when this key represents a parameterized JSR 330 `Provider`. */
-private val Key.isProvider get() = type is ParameterizedTypeName && type.rawType == JAVAX_PROVIDER
-
 private val DependencyRequest.providerType: TypeName
   get() {
-    val type = if (key.isProvider) {
-      key.type // Do not wrap a Provider inside another Provider.
-    } else {
+    val type = if (key.useProvider) {
       ParameterizedTypeName.get(JAVAX_PROVIDER, key.type.box())
+    } else {
+      key.type
     }
     key.qualifier?.let {
       return type.annotated(it)
@@ -115,7 +112,7 @@ private val DependencyRequest.providerType: TypeName
   }
 
 private val DependencyRequest.argumentProvider
-  get() = CodeBlock.of(if (isAssisted || key.isProvider) "\$N" else "\$N.get()", name)
+  get() = CodeBlock.of(if (isAssisted || !key.useProvider) "\$N" else "\$N.get()", name)
 
 fun ClassName.assistedInjectFactoryName(): ClassName =
     peerClassWithReflectionNesting(simpleName() + "_AssistedFactory")
diff --git a/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/Key.kt b/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/Key.kt
index 3d26092..2c38980 100644
--- a/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/Key.kt
+++ b/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/Key.kt
@@ -15,9 +15,13 @@
  */
 package com.squareup.inject.assisted.processor
 
+import com.google.auto.common.MoreTypes
+import com.squareup.inject.assisted.AssistedInject
 import com.squareup.inject.assisted.processor.internal.hasAnnotation
+import com.squareup.inject.assisted.processor.internal.toAnnotationSpec
 import com.squareup.inject.assisted.processor.internal.toTypeName
 import com.squareup.javapoet.AnnotationSpec
+import com.squareup.javapoet.ParameterizedTypeName
 import com.squareup.javapoet.TypeName
 import javax.lang.model.element.VariableElement
 import javax.lang.model.type.TypeMirror
@@ -25,13 +29,27 @@ import javax.lang.model.type.TypeMirror
 /** Represents a type and an optional qualifier annotation for a binding. */
 data class Key(
   val type: TypeName,
-  val qualifier: AnnotationSpec? = null
+  val qualifier: AnnotationSpec? = null,
+  val useProvider: Boolean = true
 ) {
   override fun toString() = qualifier?.let { "$it $type" } ?: type.toString()
 }
 
 /** Create a [Key] from this type and any qualifier annotation. */
-fun VariableElement.asKey(mirror: TypeMirror = asType()) = Key(mirror.toTypeName(),
-    annotationMirrors.find {
-      it.annotationType.asElement().hasAnnotation("javax.inject.Qualifier")
-    }?.let { AnnotationSpec.get(it) })
+fun VariableElement.asKey(mirror: TypeMirror = asType()): Key {
+  val type = mirror.toTypeName()
+  val qualifier = annotationMirrors.find {
+    it.annotationType.asElement().hasAnnotation("javax.inject.Qualifier")
+  }?.toAnnotationSpec()
+
+  // Do not wrap a Provider inside another Provider.
+  val provider = type is ParameterizedTypeName && type.rawType == JAVAX_PROVIDER
+
+  val typeElement = if (type.isPrimitive) null else MoreTypes.asElement(mirror)
+  // Minor optimization. Don't wrap one of our stateless Factory instances in a stateless Provider.
+  val assistedInjectFactory = typeElement?.hasAnnotation<AssistedInject.Factory>() ?: false
+  // Dagger forbids requesting an @AssistedFactory-annotated type inside of a Provider.
+  val daggerAssistedFactory = typeElement?.hasAnnotation("dagger.assisted.AssistedFactory") ?: false
+
+  return Key(type, qualifier, !provider && !assistedInjectFactory && !daggerAssistedFactory)
+}
diff --git a/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/internal/javaPoet.kt b/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/internal/javaPoet.kt
index 4a0cd2d..be3a18f 100644
--- a/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/internal/javaPoet.kt
+++ b/assisted-inject-processor/src/main/java/com/squareup/inject/assisted/processor/internal/javaPoet.kt
@@ -1,9 +1,11 @@
 package com.squareup.inject.assisted.processor.internal
 
+import com.squareup.javapoet.AnnotationSpec
 import com.squareup.javapoet.ClassName
 import com.squareup.javapoet.CodeBlock
 import com.squareup.javapoet.ParameterizedTypeName
 import com.squareup.javapoet.TypeName
+import javax.lang.model.element.AnnotationMirror
 import javax.lang.model.element.TypeElement
 import javax.lang.model.type.TypeMirror
 import kotlin.reflect.KClass
@@ -12,6 +14,8 @@ fun TypeElement.toClassName(): ClassName = ClassName.get(this)
 fun TypeMirror.toTypeName(): TypeName = TypeName.get(this)
 fun KClass<*>.toClassName(): ClassName = ClassName.get(java)
 
+fun AnnotationMirror.toAnnotationSpec(): AnnotationSpec = AnnotationSpec.get(this)
+
 fun Iterable<CodeBlock>.joinToCode(separator: String = ", ") = CodeBlock.join(this, separator)
 
 /**
diff --git a/build.gradle b/build.gradle
index b562505..63ae49e 100644
--- a/build.gradle
+++ b/build.gradle
@@ -1,7 +1,7 @@
 buildscript {
   ext.versions = [
       'kotlin': '1.3.41',
-      'dagger': '2.24',
+      'dagger': '2.33',
       'incap' : '0.2',
   ]
 


Test patch:
diff --git a/assisted-inject-processor/src/test/java/com/squareup/inject/assisted/processor/AssistedInjectProcessorTest.kt b/assisted-inject-processor/src/test/java/com/squareup/inject/assisted/processor/AssistedInjectProcessorTest.kt
index 9958009..180a386 100644
--- a/assisted-inject-processor/src/test/java/com/squareup/inject/assisted/processor/AssistedInjectProcessorTest.kt
+++ b/assisted-inject-processor/src/test/java/com/squareup/inject/assisted/processor/AssistedInjectProcessorTest.kt
@@ -188,6 +188,122 @@ class AssistedInjectProcessorTest {
         .generatesSources(expected)
   }
 
+  @Test fun assistedFactoryDoesNotUseProvider() {
+    val input = JavaFileObjects.forSourceString("test.Test", """
+      package test;
+
+      import com.squareup.inject.assisted.Assisted;
+      import com.squareup.inject.assisted.AssistedInject;
+
+      class Test {
+        @AssistedInject
+        Test(Other.Factory foo, @Assisted String bar) {}
+
+        @AssistedInject.Factory
+        interface Factory {
+          Test create(String bar);
+        }
+      }
+
+      class Other {
+        @AssistedInject
+        Other(String a, @Assisted String b) {}
+
+        @AssistedInject.Factory
+        interface Factory {
+          Other create(String b);
+        }
+      }
+    """)
+
+    val expected = JavaFileObjects.forSourceString("test.Test_AssistedFactory", """
+      package test;
+
+      import java.lang.Override;
+      import java.lang.String;
+      import $GENERATED_TYPE;
+      import javax.inject.Inject;
+
+      $GENERATED_ANNOTATION
+      public final class Test_AssistedFactory implements Test.Factory {
+        private final Other.Factory foo;
+
+        @Inject public Test_AssistedFactory(Other.Factory foo) {
+          this.foo = foo;
+        }
+
+        @Override public Test create(String bar) {
+          return new Test(foo, bar);
+        }
+      }
+    """)
+
+    assertAbout(javaSource())
+        .that(input)
+        .processedWith(AssistedInjectProcessor())
+        .compilesWithoutError()
+        .and()
+        .generatesSources(expected)
+  }
+
+  @Test fun daggerAssistedFactoryDoesNotUseProvider() {
+    val input = JavaFileObjects.forSourceString("test.Test", """
+      package test;
+
+      import dagger.assisted.AssistedFactory;
+      import com.squareup.inject.assisted.Assisted;
+      import com.squareup.inject.assisted.AssistedInject;
+
+      class Test {
+        @AssistedInject
+        Test(Other.Factory foo, @Assisted String bar) {}
+
+        @AssistedInject.Factory
+        interface Factory {
+          Test create(String bar);
+        }
+      }
+
+      class Other {
+        Other(String a, String b) {}
+
+        @AssistedFactory
+        interface Factory {
+          Other create(String b);
+        }
+      }
+    """)
+
+    val expected = JavaFileObjects.forSourceString("test.Test_AssistedFactory", """
+      package test;
+
+      import java.lang.Override;
+      import java.lang.String;
+      import $GENERATED_TYPE;
+      import javax.inject.Inject;
+
+      $GENERATED_ANNOTATION
+      public final class Test_AssistedFactory implements Test.Factory {
+        private final Other.Factory foo;
+
+        @Inject public Test_AssistedFactory(Other.Factory foo) {
+          this.foo = foo;
+        }
+
+        @Override public Test create(String bar) {
+          return new Test(foo, bar);
+        }
+      }
+    """)
+
+    assertAbout(javaSource())
+        .that(input)
+        .processedWith(AssistedInjectProcessor())
+        .compilesWithoutError()
+        .and()
+        .generatesSources(expected)
+  }
+
   @Test fun factoryParameterNameIsNotUsedInTheImpl() {
     val input = JavaFileObjects.forSourceString("test.Test", """
       package test;
diff --git a/inflation-inject-processor/src/test/java/com/squareup/inject/inflation/processor/InflationInjectProcessorTest.kt b/inflation-inject-processor/src/test/java/com/squareup/inject/inflation/processor/InflationInjectProcessorTest.kt
index 02f4149..eec5b48 100644
--- a/inflation-inject-processor/src/test/java/com/squareup/inject/inflation/processor/InflationInjectProcessorTest.kt
+++ b/inflation-inject-processor/src/test/java/com/squareup/inject/inflation/processor/InflationInjectProcessorTest.kt
@@ -106,6 +106,191 @@ class InflationInjectProcessorTest {
         .generatesSources(expectedFactory, expectedModule)
   }
 
+  @Test fun injectAssistedInjectFactoryDoesNotUseProvider() {
+    val inputView = JavaFileObjects.forSourceString("test.TestView", """
+      package test;
+
+      import android.content.Context;
+      import android.util.AttributeSet;
+      import android.view.View;
+      import com.squareup.inject.assisted.Assisted;
+      import com.squareup.inject.assisted.AssistedInject;
+      import com.squareup.inject.inflation.InflationInject;
+
+      class TestView extends View {
+        @InflationInject
+        TestView(@Assisted Context context, @Assisted AttributeSet attrs, Other.Factory foo) {
+          super(context, attrs);
+        }
+      }
+
+      class Other {
+        @AssistedInject
+        Other(String a, @Assisted String b) {}
+
+        @AssistedInject.Factory
+        interface Factory {
+          Other create(String b);
+        }
+      }
+    """)
+    val inputModule = JavaFileObjects.forSourceString("test.TestModule", """
+      package test;
+
+      import com.squareup.inject.inflation.InflationModule;
+      import dagger.Module;
+
+      @InflationModule
+      @Module(includes = InflationInject_TestModule.class)
+      abstract class TestModule {}
+    """)
+
+    val expectedFactory = JavaFileObjects.forSourceString("test.TestView_AssistedFactory", """
+      package test;
+
+      import android.content.Context;
+      import android.util.AttributeSet;
+      import android.view.View;
+      import com.squareup.inject.inflation.ViewFactory;
+      import java.lang.Override;
+      import $GENERATED_TYPE;
+      import javax.inject.Inject;
+
+      $GENERATED_ANNOTATION
+      public final class TestView_AssistedFactory implements ViewFactory {
+        private final Other.Factory foo;
+
+        @Inject public Test_AssistedFactory(Other.Factory foo) {
+          this.foo = foo;
+        }
+
+        @Override public View create(Context context, AttributeSet attrs) {
+          return new TestView(context, attrs, foo);
+        }
+      }
+    """)
+    val expectedModule = JavaFileObjects.forSourceString("test.InflationModule_TestModule", """
+      package test;
+
+      import com.squareup.inject.inflation.ViewFactory;
+      import dagger.Binds;
+      import dagger.Module;
+      import dagger.multibindings.IntoMap;
+      import dagger.multibindings.StringKey;
+      import $GENERATED_TYPE;
+
+      @Module
+      $GENERATED_ANNOTATION
+      abstract class InflationInject_TestModule {
+        private InflationInject_TestModule() {}
+
+        @Binds
+        @IntoMap
+        @StringKey("test.TestView")
+        abstract ViewFactory bind_test_TestView(TestView_AssistedFactory factory);
+      }
+    """)
+
+    assertAbout(javaSources())
+        .that(listOf(inputView, inputModule))
+        .processedWith(InflationInjectProcessor())
+        .compilesWithoutError()
+        .and()
+        .generatesSources(expectedFactory, expectedModule)
+  }
+
+  @Test fun injectDaggerAssistedFactoryDoesNotUseProvider() {
+    val inputView = JavaFileObjects.forSourceString("test.TestView", """
+      package test;
+
+      import android.content.Context;
+      import android.util.AttributeSet;
+      import android.view.View;
+      import dagger.assisted.AssistedFactory;
+      import com.squareup.inject.assisted.Assisted;
+      import com.squareup.inject.inflation.InflationInject;
+
+      class TestView extends View {
+        @InflationInject
+        TestView(@Assisted Context context, @Assisted AttributeSet attrs, Other.Factory foo) {
+          super(context, attrs);
+        }
+      }
+
+      class Other {
+        Other(String a, String b) {}
+
+        @AssistedFactory
+        interface Factory {
+          Other create(String b);
+        }
+      }
+    """)
+    val inputModule = JavaFileObjects.forSourceString("test.TestModule", """
+      package test;
+
+      import com.squareup.inject.inflation.InflationModule;
+      import dagger.Module;
+
+      @InflationModule
+      @Module(includes = InflationInject_TestModule.class)
+      abstract class TestModule {}
+    """)
+
+    val expectedFactory = JavaFileObjects.forSourceString("test.TestView_AssistedFactory", """
+      package test;
+
+      import android.content.Context;
+      import android.util.AttributeSet;
+      import android.view.View;
+      import com.squareup.inject.inflation.ViewFactory;
+      import java.lang.Override;
+      import $GENERATED_TYPE;
+      import javax.inject.Inject;
+
+      $GENERATED_ANNOTATION
+      public final class TestView_AssistedFactory implements ViewFactory {
+        private final Other.Factory foo;
+
+        @Inject public Test_AssistedFactory(Other.Factory foo) {
+          this.foo = foo;
+        }
+
+        @Override public View create(Context context, AttributeSet attrs) {
+          return new TestView(context, attrs, foo);
+        }
+      }
+    """)
+    val expectedModule = JavaFileObjects.forSourceString("test.InflationModule_TestModule", """
+      package test;
+
+      import com.squareup.inject.inflation.ViewFactory;
+      import dagger.Binds;
+      import dagger.Module;
+      import dagger.multibindings.IntoMap;
+      import dagger.multibindings.StringKey;
+      import $GENERATED_TYPE;
+
+      @Module
+      $GENERATED_ANNOTATION
+      abstract class InflationInject_TestModule {
+        private InflationInject_TestModule() {}
+
+        @Binds
+        @IntoMap
+        @StringKey("test.TestView")
+        abstract ViewFactory bind_test_TestView(TestView_AssistedFactory factory);
+      }
+    """)
+
+    assertAbout(javaSources())
+        .that(listOf(inputView, inputModule))
+        .processedWith(InflationInjectProcessor())
+        .compilesWithoutError()
+        .and()
+        .generatesSources(expectedFactory, expectedModule)
+  }
+
   @Test fun public() {
     val inputView = JavaFileObjects.forSourceString("test.TestView", """
       package test;