Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
GH-141312: Allow only integers to longrangeiter_setstate state#141317
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 ourterms of service andprivacy 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-141312: Allow only integers to longrangeiter_setstate state#141317
Conversation
sergey-miryanov commentedNov 9, 2025
@serhiy-storchaka Could you please take a look? |
serhiy-storchaka left a comment
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.
Why not simply checkstate? Normally, it can only beint.
UsingPyLong_Check() is not enough -- it would pass for anint subclass with overridden__radd__,__mul__, etc.PyLong_CheckExact() is needed.
sergey-miryanov commentedNov 9, 2025
Should we change |
| if (!PyLong_CheckExact(state)) { | ||
| PyErr_Format(PyExc_TypeError, | ||
| "'%T' object cannot be interpreted as an integer",state); | ||
| returnNULL; | ||
| } |
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.
| if (!PyLong_CheckExact(state)) { | |
| PyErr_Format(PyExc_TypeError, | |
| "'%T' object cannot be interpreted as an integer",state); | |
| returnNULL; | |
| } | |
| if (!PyLong_CheckExact(state)) { | |
| PyErr_Format(PyExc_TypeError,"state must be an int, not %T",state); | |
| returnNULL; | |
| } |
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 is exactly what I added before checking whatrangeiter returns for a wrong state. Is it ok if our messages will be different?
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.
Oh so that is the message of rangeiter. Ok keep it the same.
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.
The proposed message is better.
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.
rangeiter_setstate() supportsint subclasses and classes with the__index__ method. If we want to support them here (__index__ is not currently supported), we should usePyNumber_Index().
But there is no good reason to do this. It would only complicate the code without adding any benefit.
picnixz commentedNov 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes we should. It's a bug fix. The rule is that most bugfixes have NEWS. |
| withself.assertRaisesRegex(TypeError,msg): | ||
| it=iter(range(10,100,2)) | ||
| it.__setstate__(1.0) |
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.
- Make a separate test method for setstate failures.
- Only assert the exception when calling
setstate. - Check that custom int subclasses are rejected. What is the current behavior with range objects not having exact ints?
| { | ||
| if (!PyLong_CheckExact(state)) { | ||
| PyErr_Format(PyExc_TypeError, | ||
| "'%T' object cannot be interpreted as an integer",state); |
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.
Such error messages is usually used when the argument can be anint subclass of has the__index__ method. But this is not needed here, onlyint is supported.
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.
See my discussion here:#141317 (comment). Should we change rangeiter to match messages?
Uh oh!
There was an error while loading.Please reload this page.
Only integers should be allowed for the start value in
longrangeiter.Two extra checks were added:
__setstate__forrangeiterfailslongrangeiter.I made the error message for
longrangeitersimilar to that forrangeiter.compute_range_length: Assertion PyLong_Check(start)' failed#141312