(The changes are now pushed, along with a few other changes.)
On Mon, Jun 30, 2014 at 10:06 PM, Luke Sandberg <[email protected]> wrote: > FYI: I have submitted this change to google's internal copy of guice and > it should be present at HEAD whenever the repo sync next happens. > > any update on the annotation processor? > > > > > On Fri, May 23, 2014 at 6:33 PM, Christian Gruber <[email protected]> > wrote: > >> Hey Steven - you had the processor in a git repo, right? I haven't had a >> chance to fully follow up on it, but we have takers to help get it all >> tested. Let's coordinate and get the code into some place where it can be >> worked on and tested. >> >> As to breaking changes, we already have some breaking changes if I recall >> in Guice 4, so it might be timely (though folks on this list should pipe up >> as to whether that's going to rain doom upon them. :) >> >> Christian >> >> >> On 23 May 2014, at 18:06, Luke Sandberg wrote: >> >> 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. >>> >> >> >> Christian Gruber :: Google, Inc. :: Java Core Libraries :: Dependency >> Injection >> email: [email protected] :::: 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. > To view this discussion on the web visit > https://groups.google.com/d/msgid/google-guice/CAO9V1MKOtQn7UhMfMHTY8PwWbO6baBVhBG-TDk-3zMt44UOdFA%40mail.gmail.com > <https://groups.google.com/d/msgid/google-guice/CAO9V1MKOtQn7UhMfMHTY8PwWbO6baBVhBG-TDk-3zMt44UOdFA%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/d/optout. > -- 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. To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/CAJEBNUdqS8LL2hyA%3DFW3%2BkJEdP%3DwA1HByEwFULLXQu%3DoONHb3g%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
