[libcxx] Better constain tuples constructors -- Fix PR23256 and PR22806

[EricWF]

Summary

This patch fixes the following bugs:

• PR23256 tuple<A> is_constructible attempts tuple<A> to A conversion:
  • Fix this by disabling the UTypes... overloads using the __disable_tuple_args_ctor<...> meta-function.
• PR22806 Move from destroyed object in tuple_cat of nested tuples
  • Fix this by disabling the tuple-like overloads using the __disable_tuple_like_ctor<...> meta-function.

The goal of the __disable_*_ctor metafunctions is to disable obviously incorrect constructor overloads before they evaluate more SFINAE and cause a hard compile error. The implementation of these traits uses NO SFINAE that could result in a hard compile error. These must be safe to evaluate always.

[EricWF]

Adding K-ballo and Howard Hinnant as reviewers. This patch could use all the eyes it could get.

[eugenis]

FYI, I ran some tests and did not find any regressions and also confirmed that this change fixes the problem PR23256 was minimized from.
Thanks for working on this!

[EricWF]

Howard gave this approach and patch the thumbs up privately.

@mclow.lists What's holding you back from approving this?

[ldionne]

This LGTM. I also just confirmed that it fixes the original issue in Hana that caused me to report PR22806.

[EricWF]

Abandoning. This is super old and a different fix has been checked in.as r266461.

include/__tuple

                                     tuple_size<_Up>::value, _Tp, _Up>
 {};
 
+
+/**
+ * A utility metafunction that gets the first element type of a tuple
+ * and returns that type with no ref qualifiers.
+ */
+template <class _Tuple>
+struct __first_tuple_elem : remove_reference<
+    typename tuple_element<0,
+        typename remove_reference<_Tuple>::type
+    >::type
+> {};
+
+/**
+ * A metafunction that determines if the tuple-like copy/move ctor should be
+ * disabled because it would construct a dangling reference to a nested tuple.
+ * See PR22806 - https://llvm.org/bugs/show_bug.cgi?id=22806.
+ *
+ * The goal of this meta-function is to disable the tuple-like ctor for
+ * instantiations like:
+ *
+ * std::tuple<std::tuple<ValueType>&&>::tuple(std::tuple<ValueType>&&);
+ *
+ * In this case the constructor would attempt to bind 'ValueType&&' to the first
+ * element of type 'std::tuple<ValueType>&&' which would create a temporary
+ * 'std::tuple<ValueType>' from 'ValueType&&' and assign it to a reference. The
+ * temporary would then go out of scope and the tuple would contain a dangling
+ * reference.
+ *
+ * This metafunction returns true if all of the following are true.
+ *   1. _From is "tuple-like". This is always true but we need it to guard other
+ *      meta-programming.
+ *
+ *   2. _Tuple has size 1. This is the only case where the copy/move ctors can
+ *      be a better match that the UTypes... ctors because a single argument
+ *      ctor is a better match than a variadic one that deduces one argument.
+ *
+ *   3. The first element of _Tuple, 'TupleElem0', is a reference.
+ *
+ *   4. The first element of _From, 'FromElem0', is not an instance of
+ *      or derived from 'TupleElem0'. If it were then the construction of
+ *     'TupleElem0' wouldn't result in a dangling reference.
+ */
+template <class _From, class _Tuple,
+          bool = __tuple_like<typename remove_reference<_From>::type>::value &&
+                tuple_size<_Tuple>::value == 1 &&
+                is_reference<typename tuple_element<0, _Tuple>::type>::value
+          >
+struct __disable_tuple_like_ctor : false_type {};
+
+template <class _From, class _Tuple>
+struct __disable_tuple_like_ctor<_From, _Tuple, true> : integral_constant<bool,
+    tuple_size<typename remove_reference<_From>::type>::value == 1 &&
+    !is_base_of<
+        typename __first_tuple_elem<_Tuple>::type,
+        typename __first_tuple_elem<_From>::type
+    >::value
+> {};
+
+
+/**
+ * A metafunction that returns 'true' when the tuple(UTypes&&...) constructor
+ * should not participate in overload resolution. This metafunction is meant to
+ * guard the other SFINAE on the tuple(UTypes&&...) from being evaluated when
+ * its clear that a tuple copy/move constructor should be chosen instead.
+ * See PR23256 - https://llvm.org/bugs/show_bug.cgi?id=23256.
+ *
+ * The metafunction returns true if and only if:
+ *   1. sizeof...(UTypes) == 1. The copy constructor only takes 1 argument
+ *     so we obviously didn't intend to call it if we provide more that one
+ *     argument.
+ *
+ *   2. The first element (and only) element type, 'UT0', is the same type as the
+ *      tuple. Ex std::tuple<A>(std::tuple<A> const&).
+ *
+ */
+template <class _Tuple, class ..._Args>
+struct __disable_tuple_args_ctor : false_type {};
+
+template <class _Tuple, class _Arg0>
+struct __disable_tuple_args_ctor<_Tuple, _Arg0>
+    : is_same<
+        _Tuple,
+        typename remove_cv<typename remove_reference<_Arg0>::type>::type
+      > {};
+
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 _LIBCPP_END_NAMESPACE_STD

include/tuple

               typename enable_if
                       <
                          sizeof...(_Up) <= sizeof...(_Tp) &&
+                        __lazy_and<
+                          __lazy_not<__disable_tuple_args_ctor<tuple, _Up...>>,
                          __tuple_convertible
                          <
                             tuple<_Up...>,

                                      sizeof...(_Up) < sizeof...(_Tp) ?
                                         sizeof...(_Up) :
                                         sizeof...(_Tp)>::type
-                         >::value &&
+                         >,
                          __all_default_constructible<
                             typename __make_tuple_types<tuple, sizeof...(_Tp),
                                 sizeof...(_Up) < sizeof...(_Tp) ?
                                     sizeof...(_Up) :
                                     sizeof...(_Tp)>::type
-                         >::value,
+                         >
+                    >::type::value,
                          bool
                       >::type = false
              >

               typename enable_if
                       <
                          sizeof...(_Up) <= sizeof...(_Tp) &&
+                        __lazy_and<
+                         __lazy_not<__disable_tuple_args_ctor<tuple, _Up...>>,
                          __tuple_constructible
                          <
                             tuple<_Up...>,

                                      sizeof...(_Up) < sizeof...(_Tp) ?
                                         sizeof...(_Up) :
                                         sizeof...(_Tp)>::type
-                         >::value &&
-                         !__tuple_convertible
+                         >,
+                         __lazy_not<__tuple_convertible
                          <
                             tuple<_Up...>,
                             typename __make_tuple_types<tuple,
                                      sizeof...(_Up) < sizeof...(_Tp) ?
                                         sizeof...(_Up) :
                                         sizeof...(_Tp)>::type
-                         >::value &&
+                         >>,
                          __all_default_constructible<
                             typename __make_tuple_types<tuple, sizeof...(_Tp),
                                 sizeof...(_Up) < sizeof...(_Tp) ?
                                     sizeof...(_Up) :
                                     sizeof...(_Tp)>::type
-                         >::value,
+                         >>::type::value,
                          bool
                       >::type =false
              >

               class = typename enable_if
                       <
                          sizeof...(_Up) <= sizeof...(_Tp) &&
+                         __lazy_and<
+                         __lazy_not<__disable_tuple_args_ctor<tuple, _Up...>>,
                          __tuple_convertible
                          <
                             tuple<_Up...>,

                                      sizeof...(_Up) < sizeof...(_Tp) ?
                                         sizeof...(_Up) :
                                         sizeof...(_Tp)>::type
-                         >::value &&
+                         >,
                          __all_default_constructible<
                             typename __make_tuple_types<tuple, sizeof...(_Tp),
                                 sizeof...(_Up) < sizeof...(_Tp) ?
                                     sizeof...(_Up) :
                                     sizeof...(_Tp)>::type
-                         >::value
+                         >>::value
                       >::type
              >
         _LIBCPP_INLINE_VISIBILITY

     template <class _Tuple,
               typename enable_if
                       <
+                        !__disable_tuple_like_ctor<_Tuple, tuple>::value &&
                          __tuple_convertible<_Tuple, tuple>::value,
                          bool
                       >::type = false

     template <class _Tuple,
               typename enable_if
                       <
+                         !__disable_tuple_like_ctor<_Tuple, tuple>::value &&
                          __tuple_constructible<_Tuple, tuple>::value &&
                          !__tuple_convertible<_Tuple, tuple>::value,
                          bool

     template <class _Alloc, class _Tuple,
               class = typename enable_if
                       <
+                         !__disable_tuple_like_ctor<_Tuple, tuple>::value &&
                          __tuple_convertible<_Tuple, tuple>::value
                       >::type
              >

test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR22806_constrain_tuple_like_ctor.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.
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: c++98, c++03
+
+// <tuple>
+
+// template <class... Types> class tuple;
+
+// template <class TupleLike>
+//   tuple(TupleLike&&);
+// template <class Alloc, class TupleLike>
+//   tuple(std::allocator_arg_t, Alloc const&, TupleLike&&);
+
+// Check that the tuple-like copy/move ctors are properly disabled when the
+// UTypes... constructor should be selected. See PR22806.
+
+#include <tuple>
+#include <memory>
+#include <cassert>
+
+#include "tracked_value.h"
+
+int main()
+{
+    { // rvalue reference
+        TrackedValue v;
+        std::tuple<TrackedValue> t1(v);
+        // This selects the wrong constructor and constructs std::tuple<Tracked>&&
+        // from a temporary.
+        std::tuple<std::tuple<TrackedValue>&&> t2(std::move(t1));
+        // This moves from the reference constructed from the temporary.
+        std::tuple<std::tuple<TrackedValue>>  t3(std::move(t2));
+    }
+    { // const lvalue reference
+        TrackedValue v;
+        std::tuple<TrackedValue> t1(v);
+        std::tuple<std::tuple<TrackedValue> const &> t2(t1);
+        std::tuple<std::tuple<TrackedValue>>  t3(t2);
+    }
+    { // lvalue reference
+        TrackedValue v;
+        std::tuple<TrackedValue> t1(v);
+        std::tuple<std::tuple<TrackedValue> &> t2(t1);
+        std::tuple<std::tuple<TrackedValue>>  t3(t2);
+    }
+    { // rvalue reference via allocator ctor
+        TrackedValue v;
+        std::tuple<TrackedValue> t1(v);
+        std::allocator<void> a;
+        std::tuple<std::tuple<TrackedValue>&&> t2(std::allocator_arg, a, std::move(t1));
+        std::tuple<std::tuple<TrackedValue>>  t3(std::move(t2));
+    }
+    { // const lvalue reference via allocator ctor
+        TrackedValue v;
+        std::tuple<TrackedValue> t1(v);
+        std::allocator<void> a;
+        std::tuple<std::tuple<TrackedValue> const &> t2(std::allocator_arg, a, t1);
+        std::tuple<std::tuple<TrackedValue>>  t3(t2);
+    }
+    { // lvalue reference via allocator ctor
+        TrackedValue v;
+        std::tuple<TrackedValue> t1(v);
+        std::allocator<void> a;
+        std::tuple<std::tuple<TrackedValue> &> t2(std::allocator_arg, a, t1);
+        std::tuple<std::tuple<TrackedValue>>  t3(t2);
+    }
+}

test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR23256_constrain_UTypes_ctor.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.
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: c++98, c++03
+
+// <tuple>
+
+// template <class... Types> class tuple;
+
+// template <class ...UTypes>
+//    EXPLICIT(...) tuple(UTypes&&...)
+
+// Check that the UTypes... ctor is properly disabled before evaluating any
+// SFINAE when the tuple-like copy/move ctor should *clearly* be selected
+// instead. This happens 'sizeof...(UTypes) == 1' and the first element of
+// 'UTypes...' is an instance of the tuple itself. See PR23256.
+
+#include <tuple>
+#include <memory>
+#include <type_traits>
+
+
+class UnconstrainedCtor {
+public:
+  UnconstrainedCtor() : value_(0) {}
+
+  template <typename T>
+  constexpr UnconstrainedCtor(T value) : value_(static_cast<int>(value)) {
+      static_assert(std::is_same<int, T>::value, "");
+  }
+
+private:
+  int value_;
+};
+
+int main() {
+    typedef UnconstrainedCtor A;
+    {
+        static_assert(std::is_trivially_copy_constructible<std::tuple<A>>::value, "");
+        static_assert(std::is_trivially_move_constructible<std::tuple<A>>::value, "");
+    }
+    {
+        static_assert(std::is_constructible<
+            std::tuple<A>,
+            std::allocator_arg_t, std::allocator<void>,
+            std::tuple<A> const&
+        >::value, "");
+        static_assert(std::is_constructible<
+            std::tuple<A>,
+            std::allocator_arg_t, std::allocator<void>,
+            std::tuple<A> &&
+        >::value, "");
+    }
+}