square / dagger

A fast dependency injector for Android and Java.
https://square.github.io/dagger/
Apache License 2.0
7.31k stars 3.06k forks source link

Multibinders don't have a way to bind a default empty set #261

Closed cgruber closed 11 years ago

cgruber commented 11 years ago

Problem

If someone makes a contribution to a set via

  @Provides(type=SET) Foo provideAFoo() {...}

then someone can do this

  @Inject Set<Foo> foos;

... and all is well. But what if someone wants to inject a set of Foos, where it is not clear whether or not someone will actually bind any Foos. An example would be ServiceManager from Guava. It takes a Set<Service>, and this will fail if it is not present, but there is no way to guarantee that the empty set is to be made available even if nothing is bound.

Guice approach

  class MyModule extends Module {
    @Override void configure(Binder b) {
      Multibinder.newSetBinder(b, Foo.class); // do nothing else.
    }
  }

Guice allows the declaration of the binding, with no contents because the way bindings are registered requires the creation of the set binding first, before additional contributions are made. Dagger simply declares the contributions and implicitly creates the Set.

Possible Solutions

Default Bindings

This is something @swankjesse and I talked about a long time ago - a sort of "default binding." That is, if there is nothing else bound in its place, use this. This would work similarly to how overrides= works in a way, except that it's at the binding level, and the default is a normal binding. But it's a way of providing a precedence order for a select number of duplicate bindings. The precedence woudl naturally be bindings in overrides= modules beats other bindings, and normal bindings beat default bindings.

Note, with this, multiple unique bindings at a given level (two defaults, or two normals, etc.) would be prohibited. This would need to be rationalized with the multibinder infrastructure to ensure that an @Provides(useByDefault=true, type=UNIQUE) Set<Foo> ... could be properly unseated by one or more @Provides(type=SET) Foo ... bindings.

If we did this, we'd also want to make sure we have a decent visual representation for this in the .dot writer.

Advantage: Multibinder declaration is still type-rich (Set<Foo<Bar>>)

Multiple value muiltibinders.

This would provide some way to contribute zero or more values to the set of a multibinder, but one would contribute zero values. Some (possibly quite horrible) example might be

  @Provides(type=SET_VALUES) Set<Foo> provideFoos() { ... }

This would contribute all values of Set to the SetBinder for Set. However, one could return an empty Set. The SetBinding for Set would then be bound, and just happen to have no elements if nothing else was bound.

Advantage: Allows multiple values to be provided by a single module, computed late.

Signal for the presence of a multibinder (possibly useful for other declarations)

A signal would be made for noting the presence of a multibinder. This could be an annotation on the module

  // Limitations... can't do complex types here.
  @SetBinders(for = {Type1.class, Type2.class})
  @Declares(type=SET) Set<Foo> declareSetBinderOfFoo() { throw new AssertionError(); }

Here, this mirrors a binding, but simply declares the binding, with no contents.

Conclusion

One way or another, we need to address the default binding problem, at least for multibinders. THe three options above are actually not mutually exclusive, as they have other possible reasons for existing than declaring a multibinder for graph completeness, but they each could solve this particular problem.

My own preference is a default bindings approach. It deals with a case where people in Guice have used Optional-Injection, is a healthy (I think) way to address the use of Dagger in the context of plugin infrastructures, and lets the graph be managed flexibly.

swankjesse commented 11 years ago

I like this approach best:

  @Provides(type=SET_VALUES) Set<Foo> provideFoos() { ... }

It's especially nice 'cause we also want to support returning multiple values for other scenarios.

codefromthecrypt commented 11 years ago

I like the @Provides(type=SET_VALUES), too. Currently, I work around by having a base module just provide Set<Foo> normally with an empty set; if my contributor says overrides=true it does what it should. A scenario where we can avoid hackish use of overrides=true would be an improvement to me.

mikosik commented 11 years ago

As cgruber mentioned: 'default bindings' and overrides= are both incarnations of the same concept - defining the precedence of duplicated bindings.

Having said that it is natural to me to name both of them using the same wording. Instead of code>@Provides(<buseByDefault=true, type=UNIQUE) Set ... just write code>@Provides(<boverridable=true, type=UNIQUE) Set ...

It might seem a small change but this way it won't increase conceptual weight of the framework. There's already override concept at the modules level so it is easy to understand what it means when applied to Provides binding (lower level). I would even risk saying that implementing default binding this way is a must - you just decided it when you invented overrides= in modules ;) You just need to enable it at the lower level.

Last thing that I'm not 100% sure but that is obvious to my symmetry seeking brain is the fact that when we would have overrides= in modues and overridable= in Providers we should also have overrides= in Providers and overridable= in modules

Note a third option (which I don't like but mentioning for completeness of discussion). Just add overridable= to modules. It would be still possible to define default binding by placing it as only Provides method in overridable module. This seems awkward to me but avoids bringing override concept to Provides level - they would be only available in modules.

codefromthecrypt commented 11 years ago

submitted a PR for this, as particularly adding this takes out some ugly workarounds wrt default to empty set.

https://github.com/square/dagger/pull/291

codefromthecrypt commented 11 years ago

any chance we can get dagger 1.1 released? I'm starting to get support email which could be avoided if I can switch to SET_VALUES..

JakeWharton commented 11 years ago

I'm going to try to today. Friday got away from me.


Jake Wharton http://about.me/jakewharton

On Mon, Aug 5, 2013 at 8:05 AM, Adrian Cole notifications@github.comwrote:

any chance we can get dagger 1.1 released? I'm starting to get support email which could be avoided if I can switch to SET_VALUES..

— Reply to this email directly or view it on GitHubhttps://github.com/square/dagger/issues/261#issuecomment-22112175 .

cgruber commented 11 years ago

I'm going to close this, as it is closed by #291.