Merge `merge`s (#297)

Summary:
We had three graph merging functions: `merge`, `mergeConservative`, and
`mergeManyConservative`. Of these, `merge` was never used outside of
code directly testing its behavior, and `mergeConservative` is a
strictly inferior version of `mergeManyConservative`. This commit
removes `merge` and `mergeConservative`, and renames
`mergeManyConservative` to `mergeConservative`.

Paired with @decentralion.

Test Plan:
Existing unit tests suffice; some useless tests pruned.

wchargin-branch: mmeerrggee

src/cli/commands/combine.js

@@ -43,6 +43,6 @@ async function combine(filenames: $ReadOnlyArray<string>) {
       )
     )
   );
-  const result = Graph.mergeManyConservative(graphs);
+  const result = Graph.mergeConservative(graphs);
   console.log(stringify(result, {space: 4}));
 }

src/core/graph.js

@@ -2,7 +2,7 @@
 
 import deepEqual from "lodash.isequal";
 import stringify from "json-stable-stringify";
-import type {Address, Addressable, AddressMapJSON} from "./address";
+import type {Address, AddressMapJSON} from "./address";
 import {AddressMap} from "./address";
 import {toCompat, fromCompat} from "../util/compat";
 import type {Compatible} from "../util/compat";
@@ -64,7 +64,7 @@ export class Graph {
   }
 
   copy(): Graph {
-    return Graph.mergeConservative(new Graph(), this);
+    return Graph.mergeConservative([this]);
   }
 
   equals(that: Graph): boolean {
@@ -411,86 +411,16 @@ export class Graph {
   }
 
   /**
-   * Merge two graphs. When two nodes have the same address, a resolver
+   * Merge a collection of graphs. If multiple graphs have a node with a
-   * function will be called with the two nodes; the resolver should
+   * particular address, then the nodes must all have identical payload,
-   * return a new node with the same address, which will take the place
+   * and a single copy of this node will be included in the result
-   * of the two nodes in the new graph. Edges have similar behavior.
+   * graph; if not all nodes are identical, then an error is thrown.
+   * Likewise, all edges at a particular address must have identical
+   * source, destination, and payload.
    *
    * The existing graph objects are not modified.
    */
-  static merge(
+  static mergeConservative(graphs: $ReadOnlyArray<Graph>): Graph {
-    g1: Graph,
-    g2: Graph,
-    nodeResolver: (Node<any>, Node<any>) => Node<any>,
-    edgeResolver: (Edge<any>, Edge<any>) => Edge<any>
-  ): Graph {
-    const result: Graph = new Graph();
-    g1.nodes().forEach((node) => {
-      if (g2.node(node.address) !== undefined) {
-        const resolved = nodeResolver(node, g2.node(node.address));
-        result.addNode(resolved);
-      } else {
-        result.addNode(node);
-      }
-    });
-    g2.nodes().forEach((node) => {
-      if (result.node(node.address) === undefined) {
-        result.addNode(node);
-      }
-    });
-    g1.edges().forEach((edge) => {
-      if (g2.edge(edge.address) !== undefined) {
-        const resolved = edgeResolver(edge, g2.edge(edge.address));
-        result.addEdge(resolved);
-      } else {
-        result.addEdge(edge);
-      }
-    });
-    g2.edges().forEach((edge) => {
-      if (result.edge(edge.address) === undefined) {
-        result.addEdge(edge);
-      }
-    });
-    return result;
-  }
-
-  /**
-   * Merge two graphs, assuming that if `g1` and `g2` both have a node
-   * with a given address, then the nodes are deep-equal (and the same
-   * for edges). If this assumption does not hold, this function will
-   * raise an error.
-   */
-  static mergeConservative(g1: Graph, g2: Graph): Graph {
-    function conservativeResolver<T: Addressable>(
-      kinds: string /* used for an error message on mismatch */,
-      a: T,
-      b: T
-    ): T {
-      if (deepEqual(a, b)) {
-        return a;
-      } else {
-        throw new Error(
-          `distinct ${kinds} with address ${JSON.stringify(a.address)}`
-        );
-      }
-    }
-    const result: Graph = Graph.merge(
-      g1,
-      g2,
-      (u, v) => conservativeResolver("nodes", u, v),
-      (e, f) => conservativeResolver("edges", e, f)
-    );
-    return result;
-  }
-
-  /**
-   * Equivalent to
-   *
-   *     graphs.reduce((g, h) => Graph.mergeConservative(g, h), new Graph()),
-   *
-   * but uses a mutable accumulator for improved performance.
-   */
-  static mergeManyConservative(graphs: $ReadOnlyArray<Graph>): Graph {
     const result = new Graph();
     graphs.forEach((graph) => {
       graph.nodes().forEach((node) => {

src/core/graph.test.js

@@ -885,7 +885,7 @@ describe("graph", () => {
       });
     });
 
-    describe("merging", () => {
+    describe("mergeConservative", () => {
       /**
        * Decompose the given graph into neighborhood graphs: for each
        * node `u`, create a graph with just that node, its neighbors,
@@ -943,80 +943,30 @@ describe("graph", () => {
         return [].concat(edgeGraphs, nodeGraphs);
       }
 
-      it("conservatively recomposes a neighborhood decomposition", () => {
+      it("recomposes a neighborhood decomposition", () => {
-        const result = neighborhoodDecomposition(
+        const result = Graph.mergeConservative(
-          demoData.advancedMealGraph()
+          neighborhoodDecomposition(demoData.advancedMealGraph())
-        ).reduce((g1, g2) => Graph.mergeConservative(g1, g2), new Graph());
+        );
         expect(result.equals(demoData.advancedMealGraph())).toBe(true);
       });
 
-      it("conservatively recomposes an edge decomposition", () => {
+      it("recomposes an edge decomposition", () => {
         const result = edgeDecomposition(demoData.advancedMealGraph()).reduce(
-          (g1, g2) => Graph.mergeConservative(g1, g2),
+          (g1, g2) => Graph.mergeConservative([g1, g2]),
           new Graph()
         );
         expect(result.equals(demoData.advancedMealGraph())).toBe(true);
       });
 
-      it("conservatively merges a graph with itself", () => {
+      it("merges a graph with itself", () => {
-        const result = Graph.mergeConservative(
+        const result = Graph.mergeConservative([
           demoData.advancedMealGraph(),
-          demoData.advancedMealGraph()
+          demoData.advancedMealGraph(),
-        );
+        ]);
         expect(result.equals(demoData.advancedMealGraph())).toBe(true);
       });
 
-      it("conservatively merges graphs of different payload types", () => {
+      it("rejects a graph with conflicting nodes", () => {
-        const data = {
-          a: () => ({
-            address: demoData.makeAddress("a", "EXPERIMENT"),
-            payload: "alpha",
-          }),
-          b: () => ({
-            address: demoData.makeAddress("b", "EXPERIMENT"),
-            payload: "bravo",
-          }),
-          u: () => ({
-            address: demoData.makeAddress("u", "EXPERIMENT"),
-            src: demoData.makeAddress("a", "EXPERIMENT"),
-            dst: demoData.makeAddress("b", "EXPERIMENT"),
-            payload: 21,
-          }),
-          c: () => ({
-            address: demoData.makeAddress("c", "EXPERIMENT"),
-            payload: true,
-          }),
-          d: () => ({
-            address: demoData.makeAddress("d", "EXPERIMENT"),
-            payload: false,
-          }),
-          v: () => ({
-            address: demoData.makeAddress("v", "EXPERIMENT"),
-            src: demoData.makeAddress("c", "EXPERIMENT"),
-            dst: demoData.makeAddress("d", "EXPERIMENT"),
-            payload: null,
-          }),
-        };
-        const g1: Graph = new Graph()
-          .addNode(data.a())
-          .addNode(data.b())
-          .addEdge(data.u());
-        const g2: Graph = new Graph()
-          .addNode(data.c())
-          .addNode(data.d())
-          .addEdge(data.v());
-        const result = Graph.mergeConservative(g1, g2);
-        const expected = new Graph()
-          .addNode(data.a())
-          .addNode(data.b())
-          .addEdge(data.u())
-          .addNode(data.c())
-          .addNode(data.d())
-          .addEdge(data.v());
-        expect(result.equals(expected)).toBe(true);
-      });
-
-      it("conservatively rejects a graph with conflicting nodes", () => {
         const makeGraph: (nodePayload: string) => Graph = (nodePayload) =>
           new Graph().addNode({
             address: demoData.makeAddress("conflicting-node", "EXPERIMENT"),
@@ -1025,11 +975,11 @@ describe("graph", () => {
         const g1 = makeGraph("one");
         const g2 = makeGraph("two");
         expect(() => {
-          Graph.mergeConservative(g1, g2);
+          Graph.mergeConservative([g1, g2]);
-        }).toThrow(/distinct nodes with address/);
+        }).toThrow(/node.*distinct contents/);
       });
 
-      it("conservatively rejects a graph with conflicting edges", () => {
+      it("rejects a graph with conflicting edges", () => {
         const srcAddress = demoData.makeAddress("src", "EXPERIMENT");
         const dstAddress = demoData.makeAddress("dst", "EXPERIMENT");
         const makeGraph: (edgePayload: string) => Graph = (edgePayload) =>
@@ -1045,40 +995,30 @@ describe("graph", () => {
         const g1 = makeGraph("one");
         const g2 = makeGraph("two");
         expect(() => {
-          Graph.mergeConservative(g1, g2);
+          Graph.mergeConservative([g1, g2]);
-        }).toThrow(/distinct edges with address/);
+        }).toThrow(/edge.*distinct contents/);
+      });
+
+      it("is the identity on a singleton list", () => {
+        const merged = Graph.mergeConservative([demoData.advancedMealGraph()]);
+        expect(merged.equals(demoData.advancedMealGraph())).toBe(true);
       });
 
-      function assertNotCalled(...args) {
-        throw new Error(`called with: ${args.join()}`);
-      }
       it("has the empty graph as a left identity", () => {
-        const merged = Graph.merge(
+        const merged = Graph.mergeConservative([
           new Graph(),
           demoData.advancedMealGraph(),
-          assertNotCalled,
+        ]);
-          assertNotCalled
-        );
         expect(merged.equals(demoData.advancedMealGraph())).toBe(true);
       });
+
       it("has the empty graph as a right identity", () => {
-        const merged = Graph.merge(
+        const merged = Graph.mergeConservative([
           demoData.advancedMealGraph(),
           new Graph(),
-          assertNotCalled,
+        ]);
-          assertNotCalled
-        );
         expect(merged.equals(demoData.advancedMealGraph())).toBe(true);
       });
-      it("trivially merges the empty graph with itself", () => {
-        const merged = Graph.merge(
-          new Graph(),
-          new Graph(),
-          assertNotCalled,
-          assertNotCalled
-        );
-        expect(merged.equals(new Graph())).toBe(true);
-      });
     });
 
     describe("JSON functions", () => {

src/plugins/git/createGraph.js

@@ -40,7 +40,7 @@ class GitGraphCreator {
     const treeAndNameToSubmoduleUrls = this.treeAndNameToSubmoduleUrls(
       repository
     );
-    return Graph.mergeManyConservative([
+    return Graph.mergeConservative([
       ...Object.keys(repository.commits).map((hash) =>
         this.commitGraph(repository.commits[hash])
       ),

src/tools/loadCombinedGraph.js

@@ -31,5 +31,5 @@ export function loadCombinedGraph(
 ): Promise<Graph> {
   const githubGraphPromise = fetchGithubGraph(repoOwner, repoName, token);
   const gitGraph = cloneGitGraph(repoOwner, repoName);
-  return githubGraphPromise.then((x) => Graph.mergeConservative(gitGraph, x));
+  return githubGraphPromise.then((x) => Graph.mergeConservative([gitGraph, x]));
 }