From a0b975965fe5df66f20a1d29193055f8538ac94e Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Tue, 27 Jul 2021 19:16:28 +0100
Subject: [PATCH] Allow python builtins to be used as callbacks (#1413)

* Allow python builtins to be used as callbacks

* Try to fix pypy segfault

* Add expected fail for PyPy

* Fix typo

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add more info to xfail

* Add env

* Try returning false

* Try removing the move for pypy

* Fix bugs

* Try removing move

* Just keep ignoring for PyPy

* Add back xfail

* Fix ctors

* Revert change of std::move

* Change to skip

* Fix bug and edit comments

* Remove clang-tidy bugprone fix skip bug

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
---
 include/pybind11/functional.h | 34 ++++++++++++++++++++--------------
 tests/test_callbacks.cpp      |  6 ++++++
 tests/test_callbacks.py       | 11 +++++++++++
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h
index cc7099c6a..b9bf38919 100644
--- a/include/pybind11/functional.h
+++ b/include/pybind11/functional.h
@@ -43,27 +43,33 @@ public:
            captured variables), in which case the roundtrip can be avoided.
          */
         if (auto cfunc = func.cpp_function()) {
-            auto c = reinterpret_borrow<capsule>(PyCFunction_GET_SELF(cfunc.ptr()));
-            auto rec = (function_record *) c;
+            auto cfunc_self = PyCFunction_GET_SELF(cfunc.ptr());
+            if (isinstance<capsule>(cfunc_self)) {
+                auto c = reinterpret_borrow<capsule>(cfunc_self);
+                auto rec = (function_record *) c;
 
-            while (rec != nullptr) {
-                if (rec->is_stateless
-                    && same_type(typeid(function_type),
-                                 *reinterpret_cast<const std::type_info *>(rec->data[1]))) {
-                    struct capture {
-                        function_type f;
-                    };
-                    value = ((capture *) &rec->data)->f;
-                    return true;
+                while (rec != nullptr) {
+                    if (rec->is_stateless
+                        && same_type(typeid(function_type),
+                                     *reinterpret_cast<const std::type_info *>(rec->data[1]))) {
+                        struct capture {
+                            function_type f;
+                        };
+                        value = ((capture *) &rec->data)->f;
+                        return true;
+                    }
+                    rec = rec->next;
                 }
-                rec = rec->next;
             }
+            // PYPY segfaults here when passing builtin function like sum.
+            // Raising an fail exception here works to prevent the segfault, but only on gcc.
+            // See PR #1413 for full details
         }
 
         // ensure GIL is held during functor destruction
         struct func_handle {
             function f;
-            func_handle(function&& f_) : f(std::move(f_)) {}
+            func_handle(function &&f_) noexcept : f(std::move(f_)) {}
             func_handle(const func_handle& f_) {
                 gil_scoped_acquire acq;
                 f = f_.f;
@@ -77,7 +83,7 @@ public:
         // to emulate 'move initialization capture' in C++11
         struct func_wrapper {
             func_handle hfunc;
-            func_wrapper(func_handle&& hf): hfunc(std::move(hf)) {}
+            func_wrapper(func_handle &&hf) noexcept : hfunc(std::move(hf)) {}
             Return operator()(Args... args) const {
                 gil_scoped_acquire acq;
                 object retval(hfunc.f(std::forward<Args>(args)...));
diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp
index a208b44d0..a50771038 100644
--- a/tests/test_callbacks.cpp
+++ b/tests/test_callbacks.cpp
@@ -156,6 +156,12 @@ TEST_SUBMODULE(callbacks, m) {
         .def(py::init<>())
         .def("triple", [](CppBoundMethodTest &, int val) { return 3 * val; });
 
+    // This checks that builtin functions can be passed as callbacks
+    // rather than throwing RuntimeError due to trying to extract as capsule
+    m.def("test_sum_builtin", [](const std::function<double(py::iterable)> &sum_builtin, const py::iterable &i) {
+      return sum_builtin(i);
+    });
+
     // test async Python callbacks
     using callback_f = std::function<void(int)>;
     m.def("test_async_callback", [](const callback_f &f, const py::list &work) {
diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py
index 397bf63d2..5bc4d1773 100644
--- a/tests/test_callbacks.py
+++ b/tests/test_callbacks.py
@@ -3,6 +3,7 @@ import pytest
 from pybind11_tests import callbacks as m
 from threading import Thread
 import time
+import env  # NOQA: F401
 
 
 def test_callbacks():
@@ -124,6 +125,16 @@ def test_movable_object():
     assert m.callback_with_movable(lambda _: None) is True
 
 
+@pytest.mark.skipif(
+    "env.PYPY",
+    reason="PyPy segfaults on here. See discussion on #1413.",
+)
+def test_python_builtins():
+    """Test if python builtins like sum() can be used as callbacks"""
+    assert m.test_sum_builtin(sum, [1, 2, 3]) == 6
+    assert m.test_sum_builtin(sum, []) == 0
+
+
 def test_async_callbacks():
     # serves as state for async callback
     class Item: