bpo-29843: raise AttributeError if given negative length by taleinat · Pull Request #10029 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation26 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This is based on @orenmn's PR #3822.
The tests are the same, but this uses _PyLong_Sign() to check for negative values rather than PyLong_AsLongAndOverflow().
This raises AttributeError since the existing code already raises that for other invalid values (non-integers).
https://bugs.python.org/issue29843
AttributeError doesn't look a correct exception. I would expect AttributeError if the attribute doesn't exist, TypeError if it has wrong type, and ValueError if it has wrong (negative) value.
Also PyObject_GetAttrString() can raise a MemoryError. Actually it can raise an arbitrary exception. They shouldn't change their type. Only AttributeError can be overridden by other AttributeError with different error message.
The issue number looks wrong.
bpo-29743: raise AttributeError if given negative _length_ bpo-29843: raise AttributeError if given negative _length_
The issue number looks wrong.
Fixed.
Co-authored-by: Oren Milman orenmn@gmail.com
@serhiy-storchaka, I've addressed the exception handling and raised exception types as per your comment. Please take another look!
| with self.assertRaises(OverflowError): |
|---|
| def test_bad_length(self): |
| import sys |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is imported at the top of the file.
| @@ -0,0 +1,4 @@ |
|---|
| Raise `ValueError` instead of `OverflowError` in case of a negative |
| ``_length_`` in a `ctypes.Array` subclass. Also raise `TypeError` |
| instead of `OverflowError` for non-integer ``_length_``. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"instead of AttributeError".
| @@ -0,0 +1,4 @@ |
|---|
| Raise `ValueError` instead of `OverflowError` in case of a negative |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues with using the default role. It is better to use :exc:`ValueError` or ``ValueError`` or just remove mark up.
| "which must be a positive integer"); |
|---|
| Py_XDECREF(length_attr); |
| if (!length_attr) { |
| if (!PyErr_Occurred() | |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyErr_Occurred() should always return true here. Remove this redundant check.
| Py_XDECREF(length_attr); |
|---|
| if (!length_attr) { |
| if (!PyErr_Occurred() | |
| PyErr_ExceptionMatches(PyExc_AttributeError)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is worth to add similar check for _type_.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a new PR for that. Should I also make a new issue?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. Just a tiny nit.
| "which must be a positive integer"); |
|---|
| Py_XDECREF(length_attr); |
| if (!length_attr) { |
| if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put { on the same line as if.
| "which must be a positive integer"); |
|---|
| Py_XDECREF(length_attr); |
| if (!length_attr) { |
| if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put { on the same line as if. It should be on a new line only for multiline conditions.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. Just a tiny nit.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. Just a tiny nit.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a tiny nit.
| "which must be a positive integer"); |
|---|
| Py_XDECREF(length_attr); |
| if (!length_attr) { |
| if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put { on the same line as if. It should be on a new line only for multiline conditions.
| "which must be a positive integer"); |
|---|
| Py_XDECREF(length_attr); |
| if (!length_attr) { |
| if (PyErr_ExceptionMatches(PyExc_AttributeError)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PEP 7 put { on the same line as if. It should be on a new line only for multiline conditions.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a nitpick.
Should this be backported, and if so, how far back?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Hum, GitHub is sick today. I approved the PR again :-)
All right until GitHub allow to merge the PR twice. 😉
Labels
An unexpected behavior, bug, or error