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

Attempt to get() a numeric value as a type which cannot represent it should throw #2310

Closed
1 task done
mjerabek opened this issue Jul 24, 2020 · 4 comments
Closed
1 task done
Labels
state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@mjerabek
Copy link

What is the issue you have?

Attempt to get() a numeric value as a type which cannot represent it should throw.

// prints 4294967295, but should throw
std::cout << "-1 as unsigned: " << json::parse("-1").get<unsigned>() << std::endl;

// prints 0, but should throw
std::cout << "65536 as uint16_t: " << json::parse("65536").get<uint16_t>() << std::endl;

// prints 3, but should throw
std::cout << "3.14 as int: " << json::parse("3.14").get<int>() << std::endl;

// 2**64 + 100
// prints -2147483648 (and might exhibit UB), but should throw
std::cout << "2**64+100 as int: " << json::parse("18446744073709551716").get<int>() << std::endl;

// this throws as expected
// std::cout << "[] as unsigned: " << json::parse("[]").get<unsigned>() << std::endl;

If a value is not reasonably representable as the type, the conversion should fail.

Please describe the steps to reproduce the issue.

Link to godbolt for the example above: https://godbolt.org/z/cs4a9s

What is the expected behavior?

  • getting negative numbers as unsigned should throw
  • getting numbers too big for the target type should throw
  • getting numbers too small for the target type should throw
  • (maybe) getting floating point numbers as integers should throw
    • I could imagine that rounding (towards zero) might be the wanted behavior in some cases, but is is easily done by static_cast<int>(j.get<double>()). However, this is still a somewhat reasonable behavior, so changing it would probably not be backward compatible.

I cannot imagine anyone relying on the current behavior of the first 3 cases, so I think fixing that should not break compatibility.

And what is the actual behavior instead?

Truncated values are printed (see example).

Which compiler and operating system are you using?

  • Compiler: gcc 9.2.0
  • Operating system: Linux (NixOS)

+ the linked godbolt

Which version of the library did you use?

  • latest release version 3.8.0
@mjerabek
Copy link
Author

The bound checking would probably go to nlohmann::detail::get_arithmetic_value.

@nlohmann
Copy link
Owner

Right now, get is behaving similar to a static_cast - if you as for signed-to-unsigned conversions, you get them. Making the function throw would mean a breaking change - and even though this affects few people, I still don't want to risk this in a minor release.

I'd like to hear more opinions about this.

@nlohmann nlohmann added state: please discuss please discuss the issue or vote for your favorite option and removed kind: bug labels Jul 25, 2020
@snagar
Copy link

snagar commented Jul 29, 2020

I think I'm leaning to agree with the topic title but my case is not similar but maybe reflect the issue of not asserting wrong type cast.
Disclaimer: I'm not a programmer it is just a hobby of mine.

In my case, I did a mistake and tried to get a "distance" value using get<std::string>() instead of get<double>().
The code ran in std::async() and the result was that the thread seem to crash (not the program) and I could not figure out why.
Only by tracing the code, I found out that the library reaches a state where I could not trace it anymore and I was left with the debugger lines but nothing to hold on.
What I would like to see (in my case anyway) is:

  • Assert or something similar so I'll be able to better pinpoint wrong implementation.
  • A function that will always allow me to fetch the "value" as a string, in many times I don't care about the type of value when I read it.

Thanks

@stale
Copy link

stale bot commented Aug 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 29, 2020
@stale stale bot closed this as completed Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

3 participants