Fix undirected graph traversal

.changeset/fix-graph-undirected-traversal.md

@@ -0,0 +1,5 @@
+---
+"effect": patch
+---
+
+Fix Graph traversal and shortest-path algorithms to traverse undirected edges independently of their stored source/target orientation.

packages/effect/src/Graph.ts

@@ -1894,6 +1894,37 @@ export const isAcyclic = <N, E, T extends Kind = "directed">(
     return graph.isAcyclic.value
   }
 
+  if (graph.type === "undirected") {
+    const visited = new Set<NodeIndex>()
+
+    for (const startNode of graph.nodes.keys()) {
+      if (visited.has(startNode)) {
+        continue
+      }
+
+      visited.add(startNode)
+      const stack: Array<{ node: NodeIndex; parent: NodeIndex | null }> = [{ node: startNode, parent: null }]
+
+      while (stack.length > 0) {
+        const { node, parent } = stack.pop()!
+        const nodeNeighbors = getUndirectedNeighbors(graph as any, node)
+
+        for (const neighbor of nodeNeighbors) {
+          if (!visited.has(neighbor)) {
+            visited.add(neighbor)
+            stack.push({ node: neighbor, parent: node })
+          } else if (neighbor !== parent) {
+            graph.isAcyclic = Option.some(false)
+            return false
+          }
+        }
+      }
+    }
+
+    graph.isAcyclic = Option.some(true)
+    return true
+  }
+
   // Stack-safe DFS cycle detection using iterative approach
   const visited = new Set<NodeIndex>()
   const recursionStack = new Set<NodeIndex>()
@@ -2074,6 +2105,21 @@ const getUndirectedNeighbors = <N, E>(
   return Array.from(neighbors)
 }
 
+const getTraversalNeighbors = <N, E, T extends Kind>(
+  graph: Graph<N, E, T> | MutableGraph<N, E, T>,
+  nodeIndex: NodeIndex,
+  direction: Direction
+): Array<NodeIndex> =>
+  graph.type === "undirected"
+    ? getUndirectedNeighbors(graph as any, nodeIndex)
+    : neighborsDirected(graph, nodeIndex, direction)
+
+const getTraversableNeighbor = <N, E, T extends Kind>(
+  graph: Graph<N, E, T> | MutableGraph<N, E, T>,
+  current: NodeIndex,
+  edge: Edge<E>
+): NodeIndex => graph.type === "undirected" && edge.target === current ? edge.source : edge.target
+
 /**
  * Find connected components in an undirected graph.
  * Each component is represented as an array of node indices.
@@ -2403,7 +2449,7 @@ export const dijkstra = <N, E, T extends Kind = "directed">(
       for (const edgeIndex of adjacencyList) {
         const edge = graph.edges.get(edgeIndex)
         if (edge !== undefined) {
-          const neighbor = edge.target
+          const neighbor = getTraversableNeighbor(graph, currentNode, edge)
           const weight = cost(edge.data)
 
           // Validate non-negative weights
@@ -2535,6 +2581,15 @@ export const floydWarshall = <N, E, T extends Kind = "directed">(
       next.get(i)!.set(j, j)
       edgeMatrix.get(i)!.set(j, edgeData.data)
     }
+
+    if (graph.type === "undirected") {
+      const reverseWeight = dist.get(j)!.get(i)!
+      if (weight < reverseWeight) {
+        dist.get(j)!.set(i, weight)
+        next.get(j)!.set(i, i)
+        edgeMatrix.get(j)!.set(i, edgeData.data)
+      }
+    }
   }
 
   // Floyd-Warshall main loop
@@ -2728,7 +2783,7 @@ export const astar = <N, E, T extends Kind = "directed">(
       for (const edgeIndex of adjacencyList) {
         const edge = graph.edges.get(edgeIndex)
         if (edge !== undefined) {
-          const neighbor = edge.target
+          const neighbor = getTraversableNeighbor(graph, currentNode, edge)
           const weight = cost(edge.data)
 
           // Validate non-negative weights
@@ -2864,6 +2919,14 @@ export const bellmanFord = <N, E, T extends Kind = "directed">(
       weight,
       edgeData: edgeData.data
     })
+    if (graph.type === "undirected" && edgeData.source !== edgeData.target) {
+      edges.push({
+        source: edgeData.target,
+        target: edgeData.source,
+        weight,
+        edgeData: edgeData.data
+      })
+    }
   }
 
   // Relax edges up to V-1 times
@@ -2905,14 +2968,8 @@ export const bellmanFord = <N, E, T extends Kind = "directed">(
         affectedNodes.add(node)
 
         // Add all nodes reachable from this node
-        const adjacencyList = graph.adjacency.get(node)
-        if (adjacencyList !== undefined) {
-          for (const edgeIndex of adjacencyList) {
-            const edge = graph.edges.get(edgeIndex)
-            if (edge !== undefined) {
-              queue.push(edge.target)
-            }
-          }
+        for (const neighbor of getTraversalNeighbors(graph, node, "outgoing")) {
+          queue.push(neighbor)
         }
       }
 
@@ -3230,7 +3287,7 @@ export const dfs = <N, E, T extends Kind = "directed">(
             continue
           }
 
-          const neighbors = neighborsDirected(graph, current, direction)
+          const neighbors = getTraversalNeighbors(graph, current, direction)
           for (let i = neighbors.length - 1; i >= 0; i--) {
             const neighbor = neighbors[i]
             if (!discovered.has(neighbor)) {
@@ -3307,7 +3364,7 @@ export const bfs = <N, E, T extends Kind = "directed">(
           if (!discovered.has(current)) {
             discovered.add(current)
 
-            const neighbors = neighborsDirected(graph, current, direction)
+            const neighbors = getTraversalNeighbors(graph, current, direction)
             for (const neighbor of neighbors) {
               if (!discovered.has(neighbor)) {
                 queue.push(neighbor)
@@ -3389,6 +3446,10 @@ export const topo = <N, E, T extends Kind = "directed">(
   graph: Graph<N, E, T> | MutableGraph<N, E, T>,
   config: TopoConfig = {}
 ): NodeWalker<N> => {
+  if (graph.type === "undirected") {
+    throw new Error("Cannot perform topological sort on undirected graph")
+  }
+
   // Check if graph is acyclic first
   if (!isAcyclic(graph)) {
     throw new Error("Cannot perform topological sort on cyclic graph")
@@ -3533,7 +3594,7 @@ export const dfsPostOrder = <N, E, T extends Kind = "directed">(
 
           if (!current.visitedChildren) {
             current.visitedChildren = true
-            const neighbors = neighborsDirected(graph, current.node, direction)
+            const neighbors = getTraversalNeighbors(graph, current.node, direction)
 
             for (let i = neighbors.length - 1; i >= 0; i--) {
               const neighbor = neighbors[i]

packages/effect/test/Graph.test.ts

@@ -1,6 +1,22 @@
 import { describe, expect, it } from "@effect/vitest"
 import { Equal, Graph, Hash, Option } from "effect"
 
+const makeReversedUndirectedPath = () =>
+  Graph.undirected<string, number>((mutable) => {
+    const a = Graph.addNode(mutable, "A")
+    const b = Graph.addNode(mutable, "B")
+    const c = Graph.addNode(mutable, "C")
+    Graph.addEdge(mutable, a, b, 1)
+    Graph.addEdge(mutable, c, b, 1)
+  })
+
+const expectSomePath = <E>(result: Option.Option<Graph.PathResult<E>>, expected: Graph.PathResult<E>) => {
+  expect(Option.isSome(result)).toBe(true)
+  if (Option.isSome(result)) {
+    expect(result.value).toEqual(expected)
+  }
+}
+
 describe("Graph", () => {
   describe("constructors", () => {
     it("should create empty directed graph", () => {
@@ -2133,6 +2149,25 @@ describe("Graph", () => {
 
         expect(Graph.isAcyclic(mixedComponents)).toBe(false)
       })
+
+      it("should treat a reversed-storage undirected chain as acyclic", () => {
+        const graph = makeReversedUndirectedPath()
+
+        expect(Graph.isAcyclic(graph)).toBe(true)
+      })
+
+      it("should detect cycles in undirected graphs", () => {
+        const graph = Graph.undirected<string, number>((mutable) => {
+          const a = Graph.addNode(mutable, "A")
+          const b = Graph.addNode(mutable, "B")
+          const c = Graph.addNode(mutable, "C")
+          Graph.addEdge(mutable, a, b, 1)
+          Graph.addEdge(mutable, b, c, 1)
+          Graph.addEdge(mutable, c, a, 1)
+        })
+
+        expect(Graph.isAcyclic(graph)).toBe(false)
+      })
     })
 
     describe("isBipartite", () => {
@@ -2410,6 +2445,18 @@ describe("Graph", () => {
           "Node 0 does not exist"
         )
       })
+
+      it("should traverse undirected edges in reverse storage direction", () => {
+        const graph = makeReversedUndirectedPath()
+
+        const result = Graph.dijkstra(graph, {
+          source: 0,
+          target: 2,
+          cost: (edge) => edge
+        })
+
+        expectSomePath(result, { path: [0, 1, 2], distance: 2, costs: [1, 1] })
+      })
     })
 
     describe("astar", () => {
@@ -2485,6 +2532,19 @@ describe("Graph", () => {
           "A* algorithm requires non-negative edge weights"
         )
       })
+
+      it("should traverse undirected edges in reverse storage direction", () => {
+        const graph = makeReversedUndirectedPath()
+
+        const result = Graph.astar(graph, {
+          source: 0,
+          target: 2,
+          cost: (edge) => edge,
+          heuristic: () => 0
+        })
+
+        expectSomePath(result, { path: [0, 1, 2], distance: 2, costs: [1, 1] })
+      })
     })
 
     describe("bellmanFord", () => {
@@ -2547,6 +2607,34 @@ describe("Graph", () => {
         const result = Graph.bellmanFord(graph, { source: 0, target: 2, cost: (edge) => edge })
         expect(Option.isNone(result)).toBe(true)
       })
+
+      it("should traverse undirected edges in reverse storage direction", () => {
+        const graph = makeReversedUndirectedPath()
+
+        const result = Graph.bellmanFord(graph, {
+          source: 0,
+          target: 2,
+          cost: (edge) => edge
+        })
+
+        expectSomePath(result, { path: [0, 1, 2], distance: 2, costs: [1, 1] })
+      })
+
+      it("should treat a reachable negative undirected edge as a negative cycle", () => {
+        const graph = Graph.undirected<string, number>((mutable) => {
+          const a = Graph.addNode(mutable, "A")
+          const b = Graph.addNode(mutable, "B")
+          Graph.addEdge(mutable, a, b, -1)
+        })
+
+        const result = Graph.bellmanFord(graph, {
+          source: 0,
+          target: 1,
+          cost: (edge) => edge
+        })
+
+        expect(Option.isNone(result)).toBe(true)
+      })
     })
 
     describe("floydWarshall", () => {
@@ -2615,6 +2703,26 @@ describe("Graph", () => {
 
         expect(() => Graph.floydWarshall(graph, (edge) => edge)).toThrow("Negative cycle detected")
       })
+
+      it("should traverse undirected edges in reverse storage direction", () => {
+        const graph = makeReversedUndirectedPath()
+
+        const result = Graph.floydWarshall(graph, (edge) => edge)
+
+        expect(result.distances.get(0)?.get(2)).toBe(2)
+        expect(result.paths.get(0)?.get(2)).toEqual([0, 1, 2])
+        expect(result.costs.get(0)?.get(2)).toEqual([1, 1])
+      })
+
+      it("should treat negative undirected edges as negative cycles", () => {
+        const graph = Graph.undirected<string, number>((mutable) => {
+          const a = Graph.addNode(mutable, "A")
+          const b = Graph.addNode(mutable, "B")
+          Graph.addEdge(mutable, a, b, -1)
+        })
+
+        expect(() => Graph.floydWarshall(graph, (edge) => edge)).toThrow("Negative cycle detected")
+      })
     })
 
     describe("Iterator Base Methods", () => {
@@ -2719,6 +2827,12 @@ describe("Graph", () => {
         expect(() => Graph.topo(cyclicGraph)).toThrow("Cannot perform topological sort on cyclic graph")
       })
 
+      it("should throw for undirected graphs", () => {
+        const graph = makeReversedUndirectedPath()
+
+        expect(() => Graph.topo(graph)).toThrow("Cannot perform topological sort on undirected graph")
+      })
+
       it("should handle corrupted graph state during topological sort", () => {
         const graph = Graph.directed<string, number>((mutable) => {
           const a = Graph.addNode(mutable, "A")
@@ -2780,6 +2894,15 @@ describe("Graph", () => {
 
         expect(entries).toEqual([[2, "C"], [1, "B"], [0, "A"]]) // Postorder: children before parents
       })
+
+      it("should traverse undirected edges in reverse storage direction", () => {
+        const graph = makeReversedUndirectedPath()
+
+        expect(Array.from(Graph.indices(Graph.dfs(graph, { start: [0] })))).toEqual([0, 1, 2])
+        expect(Array.from(Graph.indices(Graph.dfs(graph, { start: [0], direction: "incoming" })))).toEqual([0, 1, 2])
+        expect(Array.from(Graph.indices(Graph.bfs(graph, { start: [0] })))).toEqual([0, 1, 2])
+        expect(Array.from(Graph.indices(Graph.dfsPostOrder(graph, { start: [0] })))).toEqual([2, 1, 0])
+      })
     })
 
     describe("DfsPostOrder Iterator", () => {