add Provides.Type.SET_VALUES

[codefromthecrypt]

fixes issue #261. Provides.Type.SET_VALUES allows default or additional contributions to Set bindings. Particularly, it allows you to specify an empty set binding without requiring downstream modules declare Module.overrides=true

Example

class TestApp implements Runnable {
  @Inject Set<String> strings;

  @Override public void run() {
    System.out.println(strings);
  }

  public static void main(String[] args) {
    ObjectGraph root = ObjectGraph.create(new RootModule());
    ObjectGraph extension = root.plus(new ExtensionModule());
    extension.get(TestApp.class).run();
  }

  @Module(injects = TestApp.class)
  static class RootModule {
    @Provides(type = SET_VALUES) Set<String> defaultToEmpty(){
      return new HashSet<String>();
    }
  }

  @Module(addsTo=RootModule.class, injects = TestApp.class)
  static class ExtensionModule {
    @Provides(type = SET) String addToSet(){
      return "contributed";
    }
  }
}

[JakeWharton]

super nit: add an extra line here :)

[cgruber]

I'd like to see SET_VALUE (singular) as a synonym for SET, and deprecate SET, as when Jesse and I talked about this idea before, we agreed that SET by itself, in the context of having SET_VALUES was ambiguous in a way that SET_VALUE wasn't.

[cgruber]

I need to dig deeper and won't have a chance until tomorrow. When I whipped up a small prototype of this for myself, I encountered a few subtleties, and I'd like to dig around a bit. Especially as regards to the case of handling providers, e.g.:

@Inject Set<Provider<String>> stringProviders

That's a direction which this may make much more difficult, but I'm not in a position to dig deeper until tomorrow.

[swankjesse]

supernit: 2013!

[swankjesse]

Wow, what a nice treat. Just a bunch of nitpicks and I think we can get this checked in!

[codefromthecrypt]

updated commit to address nits.

@cgruber I added tests to show SET_VALUES bindings of Set<A> do not impact Set<Provider<A>>. Whether or not that behavior is desired is a different question :) IMHO, multibindings shouldn't clash, particularly since they can be seeded from a combination of unique and set collaborators.

[codefromthecrypt]

ps lemme know if we want to change SET_VALUES to SET_VALUE

[swankjesse]

I would have expected this to fail because the Set has a conflicting definition. From the root it's an empty set, but in the extended graph it contains a string. These two shouldn't disagree!

[codefromthecrypt]

I thought that they should be collaborating into the same output set (ex. SET_VALUES ~ addAll while SET ~ add). Why would empty set be invalid? Without allowing empty set contribution, my original intent is thwarted...

[codefromthecrypt]

I suppose it would make more sense as Provides(type = ADD) and Provides(type = ADD_ALL) in hindsight.

[swankjesse]

No, the problem is that the root graph should be immutable. By extending it you're mutating it. That's no good.

[codefromthecrypt]

oh i see. should I attempt a copyOf on graph.plus, then?

[codefromthecrypt]

ok, so basically convert this test from one that succeeds to one that fails :)

[codefromthecrypt]

well, I guess there's one that should succeed... a second module in the same graph should succeed.

[codefromthecrypt]

OK, then I'll wait to you decide what to do with this branch. no worries if the answer is close

[cgruber]

Agree with Jesse's constraint - shouldn't be able to mutate the graph from a child - that's massive unsafety right there. Consider the leaks! :) Definitely, a second module in the same graph should succeed.

[codefromthecrypt]

adding a second integration test: multiple-modules-setvalues for this

[cgruber]

I definitely would like to see SET_VALUES, but as a synonym for SET, with SET being a deprecated annotation property (can we do that?). So we don't break people (and don't have to maek this 2.0). Let's then remove that older facility when we throw down 2.0.

I'll dig in and look at the Set<Provides> implications in your impl. Finally got done with that stupid change to classloading caching, so I'm way behind.

[swankjesse]

Overall this change is exercising a bug in our code where multibindings aren't closed to extended graphs. But I don't think it's new regression, so we should be able to commit this and fix that separately. Let's do that.

[swankjesse]

This should use the declared return type, not the instance type. Otherwise you can't use multibindings to create a Set<Set<String>>.

[codefromthecrypt]

ok adding a test for this

[codefromthecrypt]

before I tidy this up, do we want to have this process on type=SET, such as @cgruber suggests? Only impact I can see is perhaps someone who literally wants a Set<Set<Foo>> might complain as there could be ambiguity.

[codefromthecrypt]

should I leave the test as-is (exposing the bug) or rewrite it to not make
a child graph?

[swankjesse]

Leave the test as-is and we'll fix it in a follow up. I don't like using one enum for both kinds; the ambiguity is lame.

[codefromthecrypt]

ok. should be ready to go now. only change besides adding tests was the following in SetBinding

      if (contributor.provideKey.equals(provideKey)) {
        result.addAll((Set<T>) contribution);

The above ensures the nested sets don't trip up logic. Before, it used instanceof Set which @swankjesse pointed out would.

[codefromthecrypt]

nudge.. any chance of mergy releasey?

[swankjesse]

Nice! Thanks Adrian!

[codefromthecrypt]

Woot, thanks for the time reviewing, folks

On Wednesday, July 24, 2013, Jesse Wilson wrote:

Nice! Thanks Adrian!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/291#issuecomment-21479599
.