google-code-export / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
2 stars 1 forks source link

InstanceBinding of HasDependencies instance doesn't return InjectionPoint Dependencies from getDependencies() #298

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
This may not be a bug, but it looks fishy enough that I thought I'd raise it.

If you bind an instance that has @Injected fields or methods, but also 
implements 
HasDependencies, the InstanceBindingImpl and ProviderInstanceBindingImpl 
classes will return just 
the instance's stated dependencies from #getDependencies().

For example, the Binding to RealMultibinder will return the dependencies on the 
Multibinder's 
bound elements, but not the Dependency on Injector, which is from the 
InjectionPoint for 
#initialize(Injector).

I think that the Binding's getDependencies() should be comprehensive, and 
return both the 
Dependencies from InjectionPoints as well as any additional Dependencies that 
the instance reports 
by implementing HasDependencies.

Original issue reported on code.google.com by phopkins on 30 Dec 2008 at 8:31

GoogleCodeExporter commented 9 years ago
This is how I'd originally coded it. But I decided that by implementing 
HasDependencies, the developer may 
want to actually *remove* dependencies from their list.

Here's why... Suppose I'm implementing Multibinder. In its implementation, it 
injects the Injector, scans 
through its explicit bindings, and picks out the ones that will go into its 
Set. So it has a very 'short lived' 
dependency on Injector, while it's finding out what its "real" dependencies 
are. I would prefer for the graph of 
the Multibinder to depend on the set's elements, but not the Injector. So now 
HasDependencies gives it an 
opportunity to 'opt-out' of its dependencies.

Similarly for assistedinject's FactoryProvider. It doesn't list the Injector as 
its dependencies, even though it 
gets injected with one.

Fortunately, the tools do have a workaround. If they need a complete world 
view, they can union the injection 
point dependencies with the reported ones.

Original comment by limpbizkit on 30 Dec 2008 at 9:58

GoogleCodeExporter commented 9 years ago
OK, makes sense. I had been starting from the InjectionPoints and rendering all 
of those Dependencies, and then 
adding in the dependencies from getDependencies().

In that case, it seems that the cleaner / expected way to work with this (in 
the context of the Grapher) would be to 
start from getDependencies(), and only render the InjectionPoints that have 
dependencies in that set. In other words, 
treat getDependencies() as an authoritative filter on the InjectionPoints.

Make sense?

Original comment by phopkins on 30 Dec 2008 at 10:15

GoogleCodeExporter commented 9 years ago
Yeah, that makes good sense. Note that when HasDependencies is overridden, the 
dependencies won't usually 
have InjectionPoints associated. I think this is okay.

Original comment by limpbizkit on 30 Dec 2008 at 10:35