-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Fix std::hash contract violation for numeric types (#5256) #5262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 3 commits
68c87ad
48face7
8557b66
10d1879
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| hash(null) = 2654435769 | ||
| hash(false) = 2654436030 | ||
| hash(0) = 2654436095 | ||
| hash(0U) = 2654436156 | ||
| hash("") = 6142509191626859748 | ||
| hash(0) = 2654436221 | ||
| hash(0U) = 2654436221 | ||
| hash(0.0) = 2654436221 | ||
| hash("") = 11160318156688833227 | ||
| hash({}) = 2654435832 | ||
| hash([]) = 2654435899 | ||
| hash({"hello": "world"}) = 4469488738203676328 | ||
| hash({"hello": "world"}) = 3701319991624763853 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ | |
| #include <cstdint> // uint8_t | ||
| #include <cstddef> // size_t | ||
| #include <functional> // hash | ||
| #include <limits> // numeric_limits | ||
| #include <cmath> // isfinite | ||
|
|
||
| #include <nlohmann/detail/abi_macros.hpp> | ||
| #include <nlohmann/detail/value_t.hpp> | ||
|
|
@@ -26,12 +28,73 @@ inline std::size_t combine(std::size_t seed, std::size_t h) noexcept | |
| return seed; | ||
| } | ||
|
|
||
| // Check if a number_integer_t value is exactly representable as number_float_t | ||
| // Returns true if static_cast<number_integer_t>(static_cast<number_float_t>(val)) == val | ||
| template<typename BasicJsonType> | ||
| inline bool is_exactly_representable_as_float(typename BasicJsonType::number_integer_t val) noexcept | ||
| { | ||
| using number_integer_t = typename BasicJsonType::number_integer_t; | ||
| using number_float_t = typename BasicJsonType::number_float_t; | ||
|
|
||
| // If the float type's mantissa covers the integer type's entire range, all values round-trip | ||
| constexpr int float_digits = std::numeric_limits<number_float_t>::digits; | ||
| constexpr int int_digits = std::numeric_limits<number_integer_t>::digits; | ||
|
|
||
| #ifdef JSON_HEDLEY_MSVC_VERSION | ||
| #pragma warning(push ) | ||
| #pragma warning(disable : 4127) // ignore warning to replace if with if constexpr | ||
| #endif | ||
| if (float_digits >= int_digits) | ||
| { | ||
| return true; | ||
| } | ||
| #ifdef JSON_HEDLEY_MSVC_VERSION | ||
| #pragma warning( pop ) | ||
| #endif | ||
|
|
||
| // For values outside float's exact range, they don't round-trip | ||
| // The safe way to check: compute the max magnitude that round-trips | ||
| // Using unsigned arithmetic to avoid UB with negating INT_MIN | ||
|
|
||
| // Max magnitude representable exactly: 2^(digits-1) - 1 for signed, 2^digits - 1 for unsigned range | ||
| // But we're checking a signed value, so use 2^digits as the threshold | ||
| constexpr auto max_exact = static_cast<number_integer_t>(1) << (float_digits - 1); | ||
|
|
||
| // Check absolute value against this threshold | ||
| if (val >= 0) | ||
| { | ||
| if (val >= max_exact) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // For negative values, check via unsigned wrapping arithmetic | ||
| // -val in unsigned domain; if it wraps, the value is too negative | ||
| auto unsigned_abs = static_cast<typename BasicJsonType::number_unsigned_t>(-val); | ||
| if (unsigned_abs >= static_cast<typename BasicJsonType::number_unsigned_t>(max_exact)) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // For values within the exact range, verify the round-trip | ||
| const auto f = static_cast<number_float_t>(val); | ||
| return std::isfinite(f) && static_cast<number_integer_t>(f) == val; | ||
| } | ||
|
|
||
| /*! | ||
| @brief hash a JSON value | ||
|
|
||
| The hash function tries to rely on std::hash where possible. Furthermore, the | ||
| type of the JSON value is taken into account to have different hash values for | ||
| null, 0, 0U, and false, etc. | ||
| most types. However, numeric types (number_integer, number_unsigned, number_float) | ||
| are hashed to satisfy the std::hash contract: if two json values compare equal, | ||
| they must have equal hash values. This means json(42), json(42u), and json(42.0) | ||
| all hash to the same value (since they compare equal). For large integer values | ||
| outside the exact representable range of the float type, integer values are hashed | ||
| in their own domain to avoid precision loss. | ||
|
|
||
| @tparam BasicJsonType basic_json specialization | ||
| @param j JSON value to hash | ||
|
|
@@ -90,20 +153,43 @@ std::size_t hash(const BasicJsonType& j) | |
|
|
||
| case BasicJsonType::value_t::number_integer: | ||
| { | ||
| const auto h = std::hash<number_integer_t> {}(j.template get<number_integer_t>()); | ||
| return combine(type, h); | ||
| const auto v = j.template get<number_integer_t>(); | ||
| // Use a shared numeric type tag so all numeric types that are equal hash the same | ||
| const auto numeric_type = static_cast<std::size_t>(BasicJsonType::value_t::number_float); | ||
|
|
||
| if (is_exactly_representable_as_float<BasicJsonType>(v)) | ||
| { | ||
| const auto h = std::hash<number_float_t> {}(static_cast<number_float_t>(v)); | ||
| return combine(numeric_type, h); | ||
| } | ||
|
|
||
| const auto h = std::hash<number_integer_t> {}(v); | ||
| return combine(numeric_type, h); | ||
| } | ||
|
|
||
| case BasicJsonType::value_t::number_unsigned: | ||
| { | ||
| const auto h = std::hash<number_unsigned_t> {}(j.template get<number_unsigned_t>()); | ||
| return combine(type, h); | ||
| const auto v = j.template get<number_unsigned_t>(); | ||
| // Normalize to signed (matching operator== behavior for U-vs-I comparison) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will no longer match if #5211 is merged.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, and thank you for flagging this. You're right — this normalization currently mirrors today's I'll wait for #5211 to merge first, then rebase this hash normalization on top of it. The forward-compatible fix will check — posted by Claude Code on behalf of @nlohmann
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Sorry for the auto-generated message. Indeed let's wait for #5211 to land first. Can you check the open question to you there please?) |
||
| const auto v_as_signed = static_cast<number_integer_t>(v); | ||
| // Use a shared numeric type tag so all numeric types that are equal hash the same | ||
| const auto numeric_type = static_cast<std::size_t>(BasicJsonType::value_t::number_float); | ||
|
|
||
| if (is_exactly_representable_as_float<BasicJsonType>(v_as_signed)) | ||
| { | ||
| const auto h = std::hash<number_float_t> {}(static_cast<number_float_t>(v_as_signed)); | ||
| return combine(numeric_type, h); | ||
| } | ||
|
|
||
| const auto h = std::hash<number_integer_t> {}(v_as_signed); | ||
| return combine(numeric_type, h); | ||
| } | ||
|
|
||
| case BasicJsonType::value_t::number_float: | ||
| { | ||
| const auto h = std::hash<number_float_t> {}(j.template get<number_float_t>()); | ||
| return combine(type, h); | ||
| const auto numeric_type = static_cast<std::size_t>(BasicJsonType::value_t::number_float); | ||
| return combine(numeric_type, h); | ||
|
nlohmann marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| case BasicJsonType::value_t::binary: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did the string hashes change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't caused by the code change — verified by comparing against the original file:
hash(null),hash(false),hash({}), andhash([])(none of which touch the modified code paths) are byte-identical to the previously committed values. Re-running the exact same compiled binary repeatedly also gives identicalhash("")results, which rules out per-process hash randomization.The
hash("")/hash({"hello": "world"})values differ only because I regenerated this file locally (Apple Clang/libc++/ARM64), which is a different toolchain than whatever originally produced the checked-in values.std::hash<std::string>legitimately differs across standard library implementations — this is exactly the platform-dependence the docs already call out ("Note the output is platform-dependent").Happy to regenerate on a Linux/GCC toolchain instead if you'd prefer the diff minimized to just the numeric lines that actually changed — let me know.
— posted by Claude Code on behalf of @nlohmann
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry for the auto-generated message - I need to learn how to restrict Claude here...)