This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Minimal std::tuple fix for PR29123
AbandonedPublic

Authored by EricWF on Aug 29 2016, 11:16 AM.

Details

Summary

This patch is only meant to demonstrate the minimal changes needed to avoid the bug.

@rsmith Any ideas?

Diff Detail

Event Timeline

EricWF updated this revision to Diff 69590.Aug 29 2016, 11:16 AM
EricWF updated this revision to Diff 69591.
EricWF retitled this revision from to [libc++] Minimal std::tuple fix for PR29123.
EricWF updated this object.
EricWF added reviewers: rsmith, mclow.lists.
EricWF added a subscriber: rsmith.
EricWF added subscribers: K-ballo, cfe-commits.

Add testcase to patch.

rsmith edited edge metadata.Aug 29 2016, 1:20 PM

Ouch. This testcase is horrible. Note that we find two different ways to convert Optional<std::tuple<dynamic>> to itself: the obvious way, and Optional<std::tuple<dynamic>> -> dynamic -> std::tuple<dynamic> -> Optional<std::tuple<dynamic>> (because tuple's conversion happens inside its converting constructor, this sidesteps the "at most one user-defined conversion" restriction). The reason for the problem is that when overload resolution explores the second path, libc++'s SFINAE checks enter a cycle, and is_convertible's result for this conversion ends up defined in terms of itself.

Now, there *is* a Clang bug here -- a constexpr function template specialization can trigger its own recursive instantiation. Reduced reproducer:

template<typename T> constexpr int f(T t) { return g(t); }
template<typename T> constexpr int g(T t) { return f(t); }
struct X {};
constexpr int k = f(X());

With that fixed, you'll get an arbitrary answer from is_convertible here rather than a compile-time error, but the root cause of the problem in this testcase (the cycle) will remain and will result in weird misbehavior in some cases. Consider the following:

template <typename T, typename U> struct is_convertible { static const bool value = __is_convertible_to(T, U); };
template<bool> struct enable_if; template<> struct enable_if<true> { using type = void; };

template<typename T> struct tuple {
  // tuple::tuple(U&&...) requires each T is constructible from the corresponding U
  template<typename U, typename = typename enable_if<is_convertible<U, T>::value>::type> tuple(U);
};
template<typename T> struct optional {
  // optional(const T&) requires T is copy-constructible, but that doesn't affect our example
  optional(T);
};
struct any {
  // any::any(ValueType&&) requires decay_t<ValueType> is CopyConstructible.
  template<typename T, typename = typename enable_if<is_convertible<T, T>::value>::type> any(T);
};

#ifdef WRONG
static_assert(is_convertible<optional<tuple<any>>, optional<tuple<any>>>::value == true);
static_assert(is_convertible<optional<tuple<any>>, any>::value == false);
#else
static_assert(is_convertible<optional<tuple<any>>, any>::value == true);
static_assert(is_convertible<optional<tuple<any>>, optional<tuple<any>>>::value == true);
#endif

This testcase has essentially the same character as the original one, except that I cut out a lot of irrelevant complexity and explicitly added a use of is_convertible to any to make the problem easier to observe (and renamed the classes to match the names in the standard library). Note that:

  • instantiating is_convertible<o<t<a>>, o<t<a>>> triggers instantiation of is_convertible<o<t<a>>, a>, because it tries to construct a t<a> from an o<t<a>>, which tries to use the tuple(U) constructor with U = o<t<a>>
  • instantiating is_convertible<o<t<a>>, a> triggers instantiation of is_convertible<o<t<a>>, o<t<a>>>, because it tries to construct a a from an o<t<a>>

Now, depending on where we enter this cycle, we get different answers for the type traits, because when we get back to our starting point in the cycle, we get a SFINAE failure (because we've not instantiated ::value yet) and the previous is_convertible step will evaluate to false if there's no other way to perform the conversion. (In the -UWRONG case, there's another way to convert o<t<a>> to o<t<a>>, but in the -DWRONG case there is no other way to convert o<t<a>> to a.)

Arguably the bug here is in the definition of class optional, which triggered overload resolution from optional to T when constructing from an argument of type optional... but it seems unreasonable to expect optional to avoid that (and likewise for user-defined types like Optional in the testcase for this patch).

EricWF abandoned this revision.Oct 14 2016, 4:22 AM