google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.42k stars 2.01k forks source link

[AssistedInject] @AssistedInject factories should not be generated at the root #2417

Open lwasyl opened 3 years ago

lwasyl commented 3 years ago

The issue is about using Dagger 2.31.2 with Gradle and Kotlin.

In Dagger 2.31.2 the following code using @Inject annotation is valid:

module A (only dagger api dependency, no kapt):

class Bar @Inject constructor()

module B (dagger api + kapt, depends on A):

@Component
interface FooComponent {

    fun bar(): Bar
}

Dagger will generate Bar_Factory in module B and use it from the DaggerFooComponent.

However, similar setup with @AssistedInject is failing compilation:

module A (same as above, only dagger api dependency but no kapt):

class Foo @AssistedInject constructor(
    @Assisted private val param: (Baz) -> Unit,
) {

    @AssistedFactory
    interface Factory {

        fun create(
            param: (Baz) -> Unit,
        ): Foo
    }
}

class Baz

module B (depends on A, dagger api + dagger processor):

@Component
interface FooComponent {

    fun foo(): Foo.Factory
}

This will fail on the line

this.factoryProvider = Foo_Factory_Impl.create(fooProvider);

because while Dagger will generate Foo_Factory, it won't generate Foo_Factory_Impl. Since a very similar setup for a regular @Inject is supported, it'd be great if it was supported for @AssistedInject as well.

Somewhat related because it also fails for @AssistedInject but not for @Inject, when the injected parameter is an inner class, then Dagger generates code that doesn't compile but in a different way:

module A (no kapt):

class Foo @AssistedInject constructor(
    @Assisted private val param: (Bar.Baz) -> Unit,
) {

    @AssistedFactory
    interface Factory {

        fun create(
            param: (Bar.Baz) -> Unit,
        ): Foo
    }
}

interface Bar {
    data class Baz(val x: Int)
}

module B:

@Component
interface FooComponent {

    fun foo(): Foo.Factory
}

This time even the generated Foo_Factory will not compile, because it's using wrong name for Bar.Baz class:

public final class Foo_Factory {
  public Foo_Factory() {
  }
  public Foo get(Function1<? super Bar$Baz, Unit> arg0) {
    return newInstance(arg0);
  }
  public static Foo_Factory create() {
    return new Foo_Factory();
  }
  public static Foo newInstance(Function1<? super Bar$Baz, Unit> arg0) {
    return new Foo(arg0);
  }
}

Compiler now complains that Bar$Baz symbol cannot be found.

bcorso commented 3 years ago

Hi @lwasyl,

Since a very similar setup for a regular @Inject is supported, it'd be great if it was supported for @AssistedInject as well.

Sorry, we don't have plans to support this feature for @AssistedInject types. The reason we have to support it for @Inject types is because the type using @Inject might need to support other DI frameworks aside from Dagger, so we can't expect that the Dagger processor is run at the source.

For annotations that Dagger owns, like @Module and @AssistedInject, we expect that the Dagger processor will be run on the source that uses the annotation, so we do not generate missing factories at the root. The reason we don't add this support for convenience is because we want the processor to always run to give better processing and incremental processing performance. It also avoids weird cases where you can accidentally generate the factory more than once (e.g. two separate @Component may try to generate the same missing factory).

Somewhat related because it also fails for @AssistedInject but not for @Inject, when the injected parameter is an inner class, then Dagger generates code that doesn't compile but in a different way

Thanks for reporting this. I'll have to take a closer look when I get time. We generally just rely on the javax/lang/model and JavaPoet for type names so it's not immediately clear where this discrepancy would be coming from.

lwasyl commented 3 years ago

Hi Brad, thanks for a detailed explanation. Do I understand correctly that when using @AssistedInject then Foo_Factory also shouldn't be generated in the other module in the first place? This actually sent me off track for couple of hours when I was trying to figure out what's wrong, because I saw something generated and assumed I did something else wrong.

Also about the other issue, just to clarify, the $ in the name only happens for @AssistedInject types that weren't processed in their own module, so in the case that you say isn't really supported anyway

bcorso commented 3 years ago

Do I understand correctly that when using @AssistedInject then Foo_Factory also shouldn't be generated in the other module in the first place? This actually sent me off track for couple of hours when I was trying to figure out what's wrong, because I saw something generated and assumed I did something else wrong.

Ohhh, thanks for pointing that out! Yes ideally we shouldn't be generating the Foo_Factory for @AssistedInject at the root either. I'm not surprised that this accidentally slipped in since internally @AssistedInject logic shares a lot of the logic from @Inject; however, I imagine we'll disable this for a future release.

In the past, we've also discussed trying (best effort) to detect cases where the factory is missing and fail with a better error message, e.g. "Foo_Factory class is missing did you forget to add the Dagger processor when processing Foo".

Also about the other issue, just to clarify, the $ in the name only happens for @AssistedInject types that weren't processed in their own module, so in the case that you say isn't really supported anyway

Ah, thanks for the clarification!