I would like to give greater visibility to the discussion around issue 96436 which proposes adding a new TP flag to the Stable API.

After spending time to understand the OP’s request and the problem it solves, I’m thinking that it is a good idea. However, it warrants a broader discussion because it would be a permanent API change. Also, I’m unclear about whether Cython’s workaround is correct, whether other workarounds are possible, and whether this is something we want to support.

Here is my recap of the problem statement and the proposed solution:

The norm is for writers of pure python sequence classes to create a __getitem__ method that includes support for negative indices. The norm for writers of C sequence classes is to attach to the sq_item slot and let negative index support be handled upstream by PySequence_GetItem.

This all works fine unless someone writes a pure python class (with negative index support) and has a C or Rust conversion tool like Cython or PyO3 generate an extension that uses the sq_item slot.

The proposed solution is to add Py_TPFLAGS_NO_SEQ_INDEX_ADJUST to be used by tools that generate compiled Python extensions from pure Python code. It instructs PySequence_GetItem to not perform its usual negative index adjustment. This would allow the pure Python code and the compiled extension to have the same logic.

1 Like

Note to others, definitely read the GH issue and discussion first. There are surprises in this situation.

It sounds like that is assuming the Python __getitem__ code is the desired canonical implementation (nice) that is being turned into a sq_item slot C implementation? When I first read your summary I questioned what the value in the check to skip the PySequence_GetItem internal negative handling logic would even be, thinking that would just mean the repurposed getitem code posing as sq_item only receives positive values so its own internal handling for negatives becomes a branch never taken… But the GH issue reveals otherwise:

PySequence_GetItem doesn’t check bounds. Index a length 5 sequence by -8 and the length “corrected” for reverse indexing value passed to sq_item becomes -3. That smells like a bug in PySequence_GetItem to me.

I wonder what depends on this IMNSHO unfortunate behavior. I’d personally prefer it if the negative was checked against length and IndexError was raised if the reverse adjusted index was still going to be negative rather than passing that on to the sq_item slot. But I’m guessing Hyrum’s Law applies and someone creatively depends on this extra negative indexing ability for some other purpose.

All sq_item implementations must still implement a range check equivalent to 0 <= index < len(self). E.g. in tupleobject.c::tupleitem I find

    if (i < 0 || i >= Py_SIZE(a)) {
        PyErr_SetString(PyExc_IndexError, "tuple index out of range");
        return NULL;

(The corresponding list code is, um, clever – it uses a cast to the unsigned size_t type to skip the < 0 check. I guess this works since the length of a list cannot be >= 2**31.)

Thank you all for taking time to read and consider this case.

The most concise summary I can give for proposing the flag after thinking over this case for some time is that I think it leads to simplest correct sequence implementations.

Looking at tuple, at the moment the adjustments for negative inputs are duplicated between mp_subscript and PySequence_GetItem. This can’t be replicated by a pure-python sequence implementing __getitem__.

After setting the proposed flag the implementation would only need to handle negative inputs in sq_item. This can be replicated by a pure-python sequence implementing __getitem__.

In turn that could enable tuple’s sq_item to raise something like IndexError: tuple index -7 out of range for tuple of length 5 or some other detailed error message (if that’s perceived useful / non-breaking).

I’m not wedded to this solution to the original problem (maybe just adding bounds check to PySequence_GetItem is enough as postulated by @gpshead).

If the preferred outcome from this thread would require a PEP, I would be willing to author / co-author it.

FWIW there’s a lot of discussion of Cython on the issue that I think is is pretty tangential to the issue. Cython only generates the mp_subscript slots itself and never generates the sq_item slot so isn’t affected by this issue.

I don’t have a lot to contribute to the general discussion here but just wanted to say “don’t get distracted by what Cython does!”

1 Like