New BindingTargetVisitor interface needs wildcard types

[GoogleCodeExporter]

The new BindingTargetVisitor parameters (while otherwise nice and useful) need 
wildcard types 
to be better-used with generic visitors.

For example, the following code does not compile:

Injector injector = Guice.createInjector();
for (Binding<?> binding : injector.getBindings().values()) {
  binding.acceptTargetVisitor(new DefaultBindingTargetVisitor<Object, Object>() {});
}

In addition, there's an inconsistency in ProviderBinding. ProviderBinding<T> 
implements 
Binding<Provider<T>>, which means that it must have an 
acceptTargetVisitor(BindingTargetVisitor<Provider<T>, V>). 
BindingTargetVisitor<Provider<T>> 
then has a visitProviderBinding(ProviderBinding<Provider<T>>) method. 
ProviderBinding<Provider<T>> implements Binding<Provider<Provider<T>>>, etc.

Attached a patch that adds "? extends" and "? super" in various places around 
the Binding 
implementations and BindingTargetVisitor. Also changed ProviderBinding to be:

interface ProviderBinding<T extends Binding<?>> extends Binding<T>

instead of:

interface ProviderBinding<T> extends Binding<Provider<T>>

Note, though, that this means that the #getProvidedKey method now has to return 
Key<?> 
instead of Key<T>. I think that this is acceptable losses.

Original issue reported on code.google.com by phopkins on 26 Dec 2008 at 3:41

Attachments:

• wildcard.patch

[GoogleCodeExporter]

Unfortunately, this is not what a wildcard means. When you say Iterable<?> it 
means an iterable of *any-one-
thing*. Not an iterable of *everything*.

This is an essential problem with naturally heterogenous collections (like 
Injector#getBindings) in Java. The 
language is not designed to support them. A better solution would be to provide 
a special method that 
returns Iterator<Object> over all the bindings and fall back to the 
inspect-and-cast route of yesteryear.

Weakening the specific instance method types to bounded wildcards seems like an 
imprudent trade to me.

Original comment by dha...@gmail.com on 26 Dec 2008 at 4:02

[GoogleCodeExporter]

Sorry, yes, "bounded wildcard" is the more correct term. It is certainly 
arguable that when you talk about Guice 
Bindings you may not strictly care about inheritance: a binding to String is 
different from a binding to 
CharSequence, and therefore you may want to keep the non-bounded types.

That being said, I don't see any "weakening" going on. With the "super"s and 
"extends" in the right places, you 
can still do:

injector.getBinding(String.class).acceptTargetVisitor(new 
DefaultBindingTargetVisitor<String, Object>() { ... });

and everything's still typesafe. (E.g. InstanceBinder#getInstance() returns a 
String.) You just need to put the 
right wildcard bounds in your visitor's method signatures.

At any rate, I'm running into this with the Graphviz grapher. I have a Visitor 
that needs to run over all of the 
Bindings in the Module, regardless of the type (and I suspect that this will be 
a common case for tools). I can 
do the unchecked cast to Binding<Object> and run the visitor on that, but it 
seems a bit of a hack.

Nevertheless, the inconsistency around ProviderBinding extending 
Binding<Provider<T>> is I think still a 
bug, regardless of the other typing.

Original comment by phopkins on 26 Dec 2008 at 4:43

[GoogleCodeExporter]

Patch applied. 

The ProviderBinding make javac a bit unhappy. I needed some '@SuppressWarnings' 
and raw types to work 
around. This is one of the unfortunate corner cases in the generics spec where 
only some compilers complain...

Original comment by limpbizkit on 28 Dec 2008 at 1:48

• Changed state: Fixed