Skip to content

gh-126618: Fix representation of itertools.count(sys.maxsize) #126620

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

Closed
wants to merge 8 commits into from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Nov 9, 2024

Backports for 3.12 will be manual since the free-threaded code will not be needed.

@picnixz picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Nov 9, 2024
@picnixz picnixz requested a review from rhettinger as a code owner November 9, 2024 13:36
@picnixz picnixz requested a review from skirpichev November 9, 2024 13:38
@picnixz picnixz changed the title gh-126618: Fix itertools.count.__repr__ when at sys.maxsize gh-126618: Fix itertools.count(sys.maxsize).__repr__ Nov 9, 2024
@picnixz picnixz changed the title gh-126618: Fix itertools.count(sys.maxsize).__repr__ gh-126618: Fix representation of itertools.count(sys.maxsize) Nov 9, 2024
@picnixz
Copy link
Member Author

picnixz commented Nov 9, 2024

Converting to draft until the CI failures are fixed by #126617.

@picnixz picnixz marked this pull request as draft November 9, 2024 14:12
@rhettinger rhettinger self-assigned this Nov 10, 2024
Copy link
Contributor

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

Here is a more simple workaround:

diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c
index 1201fa0949..a00b867d73 100644
--- a/Modules/itertoolsmodule.c
+++ b/Modules/itertoolsmodule.c
@@ -3415,7 +3418,7 @@ count_next(countobject *lz)
 static PyObject *
 count_repr(countobject *lz)
 {
-    if (lz->cnt != PY_SSIZE_T_MAX)
+    if (lz->long_cnt == NULL)
         return PyUnicode_FromFormat("%s(%zd)",
                                     _PyType_Name(Py_TYPE(lz)), lz->cnt);
 

But, perhaps, your solution is better, as it ensure that either:

assert(cnt != PY_SSIZE_T_MAX && long_cnt == NULL && long_step==1)

or

assert(cnt == PY_SSIZE_T_MAX && long_cnt != NULL && long_step != NULL)

kept.

@picnixz
Copy link
Member Author

picnixz commented Nov 10, 2024

Yea, I wanted to keep those invariants but if performances matter more, we may use your solution (it would reduce the number of operations to perform in general).

@picnixz picnixz marked this pull request as ready for review November 12, 2024 13:28
@skirpichev skirpichev self-requested a review November 12, 2024 14:35
@rhettinger
Copy link
Contributor

Is there a simpler fix? I was expecting a much more minor edit to just the repr code.

@picnixz
Copy link
Member Author

picnixz commented Nov 12, 2024

Is there a simpler fix? I was expecting a much more minor edit to just the repr code.

Sergey's patch could be the solution if you don't mind having the invariants invalid.

@skirpichev
Copy link
Contributor

Perhaps, here is slightly another alternative to this fix and #126617:

diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c
index 78fbdcdf77..6ca93294b6 100644
--- a/Modules/itertoolsmodule.c
+++ b/Modules/itertoolsmodule.c
@@ -3291,9 +3291,6 @@ itertools_count_impl(PyTypeObject *type, PyObject *long_cnt,
                 PyErr_Clear();
                 fast_mode = 0;
             }
-            else if (cnt == PY_SSIZE_T_MAX) {
-                fast_mode = 0;
-            }
         }
     } else {
         cnt = 0;
@@ -3325,7 +3322,7 @@ itertools_count_impl(PyTypeObject *type, PyObject *long_cnt,
     else
         cnt = PY_SSIZE_T_MAX;
 
-    assert((cnt != PY_SSIZE_T_MAX && long_cnt == NULL && fast_mode) ||
+    assert((long_cnt == NULL && fast_mode) ||
            (cnt == PY_SSIZE_T_MAX && long_cnt != NULL && !fast_mode));
     assert(!fast_mode ||
            (PyLong_Check(long_step) && PyLong_AS_LONG(long_step) == 1));
@@ -3418,7 +3415,7 @@ count_next(countobject *lz)
 static PyObject *
 count_repr(countobject *lz)
 {
-    if (lz->cnt != PY_SSIZE_T_MAX)
+    if (lz->long_cnt == NULL)
         return PyUnicode_FromFormat("%s(%zd)",
                                     _PyType_Name(Py_TYPE(lz)), lz->cnt);
 
This reverts fix from #126617 and adjusts assert instead. This will require also documentation change ("Counting logic and invariants" comment).

Copy link
Contributor

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

It seems, CI failure is related to the patch.

Alternative solution (from the thread) is presented in #127048.

@picnixz
Copy link
Member Author

picnixz commented Nov 20, 2024

The CI failure is in importlib so I don't really know but I'll have a look at your patch when I am back.

@skirpichev
Copy link
Contributor

The CI failure is in importlib so I don't really know

Yes, it's not obvious how this failure is triggered. But your code alters free-threading code - and that failure clearly belongs to it, I did several updates.

@picnixz
Copy link
Member Author

picnixz commented Nov 30, 2024

Closing in favor of #127048.

@picnixz picnixz closed this Nov 30, 2024
@picnixz picnixz deleted the fix/itertools-count-repr-126618 branch November 30, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants