[libc++] PR34298: Change std::function constructor and move assignment operator SFINAE checks to allow std::function with an incomplete return type

[EricWF]

Summary

This patch fixes PR34298 (https://bugs.llvm.org/show_bug.cgi?id=34298). Since Clang changed in r284549, Clang and libc++ prohibit the use of std::function with an incomplete return type, like in the example below:

struct Continuation {
  std::function<Continuation ()> fn;
};

This code failed to compile because of the way SFINAE checks were performed in libc++. Essentially when Continuation was defining a copy-constructor, it tried to find a matching overload for the copy constructor of fn, and thus tried to instantiate std::functions function(_Fp) constructor with a std::function<Continuation ()> substitute for _Fp. That constructor did check against function in its SFINAE checks, but it did so after trying to invoke _Fp. This caused an error while evaluating __invokable_r because Continuation is incomplete, and the incomplete type checker for __invokable_r didn't take std::function<> types into account.

This patch ensures that the SFINAE check verify that _Fp is not a std::function before trying to figure out if _Fp is invokable.

[EricWF]

@arphaman Do you mind if I commit my own version of this?

[arphaman]

Not at all, go ahead

[arphaman]

FYI, I'd like to see this merged into LLVM5 if it's still possible, but it's not super critical

[mclow.lists]

test/libcxx/utilities/function.objects/func.require/incomplete_return_type.pass.cpp

1

Is this file in the right place?
If it's supposed to test functionality that is required by the standard, it should go in test/std somewhere.
If it's supposed to test a libc++ extension, then it should go into test/libcxx.

1

Never mind - I missed the "Non-standard extension" bit.

[rsmith]

test/libcxx/utilities/function.objects/func.require/incomplete_return_type.pass.cpp

17–19

Would it make sense to also check that this struct is copy-constructible? As-is, whether this test tests anything is entirely dependent on what amount to implementation details of the constructor.

[mclow.lists]

test/libcxx/utilities/function.objects/func.require/incomplete_return_type.pass.cpp

22

Is copy constructible; can be instantiated.

static_assert( std::is_copy_constructible<IncompleteReturnType>::value, "" );
IncompleteReturnType ir;
assert( ir.fn == nullptr );

[EricWF]

I committed a different version of this fix in r312890.

Commandeering and closing this review. @arphaman Thanks for the initial patch.

include/functional

public:
     // construct/copy/destroy:
     _LIBCPP_INLINE_VISIBILITY
     function() _NOEXCEPT : __f_(0) {}
     _LIBCPP_INLINE_VISIBILITY
     function(nullptr_t) _NOEXCEPT : __f_(0) {}
     function(const function&);
     function(function&&) _NOEXCEPT;
     template<class _Fp, class = typename enable_if<
-        __callable<_Fp>::value && !is_same<_Fp, function>::value
+        is_same<typename enable_if<!is_same<_Fp, function>::value>::type,
+                void
+        >::value &&
+        __callable<_Fp>::value
     >::type>
     function(_Fp);
 
 #if _LIBCPP_STD_VER <= 14
     template<class _Alloc>
       _LIBCPP_INLINE_VISIBILITY
       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
     template<class _Alloc>
       _LIBCPP_INLINE_VISIBILITY
       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : __f_(0) {}
     template<class _Alloc>
       function(allocator_arg_t, const _Alloc&, const function&);
     template<class _Alloc>
       function(allocator_arg_t, const _Alloc&, function&&);
     template<class _Fp, class _Alloc, class = typename enable_if<__callable<_Fp>::value>::type>
       function(allocator_arg_t, const _Alloc& __a, _Fp __f);
 #endif
 
     function& operator=(const function&);
     function& operator=(function&&) _NOEXCEPT;
     function& operator=(nullptr_t) _NOEXCEPT;
     template<class _Fp>
       typename enable_if
       <
-        __callable<typename decay<_Fp>::type>::value &&
-        !is_same<typename remove_reference<_Fp>::type, function>::value,
+        is_same<typename enable_if<
+         !is_same<typename remove_reference<_Fp>::type, function>::value>::type,
+         void
+        >::value &&
+        __callable<typename decay<_Fp>::type>::value,
         function&
       >::type
       operator=(_Fp&&);
 
     ~function();
 
     // function modifiers:
     void swap(function&) _NOEXCEPT;
function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
     __f_ = 0;
     return *this;
 }
 
 template<class _Rp, class ..._ArgTypes>
 template <class _Fp>
 typename enable_if
 <
-    function<_Rp(_ArgTypes...)>::template __callable<typename decay<_Fp>::type>::value &&
-    !is_same<typename remove_reference<_Fp>::type, function<_Rp(_ArgTypes...)>>::value,
+    is_same<typename enable_if<
+     !is_same<typename remove_reference<_Fp>::type, function<_Rp(_ArgTypes...)>>::value>::type,
+     void
+    >::value &&
+    function<_Rp(_ArgTypes...)>::template __callable<typename decay<_Fp>::type>::value,
     function<_Rp(_ArgTypes...)>&
 >::type
 function<_Rp(_ArgTypes...)>::operator=(_Fp&& __f)
 {
     function(_VSTD::forward<_Fp>(__f)).swap(*this);
     return *this;
 }
 

test/libcxx/utilities/function.objects/func.require/incomplete_return_type.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// Non-standard extension:
+// std::function is allowed to take a function with an incomplete return type.
+
+// [func.require]
+
+#include <functional>
+
+struct IncompleteReturnType {
+  std::function<IncompleteReturnType ()> fn;
+};
+
+int main() {
+  return 0;
+}