-
Notifications
You must be signed in to change notification settings - Fork 460
BUG: Defined safe integer range is invalid #1238
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
Comments
What's the origin of that definition? Or, why is it necessary to have both an exactly representable successor and exactly representable predecessor? What are the pros/cons compared to allowing exactly ±2^53? My naive expectation would be that a safe integer range needs to have the property that every integer in the range is exactly representable. So, for example, 2^60 is exactly representable but clearly cannot be part of a safe integer range, since 2^60 - 1 is not exactly representable. I'm open to changing it, but I'd like to understand why. |
Sorry, replied too soon.
Yes, these should be fixed. The part I'm missing is why reducing the accepted range by 1 (at each end) is necessary (or sufficient!) to solve this. |
2^53 is not exactly representable, as we can't differentiate between 2^53 and 2^53+1, and because of that we have |
Hmm. Ok, I see. So, the problem is not quite that 2^53 is not representable, but that it's not distinguishable. That was a fundamental mistake on my part during review of the PR, sorry. Alright, let's change it. |
By that logic,
I think this terminology is only standardized in JS, as every other language has distinct type for integer values, and has no need to represent integers as f64. Though I-JSON also uses this range for representing "exact integer values", and it is described in RFC: https://datatracker.ietf.org/doc/html/rfc7493
As jsonnet follows the same limitations as JS/I-JSON in that regard, it makes sense to reuse the same definitions. Rust jsonnet implementation however has a distinct type for integer values, mimicking JS's BigInt type, but this is a distinct type, not compatible with standard number type. |
It is representable! 😆 But that one isn't safe even by my earlier naive (and incorrect) definition that the safe range must have contiguous exactly representable integers (that definition would only require a successor or predecessor, not necessarily both).
Yeah, looks like it. I see MDN documents it this way. And that is probably a good reference to have - we can point to it from the Jsonnet docs. |
@thevilledev Just to let you know, since you contributed the change: I was incorrect in my review on this. It seems 2^53-1 really was the right bounds to set, and I had failed to think of one of the key requirements here. Sorry to have misled you! |
Thanks @johnbartholomew and @CertainLach so much for the update and for the clarification! I really appreciate you circling back on this. These floating-point and integer boundary details can definitely be subtle. I learned something new again :-) |
Uh oh!
There was an error while loading. Please reload this page.
Jsonnet indeed can represent integers up to 2^53, however "safe" integer is the one that has defined successor and predecessor
2^53 does not have successor, thus arithmetic with it is already invalid, we can't distinguish between 2^53 and 2^53+1, meaning we are not sure if the integer we received from jsonnet code is safe in this case.
For example:
The bitwise operation here has silently truncated value, despite documentation stating that bitwise operations perform safe integer range checks.
9007199254740993
is outside of safe integer range, but safe integer range check does not report error here, as the last bit of this number is already truncated on jsonnet number type side.In other words: if unsafe integer can pass the safe integer check, then is this safe integer check correct?
Only overflow with larger integer can be detected
Originally reported at #1217 (comment)
The text was updated successfully, but these errors were encountered: