Add type_caster<PyObject> (#4601)

* Add `type_caster<PyObject>` (tests are still incomplete).

* Fix oversight (`const PyObject *`).

* Ensure `type_caster<PyObject>` only works for `PyObject *`

* Move `is_same_ignoring_cvref` into `detail` namespace.

* Add test_cast_nullptr

* Change is_same_ignoring_cvref from variable template to using.

```
test_type_caster_pyobject_ptr.cpp:8:23: error: variable templates only available with ‘-std=c++14’ or ‘-std=gnu++14’ [-Werror]
    8 | static constexpr bool is_same_ignoring_cvref = std::is_same<detail::remove_cvref_t<T>, U>::value;
      |                       ^~~~~~~~~~~~~~~~~~~~~~
```

* Remove `return_value_policy::reference_internal` `keep_alive` feature (because of doubts about it actually being useful).

* Add missing test, fix bug (missing `throw error_already_set();`), various cosmetic changes.

* Move `type_caster<PyObject>` from test to new include (pybind11/type_caster_pyobject_ptr.h)

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

* Backport changes from google/pybind11clif#30021 to #4601

* 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.

* Change `type_caster<PyObject>::name` to `object`, as suggested by @Skylion007

* Expand comment for the new `T cast(const handle &handle)` [`T` = `PyObject *`]

* Add `T cast(object &&obj)` overload as suggested by @Skylion007

The original suggestion leads to `error: call to 'cast' is ambiguous` (full error message below), therefore SFINAE guarding is needed.

```
clang++ -o pybind11/tests/test_type_caster_pyobject_ptr.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:1:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/functional.h:12:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:13:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/class.h:12:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/attr.h:14:
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1165:12: error: call to 'cast' is ambiguous
    return pybind11::cast<T>(std::move(*this));
           ^~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/functional.h:109:70: note: in instantiation of function template specialization 'pybind11::object::cast<_object *>' requested here
                return hfunc.f(std::forward<Args>(args)...).template cast<Return>();
                                                                     ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/functional.h:103:16: note: in instantiation of member function 'pybind11::detail::type_caster<std::function<_object *(int)>>::load(pybind11::handle, bool)::func_wrapper::operator()' requested here
        struct func_wrapper {
               ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1456:47: note: in instantiation of member function 'pybind11::detail::type_caster<std::function<_object *(int)>>::load' requested here
        if ((... || !std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is]))) {
                                              ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1434:50: note: in instantiation of function template specialization 'pybind11::detail::argument_loader<const std::function<_object *(int)> &, int>::load_impl_sequence<0UL, 1UL>' requested here
    bool load_args(function_call &call) { return load_impl_sequence(call, indices{}); }
                                                 ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:227:33: note: in instantiation of member function 'pybind11::detail::argument_loader<const std::function<_object *(int)> &, int>::load_args' requested here
            if (!args_converter.load_args(call)) {
                                ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:101:9: note: in instantiation of function template specialization 'pybind11::cpp_function::initialize<(lambda at /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:50:9), _object *, const std::function<_object *(int)> &, int, pybind11::name, pybind11::scope, pybind11::sibling, pybind11::return_value_policy>' requested here
        initialize(
        ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1163:22: note: in instantiation of function template specialization 'pybind11::cpp_function::cpp_function<(lambda at /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:50:9), pybind11::name, pybind11::scope, pybind11::sibling, pybind11::return_value_policy, void>' requested here
        cpp_function func(std::forward<Func>(f),
                     ^
/usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:48:7: note: in instantiation of function template specialization 'pybind11::module_::def<(lambda at /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:50:9), pybind11::return_value_policy>' requested here
    m.def(
      ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1077:3: note: candidate function [with T = _object *, $1 = 0]
T cast(object &&obj) {
  ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1149:1: note: candidate function [with T = _object *]
cast(object &&object) {
^
1 error generated.
```

Author: rwgk

CMakeLists.txt

@@ -139,7 +139,8 @@ set(PYBIND11_HEADERS
     include/pybind11/pytypes.h
     include/pybind11/stl.h
     include/pybind11/stl_bind.h
-    include/pybind11/stl/filesystem.h)
+    include/pybind11/stl/filesystem.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

@@ -1041,7 +1041,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,
@@ -1055,6 +1059,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

@@ -661,6 +661,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,61 @@
+// 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) {
+            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

@@ -154,6 +154,7 @@ set(PYBIND11_TEST_FILES
     test_stl_binders
     test_tagbased_polymorphic
     test_thread
+    test_type_caster_pyobject_ptr
     test_union
     test_unnamed_namespace_a
     test_unnamed_namespace_b

tests/extra_python_package/test_files.py

@@ -43,6 +43,7 @@
     "include/pybind11/pytypes.h",
     "include/pybind11/stl.h",
     "include/pybind11/stl_bind.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))