From fa98804a07982cef41d554ff96f71334bac92445 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Thu, 14 Apr 2022 09:51:27 -0500 Subject: [PATCH] Adds set_name method of pybind11::capsule class (#3866) * Adds set_name method of pybind11::capsule class This calls PyCapsule_SetName on the underlying capsule object. modified destructors to query capsules's Name [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Handle possible exception thrown by PyCapsule_GetName Also removed accidentally reintroduced use of `const char *&`. [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Fixed function name * Introduced private static function to reuse get_name_or_throw * added tests for capsule renaming * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * handle python error in flight * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Initialized PyObject * variables to nullptr * use write-unraisable if PyCapsule_GetName raises * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * get_name_or_throw->get_name_no_throw If PyCapsule_GetName raises an error we should write as unraisable to consume it and notify user, and then restore the error in flight if any. This way this method called from destructor would not modify interpreter error state. * used error_scope struct * Renamed get_name_no_throw->get_name_in_error_scope Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- include/pybind11/pytypes.h | 34 +++++++++++++++++++++++++++++++--- tests/test_pytypes.cpp | 20 ++++++++++++++++++++ tests/test_pytypes.py | 26 ++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index ba0fda0a8c..7d52774c8b 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1588,7 +1588,8 @@ class capsule : public object { } pybind11_fail("Unable to get capsule context"); } - void *ptr = PyCapsule_GetPointer(o, nullptr); + const char *name = get_name_in_error_scope(o); + void *ptr = PyCapsule_GetPointer(o, name); if (ptr == nullptr) { throw error_already_set(); } @@ -1602,7 +1603,8 @@ class capsule : public object { explicit capsule(void (*destructor)()) { m_ptr = PyCapsule_New(reinterpret_cast(destructor), nullptr, [](PyObject *o) { - auto destructor = reinterpret_cast(PyCapsule_GetPointer(o, nullptr)); + const char *name = get_name_in_error_scope(o); + auto destructor = reinterpret_cast(PyCapsule_GetPointer(o, name)); if (destructor == nullptr) { throw error_already_set(); } @@ -1637,7 +1639,33 @@ class capsule : public object { } } - const char *name() const { return PyCapsule_GetName(m_ptr); } + const char *name() const { + const char *name = PyCapsule_GetName(m_ptr); + if ((name == nullptr) && PyErr_Occurred()) { + throw error_already_set(); + } + return name; + } + + /// Replaces a capsule's name *without* calling the destructor on the existing one. + void set_name(const char *new_name) { + if (PyCapsule_SetName(m_ptr, new_name) != 0) { + throw error_already_set(); + } + } + +private: + static const char *get_name_in_error_scope(PyObject *o) { + error_scope error_guard; + + const char *name = PyCapsule_GetName(o); + if ((name == nullptr) && PyErr_Occurred()) { + // write out and consume error raised by call to PyCapsule_GetName + PyErr_WriteUnraisable(o); + } + + return name; + } }; class tuple : public object { diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index b859497b83..d1e9b81a73 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -159,6 +159,15 @@ TEST_SUBMODULE(pytypes, m) { return py::capsule([]() { py::print("destructing capsule"); }); }); + m.def("return_renamed_capsule_with_destructor", []() { + py::print("creating capsule"); + auto cap = py::capsule([]() { py::print("destructing capsule"); }); + static const char *capsule_name = "test_name1"; + py::print("renaming capsule"); + cap.set_name(capsule_name); + return cap; + }); + m.def("return_capsule_with_destructor_2", []() { py::print("creating capsule"); return py::capsule((void *) 1234, [](void *ptr) { @@ -166,6 +175,17 @@ TEST_SUBMODULE(pytypes, m) { }); }); + m.def("return_renamed_capsule_with_destructor_2", []() { + py::print("creating capsule"); + auto cap = py::capsule((void *) 1234, [](void *ptr) { + py::print("destructing capsule: {}"_s.format((size_t) ptr)); + }); + static const char *capsule_name = "test_name2"; + py::print("renaming capsule"); + cap.set_name(capsule_name); + return cap; + }); + m.def("return_capsule_with_name_and_destructor", []() { auto capsule = py::capsule((void *) 12345, "pointer type description", [](PyObject *ptr) { if (ptr) { diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 85afb94232..5c715ada6b 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -195,6 +195,19 @@ def test_capsule(capture): """ ) + with capture: + a = m.return_renamed_capsule_with_destructor() + del a + pytest.gc_collect() + assert ( + capture.unordered + == """ + creating capsule + renaming capsule + destructing capsule + """ + ) + with capture: a = m.return_capsule_with_destructor_2() del a @@ -207,6 +220,19 @@ def test_capsule(capture): """ ) + with capture: + a = m.return_renamed_capsule_with_destructor_2() + del a + pytest.gc_collect() + assert ( + capture.unordered + == """ + creating capsule + renaming capsule + destructing capsule: 1234 + """ + ) + with capture: a = m.return_capsule_with_name_and_destructor() del a