toProvider() should continue to use Guice's Provider, not JSR-330's

[GoogleCodeExporter]

While the two Provider types have the same name and structure, they have 
different semantics. If the user is reading documentation and navigates to 
Provider from toProvider(), they should go to Guice's Provider. They'll likely 
get confused if they're taken to JSR-330's Provider.

Original issue reported on code.google.com by crazybob...@gmail.com on 1 Jul 2010 at 2:01

[GoogleCodeExporter]

Likewise, Guice's Provider shouldn't extend JSR-330's Provider.

Original comment by crazybob...@gmail.com on 1 Jul 2010 at 2:03

[GoogleCodeExporter]

It's going to be real hard to have "seamless" JSR 330 integration without doing 
this.

Original comment by sberlin on 1 Jul 2010 at 3:34

[GoogleCodeExporter]

Sam, can you go into more detail WRT what kinds of problems users will run 
into? Having two types with the same name can be confusing, but combining them 
causes confusion, too.

Original comment by crazybob...@gmail.com on 1 Jul 2010 at 6:10

[GoogleCodeExporter]

Sure thing.

Consider a user class with an injectable field:
   @Inject javax.inject.Provider<Foo> fooProvider;  
If c.g.i.Provider doesn't extend j.i.Provider, then in order to construct that 
field, Guice will have to do one of two things:
 a) Construct a wrapper j.i.Provider on the fly and inject that, or
 b) Change BindingImpl to cache a secondary j.i.Provider (in addition to c.g.i.Provider).

Neither is show-stopping, but both aren't that great (especially for platforms 
like Android, where GC & memory usage matter).

If toProvider required Guice's provider (or if Guice's provider didn't extend 
JSR-330s) and a user has a custom javax.inject.Provider, then using 
toProvider(Key) or (Class), or (TypeLiteral) will be very strange.  Using 
toProvider(Provider) requires wrapping the provider using Jsr330.guicify right 
now, but I don't think wrapping can be done in the key/class/typeLiteral case 
(how do you convert from one to the other?)  I think the only way to make it 
work would be to 
 a) Guice will need to add more toProvider(..) methods that take the j.i.Provider version (and they'll have to be named differently, because the erasures of the c.g.i.Provider version would conflict), or
 b) The user will need to construct their j.i.Provider (can't inject anything into the constructor) and delegate to it from another c.g.i.Provider they also will have to construct.

If anything here's wrong, please point it out!  I agree it's going to be a bad 
thing to have the Provider they're navigated to from toProvider pointing to the 
wrong provider, but I'm not sure how else this can be done.

Original comment by sberlin on 1 Jul 2010 at 7:52

[GoogleCodeExporter]

Users aren't supposed to directly implement javax.inject.Provider.

Making Guice's Provider extend JSR-330's makes things simpler for us (the Guice 
implementors), but it makes things more confusing for users. We should keep 
them separate.

Ideally, Guice would have two interfaces, Provider and Locator, and we'd have 
toLocator() instead of toProvider(). (It used to actually work like this.) It 
was a mistake to conflate the two concepts, and we shouldn't make it worse by 
tying Guice's Provider to JSR-330's.

Original comment by crazybob...@gmail.com on 1 Jul 2010 at 8:41

[GoogleCodeExporter]

I didn't know users weren't supposed to implement j.i.Provider... that does 
change my thoughts a bit.  Still, I'm not certain I agree that making 
c.g.i.Provider extend j.i.Provider makes things more confusing for users.  I 
think it actually makes it easier.

If Guice had gone the Locater & Provider route, I'd agree.  But as-is, Guice 
uses Provider, @Provides, toProvider(..), etc.. which means that Guice 
considers concept locating & providing to be the same thing.

Right now, the difference between c.g.i.Provider & j.i.Provider only matter 
when you decide what to import.  If we make it any more complex than that, 
we're adding confusion into the mix, not reducing it.  Guice 3.0 already will 
force the jsr-330 jar into the classpath, so there will always be a choice of 
Providers.. and if someone accidentally picks the wrong one, it will be hard to 
understand what's going wrong.

There's also a lot of times where you have pre-existing code that acts as a 
Provider and I want to bind it with toProvider.  If c.g.i.Provider doesn't 
extend j.i.Provider, then you're forced to make it implement c.g.i.Provider 
(whereas I could have made it extend j.i.Provider).  Couple that with other 
code that is written to be DI-agnostic and thus wants j.i.Providers injected... 
in order to test, you need to use a wrapper class that converts from 
c.g.i.Provider to j.i.Provider, adding complexity.

Ultimately, I think that making c.g.i.Provider extend j.i.Provider ends up 
making life simpler for the user.  They don't have to worry about wrong imports 
and don't have to worry about converting from one to the other.

I understand the confusion with the toProvider(Key|TypeLiteral|Class) methods 
having the generic types linking to j.i.Provider, but I feel like I'm missing 
some other confusion.  IMO, the wins for the user definitely outweigh the 
javadoc confusion (which can be cleaned up in documentation).

Original comment by sberlin on 2 Jul 2010 at 3:37

[GoogleCodeExporter]

I agree with Sam on this.

Asking users to use the c.g.i.Provider when implementing the interface but the 
j.i.Provider when injecting it is a recipe for disaster. Particularly since 
there's plenty of code like this floating around:

  class FooProvider implements Provider<Foo> {

    @Inject Provider<Bar> barProvider;
    @Inject Provider<Baz> bazProvider;

    public Foo get() { ... }
  }

It would be a huge loss to legibility if we demanded c.g.i.Provider on the 
implements clause and j.i.Provider on the injections.

The only thing that has me anxious here is the binary compatibility on methods 
like .toProvider(Provider). I personally wouldn't be offended if Binder exposed 
1 extra method for receiving the provider instance.

Ideally new users should be able to abandon the c.g.i annotations altogether!

Original comment by limpbizkit on 2 Jul 2010 at 8:25

[GoogleCodeExporter]

So, the use case is the user shouldn't have to worry about which Provider they 
import when they implement a Provider that injects other Providers.

I'm not sure this is strong enough justification for tying the Providers 
together and adding more toProvider() methods to LinkedBindingBuilder.

Do we have more use cases?

Note: Even if we add more toProvider() methods, we don't necessarily have to 
make c.g.i.Provider extend j.i.Provider.

Original comment by crazybob...@gmail.com on 2 Jul 2010 at 5:10

[GoogleCodeExporter]

It strikes me that the arguments for adding toProvider(j.i.Provider) also apply 
to adding Scope.scope(j.i.Provider) (I don't think we should do that either).

Original comment by crazybob...@gmail.com on 2 Jul 2010 at 5:13

[GoogleCodeExporter]

Other use cases include:
 - the user shouldn't have to worry which provider they implemented when they want to call bind(..).toProvider(MyProvider.class), (or new TypeLiteral<MyProvider<SomeType>)(){}, or Key.get(MyProvider.class)).
 - the user shouldn't have to worry about which provider was implemented when they want to test a class that uses Provider<Foo> in its constructor.
 - the user should be able to write all their code (except for Modules, but including custom locaters/providers) using the jsr-330 jar

I agree about the arguments for allowing toProvider(j.i.Provider) are the same 
as scope(j.i.Provider), and think it would be useful... but that binary 
compatible outweighs the importance in these cases.  Fortunately, binary 
compatible remains the same with the other changes (I think).

Original comment by sa...@google.com on 2 Jul 2010 at 8:46

[GoogleCodeExporter]

I'd like to phase out Guice's Provider, @Inject, @Named and @BindingAnnotation 
types, encouraging users to use their JSR-330 replacements. Making 
c.g.i.Provider extend j.i.Provider enables users to migrate incrementally.

Original comment by limpbizkit on 3 Jul 2010 at 2:34

[GoogleCodeExporter]

Original comment by sberlin on 23 Nov 2010 at 1:30

• Changed state: WontFix