Compare integers with floats exactly instead of widening the integer (#5459)

The mixed number arms of JSON_IMPLEMENT_OPERATOR cast the integer to
number_float_t before comparing. Past the float's mantissa that cast is lossy:
2^63-2 and 2^63-1 both round to 2^63, so each compares equal to that float
while differing from each other. Equality is therefore intransitive and the
ordering is not a strict weak ordering, which makes std::sort over such values,
or using them as keys in std::set or std::map, undefined behavior.

Compare the two exactly instead. The integer's range is a power of two the
float represents exactly, so a float outside it is ordered by magnitude alone;
inside it, truncating the float is exact, and the integer parts and then any
fractional part decide. The helper hands back a pair whose comparison with the
original operator reproduces that ordering, which keeps every operator's return
type as it was, including partial_ordering for the spaceship.

A NaN operand is returned in both members, so NaN stays false for the
relational operators and unordered for <=>. Values a float represents exactly
still compare equal, so json(1) == json(1.0) is unchanged.

Signed-off-by: qatcod <79017227+qatcod@users.noreply.github.com>

include/nlohmann/detail/value_t.hpp

@@ -9,9 +9,12 @@
 #pragma once
 
 #include <array> // array
+#include <cmath> // isnan, ldexp, trunc
 #include <cstddef> // size_t
 #include <cstdint> // uint8_t
+#include <limits> // numeric_limits
 #include <string> // string
+#include <type_traits> // is_signed
 
 #include <nlohmann/detail/macro_scope.hpp>
 #if JSON_HAS_THREE_WAY_COMPARISON
@@ -114,5 +117,67 @@ inline bool operator<(const value_t lhs, const value_t rhs) noexcept
 }
 #endif
 
+
+/*!
+@brief compare an integer with a floating point number without precision loss
+
+Widening the integer to the floating point type loses precision beyond the
+float's mantissa, which makes equality intransitive: both 2^63-2 and 2^63-1
+round to 2^63, so each compares equal to that float while differing from each
+other. Ordering built on that is not a strict weak ordering, so sorting such
+values, or using them as keys in an ordered container, is undefined behavior.
+
+Returns a value to be compared against zero with the original operator, which
+reproduces the exact ordering. A NaN operand is returned as is, so comparing it
+against zero keeps NaN's semantics: false for the relational operators and
+unordered for `<=>`.
+*/
+template<typename IntegerType, typename FloatType>
+FloatType compare_integer_with_float(const IntegerType i, const FloatType f) noexcept
+{
+    const auto ordered = [](int c) noexcept
+    {
+        return static_cast<FloatType>(c);
+    };
+
+    if (std::isnan(f))
+    {
+        return f;
+    }
+
+    // values of IntegerType lie in [-bound, bound) when signed and in
+    // [0, bound) when unsigned; digits excludes the sign bit, so bound is a
+    // power of two that the float represents exactly
+    const FloatType bound = std::ldexp(static_cast<FloatType>(1), std::numeric_limits<IntegerType>::digits);
+    if (f >= bound)
+    {
+        return ordered(-1);
+    }
+    if (std::is_signed<IntegerType>::value ? (f < -bound) : (f < static_cast<FloatType>(0)))
+    {
+        return ordered(1);
+    }
+
+    // f is now within the integer's range, so truncating it is exact
+    const FloatType truncated = std::trunc(f);
+    const auto as_integer = static_cast<IntegerType>(truncated);
+    if (i != as_integer)
+    {
+        return ordered(i < as_integer ? -1 : 1);
+    }
+
+    // the integer parts agree, so any fractional part decides
+    const FloatType fraction = f - truncated;
+    if (fraction > static_cast<FloatType>(0))
+    {
+        return ordered(-1);
+    }
+    if (fraction < static_cast<FloatType>(0))
+    {
+        return ordered(1);
+    }
+    return ordered(0);
+}
+
 }  // namespace detail
 NLOHMANN_JSON_NAMESPACE_END

include/nlohmann/json.hpp

@@ -3703,19 +3703,19 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     }                                                                                                    \
     else if (lhs_type == value_t::number_integer && rhs_type == value_t::number_float)                   \
     {                                                                                                    \
-        return static_cast<number_float_t>(lhs.m_data.m_value.number_integer) op rhs.m_data.m_value.number_float;      \
+        return (detail::compare_integer_with_float(lhs.m_data.m_value.number_integer, rhs.m_data.m_value.number_float)) op (static_cast<number_float_t>(0)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_float && rhs_type == value_t::number_integer)                   \
     {                                                                                                    \
-        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_integer);      \
+        return (static_cast<number_float_t>(0)) op (detail::compare_integer_with_float(rhs.m_data.m_value.number_integer, lhs.m_data.m_value.number_float)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_float)                  \
     {                                                                                                    \
-        return static_cast<number_float_t>(lhs.m_data.m_value.number_unsigned) op rhs.m_data.m_value.number_float;     \
+        return (detail::compare_integer_with_float(lhs.m_data.m_value.number_unsigned, rhs.m_data.m_value.number_float)) op (static_cast<number_float_t>(0)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_float && rhs_type == value_t::number_unsigned)                  \
     {                                                                                                    \
-        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_unsigned);     \
+        return (static_cast<number_float_t>(0)) op (detail::compare_integer_with_float(rhs.m_data.m_value.number_unsigned, lhs.m_data.m_value.number_float)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer)                \
     {                                                                                                    \

single_include/nlohmann/json.hpp

@@ -223,9 +223,12 @@
 
 
 #include <array> // array
+#include <cmath> // isnan, ldexp, trunc
 #include <cstddef> // size_t
 #include <cstdint> // uint8_t
+#include <limits> // numeric_limits
 #include <string> // string
+#include <type_traits> // is_signed
 
 // #include <nlohmann/detail/macro_scope.hpp>
 //     __ _____ _____ _____
@@ -3283,6 +3286,68 @@ inline bool operator<(const value_t lhs, const value_t rhs) noexcept
 }
 #endif
 
+
+/*!
+@brief compare an integer with a floating point number without precision loss
+
+Widening the integer to the floating point type loses precision beyond the
+float's mantissa, which makes equality intransitive: both 2^63-2 and 2^63-1
+round to 2^63, so each compares equal to that float while differing from each
+other. Ordering built on that is not a strict weak ordering, so sorting such
+values, or using them as keys in an ordered container, is undefined behavior.
+
+Returns a value to be compared against zero with the original operator, which
+reproduces the exact ordering. A NaN operand is returned as is, so comparing it
+against zero keeps NaN's semantics: false for the relational operators and
+unordered for `<=>`.
+*/
+template<typename IntegerType, typename FloatType>
+FloatType compare_integer_with_float(const IntegerType i, const FloatType f) noexcept
+{
+    const auto ordered = [](int c) noexcept
+    {
+        return static_cast<FloatType>(c);
+    };
+
+    if (std::isnan(f))
+    {
+        return f;
+    }
+
+    // values of IntegerType lie in [-bound, bound) when signed and in
+    // [0, bound) when unsigned; digits excludes the sign bit, so bound is a
+    // power of two that the float represents exactly
+    const FloatType bound = std::ldexp(static_cast<FloatType>(1), std::numeric_limits<IntegerType>::digits);
+    if (f >= bound)
+    {
+        return ordered(-1);
+    }
+    if (std::is_signed<IntegerType>::value ? (f < -bound) : (f < static_cast<FloatType>(0)))
+    {
+        return ordered(1);
+    }
+
+    // f is now within the integer's range, so truncating it is exact
+    const FloatType truncated = std::trunc(f);
+    const auto as_integer = static_cast<IntegerType>(truncated);
+    if (i != as_integer)
+    {
+        return ordered(i < as_integer ? -1 : 1);
+    }
+
+    // the integer parts agree, so any fractional part decides
+    const FloatType fraction = f - truncated;
+    if (fraction > static_cast<FloatType>(0))
+    {
+        return ordered(-1);
+    }
+    if (fraction < static_cast<FloatType>(0))
+    {
+        return ordered(1);
+    }
+    return ordered(0);
+}
+
 }  // namespace detail
 NLOHMANN_JSON_NAMESPACE_END
 
@@ -25057,19 +25122,19 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     }                                                                                                    \
     else if (lhs_type == value_t::number_integer && rhs_type == value_t::number_float)                   \
     {                                                                                                    \
-        return static_cast<number_float_t>(lhs.m_data.m_value.number_integer) op rhs.m_data.m_value.number_float;      \
+        return (detail::compare_integer_with_float(lhs.m_data.m_value.number_integer, rhs.m_data.m_value.number_float)) op (static_cast<number_float_t>(0)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_float && rhs_type == value_t::number_integer)                   \
     {                                                                                                    \
-        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_integer);      \
+        return (static_cast<number_float_t>(0)) op (detail::compare_integer_with_float(rhs.m_data.m_value.number_integer, lhs.m_data.m_value.number_float)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_float)                  \
     {                                                                                                    \
-        return static_cast<number_float_t>(lhs.m_data.m_value.number_unsigned) op rhs.m_data.m_value.number_float;     \
+        return (detail::compare_integer_with_float(lhs.m_data.m_value.number_unsigned, rhs.m_data.m_value.number_float)) op (static_cast<number_float_t>(0)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_float && rhs_type == value_t::number_unsigned)                  \
     {                                                                                                    \
-        return lhs.m_data.m_value.number_float op static_cast<number_float_t>(rhs.m_data.m_value.number_unsigned);     \
+        return (static_cast<number_float_t>(0)) op (detail::compare_integer_with_float(rhs.m_data.m_value.number_unsigned, lhs.m_data.m_value.number_float)); \
     }                                                                                                    \
     else if (lhs_type == value_t::number_unsigned && rhs_type == value_t::number_integer)                \
     {                                                                                                    \

tests/src/unit-comparison.cpp

@@ -326,6 +326,57 @@ TEST_CASE("lexicographical comparison operators")
 #endif
         }
 
+        SECTION("integer/float mixed comparison is exact")
+        {
+            // Widening the integer to a double loses precision past the
+            // mantissa, so 2^63-2 and 2^63-1 both used to compare equal to the
+            // double 2^63 while differing from each other. That makes equality
+            // intransitive and the ordering not a strict weak ordering.
+            const json below_two_63 = static_cast<std::int64_t>(9223372036854775806LL);
+            const json max_int64 = (std::numeric_limits<std::int64_t>::max)();
+            const json two_63 = 9223372036854775808.0;
+
+            CHECK_FALSE(below_two_63 == two_63);
+            CHECK_FALSE(max_int64 == two_63);
+            CHECK(below_two_63 != max_int64);
+            CHECK(below_two_63 < max_int64);
+            CHECK(below_two_63 < two_63);
+            CHECK(max_int64 < two_63);
+            CHECK(two_63 > max_int64);
+            CHECK_FALSE(two_63 < max_int64);
+
+            // the same past the unsigned range
+            const json max_uint64 = (std::numeric_limits<std::uint64_t>::max)();
+            const json two_64 = 18446744073709551616.0;
+            CHECK_FALSE(max_uint64 == two_64);
+            CHECK(max_uint64 < two_64);
+            CHECK(two_64 > max_uint64);
+
+            // values a double represents exactly still compare equal
+            CHECK(json(1) == json(1.0));
+            CHECK(json(1u) == json(1.0));
+            CHECK(json(-3) == json(-3.0));
+            CHECK(json(1) < json(1.5));
+            CHECK(json(1.5) < json(2));
+            CHECK(json(2) > json(1.5));
+
+            // a NaN operand stays unordered against either integer kind
+            CHECK_FALSE(json(1) == json(nan));
+            CHECK_FALSE(json(1) < json(nan));
+            CHECK_FALSE(json(nan) < json(1));
+            CHECK_FALSE(json(1u) == json(nan));
+
+#if JSON_HAS_THREE_WAY_COMPARISON
+            // JSON_HAS_CPP_20 (do not remove; see note at top of file)
+            CHECK((max_int64 <=> two_63) == std::partial_ordering::less); // *NOPAD*
+            CHECK((two_63 <=> max_int64) == std::partial_ordering::greater); // *NOPAD*
+            CHECK((below_two_63 <=> max_int64) == std::partial_ordering::less); // *NOPAD*
+            CHECK((max_uint64 <=> two_64) == std::partial_ordering::less); // *NOPAD*
+            CHECK((json(1) <=> json(1.0)) == std::partial_ordering::equivalent); // *NOPAD*
+            CHECK((json(1) <=> json(nan)) == std::partial_ordering::unordered); // *NOPAD*
+#endif
+        }
+
         SECTION("compares unordered")
         {
             std::vector<std::vector<bool>> expected =