google / dagger

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

@AndroidEntryPoint-annotated classes cannot have type parameters. #3370

Open Peacemaker-Otoo opened 2 years ago

Peacemaker-Otoo commented 2 years ago

@AndroidEntryPoint abstract class BaseFragment<VM : ViewModel, viewBinding : ViewDataBinding, repository : BaseRepository> : Fragment() { protected lateinit var binding: viewBinding protected lateinit var viewModel: VM var remoteDataSource = RemoteDataSource() ..................................... }

the error code image

bcorso commented 2 years ago

The solution is to put the @AndroidEntryPoint annotation on each of your subclasses instead of the base class, e.g.

// Put the annotation here instead of on the base class.
@AndroidEntryPoint
class MyFragment: BaseFragment<MyViewModel, MyViewDataBinding>() {
  ...
}

See https://github.com/google/dagger/issues/2042#issuecomment-673078030

otoo-peacemaker commented 2 years ago

I have tried this solution was still have problems.

bcorso commented 2 years ago

I have tried this solution was still have problems.

Do you have more details (e.g. error message or stack trace)?

Archerkills commented 2 years ago

The solution is to put the @AndroidEntryPoint annotation on each of your subclasses instead of the base class, e.g.

This solves the compile error for me. But if I remove @AndroidEntryPoint from base class, the @Inject fields in base class will NOT BE SET. Is it a known issue? Do we have plan to support it?

It's a valid cases where both base class and child class have injected fields.

bcorso commented 2 years ago

This solves the compile error for me. But if I remove @AndroidEntryPoint from base class, the @Inject fields in base class will NOT BE SET. Is it a known issue? Do we have plan to support it?

@Archerkills All of the @Inject parameters from a supertype should be injected when you inject a subtype. You can look at the test here as an example: https://github.com/google/dagger/blob/master/javatests/dagger/functional/GenericTest.java#L93-L114

However, if you can provide a sample project where this isn't working for you I'm happy to take a look.

Archerkills commented 2 years ago

@bcorso thanks for a quick response, I am working on WhatsApp Android code base and I will forward your response to our internal DI dicussion.

Archerkills commented 2 years ago

@bcorso the problem turns out to be that we have a lot of logic to access injected params in base View's constructor to init view state. But Hilt_BaseView's constructor calls inject() AFTER super(), causing NPE.

@AndroidEntryPoint
class ChildView extends MiddleView<SomeType>{
  ChildView(Context context, ...) {
    super(context, ...);
  }
}

class abstract <T extends E> MiddleView extends BaseView<T> {
  @Inject Foo foo

  MiddleView(Context context, ...) {
    super(context, ...);
    this.foo.callSomeMethod(); // access foo before injected -> NPE
  }
}

class BaseView ....

=====

// generated constructor for Hilt_ChildView.class
Hilt_MiddleView(Context, ....) {
  super(context, ...);
  inject(); // foo is injected here.
}

Note that is was not a problem before we added generic types and remove @AndroidEntryPoint from class MiddleView. Do you have thoughts how to work around this?

bcorso commented 2 years ago

Unfortunately, Java requires the super() call before any other calls within a constructor, so we don't really have a choice on that.

TBH, we haven't hit this use case yet, so I'll have to talk to the team to see how we want to officially support it. If possible, it would be great to try to avoid doing work in your view's constructor, but I do agree that this is unfortunate since normally this approach works within the constructor of an @AndroidEntryPoint annotated class.

One (pretty bad) workaround is to define an abstract inject() method in your MiddleView and call it manually from the constructor:

abstract class MiddleView<T extends E> extends BaseView<T> {
  @Inject Foo foo;

  MiddleView(Context context, ...) {
    super(context, ...);
    inject(); // <-- Call injection manually here before using foo
    this.foo.callSomeMethod();
  }

  // This is a hack to allow injection early in the base class constructor without using @AndroidEntryPoint.
  // This method will be implemented by Hilt's generated class.
  protected abstract void inject();
}

Note that this works because the Hilt generated code will generate the implementation of the inject() method and we ensure that calling it multiple times is idempotent (i.e. it won't double inject).

However, the reason this is a "bad" approach is because it relies on an implementation detail of Hilt's generated code. In particular, the inject() method is not meant to be public API or called by user code, and we want to reserve the right to change it in the future. However, until we have a more official API for this, it may be a decent workaround for you.

Archerkills commented 2 years ago

@bcorso Adding protected abstract void inject(); to MiddleView works! Thanks for you detailed tips! Does calling additional inject() increase latency? Hilt is currently a big part in view render latency.

bcorso commented 2 years ago

Hilt is currently a big part in view render latency.

Is this from profiling your app? If so, can you share the profile?

Usually the latency is either coming from creating all of the objects needed to inject your view (and their transitive dependencies), or from doing extraneous work in your constructor (e.g. this.foo.callSomeMethod() in the example above).

Archerkills commented 2 years ago

Is this from profiling your app? If so, can you share the profile? Usually the latency is either coming from creating all of the objects needed to inject your view (and their transitive dependencies), or from doing extraneous work in your constructor

You are right. It's from CPU profiling our app. I am not sure if I am allowed to share our CPU profile before I consulted our leadership. But I looked into some of the big inject() stack in CPU trace and it is usually caused by creating one of object that is particularly slow (for example one of the class parses Json in its constructor! And it should have been avoided).

Back to inject() latency. I looked into @Inject params in our MiddleView and all of them are @Singleton, does that mean:

  1. calling inject() 1st time will create any missing singletons (and transitive deps) in dagger component, and calling inject() 2nd time will just re-insert the existing singletons into MiddleView -- and it should have very minimum overhead?
  2. Imagine if MiddleView instead depends on a lot of non-singleton @Inject param, will calling inject() RE-CREATE and re-insert those non-singletons -- and it should have significant overhead?

Can you confirm those 2 assumptions are correct?

bcorso commented 2 years ago

Calling the inject() method any number of times after the first is effectively a no-op. You can check the generated implementation for your view, but it should looks something like this:

  protected void inject() {
    if (!injected) {
      injected = true;
      // ... injection happens here
    }
  }
Archerkills commented 2 years ago

@bcorso good to know! Thanks for the insight!

Is there any plan to support generic types in @AndroidEntryPoint annotated class?

bcorso commented 2 years ago

I initially concluded that this wasn't possible (https://github.com/google/dagger/issues/2042#issuecomment-673252618) since we couldn't implement the inject() method. However, thinking about it more we can probably support it similar to the workaround I suggested above as long as the generic base class is abstract.

In particular, the generated Hilt_MiddleView class wouldn't actually implement inject(), it would just add the abstract inject() method and the constructor call for you, just like you're currently doing manually.

I think this should be a pretty easy thing for us to fix.

Archerkills commented 2 years ago

we can probably support it similar to the workaround I suggested above as long as the generic base class is abstract.

+1. That would be super helpful! To add protected void inject(){} to MiddleView I probably have to defend myself in code review. But if Hilt can support it then we can just benefit from it for free!

Peacemaker-Otoo commented 2 years ago

Please, it's seems solution is hard to implement. So, why don't you build a small project on how to go about your workaround?

On Fri, May 13, 2022, 7:15 PM Brad Corso @.***> wrote:

I initially concluded that this wasn't possible (#2042 (comment) https://github.com/google/dagger/issues/2042#issuecomment-673252618) since we couldn't implement the inject() method. However, thinking about it more we can probably support it similar to the workaround I suggested above as long as the generic base class is abstract.

In particular, the generated Hilt_MiddleView class wouldn't actually implement inject(), it would just add the abstract inject() method and the constructor call for you, just like you're currently doing manually.

I think this should be a pretty easy thing for us to fix.

— Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/3370#issuecomment-1126367780, or unsubscribe https://github.com/notifications/unsubscribe-auth/AW5JCVYOEOAMSJIU32NQ5RDVJ2S63ANCNFSM5UOO672Q . You are receiving this because you authored the thread.Message ID: @.***>