[libc++] Regression with std::variant move construction

[ldionne]

This breaks some valid code. Here a partially reduced test case that depends on libc++ and Google Test: https://gcc.godbolt.org/z/dPznsz44M

#include <variant>
#include <vector>

#include <gmock/gmock.h>

template <typename T>
class Nullable {
 public:
  Nullable() : value_() {}
  template <typename V>
  Nullable(V value) : value_(value) {}

  const T& value() const { return value_; }

 private:
  T value_;
};

template <typename L, typename R>
inline bool operator== (const Nullable<L>& lhs, const Nullable<R>& rhs) {
  return lhs.value() == rhs.value();
}

void f() {
  std::vector<std::variant<int, Nullable<int>>> v;
  ASSERT_THAT(v, ::testing::ElementsAre(16));
}

It used to compile fine, but now it triggers a compilation error:

/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/variant:1186:37: fatal error: recursive template instantiation exceeded maximum depth of 1024
 1186 |              enable_if_t<!is_same_v<__remove_cvref_t<_Arg>, variant>, int>        = 0,
      |                                     ^
/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/variant:1192:35: note: while substituting prior template arguments into non-type template parameter [with _Arg = testing::Matcher<const std::variant<int, Nullable<int>> &>]
 1192 |   _LIBCPP_HIDE_FROM_ABI constexpr variant(_Arg&& __arg) noexcept(is_nothrow_constructible_v<_Tp, _Arg>)
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1193 |       : __impl_(in_place_index<_Ip>, std::forward<_Arg>(__arg)) {}
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/__type_traits/invoke.h:71:69: note: while substituting deduced template arguments into function template 'variant' [with _Arg = testing::Matcher<const std::variant<int, Nullable<int>> &>, $1 = (no value), $2 = (no value), $3 = (no value), _Tp = (no value), _Ip = (no value), $6 = (no value)]
   71 | using __invoke_result_t _LIBCPP_NODEBUG = decltype(__builtin_invoke(std::declval<_Args>()...));
      |                                                                     ^
/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/__type_traits/invoke.h:387:1: note: in instantiation of template type alias '__invoke_result_t' requested here
  387 | using invoke_result_t = __invoke_result_t<_Fn, _Args...>;
      | ^
/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/variant:1140:1: note: in instantiation of template type alias 'invoke_result_t' requested here
 1140 | using __best_match_t _LIBCPP_NODEBUG = typename invoke_result_t<_MakeOverloads<_Types...>, _Tp, _Tp>::type;
      | ^
/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/variant:1189:45: note: in instantiation of template type alias '__best_match_t' requested here
 1189 |              class _Tp  = __variant_detail::__best_match_t<_Arg, _Types...>,
      |                                             ^
/opt/compiler-explorer/clang-trunk-20250722/bin/../include/c++/v1/variant:1192:35: note: (skipping 2533 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
 1192 |   _LIBCPP_HIDE_FROM_ABI constexpr variant(_Arg&& __arg) noexcept(is_nothrow_constructible_v<_Tp, _Arg>)
      |                                   ^
/opt/compiler-explorer/libs/googletest/trunk/googlemock/include/gmock/gmock-matchers.h:356:12: note: in instantiation of function template specialization 'testing::internal::MatcherCastImpl<const std::vector<std::variant<int, Nullable<int>>> &, testing::internal::ElementsAreMatcher<std::tuple<int>>>::CastImpl<false>' requested here
  356 |     return CastImpl(polymorphic_matcher_or_value,
      |            ^
/opt/compiler-explorer/libs/googletest/trunk/googlemock/include/gmock/gmock-matchers.h:529:43: note: in instantiation of member function 'testing::internal::MatcherCastImpl<const std::vector<std::variant<int, Nullable<int>>> &, testing::internal::ElementsAreMatcher<std::tuple<int>>>::Cast' requested here
  529 |   return internal::MatcherCastImpl<T, M>::Cast(matcher);
      |                                           ^
/opt/compiler-explorer/libs/googletest/trunk/googlemock/include/gmock/gmock-matchers.h:536:10: note: in instantiation of function template specialization 'testing::MatcherCast<const std::vector<std::variant<int, Nullable<int>>> &, testing::internal::ElementsAreMatcher<std::tuple<int>>>' requested here
  536 |   return MatcherCast<T>(polymorphic_matcher_or_value);
      |          ^
/opt/compiler-explorer/libs/googletest/trunk/googlemock/include/gmock/gmock-matchers.h:1661:39: note: in instantiation of function template specialization 'testing::SafeMatcherCast<const std::vector<std::variant<int, Nullable<int>>> &, testing::internal::ElementsAreMatcher<std::tuple<int>>>' requested here
 1661 |     const Matcher<const T&> matcher = SafeMatcherCast<const T&>(matcher_);
      |                                       ^
<source>:26:3: note: in instantiation of function template specialization 'testing::internal::PredicateFormatterFromMatcher<testing::internal::ElementsAreMatcher<std::tuple<int>>>::operator()<std::vector<std::variant<int, Nullable<int>>>>' requested here
   26 |   ASSERT_THAT(v, ::testing::ElementsAre(16));
      |   ^
/opt/compiler-explorer/libs/googletest/trunk/googlemock/include/gmock/gmock-matchers.h:5736:3: note: expanded from macro 'ASSERT_THAT'
 5736 |   ASSERT_PRED_FORMAT1(              \
      |   ^
/opt/compiler-explorer/libs/googletest/trunk/googletest/include/gtest/gtest_pred_impl.h:112:3: note: expanded from macro 'ASSERT_PRED_FORMAT1'
  112 |   GTEST_PRED_FORMAT1_(pred_format, v1, GTEST_FATAL_FAILURE_)
      |   ^
/opt/compiler-explorer/libs/googletest/trunk/googletest/include/gtest/gtest_pred_impl.h:100:28: note: expanded from macro 'GTEST_PRED_FORMAT1_'
  100 |   GTEST_ASSERT_(pred_format(#v1, v1), on_failure)
      |                            ^
/opt/compiler-explorer/libs/googletest/trunk/googletest/include/gtest/gtest_pred_impl.h:79:52: note: expanded from macro 'GTEST_ASSERT_'
   79 |   if (const ::testing::AssertionResult gtest_ar = (expression)) \
      |                                                    ^
1 error generated.

@philnik777 please take a look.

Originally posted by @alexfh in #116709 (comment)

[ldionne]

This is @philnik777 's smallest reproducer:

#include <variant>
#include <utility>

struct Nullable {
  template <typename V> Nullable(V);
};

struct Matcher {
  Matcher();
  Matcher(std::variant<Nullable>);
};

void f() {
  Matcher vec;
  Matcher m = std::move(vec);
}

Based on that, I think what's happening is that:

1. We try to check whether Matcher is constructible from Matcher&&. There are two candidates, the implicitly generated move constructor Matcher(Matcher&&) and the Matcher(std::variant<Nullable>) constructor.
2. The Matcher(Matcher&&) constructor is obviously a valid match. But we need to check if the other one would also match. Hence we end up checking whether std::variant<Nullable> is constructible from Matcher&&.
3. To do that, we end up in the variant(Arg&&) constructor here. That constructor tries to find the best variant alternative to construct and there's only one to check, so it asks whether Nullable is constructible from the incoming argument (Matcher&&).
4. Since there's a template <typename V> Nullable(V) constructor, the answer is yes.. except that the constructor takes V by value, which means we need to move-construct the argument.
5. This ends up asking whether we can construct a V == Matcher from Matcher&&, and boom that's our recursion.

This theory is validated by changing to template <typename V> Nullable(V const&), which does not produce an error.

The mystery here is why this isn't an issue when we hide __builtin_invokable behind the instantiation of a type trait -- IMO that should always fail.

I think we've seen similar issues in the past in e.g. tuple and we ended up lazy-evaluating the SFINAE in the tuple constructors, for example https://github.com/llvm/llvm-project/blob/main/libcxx/include/tuple#L663-L664. Based on this, I'm tempted to say that this is a bug in our implementation of std::variant rather than a bug in the implementation of std::invoke, but I'd like a compiler expert (@cor3ntin ?) to also weigh in. Is there a reason why using the builtin directly inside decltype should produce a different behavior than hiding it behind a type trait?

Also, I would originally have been tempted to declare the code above as non-conforming because (1)-(5) should always blow up, but other implementations are happy with the code so I'm not sure. We should at least fix it temporarily while we figure out what's the root cause.