Support literal-zero detectors using consteval int constructors

horenmar · horenmar · commit dc51386b9fd6 · 2024-02-12T00:52:53.000+01:00
This was originally motivated by `REQUIRE((a &lt;=&gt; b) == 0)` no
longer compiling using MSVC. After some investigation, I found
that they changed their implementation of the zero literal
detector from the previous pointer-constructor with deleted
other constructors, into one that uses `consteval` constructor
from int.

This breaks the previous detection logic, because now
`is_foo_comparable&lt;std::strong_ordering, int&gt;` is true, but
actually trying to compare them is a compile-time error...
The solution was to make the decomposition `constexpr` and rely
on a late C++20 DR that makes it so that `consteval` propagates
up through the callstack of `constexpr` functions, until it either
runs out of `constexpr` functions, or succeeds.

However, the default handling of types in decomposition is to
take a reference to them. This reference never becomes dangling,
but because the constexpr evaluation engine cannot prove this,
decomposition paths taking references to objects cannot be
actually evaluated at compilation time. Thankfully we already
did have a value-oriented decomposition path for arithmetic types
(as these are common linkage-less types), so we could just
explicitly spell out the `std::foo_ordering` types as also being
supposed to be decomposed by-value.

Two more fun facts about these changes
 1) The original motivation of the MSVC change was to avoid
    trigering a `Wzero-as-null-pointer-constant` warning. I still
    do not believe this was a good decision.
 2) Current latest version of MSVC does not actually implement the
    aforementioned C++20 DR, so even with this commit, MSVC cannot
    compile `REQUIRE((a &lt;=&gt; b) == 0)`.
diff --git a/src/catch2/internal/catch_compiler_capabilities.hpp b/src/catch2/internal/catch_compiler_capabilities.hpp
@@ -37,6 +37,10 @@
 #    define CATCH_CPP17_OR_GREATER
 #  endif
 
+#  if (__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= 202002L)
+#    define CATCH_CPP20_OR_GREATER
+#  endif
+
 #endif
 
 // Only GCC compiler should be used in this block, so other compilers trying to
diff --git a/src/catch2/internal/catch_decomposer.cpp b/src/catch2/internal/catch_decomposer.cpp
@@ -10,7 +10,12 @@
 
 namespace Catch {
 
-    ITransientExpression::~ITransientExpression() = default;
+    void ITransientExpression::streamReconstructedExpression(
+        std::ostream& os ) const {
+        // We can't make this function pure virtual to keep ITransientExpression
+        // constexpr, so we write error message instead
+        os << "Some class derived from ITransientExpression without overriding streamReconstructedExpression";
+    }
 
     void formatReconstructedExpression( std::ostream &os, std::string const& lhs, StringRef op, std::string const& rhs ) {
         if( lhs.size() + rhs.size() < 40 &&
diff --git a/src/catch2/internal/catch_decomposer.hpp b/src/catch2/internal/catch_decomposer.hpp
@@ -13,6 +13,7 @@
 #include <catch2/internal/catch_compare_traits.hpp>
 #include <catch2/internal/catch_test_failure_exception.hpp>
 #include <catch2/internal/catch_logical_traits.hpp>
+#include <catch2/internal/catch_compiler_capabilities.hpp>
 
 #include <type_traits>
 #include <iosfwd>
@@ -34,8 +35,33 @@
 #  pragma GCC diagnostic ignored "-Wsign-compare"
 #endif
 
+#if defined(CATCH_CPP20_OR_GREATER) && __has_include(<compare>)
+#  include <compare>
+#    if defined( __cpp_lib_three_way_comparison ) && \
+            __cpp_lib_three_way_comparison >= 201907L
+#      define CATCH_CONFIG_CPP20_COMPARE_OVERLOADS
+#    endif
+#endif
+
 namespace Catch {
 
+    // Note: There is nothing that stops us from extending this,
+    //       e.g. to `std::is_scalar`, but the more encompassing
+    //       traits are usually also more expensive. For now we
+    //       keep this as it used to be and it can be changed later.
+    template <typename T>
+    struct capture_by_value
+        : std::integral_constant<bool, std::is_arithmetic<T>{}> {};
+
+#if defined( CATCH_CONFIG_CPP20_COMPARE_OVERLOADS )
+    template <>
+    struct capture_by_value<std::strong_ordering> : std::true_type {};
+    template <>
+    struct capture_by_value<std::weak_ordering> : std::true_type {};
+    template <>
+    struct capture_by_value<std::partial_ordering> : std::true_type {};
+#endif
+
     template <typename T>
     struct always_false : std::false_type {};
 
@@ -44,11 +70,12 @@ namespace Catch {
         bool m_result;
 
     public:
-        auto isBinaryExpression() const -> bool { return m_isBinaryExpression; }
-        auto getResult() const -> bool { return m_result; }
-        virtual void streamReconstructedExpression( std::ostream &os ) const = 0;
+        constexpr auto isBinaryExpression() const -> bool { return m_isBinaryExpression; }
+        constexpr auto getResult() const -> bool { return m_result; }
+        //! This function **has** to be overriden by the derived class.
+        virtual void streamReconstructedExpression( std::ostream& os ) const;
 
-        ITransientExpression( bool isBinaryExpression, bool result )
+        constexpr ITransientExpression( bool isBinaryExpression, bool result )
         :   m_isBinaryExpression( isBinaryExpression ),
             m_result( result )
         {}
@@ -59,7 +86,7 @@ namespace Catch {
 
         // We don't actually need a virtual destructor, but many static analysers
         // complain if it's not here :-(
-        virtual ~ITransientExpression(); // = default;
+        virtual ~ITransientExpression() = default;
 
         friend std::ostream& operator<<(std::ostream& out, ITransientExpression const& expr) {
             expr.streamReconstructedExpression(out);
@@ -81,7 +108,7 @@ namespace Catch {
         }
 
     public:
-        BinaryExpr( bool comparisonResult, LhsT lhs, StringRef op, RhsT rhs )
+        constexpr BinaryExpr( bool comparisonResult, LhsT lhs, StringRef op, RhsT rhs )
         :   ITransientExpression{ true, comparisonResult },
             m_lhs( lhs ),
             m_op( op ),
@@ -154,7 +181,7 @@ namespace Catch {
         }
 
     public:
-        explicit UnaryExpr( LhsT lhs )
+        explicit constexpr UnaryExpr( LhsT lhs )
         :   ITransientExpression{ false, static_cast<bool>(lhs) },
             m_lhs( lhs )
         {}
@@ -165,30 +192,30 @@ namespace Catch {
     class ExprLhs {
         LhsT m_lhs;
     public:
-        explicit ExprLhs( LhsT lhs ) : m_lhs( lhs ) {}
+        explicit constexpr ExprLhs( LhsT lhs ) : m_lhs( lhs ) {}
 
 #define CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( id, op )           \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT&& rhs )                       \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs )             \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>,      \
-                                Detail::negation<std::is_arithmetic<           \
+                                Detail::negation<capture_by_value<             \
                                     std::remove_reference_t<RhsT>>>>::value,   \
             BinaryExpr<LhsT, RhsT const&>> {                                   \
         return {                                                               \
             static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>,      \
-                                std::is_arithmetic<RhsT>>::value,              \
+                                capture_by_value<RhsT>>::value,                \
             BinaryExpr<LhsT, RhsT>> {                                          \
         return {                                                               \
             static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<                                               \
                 Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>,    \
@@ -202,7 +229,7 @@ namespace Catch {
             static_cast<bool>( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs };   \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<                                               \
                 Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>,    \
@@ -220,28 +247,29 @@ namespace Catch {
 
     #undef CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR
 
+
 #define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op )         \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT&& rhs )                       \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs )             \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>,      \
-                                Detail::negation<std::is_arithmetic<           \
+                                Detail::negation<capture_by_value<             \
                                     std::remove_reference_t<RhsT>>>>::value,   \
             BinaryExpr<LhsT, RhsT const&>> {                                   \
         return {                                                               \
             static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>,      \
-                                std::is_arithmetic<RhsT>>::value,              \
+                                capture_by_value<RhsT>>::value,                \
             BinaryExpr<LhsT, RhsT>> {                                          \
         return {                                                               \
             static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<                                               \
                 Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>,    \
@@ -253,7 +281,7 @@ namespace Catch {
             static_cast<bool>( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs };   \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
         ->std::enable_if_t<                                                    \
             Detail::conjunction<                                               \
                 Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>,    \
@@ -274,16 +302,16 @@ namespace Catch {
 
 #define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op )                        \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT&& rhs )                       \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs )             \
         ->std::enable_if_t<                                                    \
-            !std::is_arithmetic<std::remove_reference_t<RhsT>>::value,         \
+            !capture_by_value<std::remove_reference_t<RhsT>>::value,           \
             BinaryExpr<LhsT, RhsT const&>> {                                   \
         return {                                                               \
             static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
     }                                                                          \
     template <typename RhsT>                                                   \
-    friend auto operator op( ExprLhs&& lhs, RhsT rhs )                         \
-        ->std::enable_if_t<std::is_arithmetic<RhsT>::value,                    \
+    constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs )               \
+        ->std::enable_if_t<capture_by_value<RhsT>::value,                      \
                            BinaryExpr<LhsT, RhsT>> {                           \
         return {                                                               \
             static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
@@ -309,19 +337,23 @@ namespace Catch {
             "wrap the expression inside parentheses, or decompose it");
         }
 
-        auto makeUnaryExpr() const -> UnaryExpr<LhsT> {
+        constexpr auto makeUnaryExpr() const -> UnaryExpr<LhsT> {
             return UnaryExpr<LhsT>{ m_lhs };
         }
     };
 
     struct Decomposer {
-        template<typename T, std::enable_if_t<!std::is_arithmetic<std::remove_reference_t<T>>::value, int> = 0>
-        friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs<T const&> {
+        template <typename T,
+                  std::enable_if_t<
+                      !capture_by_value<std::remove_reference_t<T>>::value,
+                      int> = 0>
+        constexpr friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs<T const&> {
             return ExprLhs<const T&>{ lhs };
         }
 
-        template<typename T, std::enable_if_t<std::is_arithmetic<T>::value, int> = 0>
-        friend auto operator <= ( Decomposer &&, T value ) -> ExprLhs<T> {
+        template <typename T,
+                  std::enable_if_t<capture_by_value<T>::value, int> = 0>
+        constexpr friend auto operator <= ( Decomposer &&, T value ) -> ExprLhs<T> {
             return ExprLhs<T>{ value };
         }
     };
diff --git a/tests/SelfTest/UsageTests/Compilation.tests.cpp b/tests/SelfTest/UsageTests/Compilation.tests.cpp
@@ -313,11 +313,12 @@ TEST_CASE("ADL universal operators don't hijack expression deconstruction", "[co
     REQUIRE(0 ^ adl::always_true{});
 }
 
-TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long) are supported", "[compilation][approvals]" ) {
+TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented as pointer conversion are supported",
+           "[compilation][approvals]" ) {
     REQUIRE( TypeWithLit0Comparisons{} < 0 );
     REQUIRE_FALSE( 0 < TypeWithLit0Comparisons{} );
     REQUIRE( TypeWithLit0Comparisons{} <= 0 );
-    REQUIRE_FALSE( 0 > TypeWithLit0Comparisons{} );
+    REQUIRE_FALSE( 0 <= TypeWithLit0Comparisons{} );
 
     REQUIRE( TypeWithLit0Comparisons{} > 0 );
     REQUIRE_FALSE( 0 > TypeWithLit0Comparisons{} );
@@ -330,6 +331,72 @@ TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long
     REQUIRE_FALSE( 0 != TypeWithLit0Comparisons{} );
 }
 
+// These tests require `consteval` to propagate through `constexpr` calls
+// which is a late DR against C++20.
+#if defined( CATCH_CPP20_OR_GREATER ) && defined( __cpp_consteval ) && \
+    __cpp_consteval >= 202211L
+// Can't have internal linkage to avoid warnings
+void ZeroLiteralErrorFunc();
+namespace {
+    struct ZeroLiteralConsteval {
+        template <class T, std::enable_if_t<std::is_same_v<T, int>, int> = 0>
+        consteval ZeroLiteralConsteval( T zero ) noexcept {
+            if ( zero != 0 ) { ZeroLiteralErrorFunc(); }
+        }
+    };
+
+    // Should only be constructible from literal 0. Uses the propagating
+    // consteval constructor trick (currently used by MSVC, might be used
+    // by libc++ in the future as well).
+    struct TypeWithConstevalLit0Comparison {
+#    define DEFINE_COMP_OP( op )                                               \
+        constexpr friend bool operator op( TypeWithConstevalLit0Comparison,    \
+                                           ZeroLiteralConsteval ) {            \
+            return true;                                                       \
+        }                                                                      \
+        constexpr friend bool operator op( ZeroLiteralConsteval,               \
+                                           TypeWithConstevalLit0Comparison ) { \
+            return false;                                                      \
+        }
+
+        DEFINE_COMP_OP( < )
+        DEFINE_COMP_OP( <= )
+        DEFINE_COMP_OP( > )
+        DEFINE_COMP_OP( >= )
+        DEFINE_COMP_OP( == )
+        DEFINE_COMP_OP( != )
+
+#undef DEFINE_COMP_OP
+    };
+
+} // namespace
+
+namespace Catch {
+    template <>
+    struct capture_by_value<TypeWithConstevalLit0Comparison> : std::true_type {};
+}
+
+TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented as consteval check are supported",
+           "[compilation][approvals]" ) {
+    REQUIRE( TypeWithConstevalLit0Comparison{} < 0 );
+    REQUIRE_FALSE( 0 < TypeWithConstevalLit0Comparison{} );
+    REQUIRE( TypeWithConstevalLit0Comparison{} <= 0 );
+    REQUIRE_FALSE( 0 <= TypeWithConstevalLit0Comparison{} );
+
+    REQUIRE( TypeWithConstevalLit0Comparison{} > 0 );
+    REQUIRE_FALSE( 0 > TypeWithConstevalLit0Comparison{} );
+    REQUIRE( TypeWithConstevalLit0Comparison{} >= 0 );
+    REQUIRE_FALSE( 0 >= TypeWithConstevalLit0Comparison{} );
+
+    REQUIRE( TypeWithConstevalLit0Comparison{} == 0 );
+    REQUIRE_FALSE( 0 == TypeWithConstevalLit0Comparison{} );
+    REQUIRE( TypeWithConstevalLit0Comparison{} != 0 );
+    REQUIRE_FALSE( 0 != TypeWithConstevalLit0Comparison{} );
+}
+
+#endif // C++20 consteval
+
+
 namespace {
     struct MultipleImplicitConstructors {
         MultipleImplicitConstructors( double ) {}
diff --git a/tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp b/tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp
@@ -12,23 +12,28 @@
 #include <type_traits>
 
 // Should only be constructible from literal 0.
+// Based on the constructor from pointer trick, used by libstdc++ and libc++
+// (formerly also MSVC, but they've moved to consteval int constructor).
 // Used by `TypeWithLit0Comparisons` for testing comparison
 // ops that only work with literal zero, the way std::*orderings do
-struct ZeroLiteralDetector {
-    constexpr ZeroLiteralDetector( ZeroLiteralDetector* ) noexcept {}
+struct ZeroLiteralAsPointer {
+    constexpr ZeroLiteralAsPointer( ZeroLiteralAsPointer* ) noexcept {}
 
     template <typename T,
               typename = std::enable_if_t<!std::is_same<T, int>::value>>
-    constexpr ZeroLiteralDetector( T ) = delete;
+    constexpr ZeroLiteralAsPointer( T ) = delete;
 };
 
+
 struct TypeWithLit0Comparisons {
-#define DEFINE_COMP_OP( op )                                                  \
-    friend bool operator op( TypeWithLit0Comparisons, ZeroLiteralDetector ) { \
-        return true;                                                          \
-    }                                                                         \
-    friend bool operator op( ZeroLiteralDetector, TypeWithLit0Comparisons ) { \
-        return false;                                                         \
+#define DEFINE_COMP_OP( op )                                       \
+    constexpr friend bool operator op( TypeWithLit0Comparisons,    \
+                                       ZeroLiteralAsPointer ) {    \
+        return true;                                               \
+    }                                                              \
+    constexpr friend bool operator op( ZeroLiteralAsPointer,       \
+                                       TypeWithLit0Comparisons ) { \
+        return false;                                              \
     }
 
     DEFINE_COMP_OP( < )
