Add safe Graph functions - #7356
Conversation
🦋 Changeset detectedLatest commit: dc0187a The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tim-smart
left a comment
tim-smart
left a comment
There was a problem hiding this comment.
Generated by AI review: The mechanical throw to Result.fail refactor looks sound, but packages/effect/test/Graph.test.ts does not directly test any safe variant. The suite was migrated to *Unsafe, so it never proves that safe calls return Result.fail, Result.succeed, or Option.none instead of throwing. Please add paired safe/unsafe assertions for the main error classes, such as a missing node, invalid radius, invalid weight, cyclic input, and invalid limit.
Generated by AI review: Blocking, packages/effect/src/Graph.ts:2643: Returning Result from addEdge turns a missing endpoint into a silent no-op whenever the function is called for its side effect inside mutate, because the callback naturally discards the return value. The repo now uses addEdgeUnsafe everywhere, while the addEdge JSDoc example discards its Result. Please reconsider whether addEdge should remain throwing, like addNode.
Generated by AI review: packages/effect/src/Graph.ts:8366, 8570, 8925: traversalStarts already validated these starts and returned Result.fail, so these discarded calls are dead and bypass the safe contract because traversalStartPositions throws. Please remove them. The cyclic-graph throw inside topo's iterator at line 8827 also appears unreachable after the eager isAcyclic check and construction-time snapshot; either remove it or document why it remains.
Generated by AI review: packages/effect/src/Graph.ts:3223 and the other kind-mismatch throws: GraphError now represents both recoverable failures returned through Result and kind mismatches that still throw. A caller using Result.getOrThrow cannot distinguish those cases by type. Please give signature-excluded kind mismatches their own error type so GraphError has one meaning.
Generated by AI review: packages/effect/src/Graph.ts:1675, 6375, 6482: These fromSnapshot calls let internal snapshot-invariant failures escape through the public recoverable error channel. Please use Result.succeed(fromSnapshotUnsafe(...)) so invariant breaks remain defects rather than caller-facing failures.
Generated by AI review: packages/effect/src/Graph.ts:7488: The outer overflow flag makes each addWeight call require a repeated follow-up failure block, and it leads to an IIFE later just to fit that check into an expression. Returning number | undefined from addWeight would make overflow explicit and remove the mutable flag and repeated blocks.
Generated by AI review: packages/effect/src/Graph.ts:6764, 7236, 7472, 7909: These edge-weight collection and validation blocks are nearly identical, followed each time by the same Result.isFailure unwrap. A small internal helper parameterized by the validity predicate and message would remove the duplication. The specialized loops in minimumSpanningForest, floydWarshall, and solveMaximumFlow should stay separate.
| assert.deepStrictEqual(Graph.successorsUnsafe(0)(graph), [1, 0, 2]) | ||
| assert.deepStrictEqual(Graph.predecessorsUnsafe(graph, 0), [2, 0]) | ||
| assert.deepStrictEqual(Graph.neighborsDirectedUnsafe(graph, 0, "outgoing"), [1, 0, 2]) | ||
| assert.deepStrictEqual(Graph.successorsUnsafe(graph, 0.5), []) |
There was a problem hiding this comment.
Generated by AI review: Blocking: This assertion contradicts the new contract and is the source of the red Node and Deno jobs. successors now returns Option.none() for a missing node, while successorsUnsafe throws GraphError. Please replace this with paired assertions, for example:
assert.deepStrictEqual(Graph.successors(graph, 0.5), Option.none())
assertGraphError(() => Graph.successorsUnsafe(graph, 0.5), "Node 0.5 does not exist")| >() | ||
| expect(Graph.topo(mutableDirected)).type.toBe<Result.Result<Graph.NodeWalker<string>, Graph.GraphError>>() | ||
|
|
||
| expect(pipe(directed, Graph.topo())).type.toBe<Result.Result<Graph.NodeWalker<string>, Graph.GraphError>>() |
There was a problem hiding this comment.
Generated by AI review: Please add signature checks for the new *Unsafe functions, especially aliases such as incomingEdgesUnsafe, predecessorsUnsafe, inDegreeUnsafe, bfsUnsafe, and dfsPostOrderUnsafe, where a wrong typeof target can still compile. This rewrite also dropped the mutable pipeline assertion expect(pipe(mutableDirected, Graph.topo())).type.toBe<...>(); please restore it.
| "effect": patch | ||
| --- | ||
|
|
||
| Make recoverable `Graph` validation and algorithm failures explicit with `Result`, represent missing indexed queries with `Option`, and add throwing `*Unsafe` variants. |
There was a problem hiding this comment.
Generated by AI review: Please document two additional behavior changes shown by the rewritten tests: walkers now capture and replay one construction-time structural snapshot, and allShortestPaths evaluates cost eagerly at construction. Both can change observable behavior for mutable graphs, so the changeset should call them out.
| assertGraphError(() => Array.from(Graph.topo(graph, { initials: [1] })), "Initial node 1 has incoming edges") | ||
| assertGraphError(() => Graph.topo(graph, { initials: [2] }), "Node 2 does not exist") | ||
| assertGraphError( | ||
| () => Array.from(Graph.topoUnsafe(graph, { initials: [1] })), |
There was a problem hiding this comment.
Generated by AI review: This validation now happens when topoUnsafe is constructed, so Array.from(...) no longer forces anything. Please remove the wrapper so the test states the eager behavior directly.
2 participants
No description provided.