pymethods: seq methods from mapping methods

CHANGELOG.md

@@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `PyErr::new_type` now takes an optional docstring and now returns `PyResult<Py<PyType>>` rather than a `ffi::PyTypeObject` pointer.
 - The `create_exception!` macro can now take an optional docstring. This docstring, if supplied, is visible to users (with `.__doc__` and `help()`) and
   accompanies your error type in your crate's documentation.
+- `__getitem__`, `__setitem__` and `__delitem__` in `#[pymethods]` now implement both a Python mapping and sequence by default. [#2065](https://github.com/PyO3/pyo3/pull/2065)
 - Improve performance and error messages for `#[derive(FromPyObject)]` for enums. [#2068](https://github.com/PyO3/pyo3/pull/2068)
 - Reduce generated LLVM code size (to improve compile times) for:
   - internal `handle_panic` helper [#2074](https://github.com/PyO3/pyo3/pull/2074)

guide/src/migration.md

@@ -5,10 +5,40 @@ For a detailed list of all changes, see the [CHANGELOG](changelog.md).
 
 ## from 0.15.* to 0.16
 
-### Drop support for older technogies
+### Drop support for older technologies
 
 PyO3 0.16 has increased minimum Rust version to 1.48 and minimum Python version to 3.7. This enables ore use of newer language features (enabling some of the other additions in 0.16) and simplifies maintenance of the project.
 
+### Container magic methods now match Python behavior
+
+In PyO3 0.15, `__getitem__`, `__setitem__` and `__delitem__` in `#[pymethods]` would generate only the _mapping_ implementation for a `#[pyclass]`. To match the Python behavior, these methods now generate both the _mapping_ **and** _sequence_ implementations.
+
+This means that classes implementing these `#[pymethods]` will now also be treated as sequences, same as a Python `class` would be. Small differences in behavior may result:
+ - PyO3 will allow instances of these classes to be cast to `PySequence` as well as `PyMapping`.
+ - Python will provide a default implementation of `__iter__` (if the class did not have one) which repeatedly calls `__getitem__` with integers (starting at 0) until an `IndexError` is raised.
+
+To explain this in detail, consider the following Python class:
+
+```python
+class ExampleContainer:
+
+    def __len__(self):
+        return 5
+
+    def __getitem__(self, idx: int) -> int:
+        if idx < 0 or idx > 5:
+            raise IndexError()
+        return idx
+```
+
+This class implements a Python [sequence](https://docs.python.org/3/glossary.html#term-sequence).
+
+The `__len__` and `__getitem__` methods are also used to implement a Python [mapping](https://docs.python.org/3/glossary.html#term-mapping). In the Python C-API, these methods are not shared: the sequence `__len__` and `__getitem__` are defined by the `sq_len` and `sq_item` slots, and the mapping equivalents are `mp_len` and `mp_subscript`. There are similar distinctions for `__setitem__` and `__delitem__`.
+
+Because there is no such distinction from Python, implementing these methods will fill the mapping and sequence slots simultaneously. A Python class with `__len__` implemented, for example, will have both the `sq_len` and `mp_len` slots filled.
+
+The PyO3 behavior in 0.16 has been changed to be closer to this Python behavior by default.
+
 ## from 0.14.* to 0.15
 
 ### Changes in sequence indexing

pyo3-macros-backend/src/pyimpl.rs

@@ -293,6 +293,8 @@ fn add_shared_proto_slots(
     );
     try_add_shared_slot!("__pow__", "__rpow__", generate_pyclass_pow_slot);
 
+    // if this assertion trips, a slot fragment has been implemented which has not been added in the
+    // list above
     assert!(implemented_proto_fragments.is_empty());
 }
 

pyo3-macros-backend/src/pymethod.rs

@@ -703,9 +703,22 @@ impl Ty {
                 quote! {
                     let #ident = #extract;
                 }
+            }
+                Ty::PySsizeT => {
+                    let ty = arg.ty;
+                    let extract = handle_error(
+                    extract_error_mode,
+                    py,
+                    quote! {
+                            ::std::convert::TryInto::<#ty>::try_into(#ident).map_err(|e| _pyo3::exceptions::PyValueError::new_err(e.to_string()))
+                    },
+                );
+                quote! {
+                    let #ident = #extract;
+                }
             }
             // Just pass other types through unmodified
-            Ty::PyBuffer | Ty::Int | Ty::PyHashT | Ty::PySsizeT | Ty::Void => quote! {},
+            Ty::PyBuffer | Ty::Int | Ty::PyHashT | Ty::Void => quote! {},
         }
     }
 }

src/class/impl_.rs

@@ -9,7 +9,12 @@ use crate::{
     type_object::{PyLayout, PyTypeObject},
     PyClass, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python,
 };
-use std::{marker::PhantomData, os::raw::c_void, ptr::NonNull, thread};
+use std::{
+    marker::PhantomData,
+    os::raw::{c_int, c_void},
+    ptr::NonNull,
+    thread,
+};
 
 /// This type is used as a "dummy" type on which dtolnay specializations are
 /// applied to apply implementations from `#[pymethods]` & `#[pyproto]`
@@ -780,3 +785,34 @@ pub(crate) unsafe extern "C" fn fallback_new(
 pub(crate) unsafe extern "C" fn tp_dealloc<T: PyClass>(obj: *mut ffi::PyObject) {
     crate::callback_body!(py, T::Layout::tp_dealloc(obj, py))
 }
+
+pub(crate) unsafe extern "C" fn sq_item_from_mapping(
+    obj: *mut ffi::PyObject,
+    index: ffi::Py_ssize_t,
+) -> *mut ffi::PyObject {
+    let index = ffi::PyLong_FromSsize_t(index);
+    if index.is_null() {
+        return std::ptr::null_mut();
+    }
+    let result = ffi::PyObject_GetItem(obj, index);
+    ffi::Py_DECREF(index);
+    result
+}
+
+pub(crate) unsafe extern "C" fn sq_ass_item_from_mapping(
+    obj: *mut ffi::PyObject,
+    index: ffi::Py_ssize_t,
+    value: *mut ffi::PyObject,
+) -> c_int {
+    let index = ffi::PyLong_FromSsize_t(index);
+    if index.is_null() {
+        return -1;
+    }
+    let result = if value.is_null() {
+        ffi::PyObject_DelItem(obj, index)
+    } else {
+        ffi::PyObject_SetItem(obj, index, value)
+    };
+    ffi::Py_DECREF(index);
+    result
+}

src/pyclass.rs

@@ -1,6 +1,8 @@
 //! `PyClass` and related traits.
 use crate::{
-    class::impl_::{fallback_new, tp_dealloc, PyClassImpl},
+    class::impl_::{
+        fallback_new, sq_ass_item_from_mapping, sq_item_from_mapping, tp_dealloc, PyClassImpl,
+    },
     ffi,
     impl_::pyclass::{PyClassDict, PyClassWeakRef},
     PyCell, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python,
@@ -114,28 +116,35 @@ unsafe fn create_type_object_impl(
         }
     }
 
+    let PyClassInfo {
+        method_defs,
+        property_defs,
+    } = method_defs_to_pyclass_info(for_each_method_def, dict_offset.is_none());
+
     // normal methods
-    let methods = py_class_method_defs(for_each_method_def);
-    if !methods.is_empty() {
-        push_slot(&mut slots, ffi::Py_tp_methods, into_raw(methods));
+    if !method_defs.is_empty() {
+        push_slot(&mut slots, ffi::Py_tp_methods, into_raw(method_defs));
     }
 
     // properties
-    let props = py_class_properties(dict_offset.is_none(), for_each_method_def);
-    if !props.is_empty() {
-        push_slot(&mut slots, ffi::Py_tp_getset, into_raw(props));
+    if !property_defs.is_empty() {
+        push_slot(&mut slots, ffi::Py_tp_getset, into_raw(property_defs));
     }
 
     // protocol methods
+    let mut has_getitem = false;
+    let mut has_setitem = false;
     let mut has_gc_methods = false;
+
     // Before Python 3.9, need to patch in buffer methods manually (they don't work in slots)
     #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))]
     let mut buffer_procs: ffi::PyBufferProcs = Default::default();
 
     for_each_proto_slot(&mut |proto_slots| {
         for slot in proto_slots {
+            has_getitem |= slot.slot == ffi::Py_mp_subscript;
+            has_setitem |= slot.slot == ffi::Py_mp_ass_subscript;
             has_gc_methods |= slot.slot == ffi::Py_tp_clear || slot.slot == ffi::Py_tp_traverse;
-
             #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))]
             if slot.slot == ffi::Py_bf_getbuffer {
                 // Safety: slot.pfunc is a valid function pointer
@@ -151,7 +160,23 @@ unsafe fn create_type_object_impl(
         slots.extend_from_slice(proto_slots);
     });
 
+    // If mapping methods implemented, define sequence methods get implemented too.
+    // CPython does the same for Python `class` statements.
+
+    // NB we don't implement sq_length to avoid annoying CPython behaviour of automatically adding
+    // the length to negative indices.
+
+    if has_getitem {
+        push_slot(&mut slots, ffi::Py_sq_item, sq_item_from_mapping as _);
+    }
+
+    if has_setitem {
+        push_slot(&mut slots, ffi::Py_sq_ass_item, sq_ass_item_from_mapping as _);
+    }
+
+    // Add empty sentinel at the end
     push_slot(&mut slots, 0, ptr::null_mut());
+
     let mut spec = ffi::PyType_Spec {
         name: py_class_qualified_name(module_name, name)?,
         basicsize: basicsize as c_int,
@@ -282,26 +307,76 @@ fn py_class_flags(has_gc_methods: bool, is_gc: bool, is_basetype: bool) -> c_uin
     flags.try_into().unwrap()
 }
 
-fn py_class_method_defs(
+struct PyClassInfo {
+    method_defs: Vec<ffi::PyMethodDef>,
+    property_defs: Vec<ffi::PyGetSetDef>,
+}
+
+fn method_defs_to_pyclass_info(
     for_each_method_def: &dyn Fn(&mut dyn FnMut(&[PyMethodDefType])),
-) -> Vec<ffi::PyMethodDef> {
-    let mut defs = Vec::new();
-
-    for_each_method_def(&mut |method_defs| {
-        defs.extend(method_defs.iter().filter_map(|def| match def {
-            PyMethodDefType::Method(def)
-            | PyMethodDefType::Class(def)
-            | PyMethodDefType::Static(def) => Some(def.as_method_def().unwrap()),
-            _ => None,
-        }));
+    dict_is_dummy: bool,
+) -> PyClassInfo {
+    let mut method_defs = Vec::new();
+    let mut property_defs_map = std::collections::HashMap::new();
+
+    for_each_method_def(&mut |class_method_defs| {
+        for def in class_method_defs {
+            match def {
+                PyMethodDefType::Getter(getter) => {
+                    getter.copy_to(
+                        property_defs_map
+                            .entry(getter.name)
+                            .or_insert(PY_GET_SET_DEF_INIT),
+                    );
+                }
+                PyMethodDefType::Setter(setter) => {
+                    setter.copy_to(
+                        property_defs_map
+                            .entry(setter.name)
+                            .or_insert(PY_GET_SET_DEF_INIT),
+                    );
+                }
+                PyMethodDefType::Method(def)
+                | PyMethodDefType::Class(def)
+                | PyMethodDefType::Static(def) => method_defs.push(def.as_method_def().unwrap()),
+                PyMethodDefType::ClassAttribute(_) => {}
+            }
+        }
     });
 
-    if !defs.is_empty() {
+    // TODO: use into_values when on MSRV Rust >= 1.54
+    let mut property_defs: Vec<_> = property_defs_map
+        .into_iter()
+        .map(|(_, value)| value)
+        .collect();
+
+    if !method_defs.is_empty() {
+        // Safety: Python expects a zeroed entry to mark the end of the defs
+        method_defs.push(unsafe { std::mem::zeroed() });
+    }
+
+    // PyPy doesn't automatically add __dict__ getter / setter.
+    // PyObject_GenericGetDict not in the limited API until Python 3.10.
+    #[cfg(not(any(PyPy, all(Py_LIMITED_API, not(Py_3_10)))))]
+    if !dict_is_dummy {
+        property_defs.push(ffi::PyGetSetDef {
+            name: "__dict__\0".as_ptr() as *mut c_char,
+            get: Some(ffi::PyObject_GenericGetDict),
+            set: Some(ffi::PyObject_GenericSetDict),
+            doc: ptr::null_mut(),
+            closure: ptr::null_mut(),
+        });
+    }
+
+    if !property_defs.is_empty() {
         // Safety: Python expects a zeroed entry to mark the end of the defs
-        defs.push(unsafe { std::mem::zeroed() });
+        property_defs.push(unsafe { std::mem::zeroed() });
     }
 
-    defs
+    PyClassInfo {
+        method_defs,
+        property_defs,
+    }
 }
 
 /// Generates the __dictoffset__ and __weaklistoffset__ members, to set tp_dictoffset and
@@ -351,52 +426,3 @@ const PY_GET_SET_DEF_INIT: ffi::PyGetSetDef = ffi::PyGetSetDef {
     doc: ptr::null_mut(),
     closure: ptr::null_mut(),
 };
-
-fn py_class_properties(
-    is_dummy: bool,
-    for_each_method_def: &dyn Fn(&mut dyn FnMut(&[PyMethodDefType])),
-) -> Vec<ffi::PyGetSetDef> {
-    let mut defs = std::collections::HashMap::new();
-
-    for_each_method_def(&mut |method_defs| {
-        for def in method_defs {
-            match def {
-                PyMethodDefType::Getter(getter) => {
-                    getter.copy_to(defs.entry(getter.name).or_insert(PY_GET_SET_DEF_INIT));
-                }
-                PyMethodDefType::Setter(setter) => {
-                    setter.copy_to(defs.entry(setter.name).or_insert(PY_GET_SET_DEF_INIT));
-                }
-                _ => (),
-            }
-        }
-    });
-
-    let mut props: Vec<_> = defs.values().cloned().collect();
-
-    // PyPy doesn't automatically adds __dict__ getter / setter.
-    // PyObject_GenericGetDict not in the limited API until Python 3.10.
-    push_dict_getset(&mut props, is_dummy);
-
-    if !props.is_empty() {
-        // Safety: Python expects a zeroed entry to mark the end of the defs
-        props.push(unsafe { std::mem::zeroed() });
-    }
-    props
-}
-
-#[cfg(not(any(PyPy, all(Py_LIMITED_API, not(Py_3_10)))))]
-fn push_dict_getset(props: &mut Vec<ffi::PyGetSetDef>, is_dummy: bool) {
-    if !is_dummy {
-        props.push(ffi::PyGetSetDef {
-            name: "__dict__\0".as_ptr() as *mut c_char,
-            get: Some(ffi::PyObject_GenericGetDict),
-            set: Some(ffi::PyObject_GenericSetDict),
-            doc: ptr::null_mut(),
-            closure: ptr::null_mut(),
-        });
-    }
-}
-
-#[cfg(any(PyPy, all(Py_LIMITED_API, not(Py_3_10))))]
-fn push_dict_getset(_: &mut Vec<ffi::PyGetSetDef>, _is_dummy: bool) {}