Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Nov 14, 2025
Merged

GH-141312: Allow only integers to longrangeiter_setstate state #141317

merged 18 commits into from
Nov 14, 2025
+17 −0

Conversation

@sergey-miryanov
Copy link
Contributor

@sergey-miryanov sergey-miryanov commented Nov 9, 2025 •
edited by bedevere-app bot
Loading

Only integers should be allowed for the start value in longrangeiter.

Two extra checks were added:

  1. The first one checks that setting a float value for __setstate__ for rangeiter fails
  2. The second check does the same for longrangeiter.

I made the error message for longrangeiter similar to that for rangeiter.

@bedevere-app bedevere-app bot mentioned this pull request Nov 9, 2025
Open
@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 9, 2025

@serhiy-storchaka Could you please take a look?
I'm not sure should we add a news entry here.

serhiy-storchaka
serhiy-storchaka reviewed Nov 9, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply check state? Normally, it can only be int.

Using PyLong_Check() is not enough -- it would pass for an int subclass with overridden __radd__, __mul__, etc. PyLong_CheckExact() is needed.

@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 9, 2025

Should we change PyLong_Check to PyLong_CheckExact in compute_range_length then?

picnixz
picnixz reviewed Nov 9, 2025
View reviewed changes
@picnixz
Copy link
Member

picnixz commented Nov 9, 2025 •
edited
Loading

I'm not sure should we add a news entry here.

Yes we should. It's a bug fix. The rule is that most bugfixes have NEWS.

picnixz
picnixz reviewed Nov 9, 2025
View reviewed changes
serhiy-storchaka
serhiy-storchaka reviewed Nov 10, 2025
View reviewed changes
@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 10, 2025

It is ready for review.
@serhiy-storchaka @picnixz Could you please take a look?

serhiy-storchaka
serhiy-storchaka reviewed Nov 11, 2025
View reviewed changes
@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 13, 2025

@serhiy-storchaka Could you please take a look?

efimov-mikhail
efimov-mikhail reviewed Nov 13, 2025
View reviewed changes
efimov-mikhail
efimov-mikhail approved these changes Nov 14, 2025
View reviewed changes
Copy link
Member

@efimov-mikhail efimov-mikhail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sergey-miryanov
Merge branch '141312-longrange-setstate-float' of github.com:sergey-m…
3be894b 
…iryanov/cpython into 141312-longrange-setstate-float
@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 14, 2025

I removed checking of exact message and check only an error type. I hope I get you right :)

serhiy-storchaka
serhiy-storchaka reviewed Nov 14, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove tests with __index__. This is an implementation detail.

@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 14, 2025

Done!

serhiy-storchaka
serhiy-storchaka reviewed Nov 14, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet few nitpicks.

@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 14, 2025

Done, please take a look.

vstinner
vstinner approved these changes Nov 14, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

serhiy-storchaka
serhiy-storchaka approved these changes Nov 14, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) November 14, 2025 12:16
@efimov-mikhail
Copy link
Member

efimov-mikhail commented Nov 14, 2025 •
edited
Loading

I suggest to backport this PR for 3.13 and 3.14 since there's an assert crash in debug builds on these Python versions.

@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Nov 14, 2025
@serhiy-storchaka serhiy-storchaka merged commit 10bec7c into python:main Nov 14, 2025
50 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Nov 14, 2025

Thanks @sergey-miryanov for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 14, 2025
@sergey-miryanov @miss-islington
pythonGH-141312: Allow only integers to longrangeiter_setstate state (p…
d0fb254 
…ythonGH-141317)

This fixes an assertion error when the new computed start is not an integer.
(cherry picked from commit 10bec7c)

Co-authored-by: Sergey Miryanov <[email protected]>
@miss-islington-app
Copy link

miss-islington-app bot commented Nov 14, 2025

Sorry, @sergey-miryanov and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 10bec7c1eb3ee27f490a067426eef452b15f78f9 3.13

@bedevere-app
Copy link

bedevere-app bot commented Nov 14, 2025

GH-141559 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Nov 14, 2025
@sergey-miryanov
Copy link
Contributor Author

sergey-miryanov commented Nov 14, 2025

Looking to manually backport it to 3.13.

@bedevere-app
Copy link

bedevere-app bot commented Nov 14, 2025

GH-141568 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 14, 2025
sergey-miryanov added a commit to sergey-miryanov/cpython that referenced this pull request Nov 14, 2025
@sergey-miryanov
[3.13] pythonGH-141312: Allow only integers to longrangeiter_setstate…
9d8a3b7 
… state (pythonGH-141317)

This fixes an assertion error when the new computed start is not an integer.
(cherry picked from commit 10bec7c)

Co-authored-by: Sergey Miryanov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@efimov-mikhail efimov-mikhail efimov-mikhail approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger

Assignees

@serhiy-storchaka serhiy-storchaka

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sergey-miryanov @picnixz @efimov-mikhail @vstinner @serhiy-storchaka