Improve rustworkx-core error interface

[kevinhartman]

What is the expected enhancement?

Currently, the Python API exposes several exception types for callers to handle specific error kinds, e.g. NodeNotFound, DAGWouldCycle, etc. In contrast, petgraph has no formal error representation; I see almost no case where its API surface returns a Result<T, E>, and certainly no cases where E is a unified Error type (i.e. an enum used across the library). Instead, the code simply panics when an invalid operation is performed.

For Rustworkx's Python API, custom exceptions are an obvious choice, given that panicking would exit the interpreter. However, it hasn't been clear if Rustworkx core should attempt to provide a formalized Error type to make usage more ergonomic for Rust callers since petgraph makes no attempt to do this, and Rustworkx core is effectively an extension to that library. However, the Rustlang book's guidlines for Error Handling indicates that returning a Result type is often preferable to panicking on things like invalid arguments received from a caller:

If someone calls your code and passes in values that don’t make sense, it’s best to return an error if you can so the user of the library can decide what they want to do in that case.

And, this is more relevant now in tandem with #1121 as we begin porting more of the algorithms exclusively available via the Python API to Rustworkx core, esp. given that there are more cases where an algorithm might fail directly in Rust in a recoverable manner (e.g. if a cycle would be introduced to a directed graph). Further, we'd like to call into Rustworkx core from our Python API implementation to avoid code duplication as much as possible, which means that it MUST offer the same degree of recoverability (no panic).

Details

The rest of this issue references important parts of this full demo branch.

To get a better understanding of the use cases we might have for this as well as how we might cleanly implement currently Python-only algorithms for generic petgraph graphs, I've tried my hand at prototyping a unified Error type and corresponding error handling machinery for Rustworkx core, and have ported a few Rustworkx Python API functions over to core to make use of it (e.g. contract_nodes).

The core Error type I ended up with looks like this:

/// The common error type.
#[derive(Debug)]
pub enum RxError<N, E, C = ()> {
    NodeId(N),
    EdgeId(E),
    Callback(C),
    DAGWouldCycle,
}

This enum attempts to incorporate the "good" error type principles defined in the Rustlang docs, Defining an error type:

• Represents different errors with the same type
• Presents nice error messages to the user
• Is easy to compare with other types
  • Good: Err(EmptyVec)
  • Bad: Err("Please use a vector with at least one element".to_owned())
• Can hold information about the error
  • Good: Err(BadChar(c, position))
  • Bad: Err("+ cannot be used here".to_owned())
• Composes well with other errors

With this approach, the idea is that all Rustworkx core functions and methods that can fail should return a Result with error type RxError<N, E, C>, which is an enum that indicates the cause of the failure, as well as some failure-specific typed data. For example, RxError::NodeId would indicate that a node ID specified by the caller was not valid for the graph, and would include that node ID as a value, typed as a GraphBase::NodeId for the graph.

Because petgraph relies heavily on traits to define the types used for NodeId and EdgeId on a per-graph basis, RxError requires generics, which make it look a bit clunkier than it ought to. However, this does not actually appear to impact the user experience all that much since uses would not need to construct an RxError manually. They'd simply match on the error value returned by a core API with syntax like:

// In this case, `err` is an `RxError<StableGraph::NodeId, StableGraph::EdgeId, PyErr>`.
// The compiler knows this and doesn't need explicit generics on each branch.
match err {
    RxError::NodeId(node_id) => handle_node(node_id),
    RxError::EdgeId(edge_id) => handle_edge(edge_id),
    RxError::Callback(error) => handle_python_callback(error),
    RxError::DAGWouldCycle => handle_would_cycle(),
};

The Callback enum member is a bit more nuanced, and is responsible for representing failures which occur in user-specified callbacks. When a callable that may fail (e.g. FnMut(...) -> Result<T, C>) is given to a Rustworkx core function as a parameter, the core function needs a way to "wrap" the callable's error type C within an RxError (since the core function should return RxError). This technique is described in the Rustlang docs on Wrapping errors, the notable difference being that the wrapped error type in the example is known ahead of time, rather than coming from a generic. But, this does not appear to be an issue.

As is done in the official example, we can easily convert the error type of any user-provided callback returning a Result to an RxError which wraps it by introducing a blanket implementation of From for RxError<N, E, C>:

impl<N, E, C> From<C> for RxError<N, E, C> {
    fn from(value: C) -> Self {
        RxError::Callback(value)
    }
}

With this in place, Rust's error propagation syntax ? automatically converts the user's error type C to RxError.

Python API re-use

As we port code from the Python-only API to core, it's easiest to replace the corresponding Python API code with a call to the core equivalent when the core code uses similar error semantics and a simple mapping can be defined from the RxError type to PyErr.

In the demo branch, PyDiGraph::contract_nodes has been ported to core as an extension trait on petgraph's directed graph types. There are two error cases for this particular function:

1. Self::RxError::DAGWouldCycle is returned when the contraction would result in a cycle.
2. Self::RxError::Callback is returned when a user-defined error occurs within the weight_combo_fn callback (happens via From).

When calling the core contract_nodes function from the Python API implementation, we must map both of these errors to Python exceptions. With a bit of trait magic (private to the Python-only part of Rustworkx), we can actually get the call-site in PyDiGraph::contract_nodes to look like this:

#[pymethods]
impl PyDiGraph {
    // ...
    pub fn contract_nodes(
        &mut self,
        py: Python,
        nodes: Vec<usize>,
        obj: PyObject,
        check_cycle: Option<bool>,
        weight_combo_fn: Option<PyObject>,
    ) -> RxPyResult<usize> {
        let merge_fn = weight_combo_fn.map(|f| move |w1: &Py<_>, w2: &Py<_>| Ok(f.call1(py, (w1, w2))?));

        // `self.graph` is a `petgraph::StableGraph` and `contract_nodes` is
        // imported via an extension trait!
        let res = self.graph.contract_nodes(
            nodes.into_iter().map(|i| NodeIndex::new(i)),
            obj,
            check_cycle.unwrap_or(self.check_cycle),
            merge_fn,
        )?;

        Ok(res.index())
    }
}

Notably, we return Result<usize, RxPyErr> (aliased as RxPyResult<usize>) instead of Result<usize, PyErr>, which uses custom error struct RxPyErr defined in the Python-only API. This struct implements both From<RxError> as well as From<PyErr>, so Rust's error propagation operator automatically converts errors from the core API to RxPyErr as well as PyO3 API call errors. The RxPyErr type also implements IntoPy, making it directly returnable from PyO3 functions and methods.

The implementation of From<RxError> for RxPyErr looks like this:

impl<N: Debug, E: Debug> From<RxError<N, E, PyErr>> for RxPyErr {
    fn from(value: RxError<N, E, PyErr>) -> Self {
        RxPyErr {
            pyerr: match value {
                RxError::NodeId(_) => PyIndexError::new_err(format!("{:?}", value)),
                RxError::EdgeId(_) => PyIndexError::new_err(format!("{:?}", value)),
                RxError::Callback(error) => error,
                RxError::DAGWouldCycle => PyValueError::new_err(format!("{:?}", value)),
            },
        }
    }
}

For node and edge index errors, we return a PyIndexError. For these, we defer to the Rust Debug trait implementation for RxError to format suitable error messages (this in effect reuses the error messages directly from core). We don't need to access the typed ID data since the error will be passed to Python anyway, but it'd be useful for Rust clients.

For callback errors, we simply unwrap the inner error. This way, the user's Python callback can effectively "throw" an exception through our Rust code and back out to Python space in a type-safe manner, without Rustworkx core needing to know about PyErr or any other concrete user error type.

For more implementation details, see here.

Open questions

I'm mostly curious what Rustworkx core contributors and the larger community thinks about this kind of approach. Is this something we'd like to have in Rustworkx core? And, is the trait-based approach I've taken following Rust best practices, or is there something that might be preferable?

Thoughts?