On Friday, May 23, 2014 4:20:30 PM UTC-7, Christian Gruber wrote: > > +1. I would even consider making it a warning if it isn't marked final, > to encourage people to add those constraints.
I'm slightly negative on forcing 'final' modifiers (purely due to boilerplate issues), though I'm sure i could be convinced. > There's an unfinished (it works, but is not sufficiently tested) > annotation processor for some guice static analysis, this could also be > implemented there (and have it enabled) so that some of these runtime > errors can be caught at compile-time. (It should remain a runtime error > as well in case people do not run the annotation processor / validator). > If someone does need the old behavior, they may mark the error > suppressed. > The annotation processor sounds awesome. If it is just a matter of writing tests I would be willing to offer some help to get that out the door :) > The legacy flag is not pretty, but it's certainly a viable option, > though here also I would spout a warning on startup. > > Since all these issues would be detected at configure time, we could probably just log warnings/severes if the error is disabled. I agree that adding a flag for this is kind of gross, but i assume it is neccesary for backwards compatibility. Though i don't know what the policy is for breaking changes like this. I don't think that this is a large issue (based on a cursory review of googles code base), but still it could definitely break users. Christian. > > On 23 May 2014, at 12:20, Luke Sandberg wrote: > > > While preparing a recent > > change< > https://code.google.com/p/google-guice/source/detail?r=409e0f578b620c38f6c8626dee78783219d2956e> > > > > to > > how @Provides methods are invoked, I came across some confusing > > behavior > > when @Provides methods are overridden. The initial issue i saw was > > for a > > case like this: > > > > class Super extends AbstractModule { > > @Override void configure() {} > > > > @Provides Number provideNumber() { > > return 1; > > } > > } > > > > class Sub extends Super { > > @Provides Integer provideNumber() { > > return 2; > > } > > } > > > > Injector injector = Guice.createInjector(new Sub()); > > assertEquals(2, injector.getInstance(Number.class)); > > assertEquals(2, injector.getInstance(Integer.class)); > > > > This happens because Sub.provideNumber is a covariant override of > > Super.provideNumber. So 2 provider methods are bound, but both call > > Sub.provideNumber. > > > > If instead you wrote: > > > > class Sub extends Super { > > @Provides Number provideNumber() { > > return 2; > > } > > } > > > > then Guice.createInjector will throw a CreationException for duplicate > > bindings for Key.get(Number.class). > > > > There are a lot of different combinations for how you could override > > an > > @Provides method depending on whether or not you mark the override > > with > > @Provides and whether or not you change the key for the method. This > > gets > > especially weird if you start adding scoping annotations into the mix > > (e.g. > > if the override is marked @Singleton but the parent isn't). > > Currently, the > > Guice implementation of @Provides doesn't do anything special to try > > to > > detect these cases. > > > > Proposal: > > My proposal is to make it an Error to override an @Provides method in > > all > > cases. Guice will essentially enforce that @Provides methods are > > private/final (whether or not they are actually declared as such). > > > > The goal of this proposal is to eliminate opportunities for confusion > > and > > to make modules easier to understand. > > > > FAQs > > > > Q: What about users who overrides @Provides methods for testing > > purposes? > > Guice has other mechanisms to override bindings (e.g. > > Modules.override), or > > you can structure your test so that the parent @Provides method isn't > > installed, maybe by splitting it into a separate Module class that you > > don't install in the test. Or if you really want to use method > > overriding > > you can use a template method pattern and just override a different > > method > > and have the @Provides method call that one. > > > > Q: What about migration of legacy code that uses this feature? > > I'm not sure yet. Guice has a some precedent for using java system > > properties for feature flags, so we may introduce a system property > > ('guice_allow_provides_overrides'?) to enable/disable this error. > > > > -- > > You received this message because you are subscribed to the Google > > Groups "google-guice" group. > > To unsubscribe from this group and stop receiving emails from it, send > > an email to [email protected] <javascript:>. > > To post to this group, send email to > > [email protected]<javascript:>. > > > Visit this group at http://groups.google.com/group/google-guice. > > For more options, visit https://groups.google.com/d/optout. > > > Christian Gruber :: Google, Inc. :: Java Core Libraries :: Dependency > Injection > email: [email protected] <javascript:> :::: mobile: +1 (646) 807-9839 > -- You received this message because you are subscribed to the Google Groups "google-guice" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/google-guice. For more options, visit https://groups.google.com/d/optout.
