Skip to content
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

Comparison of NaN differs between json and float #3409

Closed
2 tasks
misos1 opened this issue Mar 31, 2022 · 11 comments · Fixed by #3446
Closed
2 tasks

Comparison of NaN differs between json and float #3409

misos1 opened this issue Mar 31, 2022 · 11 comments · Fixed by #3446
Assignees
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@misos1
Copy link

misos1 commented Mar 31, 2022

Description

https://json.nlohmann.me/api/basic_json/operator_le/

Compares whether one JSON value lhs is less than or equal to another JSON value rhs by calculating #cpp !(rhs < lhs).

This means that <= always has the opposite result to > even for NaN. Which gives weird results like json(NAN) <= json(NAN) is true but json(NAN) < json(NAN) is false while json(NAN) == json(NAN) is also false #514.

Reproduction steps

printf("%i %i\n", json(NAN) <= json(NAN), NAN <= NAN);
printf("%i %i\n", json(NAN) < json(NAN), NAN < NAN);

Expected vs. actual results

Actual:

1 0
0 0

Expected:

0 0
0 0

Minimal code example

No response

Error messages

No response

Compiler and operating system

clang

Library version

3.7.0

Validation

@gregmarr
Copy link
Contributor

Compares whether one JSON value lhs is less than or equal to another JSON value rhs by calculating !(rhs < lhs).

Yeah, that's not quite right in the case of NAN. A special case is probably needed here for proper NAN handling:

friend bool operator<=(const_reference lhs, const_reference rhs) noexcept
{
    if (lhs.type() == value_t::float && rhs.type() == value_t::float)
    {
        return lhs.m_value.number_float <= rhs.m_value.number_float;
    }

    return !(rhs < lhs);
}

@misos1
Copy link
Author

misos1 commented Apr 1, 2022

if (lhs.type() == value_t::float && rhs.type() == value_t::float)

One of them can also be integer or unsigned (json(NAN) <= json(0)).

@nlohmann
Copy link
Owner

nlohmann commented Apr 1, 2022

I see. But I guess fixing this would be a breaking change. 🤔

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Apr 1, 2022
@gregmarr
Copy link
Contributor

gregmarr commented Apr 1, 2022

One of them can also be integer or unsigned (json(NAN) <= json(0)).

Yeah, it would have to include all the cases that include value_t::float.

I see. But I guess fixing this would be a breaking change. 🤔

That's unfortunate.

@nlohmann
Copy link
Owner

nlohmann commented Apr 3, 2022

What do you think of adding a policy macro to guard this fix. Something like JSON_NAN_COMPARISON, that, if defined, switches to a fixed version of the comparison code. This would make the fix made available immediately, and we could add documentation that this will be the default behavior in the next major release.

@falbrechtskirchinger
Copy link
Contributor

What do you think of adding a policy macro to guard this fix. Something like JSON_NAN_COMPARISON, that, if defined, switches to a fixed version of the comparison code. This would make the fix made available immediately, and we could add documentation that this will be the default behavior in the next major release.

If you really think a policy macro is required for this case, it's certainly better than fixing the issue in the next major release. I'd only suggest to at least default to the correct behavior and allow users to restore the old, broken behavior as needed.

@t-b
Copy link
Contributor

t-b commented Apr 4, 2022

Last time I checked NaN floating point values were not supported by the JSON RFC and this library. So I'm a bit suprised to see now that it actually kind of is supported. But as it is internally converted to null, maybe that step should happen earlier?

See https://godbolt.org/z/oqr8azoGG.

@gregmarr
Copy link
Contributor

gregmarr commented Apr 4, 2022

@t-b https://json.nlohmann.me/features/types/number_handling/#number-serialization

NaN handling

That is, there is no way to parse a NaN value. However, NaN values can be stored in a JSON value by assignment.

This library serializes NaN values as null. This corresponds to the behavior of JavaScript's JSON.stringify function.

NaN is only converted to null when converting to text form, such as by the dump() function.

@falbrechtskirchinger
Copy link
Contributor

I'm currently working on implementing the spaceship operator.
With three-way comparison NaN handling will be fixed for code using C++20 onward because all relational operators are synthesized from this comparison:

  case value_t::number_float:
      return (lhs.m_value.number_float) <=> (rhs.m_value.number_float);

As I'm already inadvertently fixing this issue for some users, I'm inclined to fix the issue in it's entirety.

Can we agree on a solution? Is a policy macro acceptable and which behavior should the policy macro enable?
Keep in mind that this only applies to pre-C++20 users. Everyone else gets the correct behavior by default. (I.e., it would be my preference to not artificially break it.)

@nlohmann
Copy link
Owner

nlohmann commented Apr 7, 2022

I am torn... On the one had, I do not want to have a breaking change. On the other hand, this is such an edge case that I begin to wonder whether anyone but @misos1 run into this. Furthermore, if the spaceship operator fixes this "accidentally", then this is one more argument that the current implementation is not just buggy, but surprising.

@falbrechtskirchinger
Copy link
Contributor

To make sure we're on the same page:

NaNs are only unordered within the domain of numbers, correct? In other words, ("some string" <=> NaN) != std::partial_ordering::unordered. Anything that isn't a signed/unsigned integer or floating-point compares to NaN according to value_t.
No on expects NaN to behave like discarded values?

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: please discuss please discuss the issue or vote for your favorite option labels Apr 24, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue Apr 29, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue Apr 30, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue May 1, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue May 2, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue May 7, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue May 20, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue May 29, 2022
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue May 29, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone May 29, 2022
@nlohmann nlohmann self-assigned this May 29, 2022
nlohmann pushed a commit that referenced this issue May 29, 2022
* Add C++20 3-way comparison operator and fix broken comparisons

Fixes #3207.
Fixes #3409.

* Fix iterators to meet (more) std::ranges requirements

Fixes #3130.
Related discussion: #3408

* Add note about CMake standard version selection to unit tests

Document how CMake chooses which C++ standard version to use when
building tests.

* Update documentation

* CI: add legacy discarded value comparison

* Fix internal linkage errors when building a module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants