Fix SystemError when convert()'ing an out-of-range int to float - #1162
Conversation
msgspec.convert(obj, float) converts a python int to a C double via PyLong_AsDouble without checking for overflow. For an int too large to represent as a float, PyLong_AsDouble sets OverflowError but still returns -1.0, and that value was passed straight through to ms_decode_float and returned to the interpreter with the exception still set - which surfaces as SystemError: <built-in function convert> returned a result with an exception set, bypassing the documented ValidationError contract entirely. json.decode already handles the equivalent case correctly, raising ValidationError: Number out of range. Apply the same value-error-with-path check convert() already uses elsewhere in this file (e.g. _constr_as_f64) so the overflow is reported the same way, for both bare and nested (e.g. dict[str, float]) targets, under both strict and lax mode. Fixes msgspec#1122.
Siyet
left a comment
There was a problem hiding this comment.
Verified locally with current main (7c2f473) merged into this branch, Python 3.14.6, -X dev:
convert(10**400, float), the nesteddict[str, float],tuple[float, ...]and Struct-field cases,strict=False, and the negative value all raiseValidationError: Number out of rangewith the right path suffix ($[...],$[1],$.x).2**1024is rejected,2**1023andint(sys.float_info.max)still convert.- The
OverflowErrorthatPyLong_AsDoublesets does not leak:__context__and__cause__areNone,sys.exc_info()is clean afterwards. - Full unit suite: 6387 passed, 143 skipped.
The check mirrors what _constr_as_f64 already does a few thousand lines up, and the message matches the JSON path. Bug fix with no API change, so I will put it in the merge queue once the CI runs (just approved for the first-time-contributor gate) come back green. The changelog entry gets added in the release-prep batch, same as the other fixes.
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Thanks @STiFLeR7, merging this. It matches the pattern Two notes from verifying it on top of current main, both worth having in the thread: It fixes more than the >>> msgspec.convert(10**400, Annotated[float, msgspec.Meta(ge=0)])
ValidationError: Expected `float` >= 0.0for a value that plainly satisfies The guard is exactly sufficient, no more needed. Full unit suite on the merge with current main: 6387 passed, 143 skipped, with the new test failing on main as a regression test should. |
Summary
msgspec.convert(obj, float)converts a Pythonintto a CdoubleviaPyLong_AsDoublewithout checking for overflow. For aninttoo large to represent as a finitefloat(e.g.10**400),PyLong_AsDoublesetsOverflowErrorinternally but still returns-1.0, and that value was passed straight through toms_decode_floatand returned to the interpreter with the exception still set — which surfaces as:This bypasses the documented
ValidationErrorcontract entirely, so a caller wrapping the call inexcept msgspec.ValidationErrordoesn't catch it.json.decodealready handles the equivalent case correctly, raisingValidationError: Number out of range. This PR applies the same "checkPyErr_Occurred()after a lossy C conversion, then report viams_error_with_path" pattern already used elsewhere in this file (e.g._constr_as_f64), soconvert()reports the overflow the same way — for both bare (convert(big, float)) and nested (convert({"x": big}, dict[str, float])) targets, under bothstrict=Trueandstrict=False.Fixes #1122.
Testing
SystemErrorbefore reapplying it.test_float_from_int_out_of_rangetoTestFloatintests/unit/test_convert.py, covering bare and nested targets under both strict modes.tests/unit/suite: 6052 passed, 473 skipped, 0 failures.