Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pyclass: add sequence option to implement sq_length #2567

Merged
merged 1 commit into from
Aug 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 4 additions & 2 deletions guide/pyclass_parameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]. |

Expand All @@ -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
[params-mapping]: https://pyo3.rs/latest/class/protocols.html#mapping--sequence-types
[params-sequence]: https://pyo3.rs/latest/class/protocols.html#mapping--sequence-types
4 changes: 3 additions & 1 deletion guide/src/class/protocols.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
4 changes: 2 additions & 2 deletions guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
1 change: 1 addition & 0 deletions pyo3-macros-backend/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
41 changes: 27 additions & 14 deletions pyo3-macros-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -82,6 +83,7 @@ enum PyClassPyO3Option {
Mapping(kw::mapping),
Module(ModuleAttribute),
Name(NameAttribute),
Sequence(kw::sequence),
Subclass(kw::subclass),
TextSignature(TextSignatureAttribute),
Unsendable(kw::unsendable),
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -315,7 +320,7 @@ fn impl_class(
vec![],
)
.doc(doc)
.impl_all();
.impl_all()?;

Ok(quote! {
const _: () = {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand All @@ -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;
Expand Down Expand Up @@ -569,9 +574,9 @@ fn impl_enum_class(
default_slots,
)
.doc(doc)
.impl_all();
.impl_all()?;

quote! {
Ok(quote! {
const _: () = {
use #krate as _pyo3;

Expand All @@ -587,7 +592,7 @@ fn impl_enum_class(
#default_richcmp
}
};
}
})
}

fn generate_default_protocol_slot(
Expand Down Expand Up @@ -752,16 +757,17 @@ impl<'a> PyClassImplsBuilder<'a> {
}
}

fn impl_all(&self) -> TokenStream {
vec![
fn impl_all(&self) -> Result<TokenStream> {
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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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! {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1002,7 +1015,7 @@ impl<'a> PyClassImplsBuilder<'a> {
}

#inventory_class
}
})
}

fn impl_freelist(&self) -> TokenStream {
Expand Down
3 changes: 3 additions & 0 deletions src/impl_/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand Down
16 changes: 16 additions & 0 deletions src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>())
} {
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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>()) }
Expand Down
27 changes: 25 additions & 2 deletions tests/test_sequence.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -279,14 +279,18 @@ fn test_generic_list_set() {
});
}

#[pyclass]
#[pyclass(sequence)]
struct OptionList {
#[pyo3(get, set)]
items: Vec<Option<i64>>,
}

#[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),
Expand Down Expand Up @@ -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() };
})
}
3 changes: 3 additions & 0 deletions tests/ui/invalid_pyclass_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,7 @@ struct InvalidModule {}
#[pyclass(weakrev)]
struct InvalidArg {}

#[pyclass(mapping, sequence)]
struct CannotBeMappingAndSequence {}

fn main() {}
10 changes: 8 additions & 2 deletions tests/ui/invalid_pyclass_args.stderr
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -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 {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^