Check for multipleOf overflow
#438
Conversation
The spec says that instances are valid if "dividing by this value results in an integer". This allows integers to be invalid for `multipleOf: 0.5` if float division overflows to infinity (a non-integer); alternatively implementations may choose to implement logic which defines all integers as multiples of 0.5. Either way, however, implementations must not raise an error due to the overflow of a legal value against a legal schema.
Zac-HD
force-pushed
the
overflow-with-multipleOf
branch
from
a2a52b1
to
c5ba4ba
Compare
|
Cool. Think these lgtm, thanks! (To add some extra color for folks -- the added required test seems consistent with the spec as well -- implementations with only IEEE floats will get |
I'm curious as to how you might expect to see this in code. Would 0.5 be special-cased somehow? |
|
Probably more realistically by doing something like what we ended up doing (falling back to non-float division on overflow). Which yeah isn't required, but the non-erroring is. |
|
That's the general solution - you could also check whether 1/multipleOf is an integer, and if so drop it from integer schemas. |
|
This is not a valid test on systems built with longdoubles (where 16 bytes are allocated to floats) -- as the inputs here are not big enough to cause an overflow. You need numbers way way larger to be sure to tickle an overflow (on my system, 'Inf' is the result in that case), and then you'll clash with the optional/bignum.json tests which suggest that implementations should also be able to handle very large numbers, which means that this edge case will be even harder to reproduce. (breadcrumb for self: an example of a failure on a smoker system is http://www.cpantesters.org/cpan/report/ee0e51be-1384-11eb-a9f9-f9eb91108de4 ) |
|
Are you referring to the optional ones or the required one? The required one passes there even in your example because indeed the result is nondivisible right? You're saying the optional test conflicts with the other optional test? |
|
I'm referring to the required one. On a longdouble system, the operation does not overflow -- the quotient is 8.10000007371000067e+308 which is an integer, so the evaluation passes even though the test says it should fail. |
|
Ah sorry I can't read... ok think I follow. Probably we can then pick a value that doesn't produce an integer with that much precision either instead of the one we have, and then that should suit both possibilities? |
|
In this section: https://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.4.2 To me, this means that 1e308 is never a multiple of 0.123456789, despite the fact that there's loss of precision given some language's implementation. Might it be useful to add a paragraph in some future draft dealing with "implementations that truncate", describing expected behaviour and limitations? Currently, the above spec reference doesn't mention any MUST or SHOULD words when talking about this. |
|
One of the reasons the Unicode-based regex tests were moved to optional/ was because .NET doesn't support them (I disagreed with that). I'm not in favour of moving things to optional/ based on the fact that implementations don't support things. Having said that, floating-point handling is probably even more fundamental than regexes, but still, for arbitrary-precision-capable implementations, these tests are still valid. I don't agree with optional or missing tests just to avoid specific implementation details. For arbitrary-precision-capable implementations, these tests shouldn't just be present, they should be non-optional. Further to that, why isn't having a 1e308//0.123456789 test a good thing? If we're only testing values that don't overflow, how will perfectly valid arbitrary-precision implementations that support ranges beyond the overflow range of most byte-limited implementations get tested? These are perfectly valid, spec-conforming, non-optional tests. Unless, is arbitrary-precision support optional? If not, these tests need to be non-optional. |
Possibly, but I don't have the time right now to compile a new interpreter with longdouble enabled, and then search for a suitable pair of numbers that do not divide evenly.. and then test them on systems with other float sizes to see that they divide as expected also. |
Testing how a JSON Schema implementation handles overflow is fine (it should return an invalid result, rather than exploding or returning true). The problem is creating a test case that is guaranteed to produce an overflow. |
|
Can you maybe open an issue to start and we can figure out whatever the right solution should be? (Or propose one if you have one -- ideally though we should end up with something "universal" that won't be forgotten as a required test, but if we can't think of how to do so we can leave that as an open issue) -- but yeah open an issue for the test being unpassable if you have longer floats? |
|
We may even be able to bribe Zac-HD :D -- maybe hypothesis could find us a pair of numbers that return non-integers for both 8 and 16 byte floats or something (and yeah sorry I'm still mid-$WORK so possibly my brain isn't thinking straight still on an obvious solution so feel free to suggest one if anyone has one) |
What's wrong with 1e308//0.123456789? Maybe I'm missing something... |
Arbitrary precision is optional yeah. You are allowed to use IEEE floats or doubles (and presumably also this -- 16 byte floats) |
This comment has been minimized.
This comment has been minimized.
|
The problem: this test passes (i.e. overflows to non-integer value infinity) at 64bit precision, and passes (i.e. gives a non-integer-valued finite result) at arbitrary precision. I also get So the problem appears to be that rounding specifically in 128-bit floats leads to an integer output from this test case. I think we can solve this just by making the quotient e.g. |
|
That calculation doesn't overflow, but it still looks like an even multipleOf (the quotient is an integer): |
|
You can see the same problem with a simpler example: The divisor is so large that even though the quotient is not an integer, it appears as if it an integer because of how floats are expressed internally. I do not see any way of correctly checking the integer status here (and the same situation will occur with smaller divisors on architectures with smaller floats). e.g. on a 64-bit float implementation: edit, 2021-12-30: I was wrong about most of what I said here :) see #534 for a more current discussion. |
The test being referred to here is multipleOf.json - "invalid instance should not raise error when float division = inf" - "always invalid, but naive implementations may raise an overflow error". It also fails when 12 bytes are allocated to floats. http://www.cpantesters.org/cpan/report/d1a4dd18-ca7c-11eb-932e-240466f710c6 |
Fixes overflow error exposed by: json-schema-org/JSON-Schema-Test-Suite#438 ``` FloatDomainError: Infinity /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:367:in `floor' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:367:in `validate_numeric' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:378:in `validate_number' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:277:in `validate_type' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:198:in `validate_instance' ```
Floats aren't accurate enough here. Fixes: #100 Also addresses overflow exposed by: json-schema-org/JSON-Schema-Test-Suite#438 ``` FloatDomainError: Infinity /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:367:in `floor' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:367:in `validate_numeric' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:378:in `validate_number' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:277:in `validate_type' /Users/dharsha/repos/json_schemer/lib/json_schemer/schema/base.rb:198:in `validate_instance' ```
The spec says that instances are valid if "dividing by this value results in an integer".
This allows integers to be invalid for
multipleOf: 0.5if float division overflows to infinity (a non-integer); alternatively implementations may choose to implement logic which defines all integers as multiples of 0.5. Either way, however, implementations must not raise an error due to the overflow of a legal value against a legal schema.Reference: python-jsonschema/jsonschema#743, cc @Julian