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

Use MembersInjector instances when inherited members have restricted visibility #2589

Open
dandc87 opened this issue May 5, 2021 · 2 comments

Comments

@dandc87
Copy link

dandc87 commented May 5, 2021

Lets say I have:

// in :dep
class Dependency @Inject constructor()

// in :lib1
open class Foo {
  @Inject internal lateinit var some: Dependency
}

// in :lib2
class Bar : Foo() {
  // IDK inject other things, it doesn't really matter
}

Since it's an internal member, one would expect to be able to use implementation project('dep') in :lib1 and not need the dependency in :lib2, but the generated Bar_MembersInjector directly calls the static accessor which requires a Provider<Dependency> and with it a dep on :dep.

It would be nice if Dagger would use a MembersInjector instance instead of directly using the static accessors, leaking implementation details. Either for just the restricted visibility cases, or all inherited injected member cases.

Also, I'm only asking this of the injectMembers() implementations, which (at least for my Hilt projects) are unused anyways. The Component can continue to call the static accessor and keep the benefits for doing so.

@Chang-Eric
Copy link
Member

Just an update on this. We started looking into this and it got a bit more complicated since it changes some of the dependencies in our BindingGraph which is exposed through the SPI, making it kind of a breaking change for SPI users. We're still evaluating the impact there though.

The other larger problem though is that trying to remove the existing code will be a breaking change with respect to version compatibility. For these factory types we try to only add code as much as possible to not break across versions, but in this case since the existence of the current code is the issue, we're not yet sure how to fix this without this being a version incompatibility issue.

So basically, even after we work out some of the first problems with the SPI, we might additionally need to wait some time before removing the problematic code.

@dandc87
Copy link
Author

dandc87 commented Jun 12, 2021

Thanks for the update! I hadn't considered how this could impact SPI, but yeah that could be rough.

For version compatibility, is the generated injector class part of the supported API, or is it just generated code to generated code? The dev site say it's not supported, but signs are not cops 😄 If it is the later, there's got to be a number of ways to detect different versions in the processor and generate appropriate call-sites (marker interfaces, annotations, inspecting constructor parameters, etc). Or just gating this behind a flag (dagger.iPromiseJustOneVersion=true).

If it's the former, then...well..., here's to waiting for Dagger3 with KSP SPI support 🤠

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

2 participants