Add type_caster<PyObject> (#30021)

* Snapshot of pybind/pybind11#4601 (squashed).

* Add new header file to CMakeLists.txt and tests/extra_python_package/test_files.py

* to-Pyton-cast: `return_value_policy::_clif_automatic` => take ownership

* `handle.inc_ref()` in `T cast(const handle &handle)` specialization for `PyObject *`

* Guard against accidental leaking by requiring `return_value_policy::reference` or `automatic_reference`

* Fix oversight in test (to resolve a valgrind leak detection error) and add a related comment in cast.h.

No production code changes.

Make tests more sensitive by using `ValueHolder` instead of empty tuples and dicts.

Manual leak checks with `while True:` & top command repeated for all tests.

* Add tests for interop with stl.h `list_caster`

(No production code changes.)

* Bug fix in test. Minor comment enhancements.

Author: rwgk

CMakeLists.txt

@@ -151,7 +151,8 @@ set(PYBIND11_HEADERS
     include/pybind11/stl.h
     include/pybind11/stl_bind.h
     include/pybind11/stl/filesystem.h
-    include/pybind11/trampoline_self_life_support.h)
+    include/pybind11/trampoline_self_life_support.h
+    include/pybind11/type_caster_pyobject_ptr.h)
 
 # Compare with grep and warn if mismatched
 if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)

include/pybind11/cast.h

@@ -1207,7 +1207,11 @@ make_caster<T> load_type(const handle &handle) {
 PYBIND11_NAMESPACE_END(detail)
 
 // pytype -> C++ type
-template <typename T, detail::enable_if_t<!detail::is_pyobject<T>::value, int> = 0>
+template <typename T,
+          detail::enable_if_t<!detail::is_pyobject<T>::value
+                                  && !detail::is_same_ignoring_cvref<T, PyObject *>::value,
+                              int>
+          = 0>
 T cast(const handle &handle) {
     using namespace detail;
     constexpr bool is_enum_cast = type_uses_type_caster_enum_type<intrinsic_t<T>>::value;
@@ -1231,6 +1235,16 @@ T cast(const handle &handle) {
     return T(reinterpret_borrow<object>(handle));
 }
 
+// Note that `cast<PyObject *>(obj)` increments the reference count of `obj`.
+// This is necessary for the case that `obj` is a temporary.
+// It is the responsibility of the caller to ensure that the reference count
+// is decremented.
+template <typename T,
+          detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value, int> = 0>
+T cast(const handle &handle) {
+    return handle.inc_ref().ptr();
+}
+
 // C++ type -> py::object
 template <typename T, detail::enable_if_t<!detail::is_pyobject<T>::value, int> = 0>
 object cast(T &&value,

include/pybind11/detail/common.h

@@ -751,6 +751,10 @@ template <class T>
 using remove_cvref_t = typename remove_cvref<T>::type;
 #endif
 
+/// Example usage: is_same_ignoring_cvref<T, PyObject *>::value
+template <typename T, typename U>
+using is_same_ignoring_cvref = std::is_same<detail::remove_cvref_t<T>, U>;
+
 /// Index sequences
 #if defined(PYBIND11_CPP14)
 using std::index_sequence;

include/pybind11/type_caster_pyobject_ptr.h

@@ -0,0 +1,62 @@
+// Copyright (c) 2023 The pybind Community.
+
+#pragma once
+
+#include "detail/common.h"
+#include "detail/descr.h"
+#include "cast.h"
+#include "pytypes.h"
+
+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
+PYBIND11_NAMESPACE_BEGIN(detail)
+
+template <>
+class type_caster<PyObject> {
+public:
+    static constexpr auto name = const_name("PyObject *");
+
+    // This overload is purely to guard against accidents.
+    template <typename T,
+              detail::enable_if_t<!is_same_ignoring_cvref<T, PyObject *>::value, int> = 0>
+    static handle cast(T &&, return_value_policy, handle /*parent*/) {
+        static_assert(is_same_ignoring_cvref<T, PyObject *>::value,
+                      "Invalid C++ type T for to-Python conversion (type_caster<PyObject>).");
+        return nullptr; // Unreachable.
+    }
+
+    static handle cast(PyObject *src, return_value_policy policy, handle /*parent*/) {
+        if (src == nullptr) {
+            throw error_already_set();
+        }
+        if (PyErr_Occurred()) {
+            raise_from(PyExc_SystemError, "src != nullptr but PyErr_Occurred()");
+            throw error_already_set();
+        }
+        if (policy == return_value_policy::take_ownership
+            || policy == return_value_policy::_clif_automatic) {
+            return src;
+        }
+        if (policy == return_value_policy::reference
+            || policy == return_value_policy::automatic_reference) {
+            return handle(src).inc_ref();
+        }
+        pybind11_fail("type_caster<PyObject>::cast(): unsupported return_value_policy: "
+                      + std::to_string(static_cast<int>(policy)));
+    }
+
+    bool load(handle src, bool) {
+        value = reinterpret_borrow<object>(src);
+        return true;
+    }
+
+    template <typename T>
+    using cast_op_type = PyObject *;
+
+    explicit operator PyObject *() { return value.ptr(); }
+
+private:
+    object value;
+};
+
+PYBIND11_NAMESPACE_END(detail)
+PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

tests/CMakeLists.txt

@@ -180,6 +180,7 @@ set(PYBIND11_TEST_FILES
     test_thread
     test_type_caster_odr_guard_1
     test_type_caster_odr_guard_2
+    test_type_caster_pyobject_ptr
     test_type_caster_std_function_specializations
     test_union
     test_virtual_functions)

tests/extra_python_package/test_files.py

@@ -46,6 +46,7 @@
     "include/pybind11/stl.h",
     "include/pybind11/stl_bind.h",
     "include/pybind11/trampoline_self_life_support.h",
+    "include/pybind11/type_caster_pyobject_ptr.h",
 }
 
 detail_headers = {

tests/test_type_caster_pyobject_ptr.cpp

@@ -0,0 +1,107 @@
+#include <pybind11/functional.h>
+#include <pybind11/stl.h>
+#include <pybind11/type_caster_pyobject_ptr.h>
+
+#include "pybind11_tests.h"
+
+#include <cstddef>
+#include <vector>
+
+namespace {
+
+std::vector<PyObject *> make_vector_pyobject_ptr(const py::object &ValueHolder) {
+    std::vector<PyObject *> vec_obj;
+    for (int i = 1; i < 3; i++) {
+        vec_obj.push_back(ValueHolder(i * 93).release().ptr());
+    }
+    // This vector now owns the refcounts.
+    return vec_obj;
+}
+
+} // namespace
+
+TEST_SUBMODULE(type_caster_pyobject_ptr, m) {
+    m.def("cast_from_pyobject_ptr", []() {
+        PyObject *ptr = PyLong_FromLongLong(6758L);
+        return py::cast(ptr, py::return_value_policy::take_ownership);
+    });
+    m.def("cast_to_pyobject_ptr", [](py::handle obj) {
+        auto rc1 = obj.ref_count();
+        auto *ptr = py::cast<PyObject *>(obj);
+        auto rc2 = obj.ref_count();
+        if (rc2 != rc1 + 1) {
+            return -1;
+        }
+        return 100 - py::reinterpret_steal<py::object>(ptr).attr("value").cast<int>();
+    });
+
+    m.def(
+        "return_pyobject_ptr",
+        []() { return PyLong_FromLongLong(2314L); },
+        py::return_value_policy::take_ownership);
+    m.def("pass_pyobject_ptr", [](PyObject *ptr) {
+        return 200 - py::reinterpret_borrow<py::object>(ptr).attr("value").cast<int>();
+    });
+
+    m.def("call_callback_with_object_return",
+          [](const std::function<py::object(int)> &cb, int value) { return cb(value); });
+    m.def(
+        "call_callback_with_pyobject_ptr_return",
+        [](const std::function<PyObject *(int)> &cb, int value) { return cb(value); },
+        py::return_value_policy::take_ownership);
+    m.def(
+        "call_callback_with_pyobject_ptr_arg",
+        [](const std::function<int(PyObject *)> &cb, py::handle obj) { return cb(obj.ptr()); },
+        py::arg("cb"), // This triggers return_value_policy::automatic_reference
+        py::arg("obj"));
+
+    m.def("cast_to_pyobject_ptr_nullptr", [](bool set_error) {
+        if (set_error) {
+            PyErr_SetString(PyExc_RuntimeError, "Reflective of healthy error handling.");
+        }
+        PyObject *ptr = nullptr;
+        py::cast(ptr);
+    });
+
+    m.def("cast_to_pyobject_ptr_non_nullptr_with_error_set", []() {
+        PyErr_SetString(PyExc_RuntimeError, "Reflective of unhealthy error handling.");
+        py::cast(Py_None);
+    });
+
+    m.def("pass_list_pyobject_ptr", [](const std::vector<PyObject *> &vec_obj) {
+        int acc = 0;
+        for (const auto &ptr : vec_obj) {
+            acc = acc * 1000 + py::reinterpret_borrow<py::object>(ptr).attr("value").cast<int>();
+        }
+        return acc;
+    });
+
+    m.def("return_list_pyobject_ptr_take_ownership",
+          make_vector_pyobject_ptr,
+          // Ownership is transferred one-by-one when the vector is converted to a Python list.
+          py::return_value_policy::take_ownership);
+
+    m.def("return_list_pyobject_ptr_reference",
+          make_vector_pyobject_ptr,
+          // Ownership is not transferred.
+          py::return_value_policy::reference);
+
+    m.def("dec_ref_each_pyobject_ptr", [](const std::vector<PyObject *> &vec_obj) {
+        std::size_t i = 0;
+        for (; i < vec_obj.size(); i++) {
+            py::handle h(vec_obj[i]);
+            if (static_cast<std::size_t>(h.ref_count()) < 2) {
+                break; // Something is badly wrong.
+            }
+            h.dec_ref();
+        }
+        return i;
+    });
+
+#ifdef PYBIND11_NO_COMPILE_SECTION // Change to ifndef for manual testing.
+    {
+        PyObject *ptr = nullptr;
+        (void) py::cast(*ptr);
+    }
+#endif
+}

tests/test_type_caster_pyobject_ptr.py

@@ -0,0 +1,91 @@
+import pytest
+
+from pybind11_tests import type_caster_pyobject_ptr as m
+
+
+# For use as a temporary user-defined object, to maximize sensitivity of the tests below.
+class ValueHolder:
+    def __init__(self, value):
+        self.value = value
+
+
+def test_cast_from_pyobject_ptr():
+    assert m.cast_from_pyobject_ptr() == 6758
+
+
+def test_cast_to_pyobject_ptr():
+    assert m.cast_to_pyobject_ptr(ValueHolder(24)) == 76
+
+
+def test_return_pyobject_ptr():
+    assert m.return_pyobject_ptr() == 2314
+
+
+def test_pass_pyobject_ptr():
+    assert m.pass_pyobject_ptr(ValueHolder(82)) == 118
+
+
+@pytest.mark.parametrize(
+    "call_callback",
+    [
+        m.call_callback_with_object_return,
+        m.call_callback_with_pyobject_ptr_return,
+    ],
+)
+def test_call_callback_with_object_return(call_callback):
+    def cb(value):
+        if value < 0:
+            raise ValueError("Raised from cb")
+        return ValueHolder(1000 - value)
+
+    assert call_callback(cb, 287).value == 713
+
+    with pytest.raises(ValueError, match="^Raised from cb$"):
+        call_callback(cb, -1)
+
+
+def test_call_callback_with_pyobject_ptr_arg():
+    def cb(obj):
+        return 300 - obj.value
+
+    assert m.call_callback_with_pyobject_ptr_arg(cb, ValueHolder(39)) == 261
+
+
+@pytest.mark.parametrize("set_error", [True, False])
+def test_cast_to_python_nullptr(set_error):
+    expected = {
+        True: r"^Reflective of healthy error handling\.$",
+        False: (
+            r"^Internal error: pybind11::error_already_set called "
+            r"while Python error indicator not set\.$"
+        ),
+    }[set_error]
+    with pytest.raises(RuntimeError, match=expected):
+        m.cast_to_pyobject_ptr_nullptr(set_error)
+
+
+def test_cast_to_python_non_nullptr_with_error_set():
+    with pytest.raises(SystemError) as excinfo:
+        m.cast_to_pyobject_ptr_non_nullptr_with_error_set()
+    assert str(excinfo.value) == "src != nullptr but PyErr_Occurred()"
+    assert str(excinfo.value.__cause__) == "Reflective of unhealthy error handling."
+
+
+def test_pass_list_pyobject_ptr():
+    acc = m.pass_list_pyobject_ptr([ValueHolder(842), ValueHolder(452)])
+    assert acc == 842452
+
+
+def test_return_list_pyobject_ptr_take_ownership():
+    vec_obj = m.return_list_pyobject_ptr_take_ownership(ValueHolder)
+    assert [e.value for e in vec_obj] == [93, 186]
+
+
+def test_return_list_pyobject_ptr_reference():
+    vec_obj = m.return_list_pyobject_ptr_reference(ValueHolder)
+    assert [e.value for e in vec_obj] == [93, 186]
+    # Commenting out the next `assert` will leak the Python references.
+    # An easy way to see evidence of the leaks:
+    # Insert `while True:` as the first line of this function and monitor the
+    # process RES (Resident Memory Size) with the Unix top command.
+    assert m.dec_ref_each_pyobject_ptr(vec_obj) == 2