[libc++] type_traits: fix short-circuiting in std::conjunction.

[jacobsa]

Summary

Replace the two-level implementation with a simpler one that directly subclasses
the predicates, avoiding the instantiation of the template to get the type
member in a situation where we should short-circuit. This prevents incorrect
diagnostics when the instantiated predicate contains a static assertion.

Add a test case that reproduced the previous problem. The existing test case
involving HasNoValue didn't catch the problem because HasNoValue was in the
final position. The bug comes up when the predicate that shouldn't be
instantiated is after the short-circuit position but there is more to follow,
because then __conjunction_impl<False, BadPredicate, ...> instantiates
__conjunction_impl<BadPredicate, ...> (in order to obtain its type member),
which in turn instantiates BadPredicate in order to obtain its value member.

In contrast the new implementation doesn't recurse in instantiation any further
than it needs to, because it doesn't require particular members of the recursive
case.

I've also updated the test cases for std::disjunction to match,
although it doesn't have the same particular bug (its implementation is
quite different).

Fixes #58490.

[ldionne]

Thanks for the fix! Some comments but generally looks like this is right.

libcxx/include/__type_traits/conjunction.h

26–28

I think this would be an equivalent fix, no?

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

23

Do we have the same issue for disjunction? Regardless, IMO we should add a test.

26

Regarding your comment on the Github issue:

Reminding my future self to look here for instructions to build and test libc++, because the getting started instructions don't work, or at least don't document the prerequisite steps.

What didn't work with the instructions at https://libcxx.llvm.org/TestingLibcxx.html#getting-started?

33

Can you explain why our existing test case for HasNoValue doesn't cover this? Is it possible that simply adding the following test case would be sufficient?

static_assert(!std::conjunction<std::false_type, HasNoValue, std::true_type>::value);

96

[jacobsa]

Simpler test case, better commit description, test std::disjunction too.

libcxx/include/__type_traits/conjunction.h

26–28

Perhaps, but is there any reason to prefer that? I don't see any benefit to the
two-level implementation; it just makes it harder to understand as far as I can
tell. Am I missing something?

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

23

I added the corresponding tests there for symmetry. They do pass, although I'm
not confident there isn't a similar bug because its implementation is a little
weird. If you would take it, I'd be happy to send a follow-up commit to make
its implementation straightforward pattern matching like this one.

26

The instructions seem to assume that you've done some previous step that isn't
documented (or even linked?) there:

jacobsa@abel ~> cd clients/llvm-project/

jacobsa@abel ~/c/llvm-project> make check-cxx
make: *** No rule to make target 'check-cxx'.  Stop.

jacobsa@abel ~/c/llvm-project 2> cd build

jacobsa@abel ~/c/l/build> make check-cxx
make: *** No rule to make target 'check-cxx'.  Stop.

I'm sure how to deal with this is obvious to someone more experienced with the
project, but for me I couldn't figure it out. In contrast the other
instructions I linked work fine.

33

Indeed, that's a much simpler reproducer, thanks. Switched to that.

I added my understanding of the reason it wasn't reproduced before to the
commit description.

[ldionne]

libcxx/include/__type_traits/conjunction.h

26–28

I believe the goal might have been to avoid partially specializing std::conjunction itself as a matter of conformance. @philnik is that something you had in mind? If so, we'd need a test for that too (which @philnik can help you with since he should have added it when he last modified these traits :-) ).

If that's not a concern, then either implementation should do.

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

15

General tip: you can mark review comments as done when you've addressed them, we generally use that to track what's still left to address on a review! In this case I'll mark this very comment as done since I'm only putting this here for your information, with no action item associated to it.

23

That may make sense, but let's first hear what @philnik has to say about the implementation of conjunction.

26

Is it possible that you are doing a bootstrapping build, and that the Note item on that page applies to you? If not, I'd be curious to see what your CMake command looks like.

[jacobsa]

libcxx/include/__type_traits/conjunction.h

26–28

It seems like [meta.logical]/3 specifically says it *will* be a specialization,
if I read the wording correctly:

https://timsong-cpp.github.io/cppwp/n4868/meta.logical#3

For a specialization conjunction<B1, …, BN>, if there is a ...

That said, is there even any way to differentiate the two from outside?

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

15

Thanks, sorry for getting this wrong. I'm quite confused by the fact that there
are threads with replies, but replies are individually marked done. If I reply
to a comment with a question or a follow-up, am I supposed to mark the original
as done? Am I supposed to mark my own comment as done (the default)? It's kind
of weird coming from Google's code review system, where the done bit applies to
entire threads.

26

My point is that those instructions don't discuss any of that. The section
mentions a bootstrapping build, but doesn't define the terms. It doesn't say
anything about using cmake first; it implies all you have to do is run `make
check-cxx`.

I'm not saying this is wrong, but it's definitely incomplete. It was not
sufficient for me to "get started". Does that make sense?

[philnik]

LGTM. Leaving final approval to @ldionne.

libcxx/include/__type_traits/conjunction.h

26–28

I don't think that's a problem; I definitely didn't have it in mind then. I also can't remember why I implemented it in this way, most likely because I replaced _And and didn't think of just specializing conjunction itself.

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

23

The disjunction implementation is a bit weird to avoid instantiating lots of classes. Unless you can show that the optimization doesn't actually do anything, I'd rather keep it as-is.

[ldionne]

LGTM w/ suggestion. If you don't have commit access and would like us to commit this for you, please provide Author Name <email@domain> for us to commit for you.

@philnik you can arc patch the diff and then git commit --am --no-edit --author='Author Name <email@domain>' to set the right author (sometimes arc patch will mess that up). Sorry if you already know that, I'm not sure you've committed stuff on behalf of others before but now you'll know :-).

libcxx/include/__type_traits/conjunction.h

26–28

My thinking was along the lines of "what if a user specializes it, would it behave correctly"? That's extremely pedantic and I'm fine with your approach.

Thanks both for the explanation, I've been stretched for time to research myself.

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

83

I would suggest adding a comment linking to the bug report, otherwise one may be tempted to remove these seemingly not very useful tests in the future.

[jacobsa]

Add test comments, as suggested by ldionne.

[jacobsa]

Thanks! I don't have commit access, so it would be great if you could do it.
Please credit:

Aaron Jacobs <[e-mail]>

libcxx/include/__type_traits/conjunction.h

26–28

Ah I see, thanks. Programs that specialize std::conjunction are forbidden according to cppreference:

It is allowed to add template specializations for any standard library class template to the namespace std only if the declaration depends on at least one program-defined type [...]

[..]

• None of the templates defined in <type_traits> may be specialized for a program-defined type, except for std::common_type and std::basic_common_reference. This includes the type traits and the class template std::integral_constant.

I believe this comes from meta.rqmts/4 in C++20:

Unless otherwise specified, the behavior of a program that adds specializations for any of the templates specified in [meta] is undefined.

libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp

23

Ack, thanks.

libcxx/include/__type_traits/conjunction.h

 #include <__type_traits/integral_constant.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#if _LIBCPP_STD_VER > 14
-
-template <class _Arg, class... _Args>
-struct __conjunction_impl {
-  using type = conditional_t<!bool(_Arg::value), _Arg, typename __conjunction_impl<_Args...>::type>;
-};
-
-template <class _Arg>
-struct __conjunction_impl<_Arg> {
-  using type = _Arg;
-};
-
-template <class... _Args>
-struct conjunction : __conjunction_impl<true_type, _Args...>::type {};
-
-template<class... _Args>
-inline constexpr bool conjunction_v = conjunction<_Args...>::value;
-
-#endif // _LIBCPP_STD_VER > 14
-
 template <class...>
 using __expand_to_true = true_type;
 
 template <class... _Pred>
 __expand_to_true<__enable_if_t<_Pred::value>...> __and_helper(int);
 
 template <class...>
 false_type __and_helper(...);
 
 template <class... _Pred>
 using _And _LIBCPP_NODEBUG = decltype(__and_helper<_Pred...>(0));
 
+#if _LIBCPP_STD_VER > 14
+
+template <class...>
+struct conjunction : true_type {};
+
+template <class _Arg>
+struct conjunction<_Arg> : _Arg {};
+
+template <class _Arg, class... _Args>
+struct conjunction<_Arg, _Args...> : conditional_t<!bool(_Arg::value), _Arg, conjunction<_Args...>> {};
+
+template <class... _Args>
+inline constexpr bool conjunction_v = conjunction<_Args...>::value;
+
+#endif // _LIBCPP_STD_VER > 14
+
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___TYPE_TRAITS_CONJUNCTION_H

libcxx/test/std/utilities/meta/meta.logical/conjunction.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, c++11, c++14
 // type_traits
 
 // template<class... B> struct conjunction;                           // C++17
 // template<class... B>
 //   constexpr bool conjunction_v = conjunction<B...>::value;         // C++17
 
 #include <cassert>
 #include <type_traits>
 
 #include "test_macros.h"
 
 struct True  { static constexpr bool value = true; };
 struct False { static constexpr bool value = false; };
 
 struct MySpecialTrueType { static constexpr auto value = true; static constexpr auto MySpecial = 23; };
 struct MyOtherSpecialTrueType { static constexpr auto value = -1; static constexpr auto MySpecial = 46; };
 struct MySpecialFalseType { static constexpr auto value = false; static constexpr auto MySpecial = 37; };
 struct HasNoValue {};
 struct ExplicitlyConvertibleToBool { explicit constexpr operator bool() const { return true; } };
 struct ValueExplicitlyConvertible { static constexpr ExplicitlyConvertibleToBool value {}; };
 
 static_assert( std::conjunction<>::value);
 static_assert( std::conjunction<std::true_type >::value);
 static_assert(!std::conjunction<std::false_type>::value);
 
 static_assert( std::conjunction_v<>);
 static_assert( std::conjunction_v<std::true_type >);
 static_assert(!std::conjunction_v<std::false_type>);
 
 static_assert( std::conjunction<std::true_type,  std::true_type >::value);
 static_assert(!std::conjunction<std::true_type,  std::false_type>::value);
 static_assert(!std::conjunction<std::false_type, std::true_type >::value);

 static_assert(std::is_base_of_v<MySpecialFalseType, std::conjunction<MySpecialFalseType, MyOtherSpecialTrueType>>);
 static_assert(std::is_base_of_v<MySpecialFalseType, std::conjunction<MyOtherSpecialTrueType, MySpecialFalseType>>);
 
 static_assert(std::is_base_of_v<std::false_type, std::conjunction<std::false_type, HasNoValue>>);
 
 static_assert(!std::conjunction<std::false_type, HasNoValue>::value);
 static_assert(!std::conjunction_v<std::false_type, HasNoValue>);
 
+// Also check the case where HasNoValue is not the last in the list (https://llvm.org/PR584900).
+static_assert(!std::conjunction<std::false_type, HasNoValue, std::true_type>::value);
+static_assert(!std::conjunction_v<std::false_type, HasNoValue, std::true_type>);
+
+static_assert(!std::conjunction<std::false_type, HasNoValue, std::false_type>::value);
+static_assert(!std::conjunction_v<std::false_type, HasNoValue, std::false_type>);
+
 static_assert(std::conjunction<MyOtherSpecialTrueType>::value == -1);
 static_assert(std::conjunction_v<MyOtherSpecialTrueType>);
 
 static_assert(std::is_base_of_v<ValueExplicitlyConvertible, std::conjunction<ValueExplicitlyConvertible>>);
 static_assert(std::conjunction_v<ValueExplicitlyConvertible, std::true_type>);

libcxx/test/std/utilities/meta/meta.logical/disjunction.compile.pass.cpp

 static_assert(std::is_base_of_v<MySpecialTrueType, std::disjunction<MySpecialTrueType, MyOtherSpecialFalseType>>);
 static_assert(std::is_base_of_v<MySpecialTrueType, std::disjunction<MyOtherSpecialFalseType, MySpecialTrueType>>);
 
 static_assert(std::is_base_of_v<std::true_type, std::disjunction<std::true_type, HasNoValue>>);
 
 static_assert(std::disjunction<std::true_type, HasNoValue>::value);
 static_assert(std::disjunction_v<std::true_type, HasNoValue>);
 
+// Also check the case where HasNoValue is not the last in the list (https://llvm.org/PR584900).
+static_assert(std::disjunction<std::true_type, HasNoValue, std::true_type>::value);
+static_assert(std::disjunction_v<std::true_type, HasNoValue, std::true_type>);
+
+static_assert(std::disjunction<std::true_type, HasNoValue, std::false_type>::value);
+static_assert(std::disjunction_v<std::true_type, HasNoValue, std::false_type>);
+
 static_assert(std::disjunction<MySpecialTrueType>::value == -1);
 static_assert(std::disjunction_v<MySpecialTrueType>);
 
 static_assert(std::is_base_of_v<ValueExplicitlyConvertible, std::disjunction<ValueExplicitlyConvertible>>);
 static_assert(std::disjunction_v<ValueExplicitlyConvertible, std::true_type>);