[libc++] Support immovable return types in std::function.

[• Quuxplusone]

Summary

Bug(?) reported by @logan-5.

This is either "oh yeah, sure, trivial, ship it" or "geez this is a really deep open question about the meaning of the standardese" — I'm not sure which. I've posted in #standardese but not otherwise tried to start a discussion/issue yet.

https://eel.is/c++draft/func.wrap.func#general-2
https://eel.is/c++draft/func.require#2
The crux seems to be what [func.require] means by "<expression> implicitly converted to R" — if the type of <expression> is already R, then "implicit conversion to R" is a no-op? Anyway, is_convertible seems to be the wrong tool for that job.

libstdc++ supports std::function<NonCopyable()>; Microsoft STL does not. (This patch would move libc++ from Microsoft's camp to libstdc++'s.)

Lénárd Szolnoki adds: "I wonder if std::function<NonDestructible()> is also legitimate..." All vendors currently agree that it's not. https://godbolt.org/z/rvzWE3

[ldionne]

I think allowing a non-copyable type to be returned makes more sense, however I think this needs a LWG issue so we can clear any doubts on the standardese. Are you willing to file it?

In the meantime, I'll mark this as "changes requested" so it shows up properly, but please ping it whenever there's an update.

[ldionne]

Per the discussion on the LWG reflector, this LGTM.

[• Quuxplusone]

Support std::function<const NonCopyable()>, and then discover that std::function<const void()> wasn't fully supported either. This is now fixed.
However, the unit test for the fix might be unacceptable, because it inevitably triggers -Wignored-qualifiers and I can't figure out how to shut it up. @ldionne, any ideas?

[ldionne]

libcxx/include/__functional_base

343

I don't understand why this change is necessary. Can you explain?

libcxx/include/functional

2352

Can you explain why we can't use std::is_convertible? Is it possible that it's simply a bug in how we implement the __is_convertible intrinsic?

libcxx/include/type_traits

1673

mutex? Do you mean any-non-moveable-type? If so, please use that in the example as it's clearer.

1680

Can't you use declval here?

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp

9

// ADDITIONAL_COMPILE_FLAGS: -Wno-ignored-qualifiers

Nifty trick. Also please add a short comment explaining why you're turning that warning off.

[• Quuxplusone]

The theme of the answers to many of your questions is "because the standard library sucks on purpose." Both is_convertible and declval are unfit for purpose in most places where a metaprogrammer might think about using them.

libcxx/include/__functional_base

343

This change is severable (if I sever some of the test cases with it); it's not directly related to NonCopyable.

It allows us to support std::function<const void()>, because std::is_void_v<const void> == true but std::is_same_v<const void, void> == false.

libcxx/include/functional

2352

std::is_convertible_v<mutex, mutex> is (intentionally) false. So we need a different type-trait or intrinsic to express the core-language idea of "can be implicitly converted" as defined in http://eel.is/c++draft/conv.general#3 . libc++ might already have such a type-trait; I admit I didn't look super hard before deciding to write this myself. My intent is that if libc++ ever needs this trait anywhere else, they can reuse __is_core_convertible — it's not intended to be std::function-specific.

libcxx/include/type_traits

1673

Yes. Will do, but I wish there were a shorter way to spell it.

1680

No, because declval<T>() gives you a T&& instead of a T, and we need specifically a T.
I did initially consider adding a helper function _Tp __core_declval<_Tp>() that could be reused elsewhere, but then decided that that was pretty silly because:

__core_declval<_Tp>()
((_Tp(*)())0)()  // same effect, shorter, no template-instantiation overhead

[ldionne]

libcxx/include/__functional_base

343

Ahhhh, so that's it. Ok to leave as-is. I guess I don't care strongly either way, someone using const void is doing something wrong, but it might be surprising if it doesn't work.

libcxx/include/functional

2352

Ok, so do I understand correctly that std::is_convertible (which is really just the __is_convertible_to builtin) returns true for std::is_convertible<std::mutex, std::mutex> because is_convertible<From, To> returns whether the following is well-formed (from https://en.cppreference.com/w/cpp/types/is_convertible):

To test() { return std::declval<From>(); }

Since std::declval<std::mutex>() is std::mutex&&, it's basically checking whether we can move-construct a mutex, which isn't the case.

Instead, what we want to check is whether we can initialize the return value with a prvalue of type std::mutex, which is true because of copy/move elision in the language. Am I following this correctly, or am I missing something?

libcxx/include/type_traits

1680

Thanks for the explanation. But __core_declval<_Tp>() does read a lot better than ((_Tp(*)())0)() IMO. Feel free to leave as-is -- hopefully we won't need this very often.

[• Quuxplusone]

Address review comments and try to silence -Wignored-qualifiers warnings.

libcxx/include/__functional_base

343

Agreed, it'd be nice if the Standard just forbade std::function<R(Args...)> for all cv-qualified R. I hope they're fixing this for function_ref and unique_function (but I haven't been following that closely).

libcxx/include/functional

2352

You're following correctly. You did make a typo above — you said "returns true for" when you meant "returns false for" — but all the rest of what you wrote clearly indicates that you've got the idea right and it was just a typo.

libcxx/include/type_traits

1680

If we did need it often, I would think about adding a macro version #define _LIBCPP_CORE_DECLVAL(_Tp) (((_Tp(*)())0)()) to eliminate the template instantiations.
(Off-topic but I wonder if anyone's ever proposed a _LIBCPP_FWD macro.)

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp

9

Actually grepping libcxx/test/ for Wno turned up a better-looking and more consistent solution. See the updated diff.

[logan-5]

Thanks @Quuxplusone for working on this. I ran into this issue while trying to write real actual code in a codebase with (for better or worse) lots of noncopyable types, so this will really be a nice-to-have once it's landed.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp

45

What about some tests for assigning NonCopyable X::* and NonCopyable (X::*)() into std::function<NonCopyable(X)>? I suppose the pointer to data member should fail at some point or another (can't copy the data member out), but the pointer to member function should work.

The void tests below might benefit from PMF/PMD coverage as well.

[• Quuxplusone]

Add some unit tests for pointers-to-member(-function)s, as suggested by @logan-5.

[ldionne]

LGTM once CI passes (after you silence the GCC warning). Ship it once it does!

libcxx/include/functional

2352

Thanks for confirming. Indeed, it was a typo.

libcxx/include/type_traits

1680

Nobody has suggested such a macro yet.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp

21

It looks like you'll need to silence this for GCC too.

[• Quuxplusone]

Silence -Wignored-qualifiers also for GCC. If this makes CI green, I'll land this.

[• Quuxplusone]

Poke the buildkite.

libcxx/include/__functional_base

 
 #else // defined(_LIBCPP_CXX03_LANG)
 
 #include <__functional_base_03>
 
 #endif  // !defined(_LIBCPP_CXX03_LANG)
 
 
-template <class _Ret>
+template <class _Ret, bool = is_void<_Ret>::value>
 struct __invoke_void_return_wrapper
 {
 #ifndef _LIBCPP_CXX03_LANG
     template <class ..._Args>
     static _Ret __call(_Args&&... __args) {
         return _VSTD::__invoke(_VSTD::forward<_Args>(__args)...);
     }
 #else
#else
 
     template <class _Fn, class _A0, class _A1, class _A2>
     static _Ret __call(_Fn __f, _A0& __a0, _A1& __a1, _A2& __a2){
         return _VSTD::__invoke(__f, __a0, __a1, __a2);
     }
 #endif
 };
 
-template <>
-struct __invoke_void_return_wrapper<void>
+template <class _Ret>
+struct __invoke_void_return_wrapper<_Ret, true>
 {
 #ifndef _LIBCPP_CXX03_LANG
     template <class ..._Args>
     static void __call(_Args&&... __args) {
         _VSTD::__invoke(_VSTD::forward<_Args>(__args)...);
     }
 #else
     template <class _Fn>

libcxx/include/functional

#endif
     template <class _Fp, bool = _And<
         _IsNotSame<__uncvref_t<_Fp>, function>,
         __invokable<_Fp, _ArgTypes...>
     >::value>
     struct __callable;
     template <class _Fp>
         struct __callable<_Fp, true>
         {
-            static const bool value = is_same<void, _Rp>::value ||
-                is_convertible<typename __invoke_of<_Fp, _ArgTypes...>::type,
+            static const bool value = is_void<_Rp>::value ||
+                __is_core_convertible<typename __invoke_of<_Fp, _ArgTypes...>::type,
                                       _Rp>::value;
         };
     template <class _Fp>
         struct __callable<_Fp, false>
         {
             static const bool value = false;
         };
 
   template <class _Fp>

libcxx/include/type_traits

struct _LIBCPP_TEMPLATE_VIS is_base_of
     : public integral_constant<bool, __is_base_of(_Bp, _Dp)> {};
 
 #if _LIBCPP_STD_VER > 14 && !defined(_LIBCPP_HAS_NO_VARIABLE_TEMPLATES)
 template <class _Bp, class _Dp>
 _LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_base_of_v
     = is_base_of<_Bp, _Dp>::value;
 #endif
 
+// __is_core_convertible
+
+// [conv.general]/3 says "E is convertible to T" whenever "T t=E;" is well-formed.
+// We can't test for that, but we can test implicit convertibility by passing it
+// to a function. Notice that __is_core_convertible<void,void> is false,
+// and __is_core_convertible<immovable-type,immovable-type> is true in C++17 and later.
+
+template <class _Tp, class _Up, class = void>
+struct __is_core_convertible : public false_type {};
+
+template <class _Tp, class _Up>
+struct __is_core_convertible<_Tp, _Up, decltype(
+    static_cast<void(*)(_Up)>(0) ( static_cast<_Tp(*)()>(0)() )
+)> : public true_type {};
+
 // is_convertible
 
 #if __has_feature(is_convertible_to) && !defined(_LIBCPP_USE_IS_CONVERTIBLE_FALLBACK)
 
 template <class _T1, class _T2> struct _LIBCPP_TEMPLATE_VIS is_convertible
     : public integral_constant<bool, __is_convertible_to(_T1, _T2)> {};
 
 #else  // __has_feature(is_convertible_to)

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
+
+// <functional>
+
+#include <functional>
+#include <cassert>
+
+#include "test_macros.h"
+
+// Prevent warning on the `const NonCopyable()` function type.
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic ignored "-Wignored-qualifiers"
+#endif
+
+struct NonCopyable {
+    NonCopyable() = default;
+    NonCopyable(NonCopyable&&) = delete;
+    friend bool operator==(NonCopyable, NonCopyable) { return true; }
+};
+
+struct LargeLambda {
+    int a[100];
+    NonCopyable operator()() const { return NonCopyable(); }
+    NonCopyable operator()(int) const { return NonCopyable(); }
+    NonCopyable f() const { return NonCopyable(); }
+};
+
+void test()
+{
+    std::function<NonCopyable()> f1a = []() { return NonCopyable(); };
+    std::function<NonCopyable()> f2a = +[]() { return NonCopyable(); };
+    std::function<NonCopyable()> f3a = LargeLambda();
+    std::function<NonCopyable()> f4a = std::ref(f1a);
+    std::function<NonCopyable(int)> f1b = [](int) { return NonCopyable(); };
+    std::function<NonCopyable(int)> f2b = +[](int) { return NonCopyable(); };
+    std::function<NonCopyable(int)> f3b = LargeLambda();
+    std::function<NonCopyable(int)> f4b = std::ref(f1b);
+
+    assert(f1a() == f2a());
+    assert(f3a() == f4a());
+    assert(f1b(1) == f2b(1));
+    assert(f3b(1) == f4b(1));
+}
+
+void const_test()
+{
+    std::function<const NonCopyable()> f1a = []() { return NonCopyable(); };
+    std::function<const NonCopyable()> f2a = +[]() { return NonCopyable(); };
+    std::function<const NonCopyable()> f3a = LargeLambda();
+    std::function<const NonCopyable()> f4a = std::ref(f1a);
+    std::function<const NonCopyable(int)> f1b = [](int) { return NonCopyable(); };
+    std::function<const NonCopyable(int)> f2b = +[](int) { return NonCopyable(); };
+    std::function<const NonCopyable(int)> f3b = LargeLambda();
+    std::function<const NonCopyable(int)> f4b = std::ref(f1b);
+
+    assert(f1a() == f2a());
+    assert(f3a() == f4a());
+    assert(f1b(1) == f2b(1));
+    assert(f3b(1) == f4b(1));
+}
+
+void void_test()
+{
+    std::function<void()> f1a = []() { return NonCopyable(); };
+    std::function<void()> f2a = +[]() { return NonCopyable(); };
+    std::function<void()> f3a = LargeLambda();
+    std::function<void()> f4a = std::ref(f1a);
+    std::function<void(int)> f1b = [](int) { return NonCopyable(); };
+    std::function<void(int)> f2b = +[](int) { return NonCopyable(); };
+    std::function<void(int)> f3b = LargeLambda();
+    std::function<void(int)> f4b = std::ref(f1b);
+}
+
+void const_void_test()
+{
+    std::function<const void()> f1a = []() { return NonCopyable(); };
+    std::function<const void()> f2a = +[]() { return NonCopyable(); };
+    std::function<const void()> f3a = LargeLambda();
+    std::function<const void()> f4a = std::ref(f1a);
+    std::function<const void(int)> f1b = [](int) { return NonCopyable(); };
+    std::function<const void(int)> f2b = +[](int) { return NonCopyable(); };
+    std::function<const void(int)> f3b = LargeLambda();
+    std::function<const void(int)> f4b = std::ref(f1b);
+}
+
+void member_pointer_test()
+{
+    std::function<NonCopyable(LargeLambda*)> f1a = &LargeLambda::f;
+    std::function<NonCopyable(LargeLambda&)> f2a = &LargeLambda::f;
+    LargeLambda ll;
+    assert(f1a(&ll) == f2a(ll));
+
+    static_assert(std::is_convertible_v<NonCopyable (LargeLambda::*)(), std::function<NonCopyable(LargeLambda*)>>);
+    static_assert(std::is_convertible_v<NonCopyable (LargeLambda::*)(), std::function<NonCopyable(LargeLambda&)>>);
+    static_assert(std::is_convertible_v<NonCopyable (LargeLambda::*)() const, std::function<NonCopyable(LargeLambda*)>>);
+    static_assert(std::is_convertible_v<NonCopyable (LargeLambda::*)() const, std::function<NonCopyable(LargeLambda&)>>);
+    static_assert(std::is_convertible_v<NonCopyable (LargeLambda::*)() const, std::function<NonCopyable(const LargeLambda*)>>);
+    static_assert(std::is_convertible_v<NonCopyable (LargeLambda::*)() const, std::function<NonCopyable(const LargeLambda&)>>);
+
+    // Verify we have SFINAE against invoking a pointer-to-data-member in a way that would have to copy the NonCopyable.
+    static_assert(!std::is_convertible_v<NonCopyable LargeLambda::*, std::function<NonCopyable(LargeLambda*)>>);
+    static_assert(!std::is_convertible_v<NonCopyable LargeLambda::*, std::function<NonCopyable(LargeLambda&)>>);
+    static_assert(!std::is_convertible_v<NonCopyable LargeLambda::*, std::function<NonCopyable&(const LargeLambda&)>>);
+    static_assert(std::is_convertible_v<NonCopyable LargeLambda::*, std::function<NonCopyable&(LargeLambda*)>>);
+    static_assert(std::is_convertible_v<NonCopyable LargeLambda::*, std::function<NonCopyable&(LargeLambda&)>>);
+    static_assert(std::is_convertible_v<NonCopyable LargeLambda::*, std::function<const NonCopyable&(const LargeLambda&)>>);
+}
+
+void ctad_test()
+{
+    std::function f1a = []() { return NonCopyable(); };
+    std::function f2a = +[]() { return NonCopyable(); };
+    std::function f1b = [](int) { return NonCopyable(); };
+    std::function f2b = +[](int) { return NonCopyable(); };
+    static_assert(std::is_same_v<decltype(f1a), std::function<NonCopyable()>>);
+    static_assert(std::is_same_v<decltype(f2a), std::function<NonCopyable()>>);
+    static_assert(std::is_same_v<decltype(f1b), std::function<NonCopyable(int)>>);
+    static_assert(std::is_same_v<decltype(f2b), std::function<NonCopyable(int)>>);
+}
+
+int main(int, char**)
+{
+    test();
+    const_test();
+    void_test();
+    const_void_test();
+    member_pointer_test();
+    ctad_test();
+    return 0;
+}