Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: #[derive(PyException)] macro for easier error interop between Rust and Python #4186

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented May 15, 2024

I had an idea about how we could potentially make working with between Rust error types and custom Python exceptions easier using a derive macro. This is a very experimental/crude WIP implementation of that idea. This provides a #[derive(PyException)] derive macro to generate the necessary conversion glue code between the original Rust error (struct or enum) and (generated) custom exceptions (implemented as unit structs).

The basic idea is to desugar the following (some ordinary Rust error type implementing std::error::Error)

#[derive(Debug, thiserror::Error, pyo3::PyException)]
#[error("some error message")]
pub struct MyRustError {}

into

#[pyclass]
#[pyo3(name = "MyRustError", extends =  PyException, subclass)]
pub struct PyMyRustError;

#[pymethods]
impl PyMyRustError {
    #[new]
    #[pyo3(signature = (*args, **kwargs))]
    pub fn new(
        args: Bound<'_, PyTuple>,
        kwargs: Option<Bound<'_, PyDict>>,
    ) -> Self {
        Self
    }
}

impl From<MyRustError> for PyErr {
    fn from(value: MyRustError) -> Self {
        // call the display impl 
        Self::new::<PyMyRustError, _>(ToString::to_string(&value))
    }
}

This allow using the Rust error type completely normally, while still throwing the corresponding exception on the Python side.

For enums this will create a base exception (catch all) and an specific exception for each variant:

#[derive(Debug, thiserror::Error, pyo3::PyException)]
pub enum LibError {
    #[error("Error A")]
    ErrorA,
    #[error("Error B: {msg}")]
    ErrorB { msg: String },
    #[error("Error C: {0}")]
    ErrorC(u32),
}
desugars into
#[pyclass]
#[pyo3(name = "LibError", extends = PyException, subclass)]
pub struct PyLibError;

#[pymethods]
impl PyLibError {
    #[new]
    #[pyo3(signature = (*args, **kwargs))]
    pub fn new(
        args: Bound<'_, PyTuple>,
        kwargs: Option<Bound<'_, PyDict>>,
    ) -> Self {
        Self
    }
}

#[pyclass]
#[pyo3(name = "ErrorA", extends = PyLibError, subclass)]
pub struct PyErrorA;

#[pymethods]
impl PyErrorA {
    #[new]
    #[pyo3(signature = (*args, **kwargs))]
    pub fn new(
        args: Bound<'_, PyTuple>,
        kwargs: Option<Bound<'_, PyDict>>
    ) ->  PyClassInitializer<Self> {
        PyClassInitializer::from(PyLibError).add_subclass(Self)
    }
 }

#[pyclass]
#[pyo3(name = "ErrorB", extends = PyLibError, subclass)]
pub struct PyErrorB;

#[pymethods]
impl PyErrorB {
   #[new]
    #[pyo3(signature = (*args, **kwargs))]
    pub fn new(
        args: Bound<'_, PyTuple>,
        kwargs: Option<Bound<'_, PyDict>>
    ) ->  PyClassInitializer<Self> {
        PyClassInitializer::from(PyLibError).add_subclass(Self)
    }
 }

#[pyclass]
#[pyo3(name = "ErrorC", extends = PyLibError, subclass)]
pub struct PyErrorC;

#[pymethods]
impl PyErrorC {
    #[new]
    #[pyo3(signature = (*args, **kwargs))]
    pub fn new(
        args: Bound<'_, PyTuple>,
        kwargs: Option<Bound<'_, PyDict>>
    ) ->  PyClassInitializer<Self> {
        PyClassInitializer::from(PyLibError).add_subclass(Self)
    }
 }

impl From<LibError>for PyErr {
    fn from(value: LibError) -> Self {
        match value {
            LibError::ErrorA { .. } => Self::new::<PyErrorA, _>(ToString::to_string(&value)),
            LibError::ErrorB { .. } => Self::new::<PyErrorB, _>(ToString::to_string(&value)),
            LibError::ErrorC { .. } => Self::new::<PyErrorC, _>(ToString::to_string(&value)),
        }
    }
}

This could remove a lot of repetitive boilerplate for the common path, while really complex structures might still require implementing the pattern above manually.

Extensions / Drawbacks

  • allow specifying the base exception to inherit from (I think the tricky part here would the constructor, depending on custom vs native exception)
  • allow renaming the generated exceptions/types
  • This implementation only works with the unlimited API since we can't subclass the native exception on ABI3. (We could potentially work around that by falling back to create_exception! in that case)
  • all the generated types need to be added to a module manually to be accessible from Python, maybe we could generate a helper to do that, or even implement a trait on the original type, so it can be used to attach all generated types to a module

What do people think of that idea? Is it worth exploring further?

Xref: #295

@davidhewitt
Copy link
Member

This is a very interesting idea! I hadn't thought of using a #[derive] macro to to do this; it's not really a trait so I always assumed something like #[pyclass(exception)] was a necessary option. But I see here the choice to use generated unit structs, which I hadn't previously thought about in depth (both generating structs and also using units only on the Python side).

I think a potential additional complication is how to add #[pymethods] to the exception class. Is that even necessary for the simple case? Maybe not. In pydantic-core we have some errors which carry a lot of data.

I think my biggest worry here is that it seems like a convenience which risks being opaque to users (we'd have to document it well) and is quite tricky to refactor gradually. If users need to add a little bit more detail or complexity to their Python exception like methods or data then they have to escape into the full-blown manual implementation.

I am excited for innovation in this space, though - removing create_exception! and having a better mechanism to do the same would please me greatly 😂

Would also be interested in hearing others' reactions. I think I want to learn more about how this feels and am not confident enough yet, so the initial thought is this makes sense to be an experimental feature or separate crate so that we get time to learn from it.


Regarding the ABI3 limitation - that's still just technical deficiency in PyO3 and we could sort that given a bit of time investment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants