8087349: Test tools/sjavac/IncCompInheritance.java is failing

Refactoring of Dependencies framework.

Reviewed-by: mcimadamore

langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java

@@ -56,7 +56,6 @@ import com.sun.tools.javac.tree.JCTree.*;
 import com.sun.tools.javac.tree.JCTree.JCPolyExpression.*;
 import com.sun.tools.javac.util.*;
 import com.sun.tools.javac.util.DefinedBy.Api;
-import com.sun.tools.javac.util.Dependencies.AttributionKind;
 import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition;
 import com.sun.tools.javac.util.JCDiagnostic.Fragment;
 import com.sun.tools.javac.util.List;
@@ -760,7 +759,6 @@ public class Attr extends JCTree.Visitor {
      */
     void attribTypeVariables(List<JCTypeParameter> typarams, Env<AttrContext> env) {
         for (JCTypeParameter tvar : typarams) {
-            dependencies.push(AttributionKind.TVAR, tvar);
             TypeVar a = (TypeVar)tvar.type;
             a.tsym.flags_field |= UNATTRIBUTED;
             a.bound = Type.noType;
@@ -775,7 +773,6 @@ public class Attr extends JCTree.Visitor {
                 types.setBounds(a, List.of(syms.objectType));
             }
             a.tsym.flags_field &= ~UNATTRIBUTED;
-            dependencies.pop();
         }
         for (JCTypeParameter tvar : typarams) {
             chk.checkNonCyclic(tvar.pos(), (TypeVar)tvar.type);

langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java

@@ -54,7 +54,6 @@ import static com.sun.tools.javac.code.TypeTag.CLASS;
 import static com.sun.tools.javac.code.TypeTag.ERROR;
 import static com.sun.tools.javac.tree.JCTree.Tag.*;
 
-import com.sun.tools.javac.util.Dependencies.AttributionKind;
 import com.sun.tools.javac.util.Dependencies.CompletionCause;
 import com.sun.tools.javac.util.JCDiagnostic.DiagnosticFlag;
 import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition;
@@ -372,7 +371,6 @@ public class TypeEnter implements Completer {
         }
 
         private void doImport(JCImport tree) {
-            dependencies.push(AttributionKind.IMPORT, tree);
             JCFieldAccess imp = (JCFieldAccess)tree.qualid;
             Name name = TreeInfo.name(imp);
 
@@ -399,7 +397,6 @@ public class TypeEnter implements Completer {
                     importNamed(tree.pos(), c, env, tree);
                 }
             }
-            dependencies.pop();
         }
 
         Type attribImportType(JCTree tree, Env<AttrContext> env) {
@@ -647,13 +644,7 @@ public class TypeEnter implements Completer {
 
             if (tree.extending != null) {
                 extending = clearTypeParams(tree.extending);
-                dependencies.push(AttributionKind.EXTENDS, tree.extending);
+                supertype = attr.attribBase(extending, baseEnv, true, false, true);
-                try {
-                    supertype = attr.attribBase(extending, baseEnv,
-                                                true, false, true);
-                } finally {
-                    dependencies.pop();
-                }
             } else {
                 extending = null;
                 supertype = ((tree.mods.flags & Flags.ENUM) != 0)
@@ -671,20 +662,14 @@ public class TypeEnter implements Completer {
             List<JCExpression> interfaceTrees = tree.implementing;
             for (JCExpression iface : interfaceTrees) {
                 iface = clearTypeParams(iface);
-                dependencies.push(AttributionKind.IMPLEMENTS, iface);
+                Type it = attr.attribBase(iface, baseEnv, false, true, true);
-                try {
+                if (it.hasTag(CLASS)) {
-                    Type it = attr.attribBase(iface, baseEnv, false, true, true);
+                    interfaces.append(it);
-                    if (it.hasTag(CLASS)) {
+                    if (all_interfaces != null) all_interfaces.append(it);
-                        interfaces.append(it);
+                } else {
-                        if (all_interfaces != null) all_interfaces.append(it);
+                    if (all_interfaces == null)
-                    } else {
+                        all_interfaces = new ListBuffer<Type>().appendList(interfaces);
-                        if (all_interfaces == null)
+                    all_interfaces.append(modelMissingTypes(it, iface, true));
-                            all_interfaces = new ListBuffer<Type>().appendList(interfaces);
-                        all_interfaces.append(modelMissingTypes(it, iface, true));
-
-                    }
-                } finally {
-                    dependencies.pop();
                 }
             }
 

langtools/src/jdk.compiler/share/classes/com/sun/tools/javac/util/Dependencies.java

@@ -30,7 +30,7 @@ import com.sun.tools.javac.code.Symbol.ClassSymbol;
 import com.sun.tools.javac.code.Symbol.Completer;
 import com.sun.tools.javac.code.Symbol.CompletionFailure;
 import com.sun.tools.javac.main.JavaCompiler;
-import com.sun.tools.javac.tree.JCTree;
+import com.sun.tools.javac.util.GraphUtils.DependencyKind;
 import com.sun.tools.javac.util.GraphUtils.DotVisitor;
 import com.sun.tools.javac.util.GraphUtils.NodeVisitor;
 
@@ -42,13 +42,13 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.EnumMap;
 import java.util.EnumSet;
-import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
-import java.util.Set;
 import java.util.Stack;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import javax.tools.JavaFileObject;
 
@@ -77,70 +77,24 @@ public abstract class Dependencies {
         context.put(dependenciesKey, this);
     }
 
-    /**
-     * This enum models different kinds of attribution actions triggered during
-     * symbol completion.
-     */
-    public enum AttributionKind {
-        /**
-         * Attribution of superclass (i.e. @{code extends} clause).
-         */
-        EXTENDS {
-            @Override
-            String format(JCTree tree) {
-                return "extends " + super.format(tree);
-            }
-        },
-        /**
-         * Attribution of superinterface (i.e. an type in the @{code interface} clause).
-         */
-        IMPLEMENTS {
-            @Override
-            String format(JCTree tree) {
-                return "implements " + super.format(tree);
-            }
-        },
-        /**
-         * Attribution of an import statement
-         */
-        IMPORT,
-        /**
-         * Attribution of type-variable bound
-         */
-        TVAR {
-            @Override
-            String format(JCTree tree) {
-                return "<" + super.format(tree) + ">";
-            }
-        };
-
-        String format(JCTree tree) {
-            return tree.toString();
-        }
-    }
-
     /**
      * Push a new completion node on the stack.
      */
     abstract public void push(ClassSymbol s, CompletionCause phase);
 
-    /**
-     * Push a new attribution node on the stack.
-     */
-    abstract public void push(AttributionKind ak, JCTree t);
-
     /**
      * Remove current dependency node from the stack.
      */
     abstract public void pop();
 
-    public enum CompletionCause {
+    public enum CompletionCause implements GraphUtils.DependencyKind {
         CLASS_READER,
         HEADER_PHASE,
         HIERARCHY_PHASE,
         IMPORTS_PHASE,
         MEMBER_ENTER,
-        MEMBERS_PHASE;
+        MEMBERS_PHASE,
+        OTHER;
     }
 
     /**
@@ -163,13 +117,8 @@ public abstract class Dependencies {
         /**
          * Register a Context.Factory to create a Dependencies.
          */
-        public static void preRegister(final Context context) {
+        public static void preRegister(Context context) {
-            context.put(dependenciesKey, new Context.Factory<Dependencies>() {
+            context.put(dependenciesKey, (Context.Factory<Dependencies>) GraphDependencies::new);
-                public Dependencies make(Context c) {
-                    Dependencies deps = new GraphDependencies(context);
-                    return deps;
-                }
-            });
         }
 
         /**
@@ -195,12 +144,11 @@ public abstract class Dependencies {
         enum DependenciesMode {
             SOURCE("source"),
             CLASS("class"),
-            REDUNDANT("redundant"),
+            REDUNDANT("redundant");
-            SIDE_EFFECTS("side-effects");
 
             final String opt;
 
-            private DependenciesMode(String opt) {
+            DependenciesMode(String opt) {
                 this.opt = opt;
             }
 
@@ -228,46 +176,19 @@ public abstract class Dependencies {
         }
 
         /**
-         * Class representing a node in the dependency graph. Nodes are of two main
+         * Class representing a node in the dependency graph.
-         * kinds: (i) symbol nodes, corresponding to symbol completion requests
-         * (either from source or classfile); (ii) attribution nodes, corresponding to
-         * attribution actions triggered during (source) completion.
          */
-        public static abstract class Node extends GraphUtils.AbstractNode<String, Node>
+        public static abstract class Node extends GraphUtils.AbstractNode<ClassSymbol, Node>
-                implements GraphUtils.DottableNode<String, Node> {
+                implements GraphUtils.DottableNode<ClassSymbol, Node> {
-
-            /**
-             * Model the dependencies between nodes.
-             */
-            public enum DependencyKind implements GraphUtils.DependencyKind {
-                /**
-                 * standard dependency - i.e. completion of the source node depends
-                 * on completion of the sink node.
-                 */
-                REQUIRES("solid"),
-                /**
-                 * soft dependencies - i.e. completion of the source node depends
-                 * on side-effects of the source node. These dependencies are meant
-                 * to capture the order in which javac processes all dependants of a given node.
-                 */
-                SIDE_EFFECTS("dashed");
-
-                final String dotStyle;
-
-                DependencyKind(String dotStyle) {
-                    this.dotStyle = dotStyle;
-                }
-            }
-
             /**
              * dependant nodes grouped by kind
              */
-            EnumMap<DependencyKind, List<Node>> depsByKind;
+            EnumMap<CompletionCause, List<Node>> depsByKind;
 
-            Node(String value) {
+            Node(ClassSymbol value) {
                 super(value);
-                this.depsByKind = new EnumMap<>(DependencyKind.class);
+                this.depsByKind = new EnumMap<>(CompletionCause.class);
-                for (DependencyKind depKind : DependencyKind.values()) {
+                for (CompletionCause depKind : CompletionCause.values()) {
                     depsByKind.put(depKind, new ArrayList<Node>());
                 }
             }
@@ -281,8 +202,7 @@ public abstract class Dependencies {
 
             @Override
             public boolean equals(Object obj) {
-                return obj instanceof Node &&
+                return obj instanceof Node && data.equals(((Node) obj).data);
-                        data.equals(((Node) obj).data);
             }
 
             @Override
@@ -292,19 +212,12 @@ public abstract class Dependencies {
 
             @Override
             public GraphUtils.DependencyKind[] getSupportedDependencyKinds() {
-                return DependencyKind.values();
+                return CompletionCause.values();
             }
 
             @Override
-            public java.util.Collection<? extends Node> getDependenciesByKind(GraphUtils.DependencyKind dk) {
+            public java.util.Collection<? extends Node> getDependenciesByKind(DependencyKind dk) {
-                List<Node> deps = depsByKind.get(dk);
+                return depsByKind.get(dk);
-                if (dk == DependencyKind.REQUIRES) {
-                    return deps;
-                } else {
-                    Set<Node> temp = new HashSet<>(deps);
-                    temp.removeAll(depsByKind.get(DependencyKind.REQUIRES));
-                    return temp;
-                }
             }
 
             @Override
@@ -317,9 +230,14 @@ public abstract class Dependencies {
             @Override
             public Properties dependencyAttributes(Node to, GraphUtils.DependencyKind dk) {
                 Properties p = new Properties();
-                p.put("style", ((DependencyKind) dk).dotStyle);
+                p.put("label", dk);
                 return p;
             }
+
+            @Override
+            public String toString() {
+                return data.getQualifiedName().toString();
+            }
         }
 
         /**
@@ -349,11 +267,9 @@ public abstract class Dependencies {
             }
 
             final Kind ck;
-            final ClassSymbol sym;
 
             CompletionNode(ClassSymbol sym) {
-                super(sym.getQualifiedName().toString());
+                super(sym);
-                this.sym = sym;
                 //infer completion kind by looking at the symbol fields
                 boolean fromClass = (sym.classfile == null && sym.sourcefile == null) ||
                         (sym.classfile != null && sym.classfile.getKind() == JavaFileObject.Kind.CLASS);
@@ -371,27 +287,7 @@ public abstract class Dependencies {
             }
 
             public ClassSymbol getClassSymbol() {
-                return sym;
+                return data;
-            }
-        }
-
-        /**
-         * This is a dependency node used to model attribution actions triggered during
-         * source symbol completion. The possible kinds of attribution actions are
-         * captured in {@link AttributionNode}.
-         */
-        static class AttributionNode extends Node {
-
-            AttributionNode(AttributionKind ak, JCTree tree) {
-                super(ak.format(tree));
-            }
-
-            @Override
-            public Properties nodeAttributes() {
-                Properties p = super.nodeAttributes();
-                p.put("shape", "box");
-                p.put("style", "solid");
-                return p;
             }
         }
 
@@ -403,25 +299,20 @@ public abstract class Dependencies {
         /**
          * map containing all dependency nodes seen so far
          */
-        Map<String, Node> dependencyNodeMap = new LinkedHashMap<>();
+        Map<ClassSymbol, Node> dependencyNodeMap = new LinkedHashMap<>();
 
         @Override
         public void push(ClassSymbol s, CompletionCause phase) {
             Node n = new CompletionNode(s);
-            if (n == push(n)) {
+            if (n == push(n, phase)) {
                 s.completer = this;
             }
         }
 
-        @Override
-        public void push(AttributionKind ak, JCTree t) {
-            push(new AttributionNode(ak, t));
-        }
-
         /**
          * Push a new dependency on the stack.
          */
-        protected Node push(Node newNode) {
+        protected Node push(Node newNode, CompletionCause cc) {
             Node cachedNode = dependencyNodeMap.get(newNode.data);
             if (cachedNode == null) {
                 dependencyNodeMap.put(newNode.data, newNode);
@@ -430,7 +321,7 @@ public abstract class Dependencies {
             }
             if (!nodeStack.isEmpty()) {
                 Node currentNode = nodeStack.peek();
-                currentNode.addDependency(Node.DependencyKind.REQUIRES, newNode);
+                currentNode.addDependency(cc, newNode);
             }
             nodeStack.push(newNode);
             return newNode;
@@ -455,10 +346,6 @@ public abstract class Dependencies {
                 //filter source completions
                 new FilterVisitor(CompletionNode.Kind.CLASS).visit(dependencyNodeMap.values(), null);
             }
-            if (dependenciesModes.contains(DependenciesMode.SIDE_EFFECTS)) {
-                //add side-effects edges
-                new SideEffectVisitor().visit(dependencyNodeMap.values(), null);
-            }
             if (dependenciesFile != null) {
                 //write to file
                 try (FileWriter fw = new FileWriter(dependenciesFile)) {
@@ -469,7 +356,7 @@ public abstract class Dependencies {
 
         @Override
         public void complete(Symbol sym) throws CompletionFailure {
-            push((ClassSymbol) sym, null);
+            push((ClassSymbol)sym, CompletionCause.OTHER);
             pop();
             sym.completer = this;
         }
@@ -483,32 +370,10 @@ public abstract class Dependencies {
             return dependencyNodeMap.values();
         }
 
-        /**
-         * This visitor is used to generate the special side-effect dependencies
-         * given a graph containing only standard dependencies.
-         */
-        private static class SideEffectVisitor extends NodeVisitor<String, Node, Void> {
-            @Override
-            public void visitNode(Node node, Void arg) {
-                //do nothing
-            }
-
-            @Override
-            public void visitDependency(GraphUtils.DependencyKind dk, Node from, Node to, Void arg) {
-                //if we are adding multiple dependencies to same node
-                //make order explicit via special 'side-effect' dependencies
-                List<Node> deps = from.depsByKind.get(dk);
-                int pos = deps.indexOf(to);
-                if (dk == Node.DependencyKind.REQUIRES && pos > 0) {
-                    to.addDependency(Node.DependencyKind.SIDE_EFFECTS, deps.get(pos - 1));
-                }
-            }
-        }
-
         /**
          * This visitor is used to prune the graph from spurious edges using some heuristics.
          */
-        private static class PruneVisitor extends NodeVisitor<String, Node, Void> {
+        private static class PruneVisitor extends NodeVisitor<ClassSymbol, Node, Void> {
             @Override
             public void visitNode(Node node, Void arg) {
                 //do nothing
@@ -517,8 +382,7 @@ public abstract class Dependencies {
             @Override
             public void visitDependency(GraphUtils.DependencyKind dk, Node from, Node to, Void arg) {
                 //heuristic - skips dependencies that are likely to be fake
-                if (from.equals(to) ||
+                if (from.equals(to)) {
-                        from.depsByKind.get(Node.DependencyKind.REQUIRES).contains(to)) {
                     to.depsByKind.get(dk).remove(from);
                 }
             }
@@ -527,7 +391,7 @@ public abstract class Dependencies {
         /**
          * This visitor is used to retain only completion nodes with given kind.
          */
-        private class FilterVisitor extends NodeVisitor<String, Node, Void> {
+        private class FilterVisitor extends NodeVisitor<ClassSymbol, Node, Void> {
 
             CompletionNode.Kind ck;
 
@@ -570,11 +434,6 @@ public abstract class Dependencies {
             //do nothing
         }
 
-        @Override
-        public void push(AttributionKind ak, JCTree t) {
-            //do nothing
-        }
-
         @Override
         public void pop() {
             //do nothing

langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/comp/dependencies/NewDependencyCollector.java

@@ -26,11 +26,13 @@
 package com.sun.tools.sjavac.comp.dependencies;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import javax.tools.JavaFileManager.Location;
 import javax.tools.JavaFileObject;
@@ -38,7 +40,11 @@ import javax.tools.StandardLocation;
 
 import com.sun.source.util.TaskEvent;
 import com.sun.source.util.TaskListener;
+import com.sun.tools.javac.code.Kinds.Kind;
+import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Symbol.ClassSymbol;
+import com.sun.tools.javac.code.Symbol.TypeSymbol;
+import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.util.Context;
 import com.sun.tools.javac.util.DefinedBy;
 import com.sun.tools.javac.util.DefinedBy.Api;
@@ -85,7 +91,6 @@ public class NewDependencyCollector implements TaskListener {
 
         return deps.getNodes()
                    .stream()
-                   .filter(n -> n instanceof CompletionNode)
                    .map(n -> (CompletionNode) n)
                    .filter(n -> n.getClassSymbol().fullname != null)
                    .filter(n -> explicits == explicitJFOs.contains(n.getClassSymbol().classfile))
@@ -128,31 +133,66 @@ public class NewDependencyCollector implements TaskListener {
             String depPkg = Util.pkgNameOfClassName(fqDep);
 
             Map<String, Set<String>> depsForThisClass = result.get(depPkg);
-            if (depsForThisClass == null)
+            if (depsForThisClass == null) {
                 result.put(depPkg, depsForThisClass = new HashMap<>());
+            }
 
-            for (Node<?,?> depNode : cnode.getDependenciesByKind(GraphDependencies.Node.DependencyKind.REQUIRES)) {
+            Set<String> fqDeps = depsForThisClass.get(fqDep);
-                boolean isCompletionNode = depNode instanceof CompletionNode;
+            if (fqDeps == null) {
-                if (isCompletionNode) {
+                depsForThisClass.put(fqDep, fqDeps = new HashSet<>());
-                    CompletionNode cDepNode = (CompletionNode) depNode;
+            }
-                    if (cDepNode == cnode)
-                        continue;
-                    if (cDepNode.getClassSymbol().fullname == null) // Anonymous class
-                        continue;
-                    Location depLoc = getLocationOf(cDepNode.getClassSymbol());
-                    boolean relevant = (cp  && depLoc == StandardLocation.CLASS_PATH)
-                                    || (!cp && depLoc == StandardLocation.SOURCE_PATH);
-                    if (!relevant)
-                        continue;
 
-                    Set<String> fqDeps = depsForThisClass.get(fqDep);
+            for (Node<?,?> depNode : getAllDependencies(cnode)) {
-                    if (fqDeps == null)
+                CompletionNode cDepNode = (CompletionNode) depNode;
-                        depsForThisClass.put(fqDep, fqDeps = new HashSet<>());
+                // Symbol is not regarded to depend on itself.
+                if (cDepNode == cnode) {
+                    continue;
+                }
+                // Skip anonymous classes
+                if (cDepNode.getClassSymbol().fullname == null) {
+                    continue;
+                }
+                if (isSymbolRelevant(cp, cDepNode.getClassSymbol())) {
                     fqDeps.add(cDepNode.getClassSymbol().outermostClass().flatname.toString());
                 }
             }
+
+            // The completion dependency graph is not transitively closed for inheritance relations.
+            // For sjavac's purposes however, a class depends on it's super super type, so below we
+            // make sure that we include super types.
+            for (ClassSymbol cs : allSupertypes(cnode.getClassSymbol())) {
+                if (isSymbolRelevant(cp, cs)) {
+                    fqDeps.add(cs.outermostClass().flatname.toString());
+                }
+            }
+
         }
         return result;
     }
 
+    public boolean isSymbolRelevant(boolean cp, ClassSymbol cs) {
+        Location csLoc = getLocationOf(cs);
+        Location relevantLocation = cp ? StandardLocation.CLASS_PATH : StandardLocation.SOURCE_PATH;
+        return csLoc == relevantLocation;
+    }
+
+    private Set<ClassSymbol> allSupertypes(TypeSymbol t) {
+        if (t == null || !(t instanceof ClassSymbol)) {
+            return Collections.emptySet();
+        }
+        Set<ClassSymbol> result = new HashSet<>();
+        ClassSymbol cs = (ClassSymbol) t;
+        result.add(cs);
+        result.addAll(allSupertypes(cs.getSuperclass().tsym));
+        for (Type it : cs.getInterfaces()) {
+            result.addAll(allSupertypes(it.tsym));
+        }
+        return result;
+    }
+
+    private Collection<? extends Node<?, ?>> getAllDependencies(CompletionNode cnode) {
+        return Stream.of(cnode.getSupportedDependencyKinds())
+                     .flatMap(dk -> cnode.getDependenciesByKind(dk).stream())
+                     .collect(Collectors.toSet());
+    }
 }

langtools/test/tools/javac/importscope/dependencies/DependenciesTest.java

@@ -193,11 +193,6 @@ public class DependenciesTest {
             inProcess.push(null);
         }
 
-        @Override
-        public void push(AttributionKind ak, JCTree t) {
-            inProcess.push(null);
-        }
-
         @Override
         public void pop() {
             inProcess.pop();