Make packaged_task accept move-only functors

microsoft · Sep 10, 2024 · dce03b0 · dce03b0
1 parent ab20dbd
commit dce03b0
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 8 deletions.
diff --git a/stl/inc/functional b/stl/inc/functional
@@ -767,7 +767,10 @@ public:
 private:
     _Mybase* _Copy(void* _Where) const override {
         auto& _Myax = _Mypair._Get_first();
-        if constexpr (_Is_large<_Func_impl>) {
+        if constexpr (!is_copy_constructible_v<_Callable>) { // used exclusively for packaged_task
+            (void) _Myax;
+            _CSTD abort(); // shouldn't be called, see GH-3888
+        } else if constexpr (_Is_large<_Func_impl>) {
             _Myalty _Rebound(_Myax);
             _Alloc_construct_ptr<_Myalty> _Constructor{_Rebound};
             _Constructor._Allocate();
@@ -854,7 +857,9 @@ public:
 
 private:
     _Mybase* _Copy(void* _Where) const override {
-        if constexpr (_Is_large<_Func_impl_no_alloc>) {
+        if constexpr (!is_copy_constructible_v<_Callable>) { // used exclusively for packaged_task
+            _CSTD abort(); // shouldn't be called, see GH-3888
+        } else if constexpr (_Is_large<_Func_impl_no_alloc>) {
             return _STD _Global_new<_Func_impl_no_alloc>(_Callee);
         } else {
             return ::new (_Where) _Func_impl_no_alloc(_Callee);
@@ -1070,6 +1075,10 @@ _NON_MEMBER_CALL(_GET_FUNCTION_IMPL_NOEXCEPT, X1, X2, X3)
 #undef _GET_FUNCTION_IMPL_NOEXCEPT
 #endif // defined(__cpp_noexcept_function_type)
 
+struct _Secret_copyability_ignoring_tag { // used exclusively for packaged_task
+    explicit _Secret_copyability_ignoring_tag() = default;
+};
+
 _EXPORT_STD template <class _Fty>
 class function : public _Get_function_impl<_Fty>::type { // wrapper for callable objects
 private:
@@ -1086,6 +1095,14 @@ public:
 
     template <class _Fx, typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0>
     function(_Fx&& _Func) {
+        static_assert(is_copy_constructible_v<decay_t<_Fx>>,
+            "The target function object type must be copy constructible (N4988 [func.wrap.func.con]/10.1).");
+        this->_Reset(_STD forward<_Fx>(_Func));
+    }
+
+    template <class _SecretTag, class _Fx, typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0,
+        enable_if_t<is_same_v<_SecretTag, _Secret_copyability_ignoring_tag>, int> = 0>
+    explicit function(_SecretTag, _Fx&& _Func) { // used exclusively for packaged_task
         this->_Reset(_STD forward<_Fx>(_Func));
     }
 
@@ -1103,6 +1120,16 @@ public:
 
     template <class _Fx, class _Alloc, typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0>
     function(allocator_arg_t, const _Alloc& _Ax, _Fx&& _Func) {
+        static_assert(is_copy_constructible_v<decay_t<_Fx>>,
+            "The target function object type must be copy constructible (N4140 [func.wrap.func.con]/7).");
+        this->_Reset_alloc(_STD forward<_Fx>(_Func), _Ax);
+    }
+
+    template <class _SecretTag, class _Fx, class _Alloc,
+        typename _Mybase::template _Enable_if_callable_t<_Fx, function>           = 0,
+        enable_if_t<is_same_v<_SecretTag, _Secret_copyability_ignoring_tag>, int> = 0>
+    explicit function(_SecretTag, allocator_arg_t, const _Alloc& _Ax, _Fx&& _Func) {
+        // used exclusively for packaged_task
         this->_Reset_alloc(_STD forward<_Fx>(_Func), _Ax);
     }
 #endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT

diff --git a/stl/inc/future b/stl/inc/future
@@ -470,12 +470,12 @@ public:
     using _Mydel  = typename _Mybase::_Mydel;
 
     template <class _Fty2>
-    _Packaged_state(_Fty2&& _Fnarg) : _Fn(_STD forward<_Fty2>(_Fnarg)) {}
+    _Packaged_state(_Fty2&& _Fnarg) : _Fn(_Secret_copyability_ignoring_tag{}, _STD forward<_Fty2>(_Fnarg)) {}
 
 #if _HAS_FUNCTION_ALLOCATOR_SUPPORT
     template <class _Fty2, class _Alloc>
     _Packaged_state(_Fty2&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
-        : _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
+        : _Mybase(_Dp), _Fn(_Secret_copyability_ignoring_tag{}, allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
 #endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT
 
     void _Call_deferred(_ArgTypes... _Args) { // set deferred call
@@ -514,12 +514,12 @@ public:
     using _Mydel  = typename _Mybase::_Mydel;
 
     template <class _Fty2>
-    _Packaged_state(_Fty2&& _Fnarg) : _Fn(_STD forward<_Fty2>(_Fnarg)) {}
+    _Packaged_state(_Fty2&& _Fnarg) : _Fn(_Secret_copyability_ignoring_tag{}, _STD forward<_Fty2>(_Fnarg)) {}
 
 #if _HAS_FUNCTION_ALLOCATOR_SUPPORT
     template <class _Fty2, class _Alloc>
     _Packaged_state(_Fty2&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
-        : _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
+        : _Mybase(_Dp), _Fn(_Secret_copyability_ignoring_tag{}, allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
 #endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT
 
     void _Call_deferred(_ArgTypes... _Args) { // set deferred call
@@ -558,12 +558,12 @@ public:
     using _Mydel  = typename _Mybase::_Mydel;
 
     template <class _Fty2>
-    _Packaged_state(_Fty2&& _Fnarg) : _Fn(_STD forward<_Fty2>(_Fnarg)) {}
+    _Packaged_state(_Fty2&& _Fnarg) : _Fn(_Secret_copyability_ignoring_tag{}, _STD forward<_Fty2>(_Fnarg)) {}
 
 #if _HAS_FUNCTION_ALLOCATOR_SUPPORT
     template <class _Fty2, class _Alloc>
     _Packaged_state(_Fty2&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
-        : _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
+        : _Mybase(_Dp), _Fn(_Secret_copyability_ignoring_tag{}, allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
 #endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT
 
     void _Call_deferred(_ArgTypes... _Args) { // set deferred call

diff --git a/tests/std/tests/Dev10_561430_list_and_tree_leaks/test.cpp b/tests/std/tests/Dev10_561430_list_and_tree_leaks/test.cpp
@@ -164,6 +164,17 @@ int main() {
         assert(f.get() == 1234);
     }
 
+    // Also test GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
+    {
+        packaged_task<int()> pt(allocator_arg, Mallocator<int>(), [uptr = make_unique<int>(172)] { return *uptr; });
+
+        future<int> f = pt.get_future();
+
+        pt();
+
+        assert(f.get() == 172);
+    }
+
     {
         int n = 4096;
 

diff --git a/tests/std/tests/Dev11_0235721_async_and_packaged_task/test.cpp b/tests/std/tests/Dev11_0235721_async_and_packaged_task/test.cpp
@@ -78,6 +78,9 @@ void test_DevDiv_725337() {
     int i           = 1729;
     auto ref_lambda = [&]() -> int& { return i; };
 
+    // GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
+    auto move_only_lambda = [uptr = make_unique<int>(42)] { return *uptr; };
+
     {
         packaged_task<int()> pt1([] { return 19937; });
         future<int> f = pt1.get_future();
@@ -90,6 +93,18 @@ void test_DevDiv_725337() {
         assert(f.get() == 19937);
     }
 
+    {
+        packaged_task<int()> pt1(move(move_only_lambda));
+        future<int> f = pt1.get_future();
+        packaged_task<int()> pt2(move(pt1));
+        packaged_task<int()> pt3;
+        pt3 = move(pt2);
+        assert(f.wait_for(0s) == future_status::timeout);
+        pt3();
+        assert(f.wait_for(0s) == future_status::ready);
+        assert(f.get() == 42);
+    }
+
     {
         packaged_task<int&()> pt1(ref_lambda);
         future<int&> f = pt1.get_future();

diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp
@@ -176,6 +176,24 @@ void run_tests() {
 
         assert(f.get().x == 7);
     }
+
+    // Also test GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
+    {
+        struct MoveOnlyFunctor {
+            MoveOnlyFunctor()                             = default;
+            MoveOnlyFunctor(MoveOnlyFunctor&&)            = default;
+            MoveOnlyFunctor& operator=(MoveOnlyFunctor&&) = default;
+
+            T operator()() const {
+                return T{172};
+            }
+        };
+        std::packaged_task<T()> pt(MoveOnlyFunctor{});
+        Future f = pt.get_future();
+        pt();
+
+        assert(f.get().x == 172);
+    }
 }
 
 int main() {

diff --git a/tests/std/tests/VSO_0000000_instantiate_iterators_misc/test.compile.pass.cpp b/tests/std/tests/VSO_0000000_instantiate_iterators_misc/test.compile.pass.cpp
@@ -554,9 +554,13 @@ void future_test() {
     swap_test(pv);
 
     packaged_task<void()> pt([]() {});
+    // GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
+    packaged_task<void()> pt2([uptr = unique_ptr<int>{}]() { (void) uptr; });
 
 #if _HAS_FUNCTION_ALLOCATOR_SUPPORT
     packaged_task<void()> pta(allocator_arg, allocator<double>{}, []() {});
+    // GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
+    packaged_task<void()> pta2(allocator_arg, allocator<double>{}, [uptr = unique_ptr<int>{}]() { (void) uptr; });
 #endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT
 
     swap_test(pt);