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
Inconsistency in overflow error messages of integer argument #60192
Comments
PyArg_ParseTuple raises inconsistent overflow error messages for small integer formats. For example: >>> import _testcapi
>>> _testcapi.getargs_b(100)
100
>>> _testcapi.getargs_b(1000)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OverflowError: unsigned byte integer is greater than maximum
>>> _testcapi.getargs_b(-1000)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OverflowError: unsigned byte integer is less than minimum
>>> _testcapi.getargs_b(100000000000000000000)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C long
>>> _testcapi.getargs_b(-100000000000000000000)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C long On platforms with 32-bit int and 64-bit long there will be more such cases. |
The reason for the different messages is clear -- there are different internal tests (I presume in different functions), each of which raises its own error. What do you propose to change? The only thing I can imagine is that when the bytes converter calls the Python int to c long function, it somehow intercepts the error return and rewrites the message for consistency. Is this your proposal? |
|
I reconsidered this in the light of bpo-21559. getargs_b requires an integer of type int in range(256). A non-int properly raises TypeError. >>> from _testcapi import getargs_b as gb
>>> gb(1.0)
Traceback (most recent call last):
File "<pyshell#7>", line 1, in <module>
gb(1.0)
TypeError: integer argument expected, got float
>>> import fractions
>>> gb(fractions.Fraction(1, 1))
Traceback (most recent call last):
File "<pyshell#12>", line 1, in <module>
gb(fractions.Fraction(1, 1))
TypeError: an integer is required (got type Fraction) An out-of-range int should, it seems to me, just raise ValueError("int %d not in range(256)" % n). Verification of the range: >>> gb(255)
255
>>> gb(256)
Traceback (most recent call last):
File "<pyshell#4>", line 1, in <module>
gb(256)
OverflowError: unsigned byte integer is greater than maximum
>>> gb(0)
0
>>> gb(-1)
Traceback (most recent call last):
File "<pyshell#6>", line 1, in <module>
gb(-1)
OverflowError: unsigned byte integer is less than minimum The last message is wrong or contradictory. An unsigned (non-negative) int cannot be less than 0. |
I would be happy to write a patch for this issue, if you don't mind. Terry, as you were the one to commit the patch for bpo-21559, I guess you are OK with keeping the OverflowError. ISTM that implementing the patch by doing the following is quite straightforward:
Serhiy, what did you mean by 'Unify an integer overflow error messages in different functions.'? |
You plan looks good, Oren. I meant what you are planning in the last two items. There are also specific functions for converting to platform-depending integer types (size_t, time_t, uid_t, etc). Two most common patterns: "Python int too large to convert to C long" and "signed short integer is less/greater than maximum". Grep all code for raising OverflowError. |
At this point in time, I have nothing further to add. So follow Serhiy's advice. |
Sorry for taking so long to come up with a patch.. ------------ proposed changes ------------
------------ diff ------------ ------------ tests ------------
In addition, I ran 'python_d.exe -m test -j0' (on my 64-bit Windows 10 and ############################################################################## Note that the inconsistent messages my patches fix are only those I found I would be happy to do that, but I am afraid I won't have time to (at least) |
Thank you Oren. I'll make a detailed review in next few days. |
The patch no longer applied cleanly (in particular to arraymodule.c). Oren, could you please update it? |
of course :) but some time has passed, so i would re-check stuff, and run tests |
I just have started reviewing the PR and it looks great! |
I think that raising OverflowError exceptions should be avoided if this is possible. If some function requires non-negative integer and raises ValueError for small negative integers and OverflowError for large negative integers, it is desirable to make it raising ValueError for all negative integers. If some function truncates integer arguments to specified limit it shouldn't raise OverflowError for large positive integers, but truncate them to that limit. |
also, IMHO, we should open an issue named 'raise ValueError instead of OverflowError when a negative int is invalid', and leave for that I didn't want to open it without your approval. In various C functions, a negative int argument is invalid. in most of these as Serhiy mentioned in http://bugs.python.org/issue15988#msg289679 and in the following functions were identified as relevant only while working on functions where OverflowError is raised only for big negative ints: functions where OverflowError is raised for any negative int (so a change would Note: while patching these functions, one should also make sure that the |
I agree. It would be better to remove from the PR changes that can be covered by bpo-29816, bpo-29819 and other changes that changes visible behavior and open one or several new issues for them. Maybe one meta-issue and few concrete issues? Such methods as read() also shouldn't raise OverflowError for negative argument. |
I am sorry, but my 2nd term starts on Monday, so I am short on time, because of that, and because you obviously have a much better |
it is hard to discuss such large patch on GitHub since diffs and comments to all files are shown on one page. Rietveld is more appropriate since it shows only one file at the time. I suggest you Oren to provide the next version of your patch as a patch for reviewing on Rietveld and make PRs only for ready patches. Even after withdrawing some changes or extracting them as separate issues the patch can be too large. I suggest to commit first the part that relates to PyArg_Parse* ang PyLong_As*. |
Here is a patch including only changes related to PyLong_As* and PyArg_Parse* (I ran the test module, and on my Windows 10, the same tests failed with |
Here is a patch including only changes related to formats using (I ran the test module, and on my Windows 10, the same tests failed with |
Here is a patch including only changes related to array. (I ran the test module, and on my Windows 10, the same tests failed with |
Here is a patch including only changes related to hashlib, lzma and pickle. (I ran the test module, and on my Windows 10, the same tests failed with |
Here is a patch including only changes related to time and re. (I ran the test module, and on my Windows 10, the same tests failed with |
Here is a patch including only changes related to curses, stat, callproc (ctypes) and sequence_repeat (abstract). (I ran the test module, and on my Windows 10, the same tests failed with |
Here is a patch including only changes related to mmap, posix, socket and select. (I ran the test module, and on my Windows 10, the same tests failed with |
ISTM that what's left is (except for my 7 sub-patches): |
How do we proceed? |
Glad to see you again Oren! Maybe first finish bpo-28261? It looks easier. |
Serhiy, you suggested in https://bugs.python.org/issue15988#msg289799 that uploading Should I open some smaller PRs? or should I combine all my patches and update |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: