Skip to content

[RFC] Add support for binding C++ Exceptions as full-featured Python classes #2333

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jesse-sony
Copy link
Contributor

Pull Request that adds support for binding C++ exceptions as full-featured classes using PyBind11, providing access to additional fields and methods. In-depth stackoverflow post here explaining my approach and why I'm not thrilled about it.
https://stackoverflow.com/questions/62087383/how-can-you-bind-exceptions-with-custom-fields-and-constructors-in-pybind11-and

Definitely would love some feedback on ways to make this more efficient (shouldn't have to waste extra memory for things which aren't exceptions).

@wjakob
Copy link
Member

wjakob commented Jul 27, 2020

That's a neat idea, but I fear it falls into the combination of categories where my suggestion is to fork pybind11:

  • quite rare use case, people don't normally need this feature
  • needs intrusive modifications of pybind11 (large patch, expands size of every pybind11 instance by 24 bytes)

@bstaletic
Copy link
Collaborator

  • quite rare use case, people don't normally need this feature

We've seen people on gitter asking for the ability to call def() on whatever py::register_exception() returns.

@virtuald
Copy link
Contributor

virtuald commented Jul 27, 2020

Regarding the memory usage: is there a way you could store the attributes in the normal python __dict__? I know pybind11 objects don't usually have one, but it wouldn't take more space to create it?

Edit: pybind11 supports __dict__ with py::dynamic_attr, so could add that to exception types automatically?

@jesse-sony
Copy link
Contributor Author

I basically pushed this up because I'm not the first person to ask for it (I actually pulled most of my ideas for how to attack this from the gitter history where others had asked and failed to accomplish this in the past). So I'm definitely not the only project that has run into this, though I don't know how wide-spread usage would be if it were part of pybind11.

* needs intrusive modifications of pybind11 (large patch, expands size of _every_ pybind11 instance by 24 bytes)

Yeah, that is why I put it up as a RFC. I think there could be a better way to do it. I originally tried to create two base structs, so that I could add the extra required fields to an exception_instance and then not have to change the existing instance struct . And then I tried to turn everywhere that used instance into a template and I quickly ran into the limitations of my c++ template abilities. But I was hoping that someone who is either a better c++ programmer than me or more experienced with the pybind11 internals might have some guidance on a way to make this functional without forcing everyone to pay the cost.

@virtuald From my understanding of CPython, that wouldn't work. It appears that python is using the actual memory layout of whatever the instance object is for some of its implementation, so we have to match the exact same layout in PyBind11. That is why every CPython struct that represents an object has to start with PyObject_HEAD or PyObject_VAR_HEAD. If you look at how PyBaseExceptionObject is defined in the C source it is as

typedef struct {
    PyObject_HEAD
    PyObject *dict;
    PyObject *args;
    PyObject *message;
} PyBaseExceptionObject;

And every other exception type matches that exact layout (possibly adding more fields). So anything we do in PyBind11 that is going to extend from that, has to match the exact same class layout.

@eullerborges
Copy link

  • quite rare use case, people don't normally need this feature

Not having anything like this makes it impossible to expose C++ code that takes exception classes as parameters to functions, since pybind doesn't know how to translate the exception to the python side. It'd be nice to have a definitive solution to this like the one proposed here.

@eullerborges
Copy link

Given there's no easy way to do this, I found a workaround, which involves creating a binding for the exception class under a different class name with pybind11::class_, while also registering the exception with the original name through pybind11::register_exception.

template <typename TException, typename... TExtras>
inline auto PyClassFromException(pybind11::handle scope, const char* name, TExtras&&... extras) { 
    static_assert(std::is_base_of_v<std::exception, TException>,
                  "This function should only be used to register exceptions.");
    static std::string sName = std::string(name) + "Data";
    pybind11::register_exception<TException>(scope, name);
    auto klass = py::class_<TException, TExtras...>(scope, sName.c_str(), std::forward<TExtras>(extras)...);
    klass.def("raise", [](const TException& exc) { throw exc; });
    return klass;
}                                                                                               

This means that python code can have access to the internals of custom exceptions when they're passed as arguments in functions, or even created on the python side, and those exceptions can be raised to the original exception type through an auxiliary method (that I called raise above):

# On C++ we PyClassFromException(module, "MyException").def_readonly("internal_info",...)
class CustomPythonClass:
    # [...]
    def HandleMyException(self, error: MyExceptionData):
        print(f"Content: {error.internal_info}")
        try:                 
            error.raise() 
        except MyException as exc:
            print("This works properly!")

And add a separate base class into the internals for when an extension
is being created/extended.
When the exception flag is passed, we need to inherit from
PyExc_Exception instead of from PyBaseObject_Type. Since PyExc_Exception
isn't recognized as a PyTypeObject, we have to cast it.

Since exceptions support dynamic attributes, we have to make sure that
GC is turned on for classes which are following that class path.

Update make_new_python_type to choose the exception_base type when
is_except has been set.
Any pybind11 type that extends PyExc_Exception has to have a instance
struct that contains the same initial layout. And in pybind11 currently,
the instance struct just has PyObject_HEAD. That means if you don't
change the instance struct, this will all compile, but when python seeks
into this object, it will do with the assumption that those extra fields
exist and then it will seek right off the end of viable memory and
you'll get all sorts of fun segfaults. So this change adds those extra
fields to every class_ in pybind11. It does not seem to break normal
classes to have these extra fields and it definitely seems to make
exceptions work correctly. I am not super happy about this approach, so
consider a proof of concept that should probably be improved.
@earonesty
Copy link

ran into this. large boostpy code base that catches custom exceptions and uses error codes. no way to expose the error code in pybind 11. i see no way that this can be called an "edge case" since "exceptions that have error codes" is very reasonable/normal and is used quite often in the wild

@rwgk
Copy link
Collaborator

rwgk commented Jan 10, 2023

Quick question/comment. I was unaware of this PR before and still haven't looked at the changes.

catches custom exceptions and uses error codes

That sounds similar to what the absl::Status bindings in https://github.com/pybind/pybind11_abseil/ are doing, and it works with pybind11 as-is. The two main files are:

Is your case similar or different? (Is the code publicly visible?)

Another completely different potential (big "maybe") approach: #4247 (comment)

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.

8 participants