From 1083540d2109e782b52cf190a582a6a531f2f318 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Sat, 17 Mar 2018 11:36:53 -0700 Subject: [PATCH] Parameterize `Graph` over node and edge payloads (#83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Closes #82. This affords clients type-safety without needing to verbosely annotate every node or edge passed into the graph functions. It also enables graph algorithms to be more expressive in their types: for instance, the merge function now clearly indicates from its type that the first graph’s nodes are passed as the first argument to the node reducer, and the second graph’s nodes to the second. Clients can upgrade immediately by using `Graph<*, *>`. Thankfully, Flow supports variance well enough for this all to be possible without too much trouble. Test Plan: Existing unit tests pass statically and at runtime. I added a test case to demonstrate that merging works covariantly. To see some failures, change `string` to an incompatible type, like `number`, in the definitions of `makeGraph` in test functions for conservatively rejecting graphs with conflicting nodes/edges (ll. 446, 462). wchargin-branch: parameterize-graph --- src/backend/graph.js | 66 +++++++++++++++++++++------------------ src/backend/graph.test.js | 53 ++++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 35 deletions(-) diff --git a/src/backend/graph.js b/src/backend/graph.js index 56e595b..4eb2bb2 100644 --- a/src/backend/graph.js +++ b/src/backend/graph.js @@ -4,26 +4,26 @@ import deepEqual from "lodash.isequal"; import type {Address, Addressable, AddressMapJSON} from "./address"; import {AddressMap} from "./address"; -export type Node = {| +export type Node<+T> = {| +address: Address, +payload: T, |}; -export type Edge = {| +export type Edge<+T> = {| +address: Address, +src: Address, +dst: Address, +payload: T, |}; -export type GraphJSON = {| - +nodes: AddressMapJSON>, - +edges: AddressMapJSON>, +export type GraphJSON = {| + +nodes: AddressMapJSON>, + +edges: AddressMapJSON>, |}; -export class Graph { - _nodes: AddressMap>; - _edges: AddressMap>; +export class Graph { + _nodes: AddressMap>; + _edges: AddressMap>; // The keyset of each of the following fields should equal the keyset // of `_nodes`. If `e` is an edge from `u` to `v`, then `e.address` @@ -40,18 +40,18 @@ export class Graph { this._inEdges = new AddressMap(); } - equals(that: Graph): boolean { + equals(that: Graph): boolean { return this._nodes.equals(that._nodes) && this._edges.equals(that._edges); } - toJSON(): GraphJSON { + toJSON(): GraphJSON { return { nodes: this._nodes.toJSON(), edges: this._edges.toJSON(), }; } - static fromJSON(json: GraphJSON): Graph { + static fromJSON(json: GraphJSON): Graph { const result = new Graph(); AddressMap.fromJSON(json.nodes) .getAll() @@ -66,7 +66,7 @@ export class Graph { return result; } - addNode(node: Node) { + addNode(node: Node): Graph { if (node == null) { throw new Error(`node is ${String(node)}`); } @@ -88,7 +88,7 @@ export class Graph { return this; } - addEdge(edge: Edge) { + addEdge(edge: Edge): Graph { if (edge == null) { throw new Error(`edge is ${String(edge)}`); } @@ -116,11 +116,11 @@ export class Graph { return this; } - getNode(address: Address): Node { + getNode(address: Address): Node { return this._nodes.get(address); } - getEdge(address: Address): Edge { + getEdge(address: Address): Edge { return this._edges.get(address); } @@ -128,7 +128,7 @@ export class Graph { * Gets the array of all out-edges from the node at the given address. * The order of the resulting array is unspecified. */ - getOutEdges(nodeAddress: Address): Edge[] { + getOutEdges(nodeAddress: Address): Edge[] { if (nodeAddress == null) { throw new Error(`address is ${String(nodeAddress)}`); } @@ -143,7 +143,7 @@ export class Graph { * Gets the array of all in-edges to the node at the given address. * The order of the resulting array is unspecified. */ - getInEdges(nodeAddress: Address): Edge[] { + getInEdges(nodeAddress: Address): Edge[] { if (nodeAddress == null) { throw new Error(`address is ${String(nodeAddress)}`); } @@ -157,14 +157,14 @@ export class Graph { /** * Gets all nodes in the graph, in unspecified order. */ - getAllNodes(): Node[] { + getAllNodes(): Node[] { return this._nodes.getAll(); } /** * Gets all edges in the graph, in unspecified order. */ - getAllEdges(): Edge[] { + getAllEdges(): Edge[] { return this._edges.getAll(); } @@ -176,13 +176,13 @@ export class Graph { * * The existing graph objects are not modified. */ - static merge( - g1: Graph, - g2: Graph, - nodeResolver: (Node, Node) => Node, - edgeResolver: (Edge, Edge) => Edge - ) { - const result = new Graph(); + static merge( + g1: Graph, + g2: Graph, + nodeResolver: (Node, Node) => Node, + edgeResolver: (Edge, Edge) => Edge + ): Graph { + const result: Graph = new Graph(); g1.getAllNodes().forEach((node) => { if (g2.getNode(node.address) !== undefined) { const resolved = nodeResolver(node, g2.getNode(node.address)); @@ -218,8 +218,11 @@ export class Graph { * for edges). If this assumption does not hold, this function will * raise an error. */ - static mergeConservative(g1: Graph, g2: Graph) { - function conservativeReducer( + static mergeConservative( + g1: Graph, + g2: Graph + ): Graph { + function conservativeResolver( kinds: string /* used for an error message on mismatch */, a: T, b: T @@ -232,11 +235,12 @@ export class Graph { ); } } - return Graph.merge( + const result: Graph = Graph.merge( g1, g2, - (u, v) => conservativeReducer("nodes", u, v), - (e, f) => conservativeReducer("edges", e, f) + (u, v) => conservativeResolver("nodes", u, v), + (e, f) => conservativeResolver("edges", e, f) ); + return result; } } diff --git a/src/backend/graph.test.js b/src/backend/graph.test.js index 1317a84..2ddd9bc 100644 --- a/src/backend/graph.test.js +++ b/src/backend/graph.test.js @@ -376,7 +376,9 @@ describe("graph", () => { * node `u`, create a graph with just that node, its neighbors, * and its incident edges (in both directions). */ - function neighborhoodDecomposition(originalGraph: Graph): Graph[] { + function neighborhoodDecomposition( + originalGraph: Graph + ): Graph[] { return originalGraph.getAllNodes().map((node) => { const miniGraph = new Graph(); miniGraph.addNode(node); @@ -403,7 +405,9 @@ describe("graph", () => { * Decompose the given graph into edge graphs: for each edge `e`, * create a graph with just that edge and its two endpoints. */ - function edgeDecomposition(originalGraph: Graph): Graph[] { + function edgeDecomposition( + originalGraph: Graph + ): Graph[] { return originalGraph.getAllEdges().map((edge) => { const miniGraph = new Graph(); miniGraph.addNode(originalGraph.getNode(edge.src)); @@ -439,8 +443,47 @@ describe("graph", () => { expect(result.equals(demoData.advancedMealGraph())).toBe(true); }); + it("conservatively merges graphs of different payload types", () => { + const data = { + a: () => ({address: demoData.makeAddress("a"), payload: "alpha"}), + b: () => ({address: demoData.makeAddress("b"), payload: "bravo"}), + u: () => ({ + address: demoData.makeAddress("u"), + src: demoData.makeAddress("a"), + dst: demoData.makeAddress("b"), + payload: 21, + }), + c: () => ({address: demoData.makeAddress("c"), payload: true}), + d: () => ({address: demoData.makeAddress("d"), payload: false}), + v: () => ({ + address: demoData.makeAddress("v"), + src: demoData.makeAddress("c"), + dst: demoData.makeAddress("d"), + 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()); + type ResultGraph = Graph; + const result: ResultGraph = Graph.mergeConservative(g1, g2); + const expected: ResultGraph = 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) => + const makeGraph: (nodePayload: string) => Graph<*, *> = (nodePayload) => new Graph().addNode({ address: demoData.makeAddress("conflicting-node"), payload: nodePayload, @@ -455,7 +498,7 @@ describe("graph", () => { it("conservatively rejects a graph with conflicting edges", () => { const srcAddress = demoData.makeAddress("src"); const dstAddress = demoData.makeAddress("dst"); - const makeGraph: (edgePayload: string) => Graph = (edgePayload) => + const makeGraph: (edgePayload: string) => Graph<*, *> = (edgePayload) => new Graph() .addNode({address: srcAddress, payload: {}}) .addNode({address: dstAddress, payload: {}}) @@ -558,6 +601,7 @@ describe("graph", () => { address: demoData.makeAddress("hello"), payload: 17, }; + // This will be a Graph. new Graph().addNode(stringNode).addNode(numberNode); }); }); @@ -578,6 +622,7 @@ describe("graph", () => { dst: dst.address, payload: 18, }; + // This will be a Graph<{}, string | number>. new Graph() .addNode(src) .addNode(dst)