Remove unnecessary `MembersInjectors.injectMembers` method #950

szymonkozak · 2017-11-14T19:56:40Z

The last use of the MembersInjectors class was removed in this commit, so it isn't used now during code generation.
(This class is still used in TypeNames as a class name constant, which is no longer necessary)

Static MembersInjectors.injectMembers is no longer used during code generation.

ronshapiro · 2017-11-14T21:25:37Z

java/dagger/internal/MembersInjectors.java

-   * members, so care should be taken to ensure appropriate use.
-   */
-  @SuppressWarnings("unchecked")
-  public static <T> MembersInjector<T> noOp() {


We may not have a test for it, but this is returned when for types that have no injected members:

class A { @Inject A() {} } @Component interface C { MembersInjector<A> aMembersInjector(); }

ronshapiro · 2017-11-14T21:26:43Z

java/dagger/internal/MembersInjectors.java

-    }
-  }
-
-  public static void checkInstanceNotNull(Object instance) {


This is use above, but we can make it private probably

ronshapiro · 2017-11-14T21:30:30Z

java/dagger/internal/codegen/TypeNames.java

@@ -79,7 +78,6 @@
  static final ClassName MAP_PRODUCER = ClassName.get(MapProducer.class);
  static final ClassName MAP_PROVIDER_FACTORY = ClassName.get(MapProviderFactory.class);
  static final ClassName MEMBERS_INJECTOR = ClassName.get(MembersInjector.class);
-  static final ClassName MEMBERS_INJECTORS = ClassName.get(MembersInjectors.class);


See MemberSelect.noOpMembersInjector()

Ok, right.
So can I remove static injectMembers from MembersInjectors?. It's seems to be not used.

ronshapiro · 2017-11-14T22:10:30Z

java/dagger/internal/MembersInjectors.java

@@ -57,7 +48,7 @@
    }
  }

-  public static void checkInstanceNotNull(Object instance) {
+  private static void checkInstanceNotNull(Object instance) {
    checkNotNull(instance, "Cannot inject members into a null reference");


Maybe just inline this above?

ronshapiro · 2017-11-14T22:21:04Z

Great, thanks! We'll submit this internally and then sync a commit with your authorship here!

Closes #950 RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175736999

Tagakov · 2017-12-21T00:32:22Z

Hi, I am using a dependency which is built with old version of dagger. After updating to 2.14.1 I am facing crash when dependency mentioned above references to MembersInjectors.injectMembers which is not presented in current version anymore.

Exception description
java.lang.NoSuchMethodError: No static method injectMembers(Ldagger/MembersInjector;Ljava/lang/Object;)

Generated code from library

public OverlayPresenter get() {
    return (OverlayPresenter)MembersInjectors.injectMembers(this.overlayPresenterMembersInjector, new OverlayPresenter((PresenterConfiguration)this.presenterConfigurationProvider.get(), (BusinessInteractor)this.businessInteractorProvider.get()));
}

I am not sure how to fix it, can you help please?

ronshapiro · 2017-12-21T01:00:01Z

You should shade the dagger.internal from one (or both) of the libraries. The old one is probably easiest

vovkab · 2018-01-24T20:46:27Z

Any reason we did breaking changes to a "public" api?
It is not really internal if it is end up being used by client code, even if it is autogenerated.

JakeWharton · 2018-01-24T20:51:16Z

Strongly disagree. The nature of generated code is such that many things have visibilities beyond what you'd otherwise desire in order to facilitate the functionality. Perhaps an annotation and error-prone check can enforce true public API for @generated code.

…

On Wed, Jan 24, 2018 at 3:46 PM Vladimir Baryshnikov < ***@***.***> wrote: Any reason we did breaking changes to a "public" api? It is not really internal if it is end up being used by client code, even if it is autogenerated. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#950 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEbsII1kDNhMjyNT4HNilPiUDCxNyks5tN5akgaJpZM4Qd56O> .

ronshapiro · 2018-01-24T20:56:19Z

It's a good idea to be shading any dagger.internal.* code if you're sharing it with other libraries that are being compiled separately/that you can't guarantee are being compiled at the same dagger version.

vovkab · 2018-01-24T20:59:05Z

@JakeWharton That is all ok if you have access to code to generate.

If you are building a library using dagger, all code generation is already done. So we either should not change "public" api to keep it compatible, or we should do real client code generation to make sure there is no dagger dependencies left to worry about, i.e. internals should not be on a class path, generate anything you may need in clients namespace.

tbroyer · 2018-01-25T14:53:02Z

I have to agree with @vovkab here, if generated code depends on dagger.internal.* then dagger.internal.* is part of the public API. If your answer is "shade it", then it at least needs to be properly documented (and then people will re-evaluate their use of Dagger in libraries –like they did for Guava for a while, before the recent change in policy– as it'll consequently bloat their apps with duplicate shaded code; and/or we'll likely see "forks" of the com.google.dagger:dagger artifact only to retain backwards compatibility).

elye · 2018-11-05T14:33:13Z

I facing this issue as well

java.lang.NoSuchMethodError: No static method injectMembers(Ldagger/MembersInjector;Ljava/lang/Object;)

What does the shade in You should shade the dagger.internal from one (or both) of the libraries. refers to?

tbroyer · 2018-11-05T15:25:37Z

What does the shade in You should shade the dagger.internal from one (or both) of the libraries. refers to?

To https://maven.apache.org/plugins/maven-shade-plugin/ or anything similar for your build tool (e.g. https://imperceptiblethoughts.com/shadow/ for Gradle, https://github.com/google/bazel-common/blob/c0a6655a70fb389dbb6473989450df0c86447ec3/tools/jarjar/jarjar.bzl for Bazel, etc.)
and specifically to "relocating" classes under a different package.

elye · 2018-11-05T23:02:40Z

Thanks @tbroyer. I'm using gradle (Android Studio).
From the reference, sound the shading referring to a process that's packaging the jar.

What if I don't have control over the jar where I can't make changes to it, is there anything I could do?

P/S: I tried exclude as below so we could have a single dagger version used in my app, but it is not working

implementation(group: "my.package", name: "lib-name, version: "$ver") {
    exclude group: 'com.google.dagger'
}

Can I assume if the library uses the same dagger version as mine (e.g. both of us move to 2.19), then this would not be an issue?

vovkab · 2018-11-05T23:25:29Z

I tried exclude as below so we could have a single dagger version used in my app, but it is not working

I don't think this is going to work, this is the same as just specifying latest version.

The idea is that you would create local copy of the dagger library just with the different package name, so they all could co-exist in a single project. Though I think this is only would work for your app, not sure if you can repackage 3rd party libraries (or maybe you can?).

Can I assume if the library uses the same dagger version as mine (e.g. both of us move to 2.19), then this would not be an issue?

Yes, if your app and libraries use "compatible" dagger versions it would work, they don't have to use the same version.

p.s. this is really a pain for us, if we have 20 libraries that use 20 different versions of dagger and all these dagger versions are incompatible, we would need to repackage 20 libraries and 20 versions of dagger :'(

ronshapiro · 2018-11-15T12:38:18Z

There's no guarantee that the generated code (i.e. factories) work well with each other across versions. We don't have any way of testing that. It's a generally good idea that any libraries that are intended to use Dagger in tandem are using the same version of Dagger.

If a library you don't own is passing along Dagger codegen to you and isn't documenting it, that's a bad idea on the library owners part.

In fairness, we don't really have a good way to share libraries across project owners that guaranteed support across multiple versions.

vovkab · 2018-11-15T18:51:49Z

There's no guarantee that the generated code (i.e. factories) work well with each other across versions.

They don't have to be, this is pre-generated code, the problem is in code that ships in dagger jar.

If a library you don't own is passing along Dagger codegen to you and isn't documenting it, that's a bad idea on the library owners part.

In my opinion, this really should not matter, this is an internal library code.

In fairness, we don't really have a good way to share libraries across project owners that guaranteed support across multiple versions.

We actually do, here are a couple suggestions:

Everything that gets referenced from client code back to dagger jar is a public code and should remain backward compatible. (this is where the problem right now, we keep breaking "public" code and making minor versions incompatible with each other)
Or/and - generate standalone di code. Copy any utils or classes generated code requires, into client namespace or inline them. This way we could keep generated code independent from dagger library.

vovkab · 2020-06-17T18:57:45Z

This is still an issue, for example now even google is packaging dagger2 into their libraries without creating a shadowed version:

+--- com.google.firebase:firebase-crashlytics:17.0.0
|    +--- com.google.android.datatransport:transport-backend-cct:2.2.1
|    |    +--- com.google.android.datatransport:transport-runtime:2.2.1
|    |    |    +--- com.google.android.datatransport:transport-api:2.2.0
|    |    |    \--- com.google.dagger:dagger:2.27

What this means is that if dagger library again introduce breaking change like this one, we would be stuck with 2.27 until google decide to update dagger version. What makes it even worse is that other libraries may use different versions of dagger which would be incompatible between each other.

Here is the firebase bug:
firebase/firebase-android-sdk#1677

googlebot added the cla: yes label Nov 14, 2017

ronshapiro reviewed Nov 14, 2017

View reviewed changes

Remove injectMembers method from MembersInjectors

b98f194

szymonkozak force-pushed the master branch from 2bd4b02 to b98f194 Compare November 14, 2017 21:52

Change checkInstanceNotNull to private static in MembersInjectors

6f810c7

ronshapiro reviewed Nov 14, 2017

View reviewed changes

Inline checkNottNull in MembersInjectors class

036f378

szymonkozak changed the title ~~Remove unnecessary MembersInjectors class~~ Remove unnecessary MembersInjectors.injectMembers method Nov 14, 2017

ronshapiro pushed a commit that referenced this pull request Nov 15, 2017

Remove unnecessary MembersInjectors.injectMembers method

a7a4edb

Closes #950 RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175736999

ronshapiro closed this in d61f198 Nov 15, 2017

uKL mentioned this pull request Dec 21, 2017

Can instantiate RxBleClient with last Dagger release dariuszseweryn/RxAndroidBle#342

Closed

AlexTrotsenko mentioned this pull request Jul 9, 2018

ReactiveCache is crashing when used in project with new version of Dagger2 VictorAlbertos/ReactiveCache#23

Closed

tbroyer mentioned this pull request Nov 15, 2018

Runtime exception #1335

Closed

jhansche mentioned this pull request Jun 17, 2020

Firebase In-App Messaging conflits with Dagger | Don't use dagger | Vendor dagger firebase/firebase-android-sdk#1677

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary `MembersInjectors.injectMembers` method #950

Remove unnecessary `MembersInjectors.injectMembers` method #950

szymonkozak commented Nov 14, 2017 •

edited

Loading

ronshapiro Nov 14, 2017

ronshapiro Nov 14, 2017 •

edited

Loading

ronshapiro Nov 14, 2017

szymonkozak Nov 14, 2017 •

edited

Loading

ronshapiro Nov 14, 2017

ronshapiro commented Nov 14, 2017

Tagakov commented Dec 21, 2017

ronshapiro commented Dec 21, 2017

vovkab commented Jan 24, 2018

JakeWharton commented Jan 24, 2018 via email

ronshapiro commented Jan 24, 2018

vovkab commented Jan 24, 2018

tbroyer commented Jan 25, 2018

elye commented Nov 5, 2018

tbroyer commented Nov 5, 2018

elye commented Nov 5, 2018

vovkab commented Nov 5, 2018

ronshapiro commented Nov 15, 2018

vovkab commented Nov 15, 2018

vovkab commented Jun 17, 2020 •

edited

Loading

Remove unnecessary MembersInjectors.injectMembers method #950

Remove unnecessary MembersInjectors.injectMembers method #950

Conversation

szymonkozak commented Nov 14, 2017 • edited Loading

ronshapiro Nov 14, 2017

Choose a reason for hiding this comment

ronshapiro Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

ronshapiro Nov 14, 2017

Choose a reason for hiding this comment

szymonkozak Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

ronshapiro Nov 14, 2017

Choose a reason for hiding this comment

ronshapiro commented Nov 14, 2017

Tagakov commented Dec 21, 2017

ronshapiro commented Dec 21, 2017

vovkab commented Jan 24, 2018

JakeWharton commented Jan 24, 2018 via email

ronshapiro commented Jan 24, 2018

vovkab commented Jan 24, 2018

tbroyer commented Jan 25, 2018

elye commented Nov 5, 2018

tbroyer commented Nov 5, 2018

elye commented Nov 5, 2018

vovkab commented Nov 5, 2018

ronshapiro commented Nov 15, 2018

vovkab commented Nov 15, 2018

vovkab commented Jun 17, 2020 • edited Loading

Remove unnecessary `MembersInjectors.injectMembers` method #950

Remove unnecessary `MembersInjectors.injectMembers` method #950

szymonkozak commented Nov 14, 2017 •

edited

Loading

ronshapiro Nov 14, 2017 •

edited

Loading

szymonkozak Nov 14, 2017 •

edited

Loading

vovkab commented Jun 17, 2020 •

edited

Loading