fix(stl_bind): correct __delitem__ for negative-step slices and re-enable contiguous erase fast path (#6088)
* fix(stl_bind): correct __delitem__ for negative-step slices and re-enable contiguous erase fast path
The slice __delitem__ binding advanced the erase index by step - 1 for
all steps. That correction is only valid for positive steps, where
erasing shifts later elements down by one. For negative steps the
visited indices are strictly decreasing and erasing never shifts them,
so the extra -1 deleted the wrong elements (e.g. del v[::-2] on
[0,1,2,3] yielded [1,2] instead of [0,2]) and del v[::-1] walked off the
front of the vector (v.begin() - 1, observed SIGBUS).
Switch to the signed slice::compute overload so negative steps stay
signed, advance by step for negative steps and step - 1 for positive
ones, and drop the && false that had disabled the O(n) contiguous fast
path since 2016.
Assisted-by: ClaudeCode:claude-fable-5
* refactor: address review — static_cast and parametrized test
Use static_cast instead of a C-style cast for the slice.compute() size
argument, and convert the __delitem__ slice test to
pytest.mark.parametrize over the slice cases.
Assisted-by: ClaudeCode:claude-fable-5
* test(stl_bind): cover slice deletion edge cases
* fix(stl_bind): erase strided slices in descending order
* Eliminate a variable and avoid redundant index increment (i + 1, ++i).
The control flow handles all relevant boundaries:
- slicelength == 0: excluded by the outer guard.
- slicelength == 1: erases once, decrements to zero, and breaks without touching start.
- Larger slices: updates start exactly when another erase remains.
- slicelength cannot underflow because the loop exits when it reaches zero.
- Mutating slicelength is harmless because it is not used afterward.
- The potentially dangerous final start += step remains eliminated.
It also removes the separate loop counter. The compiler would probably
optimize the former i + 1, ++i mechanics away, but the new source expresses
the real state more directly: "number of erasures remaining."
The unconditional while (true) is safe because entry is strictly guarded by
slicelength > 0, and the decrement guarantees eventual termination.
---------
Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h
index 8202300..360c2cd 100644
--- a/include/pybind11/stl_bind.h
+++ b/include/pybind11/stl_bind.h
@@ -286,18 +286,28 @@
cl.def(
"__delitem__",
[](Vector &v, const slice &slice) {
- size_t start = 0, stop = 0, step = 0, slicelength = 0;
+ ssize_t start = 0, stop = 0, step = 0, slicelength = 0;
- if (!slice.compute(v.size(), &start, &stop, &step, &slicelength)) {
+ if (!slice.compute(
+ static_cast<ssize_t>(v.size()), &start, &stop, &step, &slicelength)) {
throw error_already_set();
}
- if (step == 1 && false) {
+ if (step == 1) {
v.erase(v.begin() + (DiffType) start, v.begin() + DiffType(start + slicelength));
- } else {
- for (size_t i = 0; i < slicelength; ++i) {
+ } else if (slicelength > 0) {
+ // Erase non-contiguous slices in descending index order so that
+ // erasing an element never shifts an index that remains to be erased.
+ if (step > 0) {
+ start += (slicelength - 1) * step;
+ step = -step;
+ }
+ while (true) {
v.erase(v.begin() + DiffType(start));
- start += step - 1;
+ if (--slicelength == 0) {
+ break;
+ }
+ start += step;
}
}
},
diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py
index 518f2df..edc6d28 100644
--- a/tests/test_stl_binders.py
+++ b/tests/test_stl_binders.py
@@ -1,5 +1,7 @@
from __future__ import annotations
+import sys
+
import pytest
from pybind11_tests import stl_binders as m
@@ -67,6 +69,40 @@
assert len(v_int2) == 0
+@pytest.mark.parametrize(
+ "s",
+ [
+ slice(1, 4),
+ slice(None, None, 2),
+ slice(1, None, 2),
+ slice(None, None, -1),
+ slice(None, None, -2),
+ slice(3, 1, -1),
+ slice(2, 2),
+ slice(None),
+ slice(5, 0, -2),
+ slice(-3, -1),
+ slice(None, None, -3),
+ slice(3, None, sys.maxsize),
+ slice(-2, -7, -2),
+ ],
+)
+def test_vector_delitem_slice(s):
+ for n in range(8):
+ ref = list(range(n))
+ got = m.VectorInt(range(n))
+ del ref[s]
+ del got[s]
+ assert list(got) == ref, f"n={n}"
+
+
+def test_vector_delitem_slice_step_zero():
+ v = m.VectorInt(range(8))
+ with pytest.raises(ValueError):
+ del v[::0]
+ assert list(v) == list(range(8))
+
+
# Older PyPy's failed here, related to the PyPy's buffer protocol.
def test_vector_buffer():
b = bytearray([1, 2, 3, 4])
