xiaodududu / google-guice

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

requestStaticInjection should not imply a request for static injection on superclass members #601

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
It is surprising to me that requestStaticInjection on a class implies a request 
for static injection on its superclass(es).

It means, for instance, that if classes A, B, and C all have injected static 
members, and B extends A and C extends A, then there is no way to inject the 
static members of A, B, and C only once; the static members of A will be 
injected at least twice.

I understand this represents a change to the public API. But I think it's at 
least worth discussing.

Original issue reported on code.google.com by d...@google.com on 10 Feb 2011 at 7:31

GoogleCodeExporter commented 9 years ago
This would be a pretty large backwards incompatible change (it caused some 
problems with Gin recently, too).  I could see a helper class doing it.  
Something like:

configure() {
   requestStaticInjection(SimpleStaticInjector.class);
   SimpleStaticInjector.inject(StaticFoo.class);
   SimpleStaticInjector.inject(StaticBar.class);
}

SimpleStaticInjector {
  List<Class> injections = Lists.newArrayList();
  void inject(Class clazz) {
    injections.add(clazz);
  }

  @Inject static doInjections(Injector injector) {
    for (Class clazz : injections) {
      for (InjectionPoint ip : InjectionPoint.forStaticMethodsAndFields(clazz)) {
        // InjectionPoint's member belongs to the exact class (not a superclass)
        // then run through the logic from MembersInjectorStore.getInjectors.
        // basically: 
        // if it's a field, get the sole dependency's Key from the injector
        // and set the field with it.
        // if it's a method, get an instance of each dependency and call the
        // method with those instances.
      }
    }
  }
}

Original comment by sberlin on 12 Feb 2011 at 4:44

GoogleCodeExporter commented 9 years ago
I agree that it would be backwards-incompatible. Does that mean we're stuck 
with what seems to me at least to be surprising functionality?

What about adding (ick) requestStaticInjection(boolean injectSuperTypes, 
Class<?>... types), where the normal requestStaticInjection is equivalent to 
calling this one passing true as the first argument?

Original comment by d...@google.com on 14 Feb 2011 at 10:03

GoogleCodeExporter commented 9 years ago
I could probably be convinced, but my thoughts right now are: If the API is 
enough to let folks write a one-off extension that houses the method, I'd 
rather not change the current API.  Can you confirm that Guice 2.0 (and 1.0) 
also injected superclasses with requestStaticInjection?

Original comment by sberlin on 14 Feb 2011 at 10:11

GoogleCodeExporter commented 9 years ago
From what I can tell, Guice 2.0 also injected superclasses, but Guice 1.0 did 
not. Was that a deliberate change?

And yes, I guess someone could write that code, but it seems like a hack.

Original comment by d...@google.com on 14 Feb 2011 at 10:33

GoogleCodeExporter commented 9 years ago
By the way, my evidence that Guice 1.0 did not inject superclasses came from 
examining the code in BinderImpl.StaticInjection.createMemberInjectors, which 
just used the fields and methods returned by clazz.getDeclaredFields and 
getDeclaredMethods without traversing up the class hierarchy. I didn't test it 
by running it.

Original comment by d...@google.com on 14 Feb 2011 at 10:35

GoogleCodeExporter commented 9 years ago
The more I think about it, the more I lean to fixing this in the Guice code 
itself, since static things really shouldn't have any hierarchy at all.  Any 
other opinions/thoughts?

Original comment by sberlin on 18 Feb 2011 at 2:50

GoogleCodeExporter commented 9 years ago
At the very least, this should be documented.

Original comment by goo...@tom-fitzhenry.me.uk on 3 Oct 2011 at 10:25