[smart_holder] git merge master

CMakeLists.txt

@@ -146,7 +146,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

@@ -1083,7 +1083,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;
     static_assert(!cast_is_temporary_value_reference<T>::value,
@@ -1097,6 +1101,34 @@ 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, and could
+// not possibly be different, given
+// 1. the established convention that the passed `handle` is borrowed, and
+// 2. we don't want to force all generic code using `cast<T>()` to special-case
+//    handling of `T` = `PyObject *` (to increment the reference count there).
+// It is the responsibility of the caller to ensure that the reference count
+// is decremented.
+template <typename T,
+          typename Handle,
+          detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value
+                                  && detail::is_same_ignoring_cvref<Handle, handle>::value,
+                              int>
+          = 0>
+T cast(Handle &&handle) {
+    return handle.inc_ref().ptr();
+}
+// To optimize way an inc_ref/dec_ref cycle:
+template <typename T,
+          typename Object,
+          detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value
+                                  && detail::is_same_ignoring_cvref<Object, object>::value,
+                              int>
+          = 0>
+T cast(Object &&obj) {
+    return obj.release().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

@@ -685,6 +685,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("object"); // See discussion under PR #4601.
+
+    // 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

@@ -177,6 +177,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_union
     test_unnamed_namespace_a
     test_unnamed_namespace_b

tests/extra_python_package/test_files.py

@@ -45,6 +45,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,130 @@
+#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_handle_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("cast_object_to_pyobject_ptr", [](py::object obj) {
+        py::handle hdl = obj;
+        auto rc1 = hdl.ref_count();
+        auto *ptr = py::cast<PyObject *>(std::move(obj));
+        auto rc2 = hdl.ref_count();
+        if (rc2 != rc1) {
+            return -1;
+        }
+        return 300 - py::reinterpret_steal<py::object>(ptr).attr("value").cast<int>();
+    });
+    m.def("cast_list_to_pyobject_ptr", [](py::list lst) {
+        // This is to cover types implicitly convertible to object.
+        py::handle hdl = lst;
+        auto rc1 = hdl.ref_count();
+        auto *ptr = py::cast<PyObject *>(std::move(lst));
+        auto rc2 = hdl.ref_count();
+        if (rc2 != rc1) {
+            return -1;
+        }
+        return 400 - static_cast<int>(py::len(py::reinterpret_steal<py::list>(ptr)));
+    });
+
+    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;
+    });
+
+    m.def("pass_pyobject_ptr_and_int", [](PyObject *, int) {});
+
+#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,104 @@
+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_handle_to_pyobject_ptr():
+    assert m.cast_handle_to_pyobject_ptr(ValueHolder(24)) == 76
+
+
+def test_cast_object_to_pyobject_ptr():
+    assert m.cast_object_to_pyobject_ptr(ValueHolder(43)) == 257
+
+
+def test_cast_list_to_pyobject_ptr():
+    assert m.cast_list_to_pyobject_ptr([1, 2, 3, 4, 5]) == 395
+
+
+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
+
+
+def test_type_caster_name_via_incompatible_function_arguments_type_error():
+    with pytest.raises(TypeError, match=r"1\. \(arg0: object, arg1: int\) -> None"):
+        m.pass_pyobject_ptr_and_int(ValueHolder(101), ValueHolder(202))