docs: `#[pyclass(sequence)]` does not make Python add the sequence length to negative indexes

[ravenexp]

Bug Description

https://pyo3.rs/v0.17.1/class/protocols.html#mapping--sequence-types

Use the #[pyclass(sequence)] annotation to instruct PyO3 to fill the sq_length slot instead of the mp_length slot for len. This will help libraries such as numpy recognise the class as a sequence, however will also cause CPython to automatically add the sequence length to any negative indices before passing them to getitem. (getitem, setitem and delitem mapping slots are still used for sequences, for slice operations.)

What actually happens is that negative indices are passed as is to __getitem__.

I guess this happens because PyO3 only fills the mapping slot for __getitem__, and the negative index correction thing only happens when calling the sequence slot for __getitem__.

Steps to Reproduce

Minimal reproduction:

use pyo3::prelude::*;                                  
                                                       
#[pyclass(sequence)]                                   
struct MyList;                                         
                                                       
#[pymethods]                                           
impl MyList {                                          
    #[new]                                             
    fn new() -> MyList {                               
        MyList {}                                      
    }                                                  
                                                       
    fn __len__(&self) -> usize {                       
        42                                             
    }                                                  
                                                       
    fn __getitem__(&self, idx: isize) -> bool {        
        assert!(idx >= 0);                             
        true                                           
    }                                                  
}                                                      
                                                       
#[pymodule]                                            
fn testmod(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_class::<MyList>()?;                          
    Ok(())                                             
}                                                      

Backtrace

>>> from testmod import MyList
>>> a = MyList()
>>> a[-1]
thread '<unnamed>' panicked at 'assertion failed: idx >= 0', src/lib.rs:18:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
pyo3_runtime.PanicException: assertion failed: idx >= 0
>>> a[0]
True

### Your operating system and version

Arch Linux

### Your Python version (`python --version`)

Python 3.10.6

### Your Rust version (`rustc --version`)

rustc 1.63.0

### Your PyO3 version

0.17.1

### How did you install python? Did you use a virtualenv?

pacman

Yes

### Additional Info

_No response_

[davidhewitt]

I strongly discourage you from relying on this behaviour of CPython to convert negative indexes. The docs should probably have the word sometimes in there.

CPython only changes negative indexes when they're passed via the PySequence_GetItem API call. Any other use, e.g. indexing operator(*) or calling the slot directly, won't adjust the negative index, so you should always write an implementation which can handle them. The problem is that because your implementation can't know if the index has been adjusted, if you have #[pyclass(sequence)] all you can do with negative indexes is error on them.

This is not a problem unique to PyO3, here is an example demonstrating both a #[pyclass(sequence)] and a Python class suffering from exactly the same problem:

use pyo3::{ffi, prelude::*, AsPyPointer};

#[pyfunction]
fn index_sequence(obj: &PyAny, i: ffi::Py_ssize_t) -> PyResult<&PyAny> {
    unsafe {
        obj.py()
            .from_owned_ptr_or_err(ffi::PySequence_GetItem(obj.as_ptr(), i))
    }
}

#[pyclass(sequence)]
struct RustSeq;

#[pymethods]
impl RustSeq {
    #[new]
    fn new() -> Self {
        Self
    }

    fn __len__(&self) -> usize {
        10
    }

    fn __getitem__(&self, idx: PyObject) -> PyObject {
        idx
    }
}

#[pymodule]
fn pyo3_scratch(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_function(wrap_pyfunction!(index_sequence, m)?)?;
    m.add_class::<RustSeq>()?;
    Ok(())
}

Sample Python code

from typing import Any

from pyo3_scratch import RustSeq, index_sequence


class Seq:
    def __len__(self) -> int:
        return 10

    def __getitem__(self, idx: Any) -> Any:
        return idx


seq = Seq()
rseq = RustSeq()

print("Python negative index via C/API:", index_sequence(seq, -5))
print("Python negative index via indexing:", seq[-5])

print("Rust negative index via C/API:", index_sequence(rseq, -5))
print("Rust negative index via indexing:", rseq[-5])

Output of running Python:

Python negative index via C/API: 5
Python negative index via indexing: -5
Rust negative index via C/API: 5
Rust negative index via indexing: -5

---

(*) - Strictly speaking, the indexing operator tries to go via the mapping mp_subscript slot first, and then sq_item. To be consistent with Python classes, PyO3 implements both by default. Even with #[pyclass(sequence)], the mp_subscript slot is needed to accept slices in __getitem__. With #[pyclass(mapping)], only mp_subscript is implemented.

[ravenexp]

Oh, I think I get it now. I was trying to revert to the old #[pyproto]-style behavior in v0.17 by creating a #[pyclass(sequence), but apparently it's no longer possible to only fill the sq_item slot. And the Python indexing operator is probably never going to hit it as long as the mp_subscript slot is also filled.

The documentation can be made clearer about the negative index adjustment though.