Runtime parent bindings are requested for some compile time bound @Inject annotated classes about dagger HOT 34 CLOSED

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.

from dagger.

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.

from dagger.

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

from dagger.

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

from dagger.

@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.

from dagger.

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.

from dagger.

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?

from dagger.

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.

from dagger.

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.

from dagger.

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.

from dagger.

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.)

from dagger.

Any update for this issue?

from dagger.

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

from dagger.

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 #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.

from dagger.

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.

from dagger.

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.

from dagger.

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.

from dagger.

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?

from dagger.

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

from dagger.

@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.

from dagger.

@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.

from dagger.

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

from dagger.

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...

from dagger.

@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.

from dagger.

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

from dagger.

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. :/

from dagger.

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
.

from dagger.

@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?

from dagger.

@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!"

from dagger.

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.

from dagger.

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...

from dagger.

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

from dagger.

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.)

from dagger.

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.

from dagger.