Fixes HasDependency instance cases in the Grapher

Adapts the graph to only show InjectionPoints and Members that come from
the Bindings' getDependency() call. Allows Multibinder, etc. to hide the
Injector dependency from the graph.

See issue 298 for discussion about this.

git-svn-id: https://google-guice.googlecode.com/svn/trunk@756 d779f126-a31b-0410-b53b-1d3aecad763e

5 files changed, 113 insertions, 82 deletions

diff --git a/extensions/grapher/src/com/google/inject/grapher/GraphingVisitor.java b/extensions/grapher/src/com/google/inject/grapher/GraphingVisitor.java
index 86531c09..2ae71cea 100644
--- a/extensions/grapher/src/com/google/inject/grapher/GraphingVisitor.java
+++ b/extensions/grapher/src/com/google/inject/grapher/GraphingVisitor.java
@@ -18,14 +18,16 @@ package com.google.inject.grapher;
 
 import com.google.common.base.Nullable;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Sets;
 import com.google.inject.Binding;
 import com.google.inject.Inject;
+import com.google.inject.Key;
+import com.google.inject.Provider;
 import com.google.inject.spi.BindingTargetVisitor;
 import com.google.inject.spi.ConstructorBinding;
 import com.google.inject.spi.ConvertedConstantBinding;
 import com.google.inject.spi.Dependency;
 import com.google.inject.spi.ExposedBinding;
+import com.google.inject.spi.HasDependencies;
 import com.google.inject.spi.InjectionPoint;
 import com.google.inject.spi.InstanceBinding;
 import com.google.inject.spi.LinkedKeyBinding;
@@ -34,9 +36,9 @@ import com.google.inject.spi.ProviderInstanceBinding;
 import com.google.inject.spi.ProviderKeyBinding;
 import com.google.inject.spi.UntargettedBinding;
 
+import java.lang.reflect.Member;
 import java.util.Collection;
 import java.util.List;
-import java.util.Set;
 
 /**
  * {@link BindingTargetVisitor} that adds nodes and edges to the graph based on
@@ -137,16 +139,11 @@ implements BindingTargetVisitor<Object, Void> {
    * {@link Binding}, where the {@link Binding} is for an instance, rather than
    * a class.
    */
-  protected M newInstanceImplementationNode(Binding<?> binding, Object instance,
-      Collection<InjectionPoint> injectionPoints) {
+  protected M newInstanceImplementationNode(Binding<?> binding, Object instance) {
     M node = implementationNodeFactory.newImplementationNode(getInstanceNodeId(binding));
     node.setSource(binding.getSource());
     node.setInstance(instance);
 
-    for (InjectionPoint injectionPoint : injectionPoints) {
-      node.addMember(injectionPoint.getMember());
-    }
-
     return node;
   }
 
@@ -167,43 +164,36 @@ implements BindingTargetVisitor<Object, Void> {
   }
 
   /**
-   * Adds {@link DependencyEdge}s to the graph from the given
-   * {@link ImplementationNode} to the {@link Key}s specified in the
-   * {@link InjectionPoint}s.
+   * Adds {@link DependencyEdge}s to the graph for each of the provided
+   * {@link Dependency}s. These will be from the given node ID to the
+   * {@link Dependency}'s {@link Key}.
    * <p>
-   * Also adds edges for any {@link Dependency}s passed in that are not covered
-   * in the set of {@link InjectionPoint}s.
+   * If a {@link Dependency} has an associated {@link InjectionPoint}, its
+   * member will be added to the given {@link ImplementationNode} and the edge
+   * will start at the {@link Member}.
    *
    * @see #newDependencyEdge(Object, InjectionPoint, Dependency)
    * 
    * @param nodeId ID of the node that should be the tail of the
    *     {@link DependencyEdge}s.
-   * @param injectionPoints {@link Collection} of {@link InjectionPoint}s on
-   *     the class or instance represented by the {@link ImplementationNode}.
+   * @param node An {@link ImplementationNode} to add {@link Member}s to.
    * @param dependencies {@link Collection} of {@link Dependency}s from the
-   *     {@link Binding}. Some {@link Binding}s may have {@link Dependency}s
-   *     even if they do not have {@link InjectionPoint}s.
+   *     {@link Binding}.
    * @return A {@link Collection} of the {@link DependencyEdge}s that were
    *     added to the graph.
    */
-  protected Collection<D> newDependencyEdges(K nodeId, Collection<InjectionPoint> injectionPoints,
+  protected Collection<D> newDependencyEdges(K nodeId, M node,
       Collection<Dependency<?>> dependencies) {
     List<D> edges = Lists.newArrayList();
 
-    // Set to keep track of which of the given Dependencies is not duplicated
-    // by the InjectionPoints.
-    Set<Dependency<?>> remainingDependencies = Sets.newHashSet(dependencies);
-    
-    for (InjectionPoint injectionPoint : injectionPoints) {
-      for (Dependency<?> dependency : injectionPoint.getDependencies()) {
-        D edge = newDependencyEdge(nodeId, injectionPoint, dependency);
-        edges.add(edge);
-        remainingDependencies.remove(dependency);
+    for (Dependency<?> dependency : dependencies) {
+      InjectionPoint injectionPoint = dependency.getInjectionPoint();
+
+      if (injectionPoint != null) {
+        node.addMember(injectionPoint.getMember());
       }
-    }
 
-    for (Dependency<?> dependency : remainingDependencies) {
-      D edge = newDependencyEdge(nodeId, null, dependency);    
+      D edge = newDependencyEdge(nodeId, injectionPoint, dependency);
       edges.add(edge);
     }
 
@@ -241,9 +231,8 @@ implements BindingTargetVisitor<Object, Void> {
    * @see #newDependencyEdges(ImplementationNode, Collection, Collection)
    */
   public Void visitConstructor(ConstructorBinding<?> binding) {
-    newClassImplementationNode(binding, binding.getInjectionPoints());
-    newDependencyEdges(getClassNodeId(binding), binding.getInjectionPoints(),
-        binding.getDependencies());
+    M node = newClassImplementationNode(binding, binding.getInjectionPoints());
+    newDependencyEdges(getClassNodeId(binding), node, binding.getDependencies());
 
     return null;
   }
@@ -299,9 +288,8 @@ implements BindingTargetVisitor<Object, Void> {
     newBindingEdge(getClassNodeId(binding), getInstanceNodeId(binding),
         BindingEdge.Type.NORMAL);
 
-    newInstanceImplementationNode(binding, binding.getInstance(), binding.getInjectionPoints());
-    newDependencyEdges(getInstanceNodeId(binding), binding.getInjectionPoints(),
-        binding.getDependencies());
+    M node = newInstanceImplementationNode(binding, binding.getInstance());
+    newDependencyEdges(getInstanceNodeId(binding), node, binding.getDependencies());
 
     return null;
   }
@@ -353,10 +341,8 @@ implements BindingTargetVisitor<Object, Void> {
     newInterfaceNode(binding);
     newBindingEdge(getClassNodeId(binding), getInstanceNodeId(binding), BindingEdge.Type.PROVIDER);
 
-    newInstanceImplementationNode(binding, binding.getProviderInstance(),
-        binding.getInjectionPoints());
-    newDependencyEdges(getInstanceNodeId(binding), binding.getInjectionPoints(),
-        binding.getDependencies());
+    M node = newInstanceImplementationNode(binding, binding.getProviderInstance());
+    newDependencyEdges(getInstanceNodeId(binding), node, binding.getDependencies());
 
     return null;
   }
diff --git a/extensions/grapher/src/com/google/inject/grapher/TransitiveDependencyVisitor.java b/extensions/grapher/src/com/google/inject/grapher/TransitiveDependencyVisitor.java
index f364b803..c4af901b 100644
--- a/extensions/grapher/src/com/google/inject/grapher/TransitiveDependencyVisitor.java
+++ b/extensions/grapher/src/com/google/inject/grapher/TransitiveDependencyVisitor.java
@@ -25,7 +25,6 @@ import com.google.inject.spi.ConvertedConstantBinding;
 import com.google.inject.spi.Dependency;
 import com.google.inject.spi.ExposedBinding;
 import com.google.inject.spi.HasDependencies;
-import com.google.inject.spi.InjectionPoint;
 import com.google.inject.spi.InstanceBinding;
 import com.google.inject.spi.LinkedKeyBinding;
 import com.google.inject.spi.ProviderBinding;
@@ -47,30 +46,22 @@ import java.util.Set;
 public class TransitiveDependencyVisitor
 implements BindingTargetVisitor<Object, Collection<Key<?>>> {
 
-  // TODO(phopkins): Remove InjectionPoints when issue 298 is fixed.
-  private Collection<Key<?>> visitHasDependencies(HasDependencies hasDependencies,
-      Collection<InjectionPoint> injectionPoints) {
+  private Collection<Key<?>> visitHasDependencies(HasDependencies hasDependencies) {
     Set<Key<?>> dependencies = Sets.newHashSet();
     
     for (Dependency<?> dependency : hasDependencies.getDependencies()) {
       dependencies.add(dependency.getKey());
     }
 
-    for (InjectionPoint injectionPoint : injectionPoints) {
-      for (Dependency<?> dependency : injectionPoint.getDependencies()) {
-        dependencies.add(dependency.getKey());
-      }
-    }
-
     return dependencies;
   }
   
   public Collection<Key<?>> visitConstructor(ConstructorBinding<?> binding) {
-    return visitHasDependencies(binding, binding.getInjectionPoints());
+    return visitHasDependencies(binding);
   }
 
   public Collection<Key<?>> visitConvertedConstant(ConvertedConstantBinding<?> binding) {
-    return visitHasDependencies(binding, ImmutableSet.<InjectionPoint>of());
+    return visitHasDependencies(binding);
   }
 
   public Collection<Key<?>> visitExposed(ExposedBinding<?> binding) {
@@ -79,7 +70,7 @@ implements BindingTargetVisitor<Object, Collection<Key<?>>> {
   }
 
   public Collection<Key<?>> visitInstance(InstanceBinding<?> binding) {
-    return visitHasDependencies(binding, binding.getInjectionPoints());
+    return visitHasDependencies(binding);
   }
 
   public Collection<Key<?>> visitLinkedKey(LinkedKeyBinding<?> binding) {
@@ -91,7 +82,7 @@ implements BindingTargetVisitor<Object, Collection<Key<?>>> {
   }
 
   public Collection<Key<?>> visitProviderInstance(ProviderInstanceBinding<?> binding) {
-    return visitHasDependencies(binding, binding.getInjectionPoints());
+    return visitHasDependencies(binding);
   }
 
   public Collection<Key<?>> visitProviderKey(ProviderKeyBinding<?> binding) {
diff --git a/extensions/grapher/test/com/google/inject/grapher/GraphingVisitorTest.java b/extensions/grapher/test/com/google/inject/grapher/GraphingVisitorTest.java
index 4f3201d4..d46d8e88 100644
--- a/extensions/grapher/test/com/google/inject/grapher/GraphingVisitorTest.java
+++ b/extensions/grapher/test/com/google/inject/grapher/GraphingVisitorTest.java
@@ -17,22 +17,26 @@
 package com.google.inject.grapher;
 
 import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expectLastCall;
 import static org.easymock.EasyMock.replay;
 import static org.easymock.EasyMock.verify;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.inject.AbstractModule;
 import com.google.inject.Guice;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
-import com.google.inject.Provides;
+import com.google.inject.Key;
 import com.google.inject.spi.ConstructorBinding;
+import com.google.inject.spi.Dependency;
+import com.google.inject.spi.HasDependencies;
 import com.google.inject.spi.InjectionPoint;
-import com.google.inject.spi.ProviderInstanceBinding;
+import com.google.inject.spi.InstanceBinding;
 
 import java.util.Collection;
 import java.util.List;
+import java.util.Set;
 
 import junit.framework.TestCase;
 
@@ -80,36 +84,52 @@ public class GraphingVisitorTest extends TestCase {
   }
 
   public void testNewDependencies_withInjectionPoints() throws Exception {
+    @SuppressWarnings("unchecked")
+    ImplementationNode<String> node = recordMock(createMock(ImplementationNode.class));
+    
+    node.addMember(Obj.class.getDeclaredField("string"));
+    expectLastCall();
+    node.addMember(Obj.class.getDeclaredField("integer"));
+    expectLastCall();
+    node.addMember(Obj.class.getDeclaredField("bool"));
+    expectLastCall();
+    node.addMember(Obj.class.getDeclaredMethod("setInteger", Integer.class));
+    expectLastCall();
+
     replayAll();
 
-    ConstructorBinding<?> binding = (ConstructorBinding<?>) createInjector().getBinding(Obj.class);
+    Injector injector = Guice.createInjector(new ClassModule());
+    ConstructorBinding<?> binding = (ConstructorBinding<?>) injector.getBinding(Obj.class);
 
-    Collection<DependencyEdge<String>> edges = graphingVisitor.newDependencyEdges("",
-        binding.getInjectionPoints(), binding.getDependencies());
+    Collection<DependencyEdge<String>> edges = graphingVisitor.newDependencyEdges("", node,
+        binding.getDependencies());
 
-    assertEquals("There should be three edges, from the InjectionPoints", 3, edges.size());
+    assertEquals("There should be four edges, from the three fields plus the method",
+        4, edges.size());
     
     verifyAll();
   }
 
   public void testNewDependencies_withDependencies() throws Exception {
+    @SuppressWarnings("unchecked")
+    ImplementationNode<String> node = recordMock(createMock(ImplementationNode.class));
+    // No members should be added to the node, since the stated dependencies
+    // have no associated {@link InjectionPoint}s.
+
     replayAll();
 
-    ProviderInstanceBinding<?> binding =
-        (ProviderInstanceBinding<?>) createInjector().getBinding(Intf.class);
+    Injector injector = Guice.createInjector(new ClassModule(), new InstanceModule());
+    InstanceBinding<?> binding = (InstanceBinding<?>) injector.getBinding(Obj.class);
 
-    Collection<DependencyEdge<String>> edges = graphingVisitor.newDependencyEdges("",
-        ImmutableList.<InjectionPoint>of(), binding.getDependencies());
+    Collection<DependencyEdge<String>> edges = graphingVisitor.newDependencyEdges("", node,
+        binding.getDependencies());
 
-    assertEquals("There should be three edges, from the parameter Dependencies", 3, edges.size());
+    assertEquals("One edge should be created, for the one stated Integer dependency",
+        1, edges.size());
     
     verifyAll();
   }
 
-  private Injector createInjector() {
-    return Guice.createInjector(new TestModule());
-  }
-
   private void replayAll() {
     for (Object mock : mocks) {
       replay(mock);
@@ -132,25 +152,31 @@ public class GraphingVisitorTest extends TestCase {
     }
   }
 
-  private static class TestModule extends AbstractModule {
+  private static class ClassModule extends AbstractModule {
     @Override
     protected void configure() {
       bind(String.class).toInstance("String");
       bind(Integer.class).toInstance(Integer.valueOf(8));
       bind(Boolean.class).toInstance(Boolean.TRUE);
     }
+  }
 
-    @Provides
-    public Intf provideIntf(String string, Integer integer, Boolean bool) {
-      return null;
+  private static class InstanceModule extends AbstractModule {
+    @Override
+    protected void configure() {
+      bind(Obj.class).toInstance(new Obj());
     }
   }
 
-  private static interface Intf {}
-
-  private static class Obj {
+  private static class Obj implements HasDependencies {
     @Inject String string;
     @Inject Integer integer;
     @Inject Boolean bool;
+
+    @Inject void setInteger(Integer integer) {}
+    
+    public Set<Dependency<?>> getDependencies() {
+      return ImmutableSet.<Dependency<?>>of(Dependency.get(Key.get(Integer.class)));
+    }
   }
 }
diff --git a/extensions/grapher/test/com/google/inject/grapher/TransitiveDependencyVisitorTest.java b/extensions/grapher/test/com/google/inject/grapher/TransitiveDependencyVisitorTest.java
index 721e109f..2ef17958 100644
--- a/extensions/grapher/test/com/google/inject/grapher/TransitiveDependencyVisitorTest.java
+++ b/extensions/grapher/test/com/google/inject/grapher/TransitiveDependencyVisitorTest.java
@@ -83,6 +83,16 @@ public class TransitiveDependencyVisitorTest extends TestCase {
     assertDependencies(dependencies, Key.get(A.class), Key.get(D.class));
   }
 
+  public void testVisitInstance_instanceHasDependencies() {
+    Binding<?> binding = getBinding(Key.get(Interface.class), new HasDependenciesModule());
+    Collection<Key<?>> dependencies = visitor.visitInstance(
+        (InstanceBinding<?>) binding);
+    
+    // Dependencies should only be on the stated
+    // HasDependencies#getDependencies() values
+    assertDependencies(dependencies, Key.get(G.class));
+  }
+
   public void testVisitLinkedKey() {
     Binding<?> binding = getBinding(Key.get(Interface.class), new LinkedKeyModule());
     Collection<Key<?>> dependencies = visitor.visitLinkedKey((LinkedKeyBinding<?>) binding);
@@ -105,7 +115,7 @@ public class TransitiveDependencyVisitorTest extends TestCase {
         (ProviderInstanceBinding<?>) binding);
     
     // Dependencies will only be on the field- and method-injected classes.
-    assertDependencies(dependencies, Key.get(E.class), Key.get(F.class), Key.get(G.class));
+    assertDependencies(dependencies, Key.get(E.class), Key.get(F.class));
   }
 
   public void testVisitProviderKey() {
@@ -147,8 +157,7 @@ public class TransitiveDependencyVisitorTest extends TestCase {
     @Inject void setD(D d) {}
   }
 
-  private static class ConstructedClassProvider
-  implements Provider<ConstructedClass>, HasDependencies {
+  private static class ConstructedClassProvider implements Provider<ConstructedClass> {
     @Inject E e;
     ConstructedClassProvider() {}
     @Inject ConstructedClassProvider(A a, B b, C c) {}
@@ -157,12 +166,17 @@ public class TransitiveDependencyVisitorTest extends TestCase {
     public ConstructedClass get() {
       return null;
     }
+  }
 
+  private static class HasDependenciesClass implements Interface, HasDependencies {
+    @Inject A a;
+    @Inject B b;
+    
     public Set<Dependency<?>> getDependencies() {
       return ImmutableSet.<Dependency<?>>of(Dependency.get(Key.get(G.class)));
     }
   }
-  
+
   private static class ConvertedConstantModule extends AbstractModule {
     @Override
     protected void configure() {
@@ -190,6 +204,13 @@ public class TransitiveDependencyVisitorTest extends TestCase {
       bind(ConstructedClass.class).toProvider(new ConstructedClassProvider());
     }
   }
+
+  private static class HasDependenciesModule extends AbstractModule {
+    @Override
+    protected void configure() {
+      bind(Interface.class).toInstance(new HasDependenciesClass());
+    }
+  }
   
   private static class ProviderKeyModule extends AbstractModule {
     @Override
diff --git a/extensions/grapher/test/com/google/inject/grapher/demo/PinballParts.java b/extensions/grapher/test/com/google/inject/grapher/demo/PinballParts.java
index 6ac11048..9ad9745b 100644
--- a/extensions/grapher/test/com/google/inject/grapher/demo/PinballParts.java
+++ b/extensions/grapher/test/com/google/inject/grapher/demo/PinballParts.java
@@ -16,4 +16,11 @@
 
 package com.google.inject.grapher.demo;
 
-class PinballParts {}
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+class PinballParts {
+  // Shows up as an injection point on an instance. We use a Provider so that
+  // it doesn't have to be satisfied when the Injector is created.
+  @Inject Provider<DocBrown> salvager;
+}