Merge pull request #2567 from davidhewitt/pyclass-sequence

pyclass: add `sequence` option to implement `sq_length`

CHANGELOG.md

@@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add support for generating PyPy Windows import library. [#2506](https://github.com/PyO3/pyo3/pull/2506)
 - Add FFI definitions for `Py_EnterRecursiveCall` and `Py_LeaveRecursiveCall`. [#2511](https://github.com/PyO3/pyo3/pull/2511)
 - Add `get_item_with_error` on `PyDict` that exposes `PyDict_GetItemWIthError` for non-PyPy. [#2536](https://github.com/PyO3/pyo3/pull/2536)
+- Add `#[pyclass(sequence)]` option. [#2567](https://github.com/PyO3/pyo3/pull/2567)
 
 ### Changed
 

guide/pyclass_parameters.md

@@ -10,8 +10,9 @@
 | `mapping` |  Inform PyO3 that this class is a [`Mapping`][params-mapping], and so leave its implementation of sequence C-API slots empty. |
 | <span style="white-space: pre">`module = "module_name"`</span> |  Python code will see the class as being defined in this module. Defaults to `builtins`. |
 | <span style="white-space: pre">`name = "python_name"`</span> | Sets the name that Python sees this class as. Defaults to the name of the Rust struct. |
-| <span style="white-space: pre">`text_signature = "(arg1, arg2, ...)"`</span> |  Sets the text signature for the Python class' `__new__` method. |
+| `sequence` |  Inform PyO3 that this class is a [`Sequence`][params-sequence], and so leave its C-API mapping length slot empty. |
 | `subclass` | Allows other Python classes and `#[pyclass]` to inherit from this class. Enums cannot be subclassed. |
+| <span style="white-space: pre">`text_signature = "(arg1, arg2, ...)"`</span> |  Sets the text signature for the Python class' `__new__` method. |
 | `unsendable` | Required if your struct is not [`Send`][params-3]. Rather than using `unsendable`, consider implementing your struct in a threadsafe way by e.g. substituting [`Rc`][params-4] with [`Arc`][params-5]. By using `unsendable`, your class will panic when accessed by another thread.|
 | `weakref` | Allows this class to be [weakly referenceable][params-6]. |
 
@@ -36,3 +37,4 @@ struct MyClass { }
 [params-5]: https://doc.rust-lang.org/std/sync/struct.Arc.html
 [params-6]: https://docs.python.org/3/library/weakref.html
 [params-mapping]: https://pyo3.rs/latest/class/protocols.html#mapping--sequence-types
+[params-sequence]: https://pyo3.rs/latest/class/protocols.html#mapping--sequence-types

guide/src/class/protocols.md

@@ -207,9 +207,11 @@ Mapping types usually will not want the sequence slots filled. Having them fille
 
 Use the `#[pyclass(mapping)]` annotation to instruct PyO3 to only fill the mapping slots, leaving the sequence ones empty. This will apply to `__getitem__`, `__setitem__`, and `__delitem__`.
 
+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.)
+
   - `__len__(<self>) -> usize`
 
-    Implements the built-in function `len()` for the sequence.
+    Implements the built-in function `len()`.
 
   - `__contains__(<self>, object) -> bool`
 

guide/src/migration.md

@@ -230,9 +230,9 @@ class ExampleContainer:
 
 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__`.
+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_length` and `sq_item` slots, and the mapping equivalents are `mp_length` 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.
+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_length` and `mp_length` slots filled.
 
 The PyO3 behavior in 0.16 has been changed to be closer to this Python behavior by default.
 

pyo3-macros-backend/src/attributes.rs

@@ -23,6 +23,7 @@ pub mod kw {
     syn::custom_keyword!(module);
     syn::custom_keyword!(name);
     syn::custom_keyword!(pass_module);
+    syn::custom_keyword!(sequence);
     syn::custom_keyword!(set);
     syn::custom_keyword!(signature);
     syn::custom_keyword!(subclass);

pyo3-macros-backend/src/pyclass.rs

@@ -65,6 +65,7 @@ pub struct PyClassPyO3Options {
     pub mapping: Option<kw::mapping>,
     pub module: Option<ModuleAttribute>,
     pub name: Option<NameAttribute>,
+    pub sequence: Option<kw::sequence>,
     pub subclass: Option<kw::subclass>,
     pub text_signature: Option<TextSignatureAttribute>,
     pub unsendable: Option<kw::unsendable>,
@@ -82,6 +83,7 @@ enum PyClassPyO3Option {
     Mapping(kw::mapping),
     Module(ModuleAttribute),
     Name(NameAttribute),
+    Sequence(kw::sequence),
     Subclass(kw::subclass),
     TextSignature(TextSignatureAttribute),
     Unsendable(kw::unsendable),
@@ -109,6 +111,8 @@ impl Parse for PyClassPyO3Option {
             input.parse().map(PyClassPyO3Option::Module)
         } else if lookahead.peek(kw::name) {
             input.parse().map(PyClassPyO3Option::Name)
+        } else if lookahead.peek(attributes::kw::sequence) {
+            input.parse().map(PyClassPyO3Option::Sequence)
         } else if lookahead.peek(attributes::kw::subclass) {
             input.parse().map(PyClassPyO3Option::Subclass)
         } else if lookahead.peek(attributes::kw::text_signature) {
@@ -164,6 +168,7 @@ impl PyClassPyO3Options {
             PyClassPyO3Option::Mapping(mapping) => set_option!(mapping),
             PyClassPyO3Option::Module(module) => set_option!(module),
             PyClassPyO3Option::Name(name) => set_option!(name),
+            PyClassPyO3Option::Sequence(sequence) => set_option!(sequence),
             PyClassPyO3Option::Subclass(subclass) => set_option!(subclass),
             PyClassPyO3Option::TextSignature(text_signature) => set_option!(text_signature),
             PyClassPyO3Option::Unsendable(unsendable) => set_option!(unsendable),
@@ -315,7 +320,7 @@ fn impl_class(
         vec![],
     )
     .doc(doc)
-    .impl_all();
+    .impl_all()?;
 
     Ok(quote! {
         const _: () = {
@@ -414,7 +419,7 @@ pub fn build_py_enum(
             .map(|attr| (get_class_python_name(&enum_.ident, &args), attr)),
     );
     let enum_ = PyClassEnum::new(enum_)?;
-    Ok(impl_enum(enum_, &args, doc, method_type))
+    impl_enum(enum_, &args, doc, method_type)
 }
 
 /// `#[pyo3()]` options for pyclass enum variants
@@ -462,7 +467,7 @@ fn impl_enum(
     args: &PyClassArgs,
     doc: PythonDoc,
     methods_type: PyClassMethodsType,
-) -> TokenStream {
+) -> Result<TokenStream> {
     let krate = get_pyo3_crate(&args.options.krate);
     impl_enum_class(enum_, args, doc, methods_type, krate)
 }
@@ -473,7 +478,7 @@ fn impl_enum_class(
     doc: PythonDoc,
     methods_type: PyClassMethodsType,
     krate: syn::Path,
-) -> TokenStream {
+) -> Result<TokenStream> {
     let cls = enum_.ident;
     let ty: syn::Type = syn::parse_quote!(#cls);
     let variants = enum_.variants;
@@ -569,9 +574,9 @@ fn impl_enum_class(
         default_slots,
     )
     .doc(doc)
-    .impl_all();
+    .impl_all()?;
 
-    quote! {
+    Ok(quote! {
         const _: () = {
             use #krate as _pyo3;
 
@@ -587,7 +592,7 @@ fn impl_enum_class(
                 #default_richcmp
             }
         };
-    }
+    })
 }
 
 fn generate_default_protocol_slot(
@@ -752,16 +757,17 @@ impl<'a> PyClassImplsBuilder<'a> {
         }
     }
 
-    fn impl_all(&self) -> TokenStream {
+    fn impl_all(&self) -> Result<TokenStream> {
-        vec![
+        let tokens = vec![
             self.impl_pyclass(),
             self.impl_extractext(),
             self.impl_into_py(),
-            self.impl_pyclassimpl(),
+            self.impl_pyclassimpl()?,
             self.impl_freelist(),
         ]
         .into_iter()
-        .collect()
+        .collect();
+        Ok(tokens)
     }
 
     fn impl_pyclass(&self) -> TokenStream {
@@ -828,7 +834,7 @@ impl<'a> PyClassImplsBuilder<'a> {
             quote! {}
         }
     }
-    fn impl_pyclassimpl(&self) -> TokenStream {
+    fn impl_pyclassimpl(&self) -> Result<TokenStream> {
         let cls = self.cls;
         let doc = self.doc.as_ref().map_or(quote! {"\0"}, |doc| quote! {#doc});
         let is_basetype = self.attr.options.subclass.is_some();
@@ -841,6 +847,12 @@ impl<'a> PyClassImplsBuilder<'a> {
             .unwrap_or_else(|| parse_quote! { _pyo3::PyAny });
         let is_subclass = self.attr.options.extends.is_some();
         let is_mapping: bool = self.attr.options.mapping.is_some();
+        let is_sequence: bool = self.attr.options.sequence.is_some();
+
+        ensure_spanned!(
+            !(is_mapping && is_sequence),
+            self.cls.span() => "a `#[pyclass]` cannot be both a `mapping` and a `sequence`"
+        );
 
         let dict_offset = if self.attr.options.dict.is_some() {
             quote! {
@@ -959,12 +971,13 @@ impl<'a> PyClassImplsBuilder<'a> {
             quote! { _pyo3::PyAny }
         };
 
-        quote! {
+        Ok(quote! {
             impl _pyo3::impl_::pyclass::PyClassImpl for #cls {
                 const DOC: &'static str = #doc;
                 const IS_BASETYPE: bool = #is_basetype;
                 const IS_SUBCLASS: bool = #is_subclass;
                 const IS_MAPPING: bool = #is_mapping;
+                const IS_SEQUENCE: bool = #is_sequence;
 
                 type Layout = _pyo3::PyCell<Self>;
                 type BaseType = #base;
@@ -1002,7 +1015,7 @@ impl<'a> PyClassImplsBuilder<'a> {
             }
 
             #inventory_class
-        }
+        })
     }
 
     fn impl_freelist(&self) -> TokenStream {

src/impl_/pyclass.rs

@@ -146,6 +146,9 @@ pub trait PyClassImpl: Sized {
     /// #[pyclass(mapping)]
     const IS_MAPPING: bool = false;
 
+    /// #[pyclass(sequence)]
+    const IS_SEQUENCE: bool = false;
+
     /// Layout
     type Layout: PyLayout<Self>;
 

src/pyclass.rs

@@ -43,6 +43,7 @@ where
             .slot(ffi::Py_tp_dealloc, tp_dealloc::<T> as *mut c_void)
             .set_is_basetype(T::IS_BASETYPE)
             .set_is_mapping(T::IS_MAPPING)
+            .set_is_sequence(T::IS_SEQUENCE)
             .class_items(T::items_iter())
             .build(py, T::NAME, T::MODULE, std::mem::size_of::<T::Layout>())
     } {
@@ -63,6 +64,7 @@ struct PyTypeBuilder {
     /// except for that it does and we have tests.
     cleanup: Vec<PyTypeBuilderCleanup>,
     is_mapping: bool,
+    is_sequence: bool,
     has_new: bool,
     has_dealloc: bool,
     has_getitem: bool,
@@ -225,6 +227,11 @@ impl PyTypeBuilder {
         self
     }
 
+    fn set_is_sequence(mut self, is_sequence: bool) -> Self {
+        self.is_sequence = is_sequence;
+        self
+    }
+
     /// # Safety
     /// All slots in the PyClassItemsIter should be correct
     unsafe fn class_items(mut self, iter: PyClassItemsIter) -> Self {
@@ -348,6 +355,15 @@ impl PyTypeBuilder {
             )));
         }
 
+        // For sequences, implement sq_length instead of mp_length
+        if self.is_sequence {
+            for slot in &mut self.slots {
+                if slot.slot == ffi::Py_mp_length {
+                    slot.slot = ffi::Py_sq_length;
+                }
+            }
+        }
+
         // Add empty sentinel at the end
         // Safety: python expects this empty slot
         unsafe { self.push_slot(0, ptr::null_mut::<c_void>()) }

tests/test_sequence.rs

@@ -1,8 +1,8 @@
 #![cfg(feature = "macros")]
 
 use pyo3::exceptions::{PyIndexError, PyValueError};
-use pyo3::prelude::*;
 use pyo3::types::{IntoPyDict, PyList, PyMapping, PySequence};
+use pyo3::{ffi, prelude::*, AsPyPointer};
 
 use pyo3::py_run;
 
@@ -279,7 +279,7 @@ fn test_generic_list_set() {
     });
 }
 
-#[pyclass]
+#[pyclass(sequence)]
 struct OptionList {
     #[pyo3(get, set)]
     items: Vec<Option<i64>>,
@@ -287,6 +287,10 @@ struct OptionList {
 
 #[pymethods]
 impl OptionList {
+    fn __len__(&self) -> usize {
+        self.items.len()
+    }
+
     fn __getitem__(&self, idx: isize) -> PyResult<Option<i64>> {
         match self.items.get(idx as usize) {
             Some(x) => Ok(*x),
@@ -332,3 +336,22 @@ fn sequence_is_not_mapping() {
     assert!(list.as_ref().downcast::<PyMapping>().is_err());
     assert!(list.as_ref().downcast::<PySequence>().is_ok());
 }
+
+#[test]
+fn sequence_length() {
+    Python::with_gil(|py| {
+        let list = PyCell::new(
+            py,
+            OptionList {
+                items: vec![Some(1), None],
+            },
+        )
+        .unwrap();
+
+        assert_eq!(list.len().unwrap(), 2);
+        assert_eq!(unsafe { ffi::PySequence_Length(list.as_ptr()) }, 2);
+
+        assert_eq!(unsafe { ffi::PyMapping_Length(list.as_ptr()) }, -1);
+        unsafe { ffi::PyErr_Clear() };
+    })
+}

tests/ui/invalid_pyclass_args.rs

@@ -21,4 +21,7 @@ struct InvalidModule {}
 #[pyclass(weakrev)]
 struct InvalidArg {}
 
+#[pyclass(mapping, sequence)]
+struct CannotBeMappingAndSequence {}
+
 fn main() {}

tests/ui/invalid_pyclass_args.stderr

@@ -1,4 +1,4 @@
-error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `mapping`, `module`, `name`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
+error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `mapping`, `module`, `name`, `sequence`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
  --> tests/ui/invalid_pyclass_args.rs:3:11
   |
 3 | #[pyclass(extend=pyo3::types::PyDict)]
@@ -34,8 +34,14 @@ error: expected string literal
 18 | #[pyclass(module = my_module)]
    |                    ^^^^^^^^^
 
-error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `mapping`, `module`, `name`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
+error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `mapping`, `module`, `name`, `sequence`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
   --> tests/ui/invalid_pyclass_args.rs:21:11
    |
 21 | #[pyclass(weakrev)]
    |           ^^^^^^^
+
+error: a `#[pyclass]` cannot be both a `mapping` and a `sequence`
+  --> tests/ui/invalid_pyclass_args.rs:25:8
+   |
+25 | struct CannotBeMappingAndSequence {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^