Graph refactor: {inEdges, outEdges}->incidentEdges (#1173)

This commit refactors the Graph class so that rather than having
separate maps for inEdges and outEdges, there is a single incidentEdges
map, which contains objects with inEdges and outEdges.

This is motivated by a forthcoming big change as part of #1136; namely, to
allow storing dangling edges in the graph. Once we do so, we'll need a
consistent source of truth that enumerates all of the node addresses
which are accessible in the graph (either because they correspond to a
node in the graph, or because they are the src or dst of a dangling
edge). We could do this by adding another field to graph which tracks
this set, but by making this refactor, we can instead use the key set of
_incidentEdges as the source of truth for which node addresses are
present.

Besides being motivated by #1136, I think it's cleaner in general. Note
there are fewer ways for the graph to be inconsistent, as it's no longer
possible for inEdges and outEdges to have inconsistent sets of node
addresses.

The most complicated piece of this change was updating the automatic
invariant checker. It was no longer possible to test 3.1 and 4.1
separately, so they needed to be merged into a new invariant. Rather
than re-enumerate the invariants, I called the new one the 'Temporary
Invariant', because it is going to disappear in a subsequent commit.

Test plan: `yarn test` passes. Since Graph has extremely thorough
testing, this gives me great confidence in this commit. Note that no
observable behavior has changed.

src/core/graph.js

@@ -153,8 +153,9 @@ export type ModificationCount = number;
 export class Graph {
   _nodes: Set<NodeAddressT>;
   _edges: Map<EdgeAddressT, Edge>;
-  _inEdges: Map<NodeAddressT, Edge[]>;
+  // Map every node address present in the graph to its inEdges (edges for
-  _outEdges: Map<NodeAddressT, Edge[]>;
+  // which it is a dst) and outEdges (edges for which it is a src)
+  _incidentEdges: Map<NodeAddressT, {|+inEdges: Edge[], +outEdges: Edge[]|}>;
 
   // Incremented each time that any change is made to the graph. Used to
   // check for comodification and to avoid needlessly checking
@@ -170,8 +171,7 @@ export class Graph {
     };
     this._nodes = new Set();
     this._edges = new Map();
-    this._inEdges = new Map();
+    this._incidentEdges = new Map();
-    this._outEdges = new Map();
     this._maybeCheckInvariants();
   }
 
@@ -236,8 +236,7 @@ export class Graph {
     NodeAddress.assertValid(a);
     if (!this._nodes.has(a)) {
       this._nodes.add(a);
-      this._inEdges.set(a, []);
+      this._incidentEdges.set(a, {inEdges: [], outEdges: []});
-      this._outEdges.set(a, []);
     }
     this._markModification();
     this._maybeCheckInvariants();
@@ -258,9 +257,11 @@ export class Graph {
    */
   removeNode(a: NodeAddressT): this {
     NodeAddress.assertValid(a);
-    const existingInEdges = this._inEdges.get(a) || [];
+    const {inEdges, outEdges} = this._incidentEdges.get(a) || {
-    const existingOutEdges = this._outEdges.get(a) || [];
+      inEdges: [],
-    const existingEdges = existingInEdges.concat(existingOutEdges);
+      outEdges: [],
+    };
+    const existingEdges = inEdges.concat(outEdges);
     if (existingEdges.length > 0) {
       const strAddress = NodeAddress.toString(a);
       const strExampleEdge = edgeToString(existingEdges[0]);
@@ -270,8 +271,7 @@ export class Graph {
         } edge(s), e.g.: ${strExampleEdge}`
       );
     }
-    this._inEdges.delete(a);
+    this._incidentEdges.delete(a);
-    this._outEdges.delete(a);
     this._nodes.delete(a);
     this._markModification();
     this._maybeCheckInvariants();
@@ -367,8 +367,8 @@ export class Graph {
       }
     } else {
       this._edges.set(edge.address, edge);
-      const inEdges = NullUtil.get(this._inEdges.get(edge.dst));
+      const {inEdges} = NullUtil.get(this._incidentEdges.get(edge.dst));
-      const outEdges = NullUtil.get(this._outEdges.get(edge.src));
+      const {outEdges} = NullUtil.get(this._incidentEdges.get(edge.src));
       inEdges.push(edge);
       outEdges.push(edge);
     }
@@ -391,11 +391,11 @@ export class Graph {
     const edge = this._edges.get(address);
     if (edge != null) {
       this._edges.delete(address);
-      const inEdges = NullUtil.get(this._inEdges.get(edge.dst));
+      const {inEdges} = NullUtil.get(this._incidentEdges.get(edge.dst));
-      const outEdges = NullUtil.get(this._outEdges.get(edge.src));
+      const {outEdges} = NullUtil.get(this._incidentEdges.get(edge.src));
       // TODO(perf): This is linear in the degree of the endpoints of the
-      // edge. Consider storing in non-list form (e.g., `_inEdges` and
+      // edge. Consider storing in non-list form (e.g., `inEdges` and
-      // `_outEdges` could be `Map<NodeAddressT, Set<EdgeAddressT>>`).
+      // `outEdges` fields in `_incidentEdges` could be `Set<EdgeAddressT>`).
       [inEdges, outEdges].forEach((edges) => {
         const index = edges.findIndex((edge) => edge.address === address);
         if (index === -1) {
@@ -557,11 +557,11 @@ export class Graph {
     const direction = options.direction;
     const adjacencies: {edges: Edge[], direction: string}[] = [];
     if (direction === Direction.IN || direction === Direction.ANY) {
-      const inEdges = NullUtil.get(this._inEdges.get(node));
+      const {inEdges} = NullUtil.get(this._incidentEdges.get(node));
       adjacencies.push({edges: inEdges, direction: "IN"});
     }
     if (direction === Direction.OUT || direction === Direction.ANY) {
-      const outEdges = NullUtil.get(this._outEdges.get(node));
+      const {outEdges} = NullUtil.get(this._incidentEdges.get(node));
       adjacencies.push({edges: outEdges, direction: "OUT"});
     }
 
@@ -724,14 +724,14 @@ export class Graph {
     // class).
 
     // Invariant 1. For a node `n`, if `n` is in the graph, then
-    // `_inEdges.has(n)` and `_outEdges.has(n)`. The values of
+    // `_incidentEdges.has(n)`. The value of `_incidentEdges.get(n)`
-    // `_inEdges.get(n)` and `_outEdges.get(n)` are arrays of `Edge`s.
+    // is an object with two fields: `inEdges` and `outEdges`, both of which are
+    // arrays of `Edge`s
     for (const node of this._nodes) {
-      if (!this._inEdges.has(node)) {
+      if (!this._incidentEdges.has(node)) {
-        throw new Error(`missing in-edges for ${NodeAddress.toString(node)}`);
+        throw new Error(
-      }
+          `missing incident-edges for ${NodeAddress.toString(node)}`
-      if (!this._outEdges.has(node)) {
+        );
-        throw new Error(`missing out-edges for ${NodeAddress.toString(node)}`);
       }
     }
 
@@ -759,66 +759,71 @@ export class Graph {
       }
     }
 
-    // Invariant 3. Suppose that `_inEdges.has(n)` and, let `es` be
+    // Temporary Invariant
-    // `_inEdges.get(n)`. Then
+    // Suppose that `_incidentEdges.has(n)`. Then `n` is in the graph.
-    //  1. `n` is in the graph;
+    // The {inEdges, outEdges} => incidentEdges refactor necessitated pulling
-    //  2. `es` contains any logical value at most once;
+    // this invariant out of invariants 3 and 4. However, the purpose of this
-    //  3. if `es` contains `e`, then `e` is in the graph; and
+    // refactor is actually to remove this invariant. So, there is no need to
-    //  4. if `es` contains `e`, then `e.dst === n`.
+    // re-enumerate the invariants as this one will disappear shortly.
+    for (const addr of this._incidentEdges.keys()) {
+      if (!this._nodes.has(addr)) {
+        throw new Error(`spurious incident-edges`);
+      }
+    }
+
+    // Invariant 3. Suppose that `_incidentEdges.has(n)` and, let `es` be
+    // `_incidentEdges.get(n).inEdges`. Then
+    //  1. `es` contains any logical value at most once;
+    //  2. if `es` contains `e`, then `e` is in the graph; and
+    //  3. if `es` contains `e`, then `e.dst === n`.
     //
-    // Invariant 4. Suppose that `_outEdges.has(n)` and, let `es` be
+    // Invariant 4. Suppose that `_incidentEdges.has(n)` and, let `es` be
-    // `_outEdges.get(n)`. Then
+    // `_incidentEdges.get(n).outEdges`. Then
-    //  1. `n` is in the graph;
+    //  1. `es` contains any logical value at most once;
-    //  2. `es` contains any logical value at most once;
+    //  2. if `es` contains `e`, then `e` is in the graph; and
-    //  3. if `es` contains `e`, then `e` is in the graph; and
+    //  3. if `es` contains `e`, then `e.src === n`.
-    //  4. if `es` contains `e`, then `e.src === n`.
     //
-    // Note that Invariant 3.2 is equivalent to the following:
+    // Note that Invariant 3.1 is equivalent to the following:
     //
-    //     Invariant 3.2*. If `a` is an address, then there is at most
+    //     Invariant 3.1*. If `a` is an address, then there is at most
     //     one index `i` such that `es[i].address` is `a`.
     //
-    // It is immediate that 3.2* implies 3.2. To see that 3.2 implies
+    // It is immediate that 3.1* implies 3.1. To see that 3.1 implies
-    // 3.2*, suppose that `i` and `j` are such that `es[i].address` and
+    // 3.1*, suppose that `i` and `j` are such that `es[i].address` and
-    // `es[j].address` are both `a`. Then, by Invariant 3.3, each of
+    // `es[j].address` are both `a`. Then, by Invariant 3.2, each of
     // `es[i]` and `es[j]` is in the graph, so each is deep-equal to
     // `_edges.get(a)`. Therefore, `es[i]` and `es[j]` are deep-equal to
-    // each other. By 3.2, `es` contains a logical value at most once,
+    // each other. By 3.1, `es` contains a logical value at most once,
     // so `i` must be equal to `j`.
     //
-    // Therefore, it is valid to verify that 3.2*, which we will do. The
+    // Therefore, it is valid to verify that 3.1*, which we will do. The
-    // same logic of course applies to Invariant 4.2.
+    // same logic of course applies to Invariant 4.1.
     const inEdgesSeen: Set<EdgeAddressT> = new Set();
     const outEdgesSeen: Set<EdgeAddressT> = new Set();
-    for (const {seen, map, baseNodeAccessor, kind} of [
+    const incidentEntries = Array.from(this._incidentEdges.entries());
+    for (const {seen, entries, baseNodeAccessor, kind} of [
       {
         seen: inEdgesSeen,
-        map: this._inEdges,
+        entries: incidentEntries.map(([a, {inEdges}]) => [a, inEdges]),
         baseNodeAccessor: (e) => e.dst,
         kind: "in-edge",
       },
       {
         seen: outEdgesSeen,
-        map: this._outEdges,
+        entries: incidentEntries.map(([a, {outEdges}]) => [a, outEdges]),
         baseNodeAccessor: (e) => e.src,
         kind: "out-edge",
       },
     ]) {
-      for (const [base, edges] of map.entries()) {
+      for (const [base, edges] of entries) {
-        if (!this._nodes.has(base)) {
-          // 3.1/4.1
-          throw new Error(
-            `spurious ${kind}s for ${NodeAddress.toString(base)}`
-          );
-        }
         for (const edge of edges) {
-          // 3.2/4.2
+          // 3.1/4.1
           if (seen.has(edge.address)) {
             throw new Error(`duplicate ${kind}: ${edgeToString(edge)}`);
           }
           seen.add(edge.address);
           const expected = this._edges.get(edge.address);
-          // 3.3/4.3
+          // 3.2/4.2
           if (!deepEqual(edge, expected)) {
             if (expected == null) {
               throw new Error(`spurious ${kind}: ${edgeToString(edge)}`);
@@ -827,7 +832,7 @@ export class Graph {
               throw new Error(`bad ${kind}: ${vs}`);
             }
           }
-          // 3.4/4.4
+          // 3.3/4.3
           const expectedBase = baseNodeAccessor(edge);
           if (base !== baseNodeAccessor(edge)) {
             throw new Error(
@@ -842,11 +847,11 @@ export class Graph {
     // We now return to check 2.4 and 2.5, with the help of the
     // structures that we have built up in checking Invariants 3 and 4.
     for (const edge of this._edges.values()) {
-      // That `_inEdges.get(n)` contains `e` for some `n` is sufficient
+      // That `_incidentEdges.get(n).inEdges` contains `e` for some `n` is
-      // to show that `_inEdges.get(e.dst)` contains `e`: if `n` were
+      // sufficient to show that `_incidentEdges.get(e.dst).inEdges` contains
-      // something other than `e.dst`, then we would have a failure of
+      // `e`: if `n` were something other than `e.dst`, then we would have a
-      // invariant 3.4, which would have been caught earlier. Likewise
+      // failure of invariant 3.3, which would have been caught earlier.
-      // for `_outEdges`.
+      // Likewise for outEdges.
       if (!inEdgesSeen.has(edge.address)) {
         throw new Error(`missing in-edge: ${edgeToString(edge)}`);
       }

src/core/graph.test.js

@@ -103,18 +103,18 @@ describe("core/graph", () => {
       const src = NodeAddress.fromParts(["src"]);
       const dst = NodeAddress.fromParts(["dst"]);
       const edgeAddress = EdgeAddress.fromParts(["edge"]);
-      const edge = () => ({src, dst, address: edgeAddress});
+      const edge = Object.freeze({src, dst, address: edgeAddress});
       const graph = () =>
         new Graph()
           .addNode(src)
           .addNode(dst)
-          .addEdge(edge());
+          .addEdge(edge);
 
       describe("caches results when the graph has not been modified", () => {
         it("with passing invariants", () => {
           const g = new Graph().addNode(src);
           g.checkInvariants(); // good
-          g._inEdges.delete(src); // corrupted, but only by poking at the internals
+          g._incidentEdges.delete(src); // corrupted, but only by poking at the internals
           expect(() => g.checkInvariants()).not.toThrow();
           expect(() => g._checkInvariants()).toThrow();
         });
@@ -122,9 +122,9 @@ describe("core/graph", () => {
         it("with failing invariants", () => {
           const g = new Graph().addNode(src);
           g.checkInvariants(); // good
-          g._inEdges.delete(src); // corrupted
+          g._incidentEdges.delete(src); // corrupted
           expect(() => g.addNode(dst)).toThrow();
-          g._inEdges.set(src, []); // fixed, but only by poking at the internals
+          g._incidentEdges.set(src, {inEdges: [], outEdges: []}); // fixed, but only by poking at the internals
           expect(() => g.checkInvariants()).toThrow();
           expect(() => g._checkInvariants()).not.toThrow();
         });
@@ -136,120 +136,126 @@ describe("core/graph", () => {
       });
 
       // Invariant 1
-      it("detects missing in-edges", () => {
+      it("detects missing incident edges", () => {
         const g = new Graph().addNode(src);
-        g._inEdges.delete(src);
+        g._incidentEdges.delete(src);
-        expect(() => g._checkInvariants()).toThrow("missing in-edges");
+        expect(() => g._checkInvariants()).toThrow("missing incident-edges");
-      });
-      it("detects missing out-edges", () => {
-        const g = new Graph().addNode(src);
-        g._outEdges.delete(src);
-        expect(() => g._checkInvariants()).toThrow("missing out-edges");
       });
 
       // Invariant 2.1
       it("detects when an edge has bad address", () => {
         const g = graph();
-        const otherEdge = () => ({
+        const differentAddressEdge = Object.freeze({
           src,
           dst,
           address: EdgeAddress.fromParts(["wat"]),
         });
-        g._edges.set(edgeAddress, otherEdge());
+        g._edges.set(edgeAddress, differentAddressEdge);
-        g._inEdges.set(dst, [otherEdge()]);
+        g._edges.set(edge.address, differentAddressEdge);
-        g._outEdges.set(src, [otherEdge()]);
+        // $ExpectFlowError
+        g._incidentEdges.get(dst).inEdges = [differentAddressEdge];
+        // $ExpectFlowError
+        g._incidentEdges.get(src).outEdges = [differentAddressEdge];
         expect(() => g._checkInvariants()).toThrow("bad edge address");
       });
       // Invariant 2.2
       it("detects when an edge has missing src", () => {
         const g = graph();
         g._nodes.delete(src);
-        g._inEdges.delete(src);
+        g._incidentEdges.delete(src);
-        g._outEdges.delete(src);
         expect(() => g._checkInvariants()).toThrow("missing src");
       });
       // Invariant 2.3
       it("detects when an edge has missing dst", () => {
         const g = graph();
         g._nodes.delete(dst);
-        g._inEdges.delete(dst);
+        g._incidentEdges.delete(dst);
-        g._outEdges.delete(dst);
         expect(() => g._checkInvariants()).toThrow("missing dst");
       });
       // Invariant 2.4
       it("detects when an edge is missing in `_inEdges`", () => {
         const g = graph();
-        g._inEdges.set(edge().dst, []);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.dst).inEdges = [];
         expect(() => g._checkInvariants()).toThrow("missing in-edge");
       });
       // Invariant 2.5
       it("detects when an edge is missing in `_outEdges`", () => {
         const g = graph();
-        g._outEdges.set(edge().src, []);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.src).outEdges = [];
         expect(() => g._checkInvariants()).toThrow("missing out-edge");
       });
 
-      // Invariant 3.1
+      // Temporary invariant
-      it("detects spurious in-edges", () => {
+      it("detects spurious incident-edges", () => {
         const g = new Graph();
-        g._inEdges.set(src, []);
+        g._incidentEdges.set(src, {inEdges: [], outEdges: []});
-        expect(() => g._checkInvariants()).toThrow("spurious in-edges");
+        expect(() => g._checkInvariants()).toThrow("spurious incident-edges");
-      });
-      // Invariant 4.1
-      it("detects spurious out-edges", () => {
-        const g = new Graph();
-        g._outEdges.set(src, []);
-        expect(() => g._checkInvariants()).toThrow("spurious out-edges");
       });
 
-      // Invariant 3.2
+      // Invariant 3.1
       it("detects when an edge is duplicated in `_inEdges`", () => {
         const g = graph();
-        g._inEdges.set(edge().dst, [edge(), edge()]);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.dst).inEdges = [edge, edge];
         expect(() => g._checkInvariants()).toThrow("duplicate in-edge");
       });
-      // Invariant 4.2
+      // Invariant 4.1
       it("detects when an edge is duplicated in `_outEdges`", () => {
         const g = graph();
-        g._outEdges.set(edge().src, [edge(), edge()]);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.src).outEdges = [edge, edge];
         expect(() => g._checkInvariants()).toThrow("duplicate out-edge");
       });
 
-      // Invariant 3.3 (two failure modes: absent or wrong data)
+      // Invariant 3.2 (two failure modes: absent or wrong data)
       it("detects when an edge is spurious in `_inEdges`", () => {
-        const g = graph().removeEdge(edge().address);
+        const g = graph().removeEdge(edge.address);
-        g._inEdges.set(edge().dst, [edge()]);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.dst).inEdges = [edge];
         expect(() => g._checkInvariants()).toThrow("spurious in-edge");
       });
       it("detects when an edge has bad `dst` in `_inEdges`", () => {
         const g = graph();
-        g._inEdges.set(edge().dst, [{src: dst, dst, address: edgeAddress}]);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.dst).inEdges = [
+          {src: dst, dst: dst, address: edge.address},
+        ];
         expect(() => g._checkInvariants()).toThrow(/bad in-edge.*vs\./);
       });
-      // Invariant 4.3 (two failure modes: absent or wrong data)
+      // Invariant 4.2 (two failure modes: absent or wrong data)
       it("detects when an edge is spurious in `_outEdges`", () => {
-        const g = graph().removeEdge(edge().address);
+        const g = graph().removeEdge(edge.address);
-        g._outEdges.set(edge().src, [edge()]);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.src).outEdges = [edge];
         expect(() => g._checkInvariants()).toThrow("spurious out-edge");
       });
       it("detects when an edge has bad `src` in `_outEdges`", () => {
         const g = graph();
-        g._outEdges.set(edge().src, [{src, dst: src, address: edgeAddress}]);
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.src).outEdges = [
+          {src: src, dst: src, address: edge.address},
+        ];
         expect(() => g._checkInvariants()).toThrow(/bad out-edge.*vs\./);
       });
 
-      // Invariant 3.4
+      // Invariant 3.3
       it("detects when an edge has bad anchor in `_inEdges`", () => {
         const g = graph();
-        g._inEdges.set(edge().dst, []);
+        // $ExpectFlowError
-        g._inEdges.set(edge().src, [edge()]);
+        g._incidentEdges.get(edge.dst).inEdges = [];
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.src).inEdges = [edge];
         expect(() => g._checkInvariants()).toThrow(/bad in-edge.*anchor/);
       });
-      // Invariant 4.4
+      // Invariant 4.3
       it("detects when an edge has bad anchor in `_outEdges`", () => {
         const g = graph();
-        g._outEdges.set(edge().src, []);
+        // $ExpectFlowError
-        g._outEdges.set(edge().dst, [edge()]);
+        g._incidentEdges.get(edge.src).outEdges = [];
+        // $ExpectFlowError
+        g._incidentEdges.get(edge.dst).outEdges = [edge];
         expect(() => g._checkInvariants()).toThrow(/bad out-edge.*anchor/);
       });
     });