Skip to content
This repository has been archived by the owner on Aug 26, 2021. It is now read-only.

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

Closed
cgruber opened this issue May 30, 2013 · 7 comments
Closed

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

cgruber opened this issue May 30, 2013 · 7 comments

Comments

@cgruber
Copy link
Collaborator

cgruber commented May 30, 2013

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
Copy link
Member

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
Copy link
Contributor

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
Copy link

mikosik commented Jun 23, 2013

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
@Provides(useByDefault=true, type=UNIQUE) Set ...
just write
@Provides(overridable=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
Copy link
Contributor

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

#291

@codefromthecrypt
Copy link
Contributor

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
Copy link
Member

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//issues/261#issuecomment-22112175
.

@cgruber
Copy link
Collaborator Author

cgruber commented Aug 7, 2013

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

@cgruber cgruber closed this as completed Aug 7, 2013
tbroyer pushed a commit to tbroyer/dagger that referenced this issue Jan 21, 2016
When subcomponent simple names conflict, prepend the enclosing types (and then package levels) until a unique simple name is generated. This fixes Github issues square#194, square#236, and square#261.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=108989611
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants