Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Peacemaker-Otoo opened this issue Apr 27, 2022 · 16 comments
Open

Comments

@Peacemaker-Otoo
Copy link

@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
Copy link

bcorso commented Apr 27, 2022

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 #2042 (comment)

@otoo-peacemaker
Copy link

I have tried this solution was still have problems.

@bcorso
Copy link

bcorso commented Apr 27, 2022

I have tried this solution was still have problems.

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

@Archerkills
Copy link

Archerkills commented May 12, 2022

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
Copy link

bcorso commented May 12, 2022

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
Copy link

@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
Copy link

Archerkills commented May 12, 2022

@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
Copy link

bcorso commented May 13, 2022

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
Copy link

@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
Copy link

bcorso commented May 13, 2022

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
Copy link

Archerkills commented May 13, 2022

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
Copy link

bcorso commented May 13, 2022

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
Copy link

@bcorso good to know! Thanks for the insight!

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

@bcorso
Copy link

bcorso commented May 13, 2022

I initially concluded that this wasn't possible (#2042 (comment)) 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
Copy link

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
Copy link
Author

Peacemaker-Otoo commented May 13, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants