8000 bpo-45018: Fix rangeiter_reduce in rangeobject.c by chilaxan · Pull Request #27938 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-45018: Fix rangeiter_reduce in rangeobject.c #27938

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 7 commits into from
Aug 27, 2021
Merged

bpo-45018: Fix rangeiter_reduce in rangeobject.c #27938

merged 7 commits into from
Aug 27, 2021

Conversation

chilaxan
Copy link
Contributor
@chilaxan chilaxan commented Aug 25, 2021 •
edited by bedevere-bot
Loading

This commit changes an i to an l as r->index is a long not an int

https://bugs.python.org/issue45018

serhiy-storchaka
serhiy-storchaka previously requested changes Aug 25, 2021
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.

Please add test and a NEWS entry.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ambv
ambv reviewed Aug 26, 2021
View reviewed changes
@ambv ambv changed the title Fix rangeiter_reduce in rangeobject.c bpo-45018: Fix rangeiter_reduce in rangeobject.c Aug 26, 2021
@ambv ambv dismissed serhiy-storchaka’s stale review August 26, 2021 16:47

Requested changes applied.

serhiy-storchaka
serhiy-storchaka reviewed Aug 26, 2021
View reviewed changes
@ambv
Copy link
Contributor
ambv commented Aug 26, 2021

@serhiy-storchaka, I shortened the test per your suggestion. Now it indeed fails quickly without the patch:

======================================================================
FAIL: test_iterator_pickling_overflowing_index (test.test_range.RangeTest) (proto=5)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ambv/Dropbox/Python/cpython/Lib/test/test_range.py", line 408, in test_iterator_pickling_overflowing_index
    self.assertEqual(idx, 2**32)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 1 != 4294967296

serhiy-storchaka
serhiy-storchaka reviewed Aug 26, 2021
View reviewed changes
Lib/test/test_range.py
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
with self.subTest(proto=proto):
it = iter(range(2**32 + 2))
_, _, idx = it.__reduce__()
Copy link
Member

Choose a reason for hiding this comment

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

Test with __reduce__ is fragile. I have an idea of making the range iterator more compact and faster. __reduce__() will return different results, although pickle compatibility will be preserved.

Other Python implementations can use such range iterator implementation from start.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we include this test for now and adapt it once your proposed PR is up? I'm happy to review it then.

Copy link
Member

Choose a reason for hiding this comment

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

Then other Python implementations will be limited in implementation of the range iterator.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the __reduce__ part is fragile then probably so is __setstate__. I think we should keep this so if an alternative implementation actually sees this failing, they have a chance of speaking up on BPO or realizing what this test is even doing.

As I said, I fully support changing this test when/if we modify __reduce__ behavior in CPython in the future. But for now I don't see a downside of including it. Calls to __reduce__ are already used in tests for descriptors, datetime, OrderedDict, and structseq.c

@ambv ambv added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Aug 27, 2021
@ambv ambv merged commit 94a3d2a into python:main Aug 27, 2021
@miss-islington
Copy link
Contributor

Thanks @chilaxan for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 27, 2021
@chilaxan @miss-islington
bpo-45018: Fix rangeiter_reduce in rangeobject.c (pythonGH-27938)
46727ab 
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 94a3d2a)

Co-authored-by: chilaxan <chilaxan@gmail.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 27, 2021
@bedevere-bot
Copy link

GH-27990 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 27, 2021
@chilaxan @miss-islington
bpo-45018: Fix rangeiter_reduce in rangeobject.c (pythonGH-27938)
1de0aeb 
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 94a3d2a)

Co-authored-by: chilaxan <chilaxan@gmail.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Aug 27, 2021
@bedevere-bot
Copy link

GH-27991 is a backport of this pull request to the 3.9 branch.

ambv pushed a commit that referenced this pull request Aug 27, 2021
@miss-islington @chilaxan
bpo-45018: Fix rangeiter_reduce in rangeobject.c (GH-27938) (GH-27991)
0243764 
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 94a3d2a)

Co-authored-by: chilaxan <chilaxan@gmail.com>
miss-islington added a commit that referenced this pull request Aug 28, 2021
@miss-islington @chilaxan
bpo-45018: Fix rangeiter_reduce in rangeobject.c (GH-27938)
cd986e9 
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 94a3d2a)

Co-authored-by: chilaxan <chilaxan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambv ambv ambv left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chilaxan @bedevere-bot @ambv @miss-islington @serhiy-storchaka @Fidget-Spinner @the-knights-who-say-ni
0