-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-74020: Raise ValueError for negative values converted to unsigned #121114
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
base: main
Are you sure you want to change the base?
gh-74020: Raise ValueError for negative values converted to unsigned #121114
Conversation
…igned Converting negative Python integer to a C unsigned integer type now raises ValueError, not OverflowError. Affected C API: PyLong_AsUnsignedLong(), PyLong_AsUnsignedLongLong(), PyLong_AsSize_t() and the "b" format unit in PyArg_Parse().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this breaks a lot of people code.
Lets do this with a deprecation period.
self.assertRaises(ValueError, (-1).to_bytes, 2, 'big', signed=False) | ||
self.assertRaises(ValueError, (-1).to_bytes, 2, 'little', signed=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should adjust int.to_bytes() docs (both docstring and sphinx).
BTW, I think there could be a test with a negative input and signed=True
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docs.
There is a test with a negative input and signed=True above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a test with a negative input and signed=True above.
Hmm, maybe. I don't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really worried that now, anyone using PyLong_AsUnsignedLong
would need to either change how they handle the exception or now be dealing with two different exceptions.
I'm not confident enough to actually just make this change. OverflowError fits in the sense that it's outside of the expected range, but the transition is too hard. I suggested raising DomainError
which would inherit from OverflowError and ValueError but code paths with distinct handling of ValueError and OverflowError could be broken.
An easy way to make it work is to introduce a new function that would properly raise what is meant to be raised, and deprecate those functions.
So I'm not -1 as strong as @rhettinger, but I still think it's better to be continue with this, even if it's annoying on our side. We could have some private helpers that would do something like:
try:
x = asulong(X)
except OverflowError:
if X < 0:
raise ValueError
raise
Or something that can store in addition the fact that something is < 0
or not, like PyLong_AsLongAndOverflow
but for ULong
instead where we add also a isnegative
flag:
int _PyLong_AsUnsignedLong(PyObject *long, int *overflow, int *negative);
@@ -1920,6 +1920,10 @@ Others | |||
integer must implement either :meth:`~object.__int__` or | |||
:meth:`~object.__index__`. (Contributed by Mark Dickinson in :gh:`119743`.) | |||
|
|||
* Converting negative Python integer to a C unsigned integer type now raises |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be moved to 3.15 and marked as an incompatible change.
Converting negative Python integer to a C unsigned integer type now raises ValueError, not OverflowError.
Affected C API: PyLong_AsUnsignedLong(), PyLong_AsUnsignedLongLong(), PyLong_AsSize_t() and the "b" format unit in PyArg_Parse().
📚 Documentation preview 📚: https://cpython-previews--121114.org.readthedocs.build/