Make std::hash<basic_json> consistent with operator== for numbers - #5447
Open
afonsojanu wants to merge 3 commits into
Open
afonsojanu wants to merge 3 commits into
afonsojanu wants to merge 3 commits into
Conversation
operator== converts between number_integer, number_unsigned, and number_float before comparing, so json(0), json(0U), and json(0.0) all compare equal. hash() folded the specific value_t into the result for each of the three numeric cases, giving each a distinct hash and breaking the standard Hash requirement that a == b implies hash(a) == hash(b). A std::unordered_set could therefore hold all three as separate elements even though they compare equal. hash() now treats all three numeric variants the same way: it converts the value to number_float_t and combines it with a single shared type tag, so any two numbers operator== considers equal hash identically regardless of which internal type actually holds them. Updated the accompanying test to check this consistency directly (including via an actual unordered_set) instead of asserting that 0, 0U, and 0.0 hash differently, since that assumption was the bug. Also corrected the function's own doc comment and the std::hash API docs, which described the old behavior as intended. Fixes nlohmann#5400
Merging the three numeric branches into one that only reads number_float_t left these two aliases unused, which several CI configurations treat as a build error under -Wunused-local-typedefs.
clang-tidy's misc-const-correctness check flagged it: the set is never mutated after construction, only read via size().
nlohmann
requested changes
Sep 2, 2026
Owner
There was a problem hiding this comment.
@afonsojanu Please fix the DCO requirements. Otherwise good to go.
nlohmann
added
the
🚀 ready to merge
Owner
|
This also fixes #5256, which is a duplicate report of the same There was a separate attempt at this in #5262, but it turned out to have a correctness bug of its own (hashing large integers in their native domain instead of through Fixes #5256 — posted by Claude Code on behalf of @nlohmann |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5400.
operator==converts betweennumber_integer,number_unsigned, andnumber_floatbefore comparing, sojson(0),json(0U), andjson(0.0)all compare equal.hash()folded the specificvalue_tinto the result for each of the three numeric cases, giving each a distinct hash and breaking the standardHashrequirement thata == bimplieshash(a) == hash(b). Astd::unordered_setcould hold all three as separate elements even though they compare equal, which is exactly the reproduction in the issue.hash()now treats all three numeric variants the same way: it converts the value tonumber_float_tand combines it with a single shared type tag, so any two numbersoperator==considers equal hash identically regardless of which internal type actually holds them.I also updated the accompanying test to check this consistency directly (including via an actual
unordered_set) instead of asserting that0,0U, and0.0hash differently, since that assumption was the bug. The function's own doc comment and thestd::hashAPI docs described the old behavior as intended, so I corrected those too, and regenerated the doc example's output sincehash(0)andhash(0U)are no longer different values.Tested locally by building the amalgamated header and compiling the full test suite (minus the parts that need the separately-downloaded test data submodule, which isn't available in my environment) plus
unit-hash.cppin isolation against both the fixed and unfixed code, confirming the new assertions fail against the originalhash()with the exact numbers from the issue and pass with the fix.