On Wed, Jun 3, 2009 at 7:29 PM, Russ Rufer <[email protected]> wrote: > > Hi Jesse, > > Gmail has been bitten a few times by Guice's choice to use a no-arg > constructor when there is no @Inject annotation. There have been cases where > someone didn't realize a collaborator wasn't Guiced already, and injected it > (but there was, unfortunately, a no-arg constructor that gets used--even > though it's inappropriate for the place it's injected). > > Difficult bugs to find. Ugly post-mortems. > > Bad enough that the team would probably like the option to supress the > automated use of unannotated no-arg constructors. I think more ideal would > be to indicate in a module that one or more packages (probably recursively) > should ignore the unannotated no-arg, which would let us still use no-args > in 3rd party code without providers. > > Not sure about all this, but would you support a feature like that? Or > maybe have an alternative? > > I know Guice changed a lot since I was crawling around in it at version > 1.0, but I might be able to implement something like this without too much > hand holding. > > - Russ >
Excellent! I agree that we want this feature, and I'd love to let you do the coding! I think this idea is totally solid, and it fixes a bug<http://code.google.com/p/google-guice/issues/detail?id=342> reported by Bob! So we're all on board. You'll want to disable use of constructors not annotated "@Inject", unless the class is explicitly mentioned - either in a single bind statement or in the target clause of a linked/provider key binding: bind(Foo.class); // Foo still doesn't need an @Inject constructor bind(Foo.class).to(FooImpl.class); // neither does FooImpl bind(Foo.class).toProvider(FooProvider.class); // or FooProvider How do we configure this? This is the biggest problem. Thus far, we don't have much prior-art on how the Injector's internal rules are changed. We have a slight precedent for system properties, but this is only for mildly supported hacks: guice.allowNulls.bad.bad.bad: used to suppress failures when @Nullable is missing guice.custom.loader: used to turn off OSGi-friendly classloading I don't think we want to go this route here. We could define a full GuiceConfiguration value object, and let the user bind() that. This is tricky because we'd need to resolve that object before processing the other bindings. This becomes a problem if the GuiceConfiguration depends on other bindings: @Provides GuiceConfiguration provideGuiceConfiguration(DatabaseConnection d) { ... } Imagine how poorly defined things get if DatabaseConnection doesn't have an @Inject-annotated constructor! We could add new configuration APIs to binder(): binder().useConfiguration(new GuiceConfiguration(...)); This is concise and well-defined, but it means you've got a new Element type that needs to be wired into about ten different places. We could do something on the Guice.createInjector() factory method: Guice.createInjector(new GuiceConfiguration(...), new MyModule()); This is nice because it lets the Injector-creator specify configuration, rather than imposing it on any individual module. The only cost is that you now need two new factory methods on Guice.createInjector(). We could create a public InjectorBuilder class, that permits configuration. It would be an alternative to the Guice.createInjector() API: Injector injector = new InjectorBuilder() .production() .requireInjectAnnotation() .add(new MyModule()) .add(moreModules) .build(); I dig this, especially since it opens up the door to even more configuration options. For example, we've long wanted to split the production stage into separate aspects: eager loading and error detection. I think the builder is my preferred option. I'm interested to hear what others have to say on this. Advice for hacking on Guice: The codebase is complex, but not that complex and I've tried reasonably hard to Javadoc implementation details. Be sure to first familiarize yourself with the Guice internals<http://code.google.com/docreader/#p=google-guice&s=google-guice&t=Internals> docs (which you'll need to update after your change). If you see something that could be easily improved, improve it! Including internal documentation. There's a very small number of places where we do weird things for a performance benefit - like using arrays instead of lists. If you get the urge to fix this stuff, don't! But if it's weird and undocumented, I suspect we're missing an "// this is for performance" note. Once you've written and tested the change (I predict you'll need to change 5 or 6 files), please email me a patch. I'll review it, and then you can submit it from your own desktop. Lookin' forward to a patch! -Jesse --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "google-guice-dev" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/google-guice-dev?hl=en -~----------~----~----~----~------~----~------~--~---
