fix: returning a copyable type with a deleted move constructor (#6142)

detail::function_ref's constructor SFINAE required
is_convertible<Ret, Ret>, which is false for a copyable type whose move
constructor is explicitly deleted (the trait tests conversion from an
xvalue). In C++17, such a prvalue return is legal via guaranteed copy
elision, so also accept an exact type match. Regression introduced in
3.1.0 by the call_impl outlining (#5887).

Fixes #6142

Assisted-by: ClaudeCode:claude-fable-5

Author: henryiii

include/pybind11/detail/function_ref.h

@@ -31,6 +31,8 @@
 // - renamed back to function_ref
 // - use pybind11 enable_if_t, remove_cvref_t, and remove_reference_t
 // - lint suppressions
+// - accept same-type non-movable returns under C++17 guaranteed copy elision
+//   (issue #6142)
 
 // torch::executor: modified from llvm::function_ref
 // - renamed to FunctionRef
@@ -55,6 +57,17 @@ PYBIND11_NAMESPACE_BEGIN(detail)
 template <typename Fn>
 class function_ref;
 
+// pybind11: is_convertible<Ret, Ret> is false for a copyable but non-movable
+// type (it tests conversion from an xvalue, which selects the deleted move
+// constructor), but in C++17 a same-type prvalue is returnable via guaranteed
+// copy elision. Accept that case explicitly. See issue #6142.
+template <typename From, typename To>
+using is_returnable_as = bool_constant<
+#if defined(PYBIND11_CPP17)
+    std::is_same<From, To>::value ||
+#endif
+    std::is_convertible<From, To>::value>;
+
 template <typename Ret, typename... Params>
 class function_ref<Ret(Params...)> {
     Ret (*callback)(intptr_t callable, Params... params) = nullptr;
@@ -81,8 +94,8 @@ class function_ref<Ret(Params...)> {
         // Functor must be callable and return a suitable type.
         enable_if_t<
             std::is_void<Ret>::value
-            || std::is_convertible<decltype(std::declval<Callable>()(std::declval<Params>()...)),
-                                   Ret>::value> * = nullptr)
+            || is_returnable_as<decltype(std::declval<Callable>()(std::declval<Params>()...)),
+                                Ret>::value> * = nullptr)
         : callback(callback_fn<remove_reference_t<Callable>>),
           callable(reinterpret_cast<intptr_t>(&callable)) {}
 

tests/test_copy_move.cpp

@@ -107,6 +107,22 @@ class CopyOnlyInt {
 
     int value;
 };
+// #6142: returning a copy-constructible type with an explicitly deleted move
+// constructor failed to compile inside detail::function_ref (3.1.0 regression).
+// Returning such a type by value requires C++17 guaranteed copy elision.
+#if defined(PYBIND11_CPP17)
+class CopyOnlyDeletedMove {
+public:
+    explicit CopyOnlyDeletedMove(int v) : value{v} {}
+    CopyOnlyDeletedMove(const CopyOnlyDeletedMove &) = default;
+    CopyOnlyDeletedMove &operator=(const CopyOnlyDeletedMove &) = delete;
+    CopyOnlyDeletedMove(CopyOnlyDeletedMove &&) = delete;
+    CopyOnlyDeletedMove &operator=(CopyOnlyDeletedMove &&) = delete;
+
+    int value;
+};
+#endif
+
 PYBIND11_NAMESPACE_BEGIN(pybind11)
 PYBIND11_NAMESPACE_BEGIN(detail)
 template <>
@@ -176,6 +192,13 @@ TEST_SUBMODULE(copy_move_policies, m) {
     py::class_<lacking_move_ctor>(m, "lacking_move_ctor")
         .def_static("get_one", &lacking_move_ctor::get_one, py::return_value_policy::move);
 
+    // test_copy_only_deleted_move (#6142)
+#if defined(PYBIND11_CPP17)
+    py::class_<CopyOnlyDeletedMove>(m, "CopyOnlyDeletedMove")
+        .def_readonly("value", &CopyOnlyDeletedMove::value);
+    m.def("get_copy_only_deleted_move", []() { return CopyOnlyDeletedMove(42); });
+#endif
+
     // test_move_and_copy_casts
     // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("move_and_copy_casts", [](const py::object &o) {

tests/test_copy_move.py

@@ -142,3 +142,14 @@ def test_unusual_op_ref():
     # Merely to test that this still exists and built successfully.
     assert m.CallCastUnusualOpRefConstRef().__class__.__name__ == "UnusualOpRef"
     assert m.CallCastUnusualOpRefMovable().__class__.__name__ == "UnusualOpRef"
+
+
+@pytest.mark.skipif(
+    not hasattr(m, "get_copy_only_deleted_move"), reason="requires C++17 copy elision"
+)
+def test_copy_only_deleted_move():
+    """#6142: a copyable type with a deleted move constructor can be returned by value
+
+    This is primarily a compile-time regression test.
+    """
+    assert m.get_copy_only_deleted_move().value == 42