From fd8026c7bb2ad565466b5ef0093362232f677978 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Fri, 19 Aug 2022 13:36:34 +0100 Subject: [PATCH] pyclass: add `sequence` option to implement `sq_length` --- CHANGELOG.md | 1 + guide/pyclass_parameters.md | 6 ++-- guide/src/class/protocols.md | 4 ++- guide/src/migration.md | 4 +-- pyo3-macros-backend/src/attributes.rs | 1 + pyo3-macros-backend/src/pyclass.rs | 41 ++++++++++++++++++--------- src/impl_/pyclass.rs | 3 ++ src/pyclass.rs | 16 +++++++++++ tests/test_sequence.rs | 27 ++++++++++++++++-- tests/ui/invalid_pyclass_args.rs | 3 ++ tests/ui/invalid_pyclass_args.stderr | 10 +++++-- 11 files changed, 93 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b21ff8be8e..ed5ec6bc520 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/guide/pyclass_parameters.md b/guide/pyclass_parameters.md index 6ec066736b9..c5006890fbb 100644 --- a/guide/pyclass_parameters.md +++ b/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. | | `module = "module_name"` | Python code will see the class as being defined in this module. Defaults to `builtins`. | | `name = "python_name"` | Sets the name that Python sees this class as. Defaults to the name of the Rust struct. | -| `text_signature = "(arg1, arg2, ...)"` | 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. | +| `text_signature = "(arg1, arg2, ...)"` | 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]. | @@ -35,4 +36,5 @@ struct MyClass { } [params-4]: https://doc.rust-lang.org/std/rc/struct.Rc.html [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 \ No newline at end of file +[params-mapping]: https://pyo3.rs/latest/class/protocols.html#mapping--sequence-types +[params-sequence]: https://pyo3.rs/latest/class/protocols.html#mapping--sequence-types diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md index 6866ab6710d..a541b5e1b2a 100644 --- a/guide/src/class/protocols.md +++ b/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__() -> usize` - Implements the built-in function `len()` for the sequence. + Implements the built-in function `len()`. - `__contains__(, object) -> bool` diff --git a/guide/src/migration.md b/guide/src/migration.md index a353642c56a..84e11841b5a 100644 --- a/guide/src/migration.md +++ b/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. diff --git a/pyo3-macros-backend/src/attributes.rs b/pyo3-macros-backend/src/attributes.rs index 901927430fb..b4d45390dbb 100644 --- a/pyo3-macros-backend/src/attributes.rs +++ b/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); diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 816d329c487..9750ccb896c 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -65,6 +65,7 @@ pub struct PyClassPyO3Options { pub mapping: Option, pub module: Option, pub name: Option, + pub sequence: Option, pub subclass: Option, pub text_signature: Option, pub unsendable: Option, @@ -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 { 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 { 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 { - vec![ + fn impl_all(&self) -> Result { + 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 { 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; type BaseType = #base; @@ -1002,7 +1015,7 @@ impl<'a> PyClassImplsBuilder<'a> { } #inventory_class - } + }) } fn impl_freelist(&self) -> TokenStream { diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 01b1e722d74..3eebc674c1f 100644 --- a/src/impl_/pyclass.rs +++ b/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; diff --git a/src/pyclass.rs b/src/pyclass.rs index 02a3a5d0ad1..d2343ea7374 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -43,6 +43,7 @@ where .slot(ffi::Py_tp_dealloc, tp_dealloc:: 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::()) } { @@ -63,6 +64,7 @@ struct PyTypeBuilder { /// except for that it does and we have tests. cleanup: Vec, 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::()) } diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 25dddce6609..6e8395999b7 100644 --- a/tests/test_sequence.rs +++ b/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>, @@ -287,6 +287,10 @@ struct OptionList { #[pymethods] impl OptionList { + fn __len__(&self) -> usize { + self.items.len() + } + fn __getitem__(&self, idx: isize) -> PyResult> { 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::().is_err()); assert!(list.as_ref().downcast::().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() }; + }) +} diff --git a/tests/ui/invalid_pyclass_args.rs b/tests/ui/invalid_pyclass_args.rs index 04560a04963..c2ff187aadf 100644 --- a/tests/ui/invalid_pyclass_args.rs +++ b/tests/ui/invalid_pyclass_args.rs @@ -21,4 +21,7 @@ struct InvalidModule {} #[pyclass(weakrev)] struct InvalidArg {} +#[pyclass(mapping, sequence)] +struct CannotBeMappingAndSequence {} + fn main() {} diff --git a/tests/ui/invalid_pyclass_args.stderr b/tests/ui/invalid_pyclass_args.stderr index 77c0bc0e0af..c80ae034242 100644 --- a/tests/ui/invalid_pyclass_args.stderr +++ b/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 {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^