gh-158102: Add tests for the slice C API by serhiy-storchaka · Pull Request #158120 · python/cpython

serhiy-storchaka · 2026-09-24T16:29:27Z

Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(), PySlice_AdjustIndices() and PySlice_GetIndicesEx().

PySlice_GetIndicesEx() is tested both as a macro and as a deprecated function, including the case when __index__() resizes the sequence whose size is passed as the length: the macro evaluates it after PySlice_Unpack(), the function before the call.

Issue: Add C API tests for the slice C API #158102

Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(), PySlice_AdjustIndices() and PySlice_GetIndicesEx() -- both the macro and the deprecated function. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

vstinner

Thanks for writing an exhaustive test suite for the PySlice C API. It's more complicated than what I expected! Here is a first review.

vstinner · 2026-09-25T08:31:25Z

+SIZE_MAX = sys.maxsize
+SIZE_MIN = -sys.maxsize - 1


In C, SIZE_MAX is a different constant: size_t maximum (_testcapi.SIZE_MAX). Since this file tests C functions, it's confusing.

Can you rename the constants to SSIZE_MAX and SSIZE_MIN?

The _testcapi uses different names: PY_SSIZE_T_MAX and PY_SSIZE_T_MIN.

vstinner · 2026-09-25T08:57:16Z

+VALUES = [None, 0, 1, 2, 3, 5, 7, -1, -2, -3, -5, -7]
+STEPS = [None, 1, 2, 3, 5, -1, -2, -3, -5]
+LENGTHS = [0, 1, 2, 5, 10]


./python -m test test_capi.test_slice -v -m test_adjustindices takes 700 ms. I'm not sure that it's worth it to test so many combinations:

Suggested change

VALUES = [None, 0, 1, 2, 3, 5, 7, -1, -2, -3, -5, -7]

STEPS = [None, 1, 2, 3, 5, -1, -2, -3, -5]

LENGTHS = [0, 1, 2, 5, 10]

VALUES = [None, 0, 1, 3, 7, -1, -3, -7]

STEPS = [None, 1, 3, 5, -1, -3, -5]

LENGTHS = [0, 1, 3, 10]

vstinner · 2026-09-25T08:59:31Z

+        # The step is asserted to be neither zero nor less than
+        # -PY_SSIZE_T_MAX.
+        # CRASHES adjust(10, 0, 10, 0)
+        # CRASHES adjust(10, 0, 10, SIZE_MIN)


PySlice_Unpack() replaces step=PY_SSIZE_T_MIN with step=PY_SSIZE_T_MAX. Would it be acceptable do the same here? Well, if we change the behavior, I would prefer to do it in a separated PR. It's better to test the current behavior without changing code first, as you do :-)

Normally, PySlice_AdjustIndices() is always called after PySlice_Unpack(), which guarantees that step is never 0 or less than -SSIZE_MAX. An additional check here would be redundant.

vstinner · 2026-09-25T09:01:36Z

+
+        # The step is asserted to be neither zero nor less than
+        # -PY_SSIZE_T_MAX.
+        # CRASHES adjust(10, 0, 10, 0)


It's unfortunate that the function cannot fail. Can you update PySlice_AdjustIndices() document to mention that step must not be equal to zero and must not be equal to PY_SSIZE_T_MIN?

I'll update the docs if they already do not mentioned this.

vstinner · 2026-09-25T09:02:15Z

+        self.assertEqual(adjust(10, SIZE_MIN, SIZE_MAX, 1), (10, 0, 10))
+        self.assertEqual(adjust(10, SIZE_MAX, SIZE_MIN, -1), (10, 9, -1))
+        self.assertEqual(adjust(0, 1, 7, 1), (0, 0, 0))
+        self.assertEqual(adjust(0, 7, 1, -1), (0, -1, -1))


Can you add tests on negative length?

vstinner · 2026-09-25T09:04:04Z

+
+// Need limited C API 3.6.1 for PySlice_Unpack() and PySlice_AdjustIndices()
+// and for PySlice_GetIndicesEx() implemented as a macro.
+#if !defined(Py_GIL_DISABLED) && !defined(Py_LIMITED_API)


The limited C API can now be tested on Free Threading. You can also define Py_LIMITED_API if Py_GIL_DISABLED is defined, Py_TARGET_ABI3T is set to Py_LIMITED_API in this case by Include/pyabi.h.

vstinner · 2026-09-25T09:06:29Z

+/* Test PySlice_GetIndicesEx() implemented as a macro using PySlice_Unpack()
+ * and PySlice_AdjustIndices(). */
+static PyObject *
+slice_getindicesex(PyObject *Py_UNUSED(module), PyObject *args)


I suggest to rename this function to "slice_getindicesex_macro()", or rename "slice_getindicesex_deprecated()" to "slice_getindicesex_func()". Same remark for the test case class name.

vstinner · 2026-09-25T09:16:27Z

+        self.assertEqual(getindicesex(slice(7, 1, -2**1000), 10),
+                         (7, 1, -SIZE_MAX, 1))
+        self.assertEqual(getindicesex(slice(7, 1, SIZE_MIN), 10),
+                         (7, 1, -SIZE_MAX, 1))


Sadly, PySlice_GetIndicesEx() also accepts negative length. You may also add tests on negative length.

Co-authored-by: Victor Stinner <vstinner@python.org>

Also remove trailing whitespace added by a suggestion.

Rename SIZE_MAX and SIZE_MIN to SSIZE_MAX and SSIZE_MIN, and the PySlice_GetIndicesEx() wrappers to slice_getindicesex_macro() and slice_getindicesex_func(). Test the limited C API also on free threading. Reduce the number of tested combinations. Add tests for zero and negative length, and document that the length must not be negative.

vstinner

LGTM. Thanks for the update. Tests should now cover all code paths.

miss-islington-app · 2026-09-25T12:49:16Z

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

miss-islington-app · 2026-09-25T12:49:20Z

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

cherry_picker 8c056c33aacea5768f5e2010401f2b98ecd7a305 3.15

miss-islington-app · 2026-09-25T12:49:26Z

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

cherry_picker 8c056c33aacea5768f5e2010401f2b98ecd7a305 3.14

miss-islington-app · 2026-09-25T12:49:30Z

Sorry, @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 8c056c33aacea5768f5e2010401f2b98ecd7a305 3.13

serhiy-storchaka requested review from a team, AA-Turner, emmatyping, erlend-aasland and itamaro as code owners September 24, 2026 16:29

bedevere-app Bot mentioned this pull request Sep 24, 2026

Add C API tests for the slice C API #158102

Open

bedevere-app Bot added the awaiting core review label Sep 24, 2026

serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes skip news tests Tests in the Lib/test dir labels Sep 24, 2026

serhiy-storchaka mentioned this pull request Sep 24, 2026

gh-158077: Remove deprecated PySlice_GetIndicesEx() function #158089

Open

vstinner reviewed Sep 25, 2026

View reviewed changes

serhiy-storchaka and others added 3 commits September 25, 2026 12:37

Apply batched suggestions from code review

1916314

Co-authored-by: Victor Stinner <vstinner@python.org>

Document the step precondition of PySlice_AdjustIndices()

731ae1f

Also remove trailing whitespace added by a suggestion.

serhiy-storchaka requested a review from ZeroIntensity as a code owner September 25, 2026 10:05

vstinner approved these changes Sep 25, 2026

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Sep 25, 2026

serhiy-storchaka enabled auto-merge (squash) September 25, 2026 11:02

serhiy-storchaka merged commit 8c056c3 into python:main Sep 25, 2026
103 of 106 checks passed

bedevere-app Bot removed the awaiting merge label Sep 25, 2026

miss-islington-app Bot assigned serhiy-storchaka Sep 25, 2026

Uh oh!

Conversation

serhiy-storchaka commented Sep 24, 2026

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app Bot commented Sep 25, 2026

Uh oh!

miss-islington-app Bot commented Sep 25, 2026

Uh oh!

miss-islington-app Bot commented Sep 25, 2026

Uh oh!

miss-islington-app Bot commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

serhiy-storchaka Sep 25, 2026 •

edited

Loading