Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
…ing assistedinject fail if the target implementation class has a scop

ing annotation on it.  Scope annotations on assistedinject targets were always ignored by Guice, and allowing them on the classes led to lots of confusion when reading code.  The new behavior makes for much more readable code.

This could potentially cause runtime errors at injector creation time if you accidentally had scoping annotations on the implementation class.  The fix is just to remove that scoping annotation -- there will be no change in behavior, because the scope was ignored.

There is an extreme edge case where this change may cause a problem, but it creates sufficiently confusing code that we are OK with turning it into a failure: You used assistedinject yet had no assisted parameters, and sometimes injected the object directly and other times constructed it through the factory.  When injecting directly, it would adhere to the scope, but when constructing through the factory it would create a new instance every time.  ... If you *really* wanted this behavior, the workaround would be to bind using toConstructor in Scopes.NO_SCOPE to a named(unscoped) version of the class, which is also more expressive.  (But, more often than not, what you really wanted was to *not do this*.)

------------------
Manually Synced.
COMMIT=43242119
  • Loading branch information
cgruber committed May 16, 2013
1 parent 94c2d2c commit 45d86df
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,15 @@ public TypeLiteral<?> getImplementationType() {
if(implementation == null) {
implementation = returnType.getTypeLiteral();
}
Class<? extends Annotation> scope =
Annotations.findScopeAnnotation(errors, implementation.getRawType());
if (scope != null) {
errors.addMessage("Found scope annotation [%s] on implementation class "
+ "[%s] of AssistedInject factory [%s].\nThis is not allowed, please"
+ " remove the scope annotation.",
scope, implementation.getRawType(), factoryType);
}

InjectionPoint ctorInjectionPoint;
try {
ctorInjectionPoint =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -462,16 +462,22 @@ public interface NotHidden {
}

public void testSingletonScopeOnAssistedClassIsIgnored() {
// production stage is important, because it will trigger eager singleton creation
Injector injector = Guice.createInjector(Stage.PRODUCTION, new AbstractModule() {
try {
Guice.createInjector(new AbstractModule() {
@Override
protected void configure() {
install(new FactoryModuleBuilder().build(SingletonFactory.class));
}
});

SingletonFactory factory = injector.getInstance(SingletonFactory.class);
assertNotSame(factory.create("foo"), factory.create("bar"));
fail();
} catch (CreationException ce) {
assertEquals(1, ce.getErrorMessages().size());
assertEquals("Found scope annotation [" + Singleton.class.getName() + "]"
+ " on implementation class [" + AssistedSingleton.class.getName() + "]"
+ " of AssistedInject factory [" + SingletonFactory.class.getName() + "]."
+ "\nThis is not allowed, please remove the scope annotation.",
Iterables.getOnlyElement(ce.getErrorMessages()).getMessage());
}
}

interface SingletonFactory {
Expand Down

0 comments on commit 45d86df

Please sign in to comment.