[libc++] Fix QoI bug with construction of std::tuple involving std::any

[ldionne]

Summary

In std::tuple, we should try to avoid calling std::is_copy_constructible
whenever we can to avoid surprising interactions with (I believe) compiler
builtins. This bug was reported in https://reviews.llvm.org/D96523#2730953.

The issue was that when tuple<_Up...> was the same as tuple<_Tp...>, we
would short-circuit the _Or (because sizeof...(_Tp) != 1) and go evaluate
the following is_constructible<_Tp, const _Up&>....

Instead, after this patch, we check whether the constructed-from tuple
is the same as the current tuple regardless of the number of elements,
since we should always prefer the normal copy constructor in that case
anyway.

[• Quuxplusone]

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/copy.with_any.compile.pass.cpp

11–14 ↗

(On Diff #342456)

I'd like to see this test additionally test with std::any itself — since this is kind of a subtle interaction between the metaprogramming in both libc++ tuple and libc++ any, right? Could go in this same file, just under TEST_STD_VER >= 17; or could go into copy.with_std_any.compile.pass.cpp or something.

[ldionne]

Properly fix the bug.

[ldionne]

Test buildkite job cancellation!

[• Quuxplusone]

@ldionne: At first I didn't see why builtins were related, but I'm coming around to your point of view. Consider: https://www.godbolt.org/z/Y45bTadaq (which oddly fails with libstdc++ but not with libc++!)

Here, the compiler is trying to figure out whether B is copy-constructible. So one of the things it asks is whether the constructor B(any) is usable as a copy-constructor. So it asks whether any is convertible-from const B&. But the compiler shouldn't ask that, because the answer is obviously irrelevant in this context! Even if any were convertible-from const B&, that would be a user-defined conversion, which means we wouldn't be able to stack it with a second user-defined conversion B(any) in order to copy-construct a B object. So indeed, I think the compiler is wrong to ask whether any is convertible-from const B&. Clang's and GCC's builtins have identical behavior, though.

And there's a weird dependence on whether I use is_copy_constructible<T> versus is_constructible<T, const T&> in various places. I guess that's because we need to be in the middle of instantiating is_constructible<T, const T&> when we hit a call to is_copy_constructible<T> — the symptom is "is_copy_constructible has an incomplete base class."

And that implies that this would be a sufficient (although obviously not intellectually satisfying) fix for the original bug report:

diff --git a/libcxx/include/any b/libcxx/include/any
index aaeaab6c8f74..997f1656e157 100644
--- a/libcxx/include/any
+++ b/libcxx/include/any
@@ -204,7 +204,7 @@ public:
     , class = enable_if_t<
         !is_same<_Tp, any>::value &&
         !__is_inplace_type<_ValueType>::value &&
-        is_copy_constructible<_Tp>::value>
+        is_constructible<_Tp, const _Tp&>::value>
     >
   _LIBCPP_INLINE_VISIBILITY
   any(_ValueType && __value);

I've tested it with your test case and it does make the test case work (but of course only the std::any test case, not your original hand-rolled ::any).

[ldionne]

In D101770#2736470, @Quuxplusone wrote:

@ldionne: At first I didn't see why builtins were related, but I'm coming around to your point of view. Consider: https://www.godbolt.org/z/Y45bTadaq (which oddly fails with libstdc++ but not with libc++!)

Here, the compiler is trying to figure out whether B is copy-constructible. So one of the things it asks is whether the constructor B(any) is usable as a copy-constructor. So it asks whether any is convertible-from const B&. But the compiler shouldn't ask that, because the answer is obviously irrelevant in this context! Even if any were convertible-from const B&, that would be a user-defined conversion, which means we wouldn't be able to stack it with a second user-defined conversion B(any) in order to copy-construct a B object. So indeed, I think the compiler is wrong to ask whether any is convertible-from const B&. Clang's and GCC's builtins have identical behavior, though.

And there's a weird dependence on whether I use is_copy_constructible<T> versus is_constructible<T, const T&> in various places. I guess that's because we need to be in the middle of instantiating is_constructible<T, const T&> when we hit a call to is_copy_constructible<T> — the symptom is "is_copy_constructible has an incomplete base class."

And that implies that this would be a sufficient (although obviously not intellectually satisfying) fix for the original bug report:

diff --git a/libcxx/include/any b/libcxx/include/any
index aaeaab6c8f74..997f1656e157 100644
--- a/libcxx/include/any
+++ b/libcxx/include/any
@@ -204,7 +204,7 @@ public:
     , class = enable_if_t<
         !is_same<_Tp, any>::value &&
         !__is_inplace_type<_ValueType>::value &&
-        is_copy_constructible<_Tp>::value>
+        is_constructible<_Tp, const _Tp&>::value>
     >
   _LIBCPP_INLINE_VISIBILITY
   any(_ValueType && __value);

I've tested it with your test case and it does make the test case work (but of course only the std::any test case, not your original hand-rolled ::any).

Very interesting, thanks for the analysis. I thought it was an issue with the builtins, but I didn't do the work of figuring out why exactly. I think we should open a bug report against Clang - do you want to do it or should I?

libcxx/include/tuple

public:
     , int> = 0>
     tuple(allocator_arg_t, const _Alloc& __alloc, tuple&& __t)
         : __base_(allocator_arg_t(), __alloc, _VSTD::move(__t))
     { }
 
     // tuple(const tuple<U...>&) constructors (including allocator_arg_t variants)
     template <class ..._Up>
     struct _EnableCopyFromOtherTuple : _And<
-        _Or<
+        _Not<is_same<tuple<_Tp...>, tuple<_Up...> > >,
+        _Lazy<_Or,
             _BoolConstant<sizeof...(_Tp) != 1>,
             // _Tp and _Up are 1-element packs - the pack expansions look
             // weird to avoid tripping up the type traits in degenerate cases
             _Lazy<_And,
-                _Not<is_same<_Tp, _Up> >...,
                 _Not<is_convertible<const tuple<_Up>&, _Tp> >...,
                 _Not<is_constructible<_Tp, const tuple<_Up>&> >...
             >
         >,
         is_constructible<_Tp, const _Up&>...
     > { };
 
     template <class ..._Up, _EnableIf<
public:
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
     explicit tuple(allocator_arg_t, const _Alloc& __a, const tuple<_Up...>& __t)
         : __base_(allocator_arg_t(), __a, __t)
     { }
 
     // tuple(tuple<U...>&&) constructors (including allocator_arg_t variants)
     template <class ..._Up>
     struct _EnableMoveFromOtherTuple : _And<
-        _Or<
+        _Not<is_same<tuple<_Tp...>, tuple<_Up...> > >,
+        _Lazy<_Or,
             _BoolConstant<sizeof...(_Tp) != 1>,
             // _Tp and _Up are 1-element packs - the pack expansions look
             // weird to avoid tripping up the type traits in degenerate cases
             _Lazy<_And,
-                _Not<is_same<_Tp, _Up> >...,
                 _Not<is_convertible<tuple<_Up>, _Tp> >...,
                 _Not<is_constructible<_Tp, tuple<_Up> > >...
             >
         >,
         is_constructible<_Tp, _Up>...
     > { };
 
     template <class ..._Up, _EnableIf<

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/cnstr_with_any.compile.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
+
+// This test makes sure that we can copy/move a std::tuple containing a type
+// that checks for copy constructibility itself, like std::any.
+//
+// Problem showcased in https://reviews.llvm.org/D96523#2730953.
+
+#include <any>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+
+#include "test_macros.h"
+
+template <class ...Pred>
+struct And : std::true_type { };
+
+template <class P1, class ...Pn>
+struct And<P1, Pn...>
+  : std::conditional<P1::value, And<Pn...>, std::false_type>::type
+{ };
+
+struct any {
+  any();
+  any(any const&) = default;
+
+  template <class ValueType,
+            class Decayed = typename std::decay<ValueType>::type,
+            class = typename std::enable_if<
+              !std::is_same<Decayed, any>::value &&
+              std::is_copy_constructible<Decayed>::value
+            >::type>
+  any(ValueType&&);
+};
+
+struct A {
+  A();
+  A(any);
+};
+
+#if TEST_STD_VER > 14
+struct B {
+  B();
+  B(std::any);
+};
+#endif
+
+void f() {
+  {
+    std::tuple<A, int> x;
+    std::tuple<A, int> y = x; (void)y;
+  }
+  {
+    std::tuple<A, int> x;
+    std::tuple<A, int> y = std::move(x); (void)y;
+  }
+
+#if TEST_STD_VER > 14
+  {
+    std::tuple<B, int> x;
+    std::tuple<B, int> y = x; (void)y;
+  }
+  {
+    std::tuple<B, int> x;
+    std::tuple<B, int> y = std::move(x); (void)y;
+  }
+#endif
+}