Dagger 2: Require `@Inject` constructor for provision?

[cgruber]

In Google's Dagger 2.x candidate we have a deviation from the normal assumptions about @Inject signals that we think will really clean things up, and we wanted to shop it to the main project, since it's the only substantial departure to Dagger 1.x semantics with respect to client code (that is, to non-configuration, non-module code). Namely, that for Bar to depend on Foo, Foo has to have an @Inject constructor in all cases, even if it uses members-injection.

context

One piece of context for this question is that in our 2.0 candidate, we've separated the notion of a provision binding from members-injection. Provision bindings (@Inject constructor or @Provides method) result in a Provider<T> being bound to the key, but members-injection (@Inject on a field or method) results in a MembersInjector<T> being bound. A class may have both a Provider<T> and a MembersInjector<T> if it is both instantiable by Dagger but also needs field/method injection. In that case, the Provider<T> concrete type depends on the MembersInjector<T>.

Because of this, it is vastly simpler, clearer, and lines up the developer's expectation of the generated code with the client code.

where it matters

The sticking point is the current Dagger behavior where a class can be both members-injectable and providable if it has an @Inject field, but a default or accessible no-args constructor, like so:

class Foo {
  @Inject Bar bar;
}

The above can be depended upon, where in Dagger 2, we are proposing that it could not - it would be suitable for members injection but not provision. To have it be provided (to dependents) it would have to add an @Inject constructor like so:

class Foo {
  @Inject Bar bar;
  @Inject Foo() {}
}

practicalities

For things like Activities, this is fine, since we won't be making Provider<? extends Activity> ever, since they can never be instantiated by Dagger anyway. They are injectable, and they will have a MembersInjector<T> created for them. But it adds a small amount of boilerplate to classes that were using field injection for convenience, readability, or possibly functional reasons, but didn't add a no-args @Inject constructor. In Dagger 2, we propose that they must, and that we never generate a Provider<T> (and therefore they will be invalid as a target for a dependency) unless they do so.

Non-dagger-constructable types can always, as now, be provided via @Provides methods, so this change is quite aside from that.

conclusion

We think this is a net improvement, conceptually, aligning the signals with the semantics a little more. It also cleans up the implementation of the generators and analyzers.

[cgruber]

@swankjesse, @JakeWharton, and others - please take a look.

[JakeWharton]

From what a quick grep has shown, we don't explicitly use the implicit type provider as far as I can tell. I know that we've always educated that a @Provides or @Inject-annotated constructor was required for downstream injection. If we do have any lingering uses, they are accidental.

I'm fine with this behavior change. It makes the behavior match what I have been telling people the behavior already was. And I would hope the use of this behavior is slim (to none) anyways.

[codefromthecrypt]

It will feel a little boiler-platey or mysterious to folks who don't understand, or are reading javax.inject.Inject javadoc which says it is optional. That said, if not too expensive to do, perhaps a special error message when there are members with Inject but no Inject ctor could do the trick.

At any rate, I'm in favor of the change. -a

[swankjesse]

I don't feel too strongly. The constructor feels a little boilerplatey; and it provides a small amount of additional safety.

[JakeWharton]

Is this really an anti-pattern? Can you provide insight into usages of field injection and downstream constructor injection (using the no-arg constructor) in your code? On May 29, 2014 9:01 PM, "Jesse Wilson" notifications@github.com wrote:

I don't feel too strongly. The constructor feels a little boilerplatey; and it provides a small amount of additional safety.

— Reply to this email directly or view it on GitHub https://github.com/square/dagger/issues/412#issuecomment-44613085.

[swankjesse]

In Cash we use DI even in our tests. We never execute the constructors directly. Which means field injection works great for us.

We don't end up with constructors so declaring a constructor just so that I can annotate it adds boilerplate.

[cgruber]

Admittedly one line of boilerplate which clarifies the implementation.
I think you (jesse) and I had a conversation in waterloo about being ok with a small amount of boilerplate to crisp things up and make the code cleaner and things simpler to understand, no? ;)

That's not to say that this, without question is the right mix of boilerplate for clarity - but I think it's a decent trade off. In particular, as I mentioned, I like the simple alignment of the signal with the semantic - if it has an @Inject constructor, you can depend on it. If not, you must have an @Provider. If not, it's not a dependency, though it could be an entry point.

[roman-mazur]

Can annotation of default constructor be replaced with class annotation? Like

@Inject class Foo {
  @Inject Bar bar;
}

Now implicit constructor is not needed and Foo is clearly marked as what we can depend on, isn't it?

[tbroyer]

@cgruber I quickly looked at the generator code (in cgruber/dagger:fix2, which doesn't build BTW), and it would seem quite easy to handle the default constructor case (refactor InjectProcessingStep a bit to call ProvisionBinding.Factory.forNoArgsConstructor when there's no @Inject-annotated constructor, it would a priori not complicate the code substantially); so I must ask: what's the rationale for the deviation from spec?

If you really want it, then maybe, at the cost of a small additional complexity, there could be a option to turn it on; e.g. call javac with -Adagger.requireInjectConstructor to suppress the generation of the ProvisionBinding for default constructors.

In case that behavior is adopted, would the following work as a workaround:

@Provides CoffeeMaker provideCoffeeMaker(MembersInjector<CoffeeMaker> membersInjector) {
  CoffeeMaker coffeeMaker = new CoffeeMaker();
  membersInjector.injectMembers(coffeeMaker);
  return coffeeMaker;
}

[cgruber]

@tbroyer - fix2 is my branch, and I've adapted some of that code to other changes in review internally (plus other changes from Greg pending). I would only trust google/dagger/tree/master or active pull requests for the current code.

That said, it's not impossible, as you point out, it's just more complex and since we're splitting up the role of "can be depended on" (gets a Factory/Provider) and "gets stuff injected into its members" (MembersInjector) it seemed like a good line to draw, though as demonstrated above, it's not without question.

@roman-mazur - I think annotating the class itself would violate the spec, whose javadoc for @javax.inject.Inject says "Identifies injectable constructors, methods, and fields."

[gk5885]

@roman-mazur, to follow up, the @Inject annotation cannot be applied to types. The target is declared as: @Target(value={METHOD,CONSTRUCTOR,FIELD})

@tbroyer, it's not a matter of whether or not you can support it. I have something locally that adds support, but the question is really whether or not we should support it. Since I convinced @cgruber of this originally, let's see if I can lay out the reasoning again.

Let's say that you want to figure out whether or not something can be provided. Here's what you have to check right now:

1. Is it explicitly configured (@Provides method)?
2. Is there an @Inject constructor?
3. Does it have a default constructor? If so:
   1. Is there an @Inject field somewhere on the class?
   2. Do any of its supertypes have @Inject fields?

So, given the rules that we have, this can't be depended upon:

class Foo {}

But, this may or may not depending on whether or not Bar (or any of its supertypes) has an @Inject field:

class Foo extends Bar {}

In the worst case, in order to figure out whether or not you can depend on a class you have to look at its constructors, its fields and all of the fields in each of its supertypes. Gross.

With the proposed change, the flowchart now looks like this:

1. Is it explicitly configured (@Provides method)?
2. Is there an @Inject constructor?

The other consideration ends up being how much boilerplate this adds in the common case. I suspect that it doesn't actually end up being that bad in practice. I'm hoping callers are either using members injection for instances created by some other means or just mixing in injected fields in instance that they expect to be provided by the graph. It would seem strange to have a single type be both graph-managed and not in the same application. (We don't want to preclude that, but I don't want to design for it either…)

In the case where we're using members injection on container-managed instances or whatnot, we won't have to add the @Inject constructor. Great. No issues there.

In the case where we're putting @Inject on fields for types that are intended to be provided, you would have to add the @Inject constructor. I think this actually has a couple of benefits:

1. It makes the intended usage explicit. No more guessing at whether or not the type is expected to be in the graph.
2. Requiring the constructor is a gentle nudge in the direction of constructor injection and final fields. I.e. if you have to make a constructor anyway, might as well use it. Guice has had this advice in the wiki for quite some time.

For those that really like the brevity of field injection, we're really only requiring one extra line. Given all of the other considerations, that seems like a reasonable concession.

[cgruber]

When greg says "In the case where we're using members injection on container-managed instances or whatnot, we won't have to add the @Inject constructor. Great. No issues there." above, he means things like Activity where we don't control instantiation, and provision isn't required (entry point or graph root vs. dependency, in effect). In that case we don't need @Inject constructors.

[roman-mazur]

As for me @Inject on a constructor does not really represents what is happening... When I look at the code

class Foo {
  final Bar bar;
  @Inject Foo(Bar bar) { this.bar = bar; }
}

I interpret it like "Use constructor to satisfy Foo's dependencies".

But in your case of

class Foo {
  @Inject Bar bar;
  @Inject Foo() { }
}

@Inject on constructor really means "Foo can be provided with default implementation", an absolutely different thing!..

As far as I understand the minimal is to have @Inject on fields/constructor to indicate what dependencies must be satisfied, and @Provide methods to specify how to get those dependencies. Obviously it would be nice to avoid boilerplate like

@Provide Foo foo() { 
  Foo foo = new Foo();
  inject(foo);
  return foo;
} 

And when you use

class Foo {
  @Inject Bar bar;
  @Inject Foo() { }
}

to avoid the bigger boilerplate above, @Inject has more to do with @Provide then with initial @Inject... Because now it answers the question "how". And meanings of @Inject on the constructor and on the field are completely different.

What about

@Provided class Foo {
  @Inject Bar bar;
}

Yes I'm again about annotating classes, and this time introducing a new annotation :). But I hope my reasoning makes sense...

[tbroyer]

OK @gk5885, I think you convinced me ;-) (the bit about parent classes)

[gk5885]

@roman-mazur, I don't think that your interpretation of javax.inject.Inject matches its specification. Constructor, field and method injection are to happen in sequence.

In this example,

class Foo {
  @Inject Bar bar;
  @Inject Foo() { }
}

the constructor is to be injected (with no dependencies), followed by the bar field. That is the only compliant implementation.

[roman-mazur]

Thanks, I see my mistake.

[relgames]

Hi @gk5885, you mention @Inject specification http://docs.oracle.com/javaee/6/api/javax/inject/Inject.html

And it clearly says: @Inject is optional for public, no-argument constructors when no other constructors are present. This enables injectors to invoke default constructors.

I think you are mixing 2 different actions together: 1) Make class providable (a candidate for instantiation), and 2) Actually instantiate a class, injecting it's dependencies: constructor (if exists), then fields, then methods.

@Inject annotation defines dependencies for a constructor, field or method. It makes little sense to use it for non-argument constructor (because there are no dependencies to be injected), and that's why the specification says it is optional.

[gk5885]

@relgames, no one disputes that an injector may use public, default constructors. But, as is the case in Guice when you invoke Binder.requireAtInjectOnConstructors(), it is valid for an injector to choose not to. Much the same way that @Inject "may apply to static as well as instance members," that behavior isn't necessarily supported -- that particular bit is actually a called out specifically as a parameter in the tck. Basically, we're interpreting that bit of the specification to mean that the behavior is enabled, but not mandated.

That said, much of the work that we've done with Dagger 2 has eliminated many of the surprises and pitfalls that we were used to seeing with reflectively built graphs, so I would be open to reexamining the tradeoffs between safety and brevity in this case. I suspect that there will always be some number of restrictions on what we allow to be injected because the full universe of injectable types per the specification includes String, Properties, Thread, etc., for which the default constructor is never the correct thing. But, a discussion about whether or not this particular tradeoff in the general is still warranted would be worth raising an issue there.

[relgames]

@gk5885 I think I see your point.

I was little bit confused by using @Inject for allowing other classes to inject a class. I came from a Spring world where multiple annotations exist: @Bean for explicitly instantiated classes (IMO it's the same as @Provides), @Autowired (@Inject) and @Service for declaring a bean valid for instantiation and DI.

So in Dagger, @Inject is used where in Spring I'd use separate @Autowired and @Service.