From 389c7a1ab50891cd67ba1f8dae6110e91f6447a7 Mon Sep 17 00:00:00 2001 From: Sebastian Lorenz Date: Tue, 16 Jun 2026 18:20:25 +0200 Subject: [PATCH] Backport graph fixes --- .changeset/graph-algorithm-fixes.md | 5 + packages/effect/src/Graph.ts | 226 ++++++++++++++++++---------- packages/effect/test/Graph.test.ts | 182 ++++++++++++++++++++++ 3 files changed, 336 insertions(+), 77 deletions(-) create mode 100644 .changeset/graph-algorithm-fixes.md diff --git a/.changeset/graph-algorithm-fixes.md b/.changeset/graph-algorithm-fixes.md new file mode 100644 index 00000000000..1763d053ff0 --- /dev/null +++ b/.changeset/graph-algorithm-fixes.md @@ -0,0 +1,5 @@ +--- +"effect": minor +--- + +Add `Graph.successors` and `Graph.predecessors`, deprecate `Graph.neighborsDirected`, and fix graph algorithm edge cases around reversal, undirected edge queries, shortest-path weight validation, topological sort initials, and strongly connected components. diff --git a/packages/effect/src/Graph.ts b/packages/effect/src/Graph.ts index 0cc47d94bce..3f789978fcb 100644 --- a/packages/effect/src/Graph.ts +++ b/packages/effect/src/Graph.ts @@ -843,6 +843,29 @@ export const mapEdges = ( } } +/** @internal */ +const rebuildAdjacency = ( + mutable: MutableGraph +): void => { + mutable.adjacency.clear() + mutable.reverseAdjacency.clear() + + for (const nodeIndex of mutable.nodes.keys()) { + mutable.adjacency.set(nodeIndex, []) + mutable.reverseAdjacency.set(nodeIndex, []) + } + + for (const [edgeIndex, edgeData] of mutable.edges) { + mutable.adjacency.get(edgeData.source)!.push(edgeIndex) + mutable.reverseAdjacency.get(edgeData.target)!.push(edgeIndex) + + if (mutable.type === "undirected") { + mutable.adjacency.get(edgeData.target)!.push(edgeIndex) + mutable.reverseAdjacency.get(edgeData.source)!.push(edgeIndex) + } + } +} + /** * Reverses all edge directions in a mutable graph by swapping source and target nodes. * @@ -869,6 +892,10 @@ export const mapEdges = ( export const reverse = ( mutable: MutableGraph ): void => { + if (mutable.type === "undirected") { + return + } + // Reverse all edges by swapping source and target for (const [index, edgeData] of mutable.edges) { mutable.edges.set(index, { @@ -878,22 +905,7 @@ export const reverse = ( }) } - // Clear and rebuild adjacency lists with reversed directions - mutable.adjacency.clear() - mutable.reverseAdjacency.clear() - - // Rebuild adjacency lists with reversed directions - for (const [edgeIndex, edgeData] of mutable.edges) { - // Add to forward adjacency (source -> target) - const sourceEdges = mutable.adjacency.get(edgeData.source) || [] - sourceEdges.push(edgeIndex) - mutable.adjacency.set(edgeData.source, sourceEdges) - - // Add to reverse adjacency (target <- source) - const targetEdges = mutable.reverseAdjacency.get(edgeData.target) || [] - targetEdges.push(edgeIndex) - mutable.reverseAdjacency.set(edgeData.target, targetEdges) - } + rebuildAdjacency(mutable) // Invalidate cycle flag since edge directions changed mutable.isAcyclic = Option.none() @@ -1423,8 +1435,11 @@ export const hasEdge = ( // Check if any edge in the adjacency list connects to the target for (const edgeIndex of adjacencyList) { const edge = graph.edges.get(edgeIndex) - if (edge !== undefined && edge.target === target) { - return true + if (edge !== undefined) { + const neighbor = graph.type === "undirected" && edge.target === source ? edge.source : edge.target + if (neighbor === target) { + return true + } } } @@ -1460,6 +1475,31 @@ export const edgeCount = ( graph: Graph | MutableGraph ): number => graph.edges.size +const getDirectedNeighbors = ( + graph: Graph | MutableGraph, + nodeIndex: NodeIndex, + direction: Direction +): Array => { + const adjacencyMap = direction === "incoming" + ? graph.reverseAdjacency + : graph.adjacency + + const adjacencyList = adjacencyMap.get(nodeIndex) + if (adjacencyList === undefined) { + return [] + } + + const result: Array = [] + for (const edgeIndex of adjacencyList) { + const edge = graph.edges.get(edgeIndex) + if (edge !== undefined) { + result.push(direction === "incoming" ? edge.source : edge.target) + } + } + + return result +} + /** * Returns the neighboring nodes (targets of outgoing edges) for a given node. * @@ -1498,24 +1538,47 @@ export const neighbors = ( return getUndirectedNeighbors(graph as any, nodeIndex) } - const adjacencyList = graph.adjacency.get(nodeIndex) - if (adjacencyList === undefined) { - return [] - } + return getDirectedNeighbors(graph as Graph | MutableGraph, nodeIndex, "outgoing") +} - const result: Array = [] - for (const edgeIndex of adjacencyList) { - const edge = graph.edges.get(edgeIndex) - if (edge !== undefined) { - result.push(edge.target) - } +/** + * Returns the outgoing neighbor node indices for a node in a directed graph. + * + * Throws a `GraphError` when used with an undirected graph. + * + * @since 3.22.0 + * @category queries + */ +export const successors = ( + graph: Graph | MutableGraph, + nodeIndex: NodeIndex +): Array => { + if ((graph as Graph | MutableGraph).type === "undirected") { + throw new GraphError({ message: "Cannot get successors of undirected graph" }) } + return getDirectedNeighbors(graph, nodeIndex, "outgoing") +} - return result +/** + * Returns the incoming neighbor node indices for a node in a directed graph. + * + * Throws a `GraphError` when used with an undirected graph. + * + * @since 3.22.0 + * @category queries + */ +export const predecessors = ( + graph: Graph | MutableGraph, + nodeIndex: NodeIndex +): Array => { + if ((graph as Graph | MutableGraph).type === "undirected") { + throw new GraphError({ message: "Cannot get predecessors of undirected graph" }) + } + return getDirectedNeighbors(graph, nodeIndex, "incoming") } /** - * Get neighbors of a node in a specific direction for bidirectional traversal. + * Get directed neighbors of a node in a specific direction. * * @example * ```ts @@ -1537,36 +1600,19 @@ export const neighbors = ( * const incoming = Graph.neighborsDirected(graph, nodeB, "incoming") * ``` * + * @deprecated Use {@link successors} for outgoing neighbors or {@link predecessors} for incoming neighbors. * @since 3.18.0 * @category queries */ -export const neighborsDirected = ( - graph: Graph | MutableGraph, +export const neighborsDirected = ( + graph: Graph | MutableGraph, nodeIndex: NodeIndex, direction: Direction ): Array => { - const adjacencyMap = direction === "incoming" - ? graph.reverseAdjacency - : graph.adjacency - - const adjacencyList = adjacencyMap.get(nodeIndex) - if (adjacencyList === undefined) { - return [] - } - - const result: Array = [] - for (const edgeIndex of adjacencyList) { - const edge = graph.edges.get(edgeIndex) - if (edge !== undefined) { - // For incoming direction, we want the source node instead of target - const neighborNode = direction === "incoming" - ? edge.source - : edge.target - result.push(neighborNode) - } + if ((graph as Graph | MutableGraph).type === "undirected") { + throw new GraphError({ message: "Cannot get directed neighbors of undirected graph" }) } - - return result + return getDirectedNeighbors(graph, nodeIndex, direction) } // ============================================================================= @@ -1961,7 +2007,11 @@ export const isAcyclic = ( recursionStack.add(node) // Get neighbors for this node - const nodeNeighbors = Array.from(neighborsDirected(graph, node, "outgoing")) + const nodeNeighbors = getDirectedNeighbors( + graph as Graph | MutableGraph, + node, + "outgoing" + ) stack[stack.length - 1] = [node, nodeNeighbors, 0, false] continue } @@ -2112,7 +2162,7 @@ const getTraversalNeighbors = ( ): Array => graph.type === "undirected" ? getUndirectedNeighbors(graph as any, nodeIndex) - : neighborsDirected(graph, nodeIndex, direction) + : getDirectedNeighbors(graph as Graph | MutableGraph, nodeIndex, direction) const getTraversableNeighbor = ( graph: Graph | MutableGraph, @@ -2202,9 +2252,13 @@ export const connectedComponents = ( * @since 3.18.0 * @category algorithms */ -export const stronglyConnectedComponents = ( - graph: Graph | MutableGraph +export const stronglyConnectedComponents = ( + graph: Graph | MutableGraph ): Array> => { + if ((graph as Graph | MutableGraph).type === "undirected") { + throw new GraphError({ message: "Cannot find strongly connected components of undirected graph" }) + } + const visited = new Set() const finishOrder: Array = [] // Iterate directly over node keys @@ -2230,7 +2284,7 @@ export const stronglyConnectedComponents = ( } visited.add(node) - const nodeNeighborsList = neighbors(graph, node) + const nodeNeighborsList = getDirectedNeighbors(graph, node, "outgoing") stack[stack.length - 1] = [node, nodeNeighborsList, 0, false] continue } @@ -2348,6 +2402,22 @@ export interface BellmanFordConfig { cost: (edgeData: E) => number } +const validateNonNegativeEdgeWeights = ( + graph: Graph | MutableGraph, + cost: (edgeData: E) => number, + algorithm: string +): Map => { + const edgeWeights = new Map() + for (const [edgeIndex, edgeData] of graph.edges) { + const weight = cost(edgeData.data) + if (weight < 0 || Number.isNaN(weight)) { + throw new GraphError({ message: `${algorithm} requires non-negative edge weights` }) + } + edgeWeights.set(edgeIndex, weight) + } + return edgeWeights +} + /** * Find the shortest path between two nodes using Dijkstra's algorithm. * @@ -2390,6 +2460,8 @@ export const dijkstra = ( throw missingNode(target) } + const edgeWeights = validateNonNegativeEdgeWeights(graph, cost, "Dijkstra's algorithm") + // Early return if source equals target if (source === target) { return Option.some({ @@ -2450,12 +2522,7 @@ export const dijkstra = ( const edge = graph.edges.get(edgeIndex) if (edge !== undefined) { const neighbor = getTraversableNeighbor(graph, currentNode, edge) - const weight = cost(edge.data) - - // Validate non-negative weights - if (weight < 0) { - throw new Error(`Dijkstra's algorithm requires non-negative edge weights, found ${weight}`) - } + const weight = edgeWeights.get(edgeIndex)! const newDistance = currentDistance + weight const neighborDistance = distances.get(neighbor)! @@ -2709,6 +2776,8 @@ export const astar = ( throw missingNode(target) } + const edgeWeights = validateNonNegativeEdgeWeights(graph, cost, "A* algorithm") + // Early return if source equals target if (source === target) { return Option.some({ @@ -2784,12 +2853,7 @@ export const astar = ( const edge = graph.edges.get(edgeIndex) if (edge !== undefined) { const neighbor = getTraversableNeighbor(graph, currentNode, edge) - const weight = cost(edge.data) - - // Validate non-negative weights - if (weight < 0) { - throw new Error(`A* algorithm requires non-negative edge weights, found ${weight}`) - } + const weight = edgeWeights.get(edgeIndex)! const tentativeGScore = currentGScore + weight const neighborGScore = gScore.get(neighbor)! @@ -3468,6 +3532,7 @@ export const topo = ( [Symbol.iterator]: () => { const inDegree = new Map() const remaining = new Set() + const initialSet = new Set(initials) const queue = [...initials] // Initialize in-degree counts @@ -3482,12 +3547,15 @@ export const topo = ( inDegree.set(edgeData.target, currentInDegree + 1) } - // Add nodes with zero in-degree to queue if no initials provided - if (initials.length === 0) { - for (const [nodeIndex, degree] of inDegree) { - if (degree === 0) { - queue.push(nodeIndex) - } + for (const nodeIndex of initials) { + if (inDegree.get(nodeIndex)! !== 0) { + throw new GraphError({ message: `Initial node ${nodeIndex} has incoming edges` }) + } + } + + for (const [nodeIndex, degree] of inDegree) { + if (degree === 0 && !initialSet.has(nodeIndex)) { + queue.push(nodeIndex) } } @@ -3499,7 +3567,11 @@ export const topo = ( remaining.delete(current) // Process outgoing edges, reducing in-degree of targets - const neighbors = neighborsDirected(graph, current, "outgoing") + const neighbors = getDirectedNeighbors( + graph as Graph | MutableGraph, + current, + "outgoing" + ) for (const neighbor of neighbors) { if (remaining.has(neighbor)) { const currentInDegree = inDegree.get(neighbor) || 0 diff --git a/packages/effect/test/Graph.test.ts b/packages/effect/test/Graph.test.ts index 5cb9d252ffa..b0b71208ab4 100644 --- a/packages/effect/test/Graph.test.ts +++ b/packages/effect/test/Graph.test.ts @@ -847,6 +847,37 @@ describe("Graph", () => { expect(Array.from(neighborsB)).toEqual([0]) // B -> A expect(Array.from(neighborsC)).toEqual([0]) // C -> A }) + + it("should preserve adjacency lists when adding edges after reversal", () => { + const graph = Graph.directed((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + + Graph.addEdge(mutable, a, b, 1) + Graph.reverse(mutable) + Graph.addEdge(mutable, a, b, 2) + }) + + expect(Graph.edgeCount(graph)).toBe(2) + expect(Graph.neighbors(graph, 0)).toEqual([1]) + expect(Graph.hasEdge(graph, 0, 1)).toBe(true) + expect(Graph.hasEdge(graph, 1, 0)).toBe(true) + }) + + it("should be a no-op for undirected graphs", () => { + const graph = Graph.undirected((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + + Graph.addEdge(mutable, a, b, 1) + Graph.reverse(mutable) + }) + + expect(Graph.neighbors(graph, 0)).toEqual([1]) + expect(Graph.neighbors(graph, 1)).toEqual([0]) + expect(Graph.hasEdge(graph, 0, 1)).toBe(true) + expect(Graph.hasEdge(graph, 1, 0)).toBe(true) + }) }) describe("filterMapNodes", () => { @@ -1467,6 +1498,17 @@ describe("Graph", () => { expect(Graph.hasEdge(graph, nodeA, nodeB)).toBe(false) }) + + it("should be symmetric for undirected graphs", () => { + const graph = Graph.undirected((mutable) => { + const nodeA = Graph.addNode(mutable, "Node A") + const nodeB = Graph.addNode(mutable, "Node B") + Graph.addEdge(mutable, nodeA, nodeB, 42) + }) + + expect(Graph.hasEdge(graph, 0, 1)).toBe(true) + expect(Graph.hasEdge(graph, 1, 0)).toBe(true) + }) }) describe("edgeCount", () => { @@ -1572,6 +1614,32 @@ describe("Graph", () => { }) }) + describe("successors and predecessors", () => { + it("should return outgoing and incoming directed neighbors", () => { + const graph = Graph.directed((mutable) => { + const nodeA = Graph.addNode(mutable, "Node A") + const nodeB = Graph.addNode(mutable, "Node B") + const nodeC = Graph.addNode(mutable, "Node C") + Graph.addEdge(mutable, nodeA, nodeB, 1) + Graph.addEdge(mutable, nodeC, nodeB, 2) + }) + + expect(Graph.successors(graph, 0)).toEqual([1]) + expect(Graph.predecessors(graph, 1).sort()).toEqual([0, 2]) + }) + + it("should throw for undirected graphs", () => { + const graph = Graph.undirected((mutable) => { + const nodeA = Graph.addNode(mutable, "Node A") + const nodeB = Graph.addNode(mutable, "Node B") + Graph.addEdge(mutable, nodeA, nodeB, 1) + }) + + expect(() => Graph.successors(graph as any, 0)).toThrow("Cannot get successors of undirected graph") + expect(() => Graph.predecessors(graph as any, 0)).toThrow("Cannot get predecessors of undirected graph") + }) + }) + describe("neighborsDirected", () => { it("should return incoming neighbors", () => { let nodeA: Graph.NodeIndex @@ -1623,6 +1691,18 @@ describe("Graph", () => { expect(Graph.neighborsDirected(graph, nodeA!, "incoming")).toEqual([]) expect(Graph.neighborsDirected(graph, nodeA!, "outgoing")).toEqual([]) }) + + it("should throw for undirected graphs", () => { + const graph = Graph.undirected((mutable) => { + const nodeA = Graph.addNode(mutable, "Node A") + const nodeB = Graph.addNode(mutable, "Node B") + Graph.addEdge(mutable, nodeA, nodeB, 1) + }) + + expect(() => Graph.neighborsDirected(graph as any, 0, "outgoing")).toThrow( + "Cannot get directed neighbors of undirected graph" + ) + }) }) }) @@ -2364,6 +2444,14 @@ describe("Graph", () => { expect(scc).toHaveLength(2) }) }) + + it("should throw for undirected graphs", () => { + const graph = makeReversedUndirectedPath() + + expect(() => Graph.stronglyConnectedComponents(graph as any)).toThrow( + "Cannot find strongly connected components of undirected graph" + ) + }) }) describe("dijkstra", () => { @@ -2438,6 +2526,40 @@ describe("Graph", () => { ) }) + it("should throw for negative weights before early target termination", () => { + const graph = Graph.directed((mutable) => { + const source = Graph.addNode(mutable, "source") + const target = Graph.addNode(mutable, "target") + const other = Graph.addNode(mutable, "other") + Graph.addEdge(mutable, source, target, 1) + Graph.addEdge(mutable, source, other, 2) + Graph.addEdge(mutable, other, target, -5) + }) + + expect(() => + Graph.dijkstra(graph, { + source: 0, + target: 1, + cost: (edge) => edge + }) + ).toThrow("Dijkstra's algorithm requires non-negative edge weights") + }) + + it("should validate weights before returning same source and target", () => { + const graph = Graph.directed((mutable) => { + const node = Graph.addNode(mutable, "node") + Graph.addEdge(mutable, node, node, -1) + }) + + expect(() => + Graph.dijkstra(graph, { + source: 0, + target: 0, + cost: (edge) => edge + }) + ).toThrow("Dijkstra's algorithm requires non-negative edge weights") + }) + it("should throw for non-existent nodes", () => { const graph = Graph.directed() @@ -2533,6 +2655,42 @@ describe("Graph", () => { ) }) + it("should throw for negative weights before early target termination", () => { + const graph = Graph.directed<{ x: number; y: number }, number>((mutable) => { + const source = Graph.addNode(mutable, { x: 0, y: 0 }) + const target = Graph.addNode(mutable, { x: 1, y: 0 }) + const other = Graph.addNode(mutable, { x: 2, y: 0 }) + Graph.addEdge(mutable, source, target, 1) + Graph.addEdge(mutable, source, other, 2) + Graph.addEdge(mutable, other, target, -5) + }) + + expect(() => + Graph.astar(graph, { + source: 0, + target: 1, + cost: (edge) => edge, + heuristic: () => 0 + }) + ).toThrow("A* algorithm requires non-negative edge weights") + }) + + it("should validate weights before returning same source and target", () => { + const graph = Graph.directed<{ x: number; y: number }, number>((mutable) => { + const node = Graph.addNode(mutable, { x: 0, y: 0 }) + Graph.addEdge(mutable, node, node, -1) + }) + + expect(() => + Graph.astar(graph, { + source: 0, + target: 0, + cost: (edge) => edge, + heuristic: () => 0 + }) + ).toThrow("A* algorithm requires non-negative edge weights") + }) + it("should traverse undirected edges in reverse storage direction", () => { const graph = makeReversedUndirectedPath() @@ -2816,6 +2974,30 @@ describe("Graph", () => { expect(entries).toEqual([[0, "A"], [1, "B"], [2, "C"]]) }) + it("should prioritize valid initials and still include all nodes", () => { + const graph = Graph.directed((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + const c = Graph.addNode(mutable, "C") + const d = Graph.addNode(mutable, "D") + Graph.addEdge(mutable, a, b, 1) + Graph.addEdge(mutable, c, d, 2) + }) + + const order = Array.from(Graph.indices(Graph.topo(graph, { initials: [2] }))) + expect(order).toEqual([2, 0, 3, 1]) + }) + + it("should reject initials with incoming edges", () => { + const graph = Graph.directed((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + Graph.addEdge(mutable, a, b, 1) + }) + + expect(() => Array.from(Graph.topo(graph, { initials: [1] }))).toThrow("Initial node 1 has incoming edges") + }) + it("should throw for cyclic graphs", () => { const cyclicGraph = Graph.directed((mutable) => { const a = Graph.addNode(mutable, "A")