Fix https://github.com/google/guice/issues/888.

When Binder.requireExplicitBindings() is in effect, don't allow any
JIT bindings to be created in parent injectors, even JIT bindings that
are normally exempt such as the targets of linked key bindings.

3 files changed, 141 insertions, 60 deletions

diff --git a/core/src/com/google/inject/internal/Errors.java b/core/src/com/google/inject/internal/Errors.java
index f684e39b..65f7e0b5 100644
--- a/core/src/com/google/inject/internal/Errors.java
+++ b/core/src/com/google/inject/internal/Errors.java
@@ -140,6 +140,10 @@ public final class Errors implements Serializable {
     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.", 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 bcf481d2..e20035ac 100644
--- a/core/src/com/google/inject/internal/InjectorImpl.java
+++ b/core/src/com/google/inject/internal/InjectorImpl.java
@@ -101,7 +101,7 @@ final class InjectorImpl implements Injector, Lookups {
     NO_JIT,
     /** allows existing just in time bindings, but does not allow new ones */
     EXISTING_JIT,
-    /** allows existing just in time bindings & allows new ones to be created */
+    /** allows existing just in time bindings & allows new ones to be created in the current injector */
     NEW_OR_EXISTING_JIT,
   }
 
@@ -779,10 +779,14 @@ final class InjectorImpl implements Injector, Lookups {
       boolean jitDisabled, JitLimitation jitType) throws ErrorsException {
     // ask the parent to create the JIT binding
     if (parent != null) {
-      try {
-        return parent.createJustInTimeBindingRecursive(key, new Errors(), jitDisabled,
-            parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType);
-      } catch (ErrorsException ignored) {
+      if (jitDisabled && jitType == JitLimitation.NEW_OR_EXISTING_JIT) {
+        throw errors.jitDisabledInParent(key).toException();
+      } else {
+        try {
+          return parent.createJustInTimeBindingRecursive(key, new Errors(), jitDisabled,
+              parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType);
+        } catch (ErrorsException ignored) {
+        }
       }
     }
 
diff --git a/core/test/com/google/inject/JitBindingsTest.java b/core/test/com/google/inject/JitBindingsTest.java
index 10ecb339..fd610219 100644
--- a/core/test/com/google/inject/JitBindingsTest.java
+++ b/core/test/com/google/inject/JitBindingsTest.java
@@ -19,7 +19,6 @@ package com.google.inject;
 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 @@ public class JitBindingsTest extends TestCase {
   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 @@ public class JitBindingsTest extends TestCase {
       });
       fail();
     } catch(CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(ScopedFooImpl.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(ScopedFooImpl.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
   }
   
@@ -191,8 +198,8 @@ public class JitBindingsTest extends TestCase {
       }).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(), "\n1) " + jitFailed(Bar.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
   }
   
@@ -208,8 +215,8 @@ public class JitBindingsTest extends TestCase {
       });
       fail("should have failed");
     } catch (CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
   }
 
@@ -223,8 +230,8 @@ public class JitBindingsTest extends TestCase {
       }).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(), "\n1) " + jitFailed(Bar.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
   }
 
@@ -240,8 +247,8 @@ public class JitBindingsTest extends TestCase {
       });
       fail("should have failed");
     } catch (CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
   }
   
@@ -311,18 +318,18 @@ public class JitBindingsTest extends TestCase {
       });
       fail("should have failed");
     } catch(CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(Foo.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(Foo.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
     
     Injector child = parent.createChildInjector(new AbstractModule() {
       @Override
       protected void configure() {
         bind(Foo.class).to(FooImpl.class);
+        bind(FooImpl.class);
       }
     });
     ensureWorks(child, Foo.class, Bar.class);
-    ensureFails(child, ALLOW_BINDING, FooImpl.class);
     ensureInChild(parent, FooImpl.class, Foo.class);
     // TODO(sameb): FooBar may or may not be in a child injector, depending on if GC has run.
     // We should fix failed child injectors to remove their contents from the parent blacklist
@@ -337,9 +344,7 @@ public class JitBindingsTest extends TestCase {
       }
     });
     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 +366,8 @@ public class JitBindingsTest extends TestCase {
       });
       fail("should have failed");
     } catch(CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(Foo.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(Foo.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
     assertEquals(totalParentBindings, parent.getAllBindings().size());
     
@@ -371,26 +376,11 @@ public class JitBindingsTest extends TestCase {
       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 +390,6 @@ public class JitBindingsTest extends TestCase {
     });
     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 +414,8 @@ public class JitBindingsTest extends TestCase {
       });
       fail("should have failed");
     } catch(CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
     
     Injector injector = Guice.createInjector(new AbstractModule() {
@@ -438,6 +426,7 @@ public class JitBindingsTest extends TestCase {
           public void configure() {
             bind(Foo.class).to(FooImpl.class);
             expose(Foo.class);
+            bind(FooImpl.class);
           }
         });
       }
@@ -464,8 +453,8 @@ public class JitBindingsTest extends TestCase {
       });
       fail("should have failed");
     } catch(CreationException expected) {
-      assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
-      assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+      assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
     }
   }
 
@@ -523,6 +512,90 @@ public class JitBindingsTest extends TestCase {
     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(), "\n1) " + jitInParentFailed(FooImpl.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+    }
+  }
+
+  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(), "\n1) " + jitInParentFailed(FooProvider.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+    }
+  }
+
+  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(), "\n1) " + jitInParentFailed(ImplByImpl.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+    }
+  }
+
+  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(), "\n1) " + jitInParentFailed(ProvByProvider.class));
+      assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+    }
+  }
   
   private void ensureWorks(Injector injector, Class<?>... classes) {
     for(int i = 0; i < classes.length; i++) {
@@ -539,16 +612,16 @@ public class JitBindingsTest extends TestCase {
         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(), "\n1) " + jitFailed(classes[i]));
+        assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
       }
       
       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(), "\n1) " + jitFailed(classes[i]));
+        assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
       }
       
       if (getBinding == GetBindingCheck.ALLOW_BINDING
@@ -563,16 +636,16 @@ public class JitBindingsTest extends TestCase {
           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(), "\n1) " + jitFailed(classes[i]));
+          assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
         }
       } 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(), "\n1) " + jitFailed(classes[i]));
+          assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
         }
       }
     }
@@ -584,24 +657,24 @@ public class JitBindingsTest extends TestCase {
         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(), "\n1) " + inChildMessage(classes[i]));
+        assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
       }
       
       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(), "\n1) " + inChildMessage(classes[i]));
+        assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
       }
       
       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(), "\n1) " + inChildMessage(classes[i]));
+        assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
       }
     }
   }