Fix std::hash contract violation for numeric types (#5256)#5262
Fix std::hash contract violation for numeric types (#5256)#5262nlohmann wants to merge 4 commits into
Conversation
Fixes #5256: json(42) == json(42u) is true, but their hashes differed, violating the std::hash contract. This also applied to float comparisons: json(42) == json(42.0) is true, but they hashed differently. Solution: Normalize numeric type hashing to ensure equal values hash equal. - Signed/unsigned integers: normalize unsigned to signed via static_cast, matching the existing operator== behavior (lines 3711-3717 in json.hpp) - Integer/float bridging: for values exactly representable as the float type, hash via the float form to collide correctly with float values - All numeric types share a single type tag to ensure hash collision The fix is rigorous for the reported issue (int/uint, any magnitude) with zero gaps. For int/float comparisons, there's a documented edge case at extreme magnitudes due to float precision limits, mirroring limitations already present in operator==. Changes: - include/nlohmann/detail/hash.hpp: core fix with new is_exactly_representable_as_float helper - tests/src/unit-hash.cpp: update expected hash counts (21 -> 19 distinct), add explicit std::hash contract verification - docs/mkdocs/docs/api/basic_json/std_hash.md: update description - docs/mkdocs/docs/examples/std_hash.cpp/.output: show the fix in action - single_include/nlohmann/json.hpp: regenerated via amalgamate Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann
added
the
review needed
This comment was marked as outdated.
This comment was marked as outdated.
| 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) |
There was a problem hiding this comment.
Good catch, and thank you for flagging this. You're right — this normalization currently mirrors today's operator== U-vs-I wraparound cast specifically to stay contract-consistent with it, so it would need to be revisited once #5211 lands (which replaces the wraparound cast with a mathematically correct comparison).
I'll wait for #5211 to merge first, then rebase this hash normalization on top of it. The forward-compatible fix will check value_in_range_of<number_integer_t>(v) before normalizing, falling back to hashing in the pure number_unsigned_t domain when a value doesn't fit — avoiding any wraparound-based collision regardless of which comparison semantics are in effect.
— posted by Claude Code on behalf of @nlohmann
There was a problem hiding this comment.
(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?)
| hash(0) = 2654436221 | ||
| hash(0U) = 2654436221 | ||
| hash(0.0) = 2654436221 | ||
| hash("") = 11160318156688833227 |
There was a problem hiding this comment.
Why did the string hashes change?
There was a problem hiding this comment.
This isn't caused by the code change — verified by comparing against the original file: hash(null), hash(false), hash({}), and hash([]) (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 identical hash("") 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.
(Sorry for the auto-generated message - I need to learn how to restrict Claude here...)
- Remove 'else' after 'return' in hash.hpp to satisfy
llvm-else-after-return / readability-else-after-return clang-tidy checks
- Regenerate single_include/nlohmann/json.hpp via 'make amalgamate'
(also fixes pre-existing amalgamation/astyle formatting drift)
- Apply astyle formatting fixes to unit-hash.cpp (space before '{}')
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann
force-pushed
the
claude/github-issue-review-b531d3
branch
from
5b9e400 to
48face7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
MSVC's /W4 flags 'if (float_digits >= int_digits)' as C4127 (conditional expression is constant), since both operands are constexpr int for any single template instantiation. This CI job builds with warnings-as-errors, failing the build. Apply the same MSVC-only pragma push/disable(4127)/pop pattern already used elsewhere in the codebase (see json.hpp's set_parents() workaround) rather than if constexpr, since this file must stay C++11-compatible. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The number_float case introduced a redundant numeric_type local that duplicated the already-computed type variable (both equal value_t::number_float within that case). Removed per review feedback. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
3 participants
Summary
Fixes #5256: The C++
std::hashcontract requires that if two values compare equal, they must hash to the same value. This fix addresses the violation wherejson(42),json(42u), andjson(42.0)all compare equal but hashed differently.Changes
Core Fix:
include/nlohmann/detail/hash.hppis_exactly_representable_as_float<BasicJsonType>()helper to safely check if an integer value round-trips losslessly through the float typestatic_cast<number_integer_t>(), exactly matching theoperator==behaviorTests:
tests/src/unit-hash.cppstd::hashcontract verification tests for bothjsonandordered_jsonDocumentation
docs/mkdocs/docs/api/basic_json/std_hash.md: Updated to describe the new numeric hash unification behavior and edge casesdocs/mkdocs/docs/examples/std_hash.cpp: Addedhash(0.0)to show all three forms collidingdocs/mkdocs/docs/examples/std_hash.output: Regenerated output showing equal hashesBuild
single_include/nlohmann/json.hpp: Regenerated viatools/amalgamateto stay in syncTesting
✅ All 108 existing tests pass
✅ Hash tests specifically verify the std::hash contract
✅ No regressions detected
Notes
operator==