Runtime parent bindings are requested for some compile time bound @Inject annotated classes

[rwinograd]

I have found a few cases in which start up will take longer than necessary due to dagger reflectively analyze the dependencies of a class.

Two of the cases:

1. Abstract classes - I have a few abstract classes that have no injected fields. I can't annotate the constructors for these classes with @Inject (dagger won't compile the class if I do) and so I don't know of a way that I can get the annotation process to create an $InjectAdapter class for the abstract base class. So, instead, at runtime I have to reflectively evaluate the inheritance chain when I try to inject an instance of the impl.

2. Classes that were not compiled using the annotation processor - A few times I use a framework for FrameworkClass and want to inject an instance of MyFrameworkClass (which extends FrameworkClass). FrameworkClass was not written with a DI framework in need and thus I know that there is no need to inject into that class. However, the same issue as above applies - the compile time binding for MyFrameworkClass requests a binding for the supertype.

---

In general it would be nice if there was a way to opt out classes from needing bindings. For example:

@Module {
  entryPoints = {}...
  noBindingsNeeded = {FrameworkClass.class}
}

Alternatively we could also annotate the class (but I would prefer the above, since to me this is analagous to requesting static injection and belongs on the Module)

@NoSuperInject
class MyFrameworkClass extends FrameworkClass {

}

[swankjesse]

Yeah, this is a known limitation. My expectation is that it's rare enough that the reflection fallback isn't going to be that much of a performance lose.

[cgruber]

On 6 Mar 2013, at 4:34, Jesse Wilson wrote:

Yeah, this is a known limitation. My expectation is that it's rare
enough that the reflection fallback isn't going to be that much of a
performance lose.

I have teams that will not use the reflection back-end, so I will need
to deal with these… but they will be in a position to annotate the
parents. We should be able to generate a no-op binding for the parent,
though.

How does dagger complain when you @Inject mark the parent constructor?
Why won't it compile? That seems weird to me, as described.

[tbroyer]

Abstract class XXX must not have an @Inject-annotated constructor.
Source:

dagger/compiler/src/main/java/dagger/internal/codegen/InjectProcessor.java

Line 169 in 6024e62

error("Abstract class " + type.getQualifiedName()

[cgruber]

Ah. Wait... pure abstract? Sorry. My bad. But we can generate a parent adapter if it needs field injection. That's worth fixing.

[rwinograd]

@cgruber so to be clear you guys can work (at some point) to fix this?

Out of curiosity, what is the reason that having an @Inject annotated constructor on an abstract class is an error? Obviously we can't create a new instance of that class via new (or via its constructor), but I don't see why we can't still create a Binding class that doesn't implement the Provider interface.

With all that being said, I still do think it is weird that you have to compile the base class with the annotation processor on the classpath or you get a runtime performance hit . It serves as an additional barrier for using any inheritance based framework.

[cgruber]

Still in transition. The reflection hit is because we use reflection as
a back-up for when you can't generate code for a given part of the
graph. The canonical use-case is frameworks over whose source you have
no control. We can do better, and we're looking at a
just-before-packaging approach for 2.0 that will be more comprehensive
and possibly can optimize some steps. But we can do some incremental
improvement here.

[rwinograd]

Is that incremental improvement that we can create $InjectAdapters for abstract classes with an @Inject constructor? Can we just remove the line that causes an error when an abstract class has an @Inject constructor?

[cgruber]

On 6 Mar 2013, at 13:46, rwinograd wrote:

Is that incremental improvement that we can create $InjectAdapters for
abstract classes with an @Inject constructor? Can we just remove the
line that causes an error when an abstract class has an @Inject
constructor?

I don't know, at time of this writing, if that's the solution. It may
be, or it may be that we simply generate the concrete types'
InjectAdapters to be wise enough to find parent fields and inject them.
That is problematic for package-friendly visibility, but I just want to
point out that there are a few ways to fix this, and I want to think
through the implications.

[rwinograd]

Just looked at the annotation processor library a bit more - previously didn't know about the Element#getEnclosingElements() and Types#asElement(TypeMirror) methods. I thought you guys were given a very limited subset of information from the annotation process, rather than having more freedom via these methods.

Thanks for the quick responses I don't mean to spam you guys, I'm just interested in the problem/solution.

[tbroyer]

Do we really want to somehow generate those no-op $InjectAdapters? If so, when should it be done? (which annotation processor? given that we're talking about a class that is not annotated)

A quick workaround is to write the class by hand, which is real easy:

import dagger.internal.Binding;
import dagger.internal.Keys;

public final class FrameworkClass$InjectAdapter extends Binding<FrameworkClass> {
  public FrameworkClass$InjectAdapter() {
    super(null, Keys.getMemberKey(FrameworkClass.class), NOT_SINGLETON, FrameworkClass.class);
  }
}

That's it!

It ties you to Dagger internals, but it's IMO an acceptable trade-off. We might want to provide a simple NullInjectAdapter to make it simpler and less dependent from Dagger internals:

package dagger;

import dagger.internal.Binding;
import dagger.internal.Keys;

public abstract class NullInjectAdapter<T> extends Binding<T> {
  protected NullInjectAdapter(Class<T> type) {
    super(null, Keys.getMemberKey(type), NOT_SINGLETON, type);
  }
}

That way, one could easily make a dagger adapter artifact for a framework, that provides all those NullInjectAdapters.

That said, I quite like the @NoSuperInject or noBindingsNeeded proposals (with better names though). The former would remove the supertype binding from the generated $InjectAdapter, while the latter would generate an $InjectAdapter like the one above.

[swankjesse]

I don't think we need @NoSuperInject, especially if we can later implement full graph codegen. (I don't like the annotation because it doesn't have any behavior impact.)

[rwinograd]

Any update for this issue?

[swankjesse]

No update. What's the concrete problem you're facing?

[rwinograd]

Based off of our conversation at GoogleIO, I'd like to reopen this.

The concrete issue we are facing is that if we are trying to transition a multi-package application to use dagger over guice. We have many abstract framework classes that don't have any fields that need to be injected, while the child classes have @Inject constructors. With this situation, an InjectAdapter is never created for the abstract class but one is created for the child object that will try delegating out to the parent InjectAdapter; the net effect is that when we go to create an instance of the child class we needlessly use reflection and get a performance hit.

The way I see it, we have a few options:

1. As mentioned by @tbroyer above, we could create our own internal InjectAdapter for the class (I believe that we now have to name that object $InjectAdapter, rather than just InjectAdapter based off of Use two dollar signs between class name and dagger-generated suffix. #224 - please correct me if I'm wrong)
2. We could also add a do nothing field to the base class that needs to be injected

We would like a simple way to generate an InjectAdapter for this class so that when we go to inject into the child class, we don't fail while looking up the binding for the parent class. Ideally we'd like to be able to just construct an InjectAdapter that doesn't rely on delegating out to another InjectAdapter for the parent class, but if that is not possible for now it would be nice if we could:

1. Allow us to annotate abstract class's constructor with @Inject. Is there an actual issue with being able to annotate the abstract class's constructor with @Inject? I can't think of a reason for the compiler to throw an exception when you encounter an abstract class with a @Inject annotated constructor...
2. Allow us to annotate a class with something that indicates that we want you to generate an InjectAdapter for it (even though it doesn't need one by your normal definitions).

Personally, I'd prefer 1.

[cgruber]

I prefer the option where we don't generate an adapter for parent classes unless the parent has injectable fields (which we then must do to work around visibility issues). But if the parent has no such injectable fields, our concrete class' adapter shouldn't try to delegate to a non-existent parent. I think we can be a little smarter about discovery.

[cgruber]

Though as a hedge, #1 above seems workable. If you label your abstract class with @Inject on its constructor (and it has no injectable fields), we generate a no-op adapter. That would be the shortest path, though not the best path, I think.

[swankjesse]

I talked to Bob. We don't want @Inject on abstract classes 'cause it violates JSR-330.

Another option: some mode in Dagger where reflection is disabled, and if a generated inject adapter doesn't exist then it is assumed the class has no annotations.

[cgruber]

Hmm. Hadn't thought of the JSR violation. :/

I'm already implementing that in effect in google/dagger, via the plugin choices, but in my naive approach it would blow up. I think we really need to generate concrete-class adapters which don't delegate to the parent if there is no need to (that is, there is nothing injectable about the parent(s).

Any reason we can't know that at processor time?

[swankjesse]

We can't do this at processor time because we don't know the superclass won't later gain any @Inject annotations.

[rwinograd]

@cgruber - totally agreed that it would be better to not pollute our application with no-op adapters (doubly true because of dex method limits), i'm just trying to find a hedge.

@swankjesse - a mode without runtime reflection would be greatly appreciated. Off the top of my head I can't think of anything for our use cases where that fallback wouldn't work.

For my own knowledge, what part of JSR-330 is violated? Only thing I could see was: "@Inject can apply to at most one constructor per class.", but we are allowed to have a non abstract class have an @Inject constructor and a child class that also has an @Inject constructor.

[cgruber]

@swankjesse - I see your point. I can see the value in requiring that people update their generated adapters when they update their dependencies, but we have no guarantees there... unless we do the full-graph generation as a separate step. Hmm.

I agree with @rwinograd, that I don't see the JSR violation here. That said, I could see having a marker annotation that we treat like a special-cased @Inject we could put on abstract classes. Yes, it's not behaviour changing, but it is a signal, that it participates in injection, and is a signal to generate adapters. It seems like the least destructive hedge other than allowing @Inject onto abstract classes.

[swankjesse]

Let's just make a mechanism to do dagger without any reflection. It's the racing version of Dagger.

[rwinograd]

In practice with Android you do have a packaging step though that everything has to go through before deployment anyways - you could move the creation of the InjectAdapters, etc... to be part of the creation of the final APK.

To me this actually makes more sense anyways, since I'm not really convinced that InjectAdapters should be created within (or included with) library jar files anyways. Seems to me that it should be up to the final consumer of those libraries to use dagger, rather than the creator of the libraries.

I think that means you'd have to stop using the annotation processor...

[swankjesse]

@rwinograd yup, totally agree that application packaging time is the right time to do Dagger code gen. And also agree that annotation processors don't have that opportunity. At the moment I'm contemplating making a tool that'll do this.

[rwinograd]

I'm a fan of having the code generation done as part of the packaging step for a few reasons:

1. It wouldn't use the annotation processor which doesn't seem to be incredibly well supported
2. It would allow for more extensive optimizations of the InjectAdapters since the entire world is known at that point
3. It would enable an easier transition for those of us with projects that have libraries that are JSR-330 compliant, since less packages need to be transitioned over

[cgruber]

Absolutely, to the issue of whole-graph codegen, we would need a lot more infrastructure for class structure analysis, and we would lose out on some information not stored in the .class files (param names, etc.). Jesse and I have talked about this before as the eventual direction of Dagger.

As to having a non-reflection mode, I'm not opposed - quite happy for it, in fact. I have been wondering about programming styles for it. Was toying with something to expose the plugins (we need it potentially in google), like:

ObjectGraph.using(plugins).create(modules);

With some constants for common plugin sets. (DEFAULT=Loader,Reflective : LOADER=Loader,AbstractHedge)

But this was intended for those who also may need to do funkier things in plugins inside Google. It might not be the right way to expose this for the average case (Though maybe under the hood, it's done that way)

It might be time to revisit the injector builder idea:

ObjectGraph.withoutReflection().create(modules);

... or some such. All graph context configuration like this would happen at the root, so .plus() extended graphs all share one plugin context. Whether we implement it by means of plugin sets or otherwise is then behind the line.

What I'm not sure how to do is to have this thing handle no-adapter parents without using reflection to detect that this is, indeed, a no-adapter, non-injectable parent that should just be ignored, except to silently succeed in that case. But silently succeeding would be the same code flow as a parent that needed an adapter but one was never generated. No way to catch the error. :/

[codefromthecrypt]

Fwiw I think we probably need using(plugins) as a separate issue. I
recently found an error in some leaves in my build which didn't use the
compiler at all. I'd love to at least in a unit test take out the ability
to use the reflective plugin.

On Wednesday, May 22, 2013, Christian Edward Gruber wrote:

Absolutely, to the issue of whole-graph codegen, we would need a lot more
infrastructure for class structure analysis, and we would lose out on some
information not stored in the .class files (param names, etc.). Jesse and I
have talked about this before as the eventual direction of Dagger.

As to having a non-reflection mode, I'm not opposed - quite happy for it,
in fact. I have been wondering about programming styles for it. Was toying
with something to expose the plugins (we need it potentially in google),
like:

ObjectGraph.using(plugins).create(modules);

With some constants for common plugin sets. (DEFAULT=Loader,Reflective :
LOADER=Loader,AbstractHedge)

But this was intended for those who also may need to do funkier things in
plugins inside Google. It might not be the right way to expose this for the
average case (Though maybe under the hood, it's done that way)

It might be time to revisit the injector builder idea:

ObjectGraph.withoutReflection().create(modules);

... or some such. All graph context configuration like this would happen
at the root, so .plus() extended graphs all share one plugin context.
Whether we implement it by means of plugin sets or otherwise is then behind
the line.

What I'm not sure how to do is to have this thing handle no-adapter
parents without using reflection to detect that this is, indeed, a
no-adapter, non-injectable parent that should just be ignored, except to
silently succeed in that case. But silently succeeding would be the same
code flow as a parent that needed an adapter but one was never generated.
No way to catch the error. :/

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/177#issuecomment-18292771
.

[rwinograd]

@cgruber: I may be misinterpreting this, but your concern is that there is no way to differentiate between the cases where missing a parent adapter is the desired behaviour versus the case where missing the parent adapter is an error?

[cgruber]

@rwinograd - yes, that's my concern. My build system failed (or I misconfigured) and so my code gen didn't happen, or didn't update, or some edge case. I won't know that it failed if we don't expect a parent's binding and simply let it go. Indeed, I'm not sure what the logic of that would look like - "if parent exists and is injectable, look it up, else don't." Especially if the parent wasn't part of that round of processing.

I mean there must be away, but it's not coming to me without some further investigation. But in the short term, how would we signal "hey, don't worry about it? No binding adapter? No big deal!"

[rwinograd]

I would assume that the RuntimeAggregatingPlugin would be configured with a FailSafePlugin that returns a no-op instance of Binding and logs the fact that no Binding was found for that key. At which point it should be clear from the logs that some classes did not have Binding objects created.

[cgruber]

But clear from the logs means no failure - just silent pass-through. Is that good enough? I don't think so. :(. hmm...

If it weren't for an un-injectable parent being turned, in a separate library, into an injectable parent, (per jesse's concern) I'd say just test for the parent's injectability and re-gen the bindings. Even more hmm...

[rwinograd]

Can we go into more detail about why the abstract class cannot have the @Inject annotation on the constructor?

[cgruber]

It's a sort of nonsense signal, that we need for code-gen, but
reflection-only JSR-330 implementations don't, and it is nonsense in
their context, since you can't construct an abstract class. The signal
doesn't help with determining dependencies, since the framework doesn't
invoke the abstract constructor, the child does, and is the only one
that can. At best, allowing it would result in code that is ambiguous
when applied to other frameworks, and could lead to mistaken impressions
of what's happening under the hood.

And there is no actual need for a parent to be injectable if it has no
injectable members. Just logically, it's a false signal. So we should
handle such parents more gracefully without requiring that they add
extra signals for dagger. Especially since it is not unlikely that such
parents will be dagger-agnostic frameworks (such as Android activities,
etc.)

[cgruber]

So, @sgoldfed introduced pull request #294 to address this, but this was seen as problematic for reasons described in that change. However, that change will be going in the google fork of dagger (http://github.com/google/dagger) per the discussion on that P/R. Once we get that in, and sync everything up, we'll release an equivalent version of dagger (I'm guessing 1.1) under the maven artifact com.google.dagger:dagger:jar:1.1.0 and this should address this issue, with the caveat that incremental compilation is broken in IntelliJ in such a way that, if you start modifying the parent types, the code-generation may not re-generate.

In the mean-time, I'm closing this as, prior to doing whole-graph code generation, we won't be fixing it in square/dagger.