Merge pull request #889 from tavianator/require-explicit-same-injector
Fix https://github.com/google/guice/issues/888.
diff --git a/core/src/com/google/inject/Binder.java b/core/src/com/google/inject/Binder.java
index dae3812..91f436f 100644
--- a/core/src/com/google/inject/Binder.java
+++ b/core/src/com/google/inject/Binder.java
@@ -429,8 +429,8 @@
/**
* Instructs the Injector that bindings must be listed in a Module in order to
* be injected. Classes that are not explicitly bound in a module cannot be
- * injected. Bindings created through a linked binding (
- * <code>bind(Foo.class).to(FooImpl.class)</code>) are allowed, but the
+ * injected. Bindings created through a linked binding
+ * (<code>bind(Foo.class).to(FooImpl.class)</code>) are allowed, but the
* implicit binding (<code>FooImpl</code>) cannot be directly injected unless
* it is also explicitly bound (<code>bind(FooImpl.class)</code>).
* <p>
@@ -447,10 +447,11 @@
* does, the behavior is limited only to that child or any grandchildren. No
* siblings of the child will require explicit bindings.
* <p>
- * If the parent did not require explicit bindings but the child does, it is
- * possible that a linked binding in the child may add a JIT binding to the
- * parent. The child will not be allowed to reference the target binding
- * directly, but the parent and other children of the parent may be able to.
+ * In the absence of an explicit binding for the target, linked bindings in
+ * child injectors create a binding for the target in the parent. Since this
+ * behavior can be surprising, it causes an error instead if explicit bindings
+ * are required. To avoid this error, add an explicit binding for the target,
+ * either in the child or the parent.
*
* @since 3.0
*/
diff --git a/core/src/com/google/inject/internal/Errors.java b/core/src/com/google/inject/internal/Errors.java
index f684e39..385d4b8 100644
--- a/core/src/com/google/inject/internal/Errors.java
+++ b/core/src/com/google/inject/internal/Errors.java
@@ -140,6 +140,13 @@
return addMessage("Explicit bindings are required and %s is not explicitly bound.", key);
}
+ public Errors jitDisabledInParent(Key<?> key) {
+ return addMessage(
+ "Explicit bindings are required and %s would be bound in a parent injector.%n"
+ + "Please add an explicit binding for it, either in the child or the parent.",
+ key);
+ }
+
public Errors atInjectRequired(Class clazz) {
return addMessage(
"Explicit @Inject annotations are required on constructors,"
diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java
index bcf481d..eef7d98 100644
--- a/core/src/com/google/inject/internal/InjectorImpl.java
+++ b/core/src/com/google/inject/internal/InjectorImpl.java
@@ -779,6 +779,12 @@
boolean jitDisabled, JitLimitation jitType) throws ErrorsException {
// ask the parent to create the JIT binding
if (parent != null) {
+ if (jitType == JitLimitation.NEW_OR_EXISTING_JIT
+ && jitDisabled && !parent.options.jitDisabled) {
+ // If the binding would be forbidden here but allowed in a parent, report an error instead
+ throw errors.jitDisabledInParent(key).toException();
+ }
+
try {
return parent.createJustInTimeBindingRecursive(key, new Errors(), jitDisabled,
parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType);
diff --git a/core/test/com/google/inject/JitBindingsTest.java b/core/test/com/google/inject/JitBindingsTest.java
index 10ecb33..243c53e 100644
--- a/core/test/com/google/inject/JitBindingsTest.java
+++ b/core/test/com/google/inject/JitBindingsTest.java
@@ -19,7 +19,6 @@
import static com.google.common.collect.ImmutableSet.of;
import static com.google.inject.Asserts.assertContains;
import static com.google.inject.JitBindingsTest.GetBindingCheck.ALLOW_BINDING;
-import static com.google.inject.JitBindingsTest.GetBindingCheck.ALLOW_BINDING_PROVIDER;
import static com.google.inject.JitBindingsTest.GetBindingCheck.FAIL_ALL;
import junit.framework.TestCase;
@@ -40,6 +39,14 @@
private String jitFailed(TypeLiteral<?> clazz) {
return "Explicit bindings are required and " + clazz + " is not explicitly bound.";
}
+
+ private String jitInParentFailed(Class<?> clazz) {
+ return jitInParentFailed(TypeLiteral.get(clazz));
+ }
+
+ private String jitInParentFailed(TypeLiteral<?> clazz) {
+ return "Explicit bindings are required and " + clazz + " would be bound in a parent injector.";
+ }
private String inChildMessage(Class<?> clazz) {
return "Unable to create binding for "
@@ -161,8 +168,8 @@
});
fail();
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(ScopedFooImpl.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(ScopedFooImpl.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
@@ -191,8 +198,8 @@
}).getInstance(Bar.class);
fail("should have failed");
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Bar.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
@@ -208,8 +215,8 @@
});
fail("should have failed");
} catch (CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Bar.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
@@ -223,8 +230,8 @@
}).getProvider(Bar.class);
fail("should have failed");
} catch (ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Bar.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
@@ -240,8 +247,8 @@
});
fail("should have failed");
} catch (CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Bar.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
@@ -311,8 +318,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Foo.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Foo.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
Injector child = parent.createChildInjector(new AbstractModule() {
@@ -339,7 +346,7 @@
ensureWorks(grandchild, FooBar.class, Foo.class, Bar.class);
ensureFails(grandchild, ALLOW_BINDING, FooImpl.class);
ensureFails(child, ALLOW_BINDING, FooImpl.class);
- ensureInChild(parent, FooImpl.class, FooBar.class, Foo.class);
+ ensureInChild(parent, FooImpl.class, FooBar.class, Foo.class);
}
public void testChildInjectorAddsOption() {
@@ -361,8 +368,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Foo.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Foo.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
assertEquals(totalParentBindings, parent.getAllBindings().size());
@@ -371,26 +378,11 @@
protected void configure() {
binder().requireExplicitBindings();
bind(Foo.class).to(FooImpl.class);
+ bind(FooImpl.class);
}
});
- totalParentBindings++; // creating this child added FooImpl to the parent.
assertEquals(totalParentBindings, parent.getAllBindings().size());
ensureWorks(child, Foo.class, Bar.class);
- ensureFails(child, ALLOW_BINDING_PROVIDER, FooImpl.class);
- // Make extra certain that if something tries to inject a FooImpl from child
- // that it fails, even if calling getBinding().getProvider works.. because
- // the binding is built with the parent injector.
- try {
- child.injectMembers(new Object() {
- @SuppressWarnings("unused")
- @Inject
- void inject(FooImpl fooImpl) {}
- });
- fail();
- } catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(FooImpl.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
- }
Injector grandchild = child.createChildInjector(new AbstractModule() {
@Override
@@ -400,8 +392,6 @@
});
assertEquals(totalParentBindings, parent.getAllBindings().size());
ensureWorks(grandchild, FooBar.class, Foo.class, Bar.class);
- ensureFails(grandchild, ALLOW_BINDING_PROVIDER, FooImpl.class);
- ensureFails(child, ALLOW_BINDING_PROVIDER, FooImpl.class);
// Make sure siblings of children don't inherit each others settings...
// a new child should be able to get FooImpl.
@@ -426,8 +416,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Bar.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
Injector injector = Guice.createInjector(new AbstractModule() {
@@ -464,8 +454,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(Bar.class));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
@@ -523,7 +513,91 @@
String data = injector.getInstance(String.class);
assertEquals("foo", data);
}
-
+
+ public void testJitLinkedBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(Foo.class).to(FooImpl.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), jitInParentFailed(FooImpl.class));
+ assertEquals(1, expected.getErrorMessages().size());
+ }
+ }
+
+ public void testJitProviderBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(Foo.class).toProvider(FooProvider.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), jitInParentFailed(FooProvider.class));
+ assertEquals(1, expected.getErrorMessages().size());
+ }
+ }
+
+ public void testJitImplementedByBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(ImplBy.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), jitInParentFailed(ImplByImpl.class));
+ assertEquals(1, expected.getErrorMessages().size());
+ }
+ }
+
+ public void testJitProvidedByBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(ProvBy.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), jitInParentFailed(ProvByProvider.class));
+ assertEquals(1, expected.getErrorMessages().size());
+ }
+ }
+
private void ensureWorks(Injector injector, Class<?>... classes) {
for(int i = 0; i < classes.length; i++) {
injector.getInstance(classes[i]);
@@ -539,16 +613,16 @@
injector.getInstance(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
try {
injector.getProvider(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
if (getBinding == GetBindingCheck.ALLOW_BINDING
@@ -563,16 +637,16 @@
if (getBinding == GetBindingCheck.ALLOW_BINDING_PROVIDER) {
throw expected;
}
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
} else {
try {
injector.getBinding(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), jitFailed(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
}
@@ -584,24 +658,24 @@
injector.getInstance(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + inChildMessage(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), inChildMessage(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
try {
injector.getProvider(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + inChildMessage(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), inChildMessage(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
try {
injector.getBinding(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + inChildMessage(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), inChildMessage(classes[i]));
+ assertEquals(1, expected.getErrorMessages().size());
}
}
}