From 0b967d427a20122fef138a0daa49eb52946a6933 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 22 Jun 2024 00:33:34 +0100 Subject: [PATCH] use `ffi::MemberGef` for `#[pyo3(get)]` fields of `Py` (#4254) * use `ffi::MemberGef` for `#[pyo3(get)]` fields of `Py` * tidy up implementation * make it work on MSRV :( * fix docs and newsfragment * clippy * internal docs and coverage * review: mejrs --- newsfragments/4254.changed.md | 1 + pyo3-macros-backend/src/pyclass.rs | 14 +- pyo3-macros-backend/src/pyimpl.rs | 14 +- pyo3-macros-backend/src/pymethod.rs | 199 ++++++++--------- src/impl_/pyclass.rs | 293 +++++++++++++++++++++++++- src/impl_/pyclass/lazy_type_object.rs | 11 +- src/impl_/pymethods.rs | 3 + src/pycell/impl_.rs | 2 +- src/pyclass.rs | 12 +- src/pyclass/create_type_object.rs | 29 ++- tests/test_class_basics.rs | 3 - tests/test_class_conversion.rs | 4 - tests/test_getter_setter.rs | 22 ++ tests/test_methods.rs | 11 +- tests/test_no_imports.rs | 3 - tests/test_sequence.rs | 3 - tests/ui/invalid_property_args.rs | 6 + tests/ui/invalid_property_args.stderr | 18 ++ 18 files changed, 503 insertions(+), 145 deletions(-) create mode 100644 newsfragments/4254.changed.md diff --git a/newsfragments/4254.changed.md b/newsfragments/4254.changed.md new file mode 100644 index 00000000..e58e0345 --- /dev/null +++ b/newsfragments/4254.changed.md @@ -0,0 +1 @@ +Optimize code generated by `#[pyo3(get)]` on `#[pyclass]` fields. diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 01377767..07d9e32e 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1415,12 +1415,14 @@ pub fn gen_complex_enum_variant_attr( }; let method_def = quote! { - #pyo3_path::class::PyMethodDefType::ClassAttribute({ - #pyo3_path::class::PyClassAttributeDef::new( - #python_name, - #cls_type::#wrapper_ident - ) - }) + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::ClassAttribute({ + #pyo3_path::class::PyClassAttributeDef::new( + #python_name, + #cls_type::#wrapper_ident + ) + }) + ) }; MethodAndMethodDef { diff --git a/pyo3-macros-backend/src/pyimpl.rs b/pyo3-macros-backend/src/pyimpl.rs index dbb1fba8..6807f908 100644 --- a/pyo3-macros-backend/src/pyimpl.rs +++ b/pyo3-macros-backend/src/pyimpl.rs @@ -197,12 +197,14 @@ pub fn gen_py_const(cls: &syn::Type, spec: &ConstSpec<'_>, ctx: &Ctx) -> MethodA }; let method_def = quote! { - #pyo3_path::class::PyMethodDefType::ClassAttribute({ - #pyo3_path::class::PyClassAttributeDef::new( - #python_name, - #cls::#wrapper_ident - ) - }) + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::ClassAttribute({ + #pyo3_path::class::PyClassAttributeDef::new( + #python_name, + #cls::#wrapper_ident + ) + }) + ) }; MethodAndMethodDef { diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index c87a3124..7a9afa54 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -330,7 +330,9 @@ pub fn impl_py_method_def( }; let methoddef = spec.get_methoddef(quote! { #cls::#wrapper_ident }, doc, ctx); let method_def = quote! { - #pyo3_path::class::PyMethodDefType::#methoddef_type(#methoddef #add_flags) + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::#methoddef_type(#methoddef #add_flags) + ) }; Ok(MethodAndMethodDef { associated_method, @@ -511,12 +513,14 @@ fn impl_py_class_attribute( }; let method_def = quote! { - #pyo3_path::class::PyMethodDefType::ClassAttribute({ - #pyo3_path::class::PyClassAttributeDef::new( - #python_name, - #cls::#wrapper_ident - ) - }) + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::ClassAttribute({ + #pyo3_path::class::PyClassAttributeDef::new( + #python_name, + #cls::#wrapper_ident + ) + }) + ) }; Ok(MethodAndMethodDef { @@ -701,11 +705,13 @@ pub fn impl_py_setter_def( let method_def = quote! { #cfg_attrs - #pyo3_path::class::PyMethodDefType::Setter( - #pyo3_path::class::PySetterDef::new( - #python_name, - #cls::#wrapper_ident, - #doc + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::Setter( + #pyo3_path::class::PySetterDef::new( + #python_name, + #cls::#wrapper_ident, + #doc + ) ) ) }; @@ -750,61 +756,6 @@ pub fn impl_py_getter_def( let python_name = property_type.null_terminated_python_name(ctx)?; let doc = property_type.doc(ctx); - let mut holders = Holders::new(); - let body = match property_type { - PropertyType::Descriptor { - field_index, field, .. - } => { - let slf = SelfType::Receiver { - mutable: false, - span: Span::call_site(), - } - .receiver(cls, ExtractErrorMode::Raise, &mut holders, ctx); - let field_token = if let Some(ident) = &field.ident { - // named struct field - ident.to_token_stream() - } else { - // tuple struct field - syn::Index::from(field_index).to_token_stream() - }; - quotes::map_result_into_ptr( - quotes::ok_wrap( - quote! { - ::std::clone::Clone::clone(&(#slf.#field_token)) - }, - ctx, - ), - ctx, - ) - } - // Forward to `IntoPyCallbackOutput`, to handle `#[getter]`s returning results. - PropertyType::Function { - spec, self_type, .. - } => { - let call = impl_call_getter(cls, spec, self_type, &mut holders, ctx)?; - quote! { - #pyo3_path::callback::convert(py, #call) - } - } - }; - - let wrapper_ident = match property_type { - PropertyType::Descriptor { - field: syn::Field { - ident: Some(ident), .. - }, - .. - } => { - format_ident!("__pymethod_get_{}__", ident) - } - PropertyType::Descriptor { field_index, .. } => { - format_ident!("__pymethod_get_field_{}__", field_index) - } - PropertyType::Function { spec, .. } => { - format_ident!("__pymethod_get_{}__", spec.name) - } - }; - let mut cfg_attrs = TokenStream::new(); if let PropertyType::Descriptor { field, .. } = &property_type { for attr in field @@ -816,36 +767,96 @@ pub fn impl_py_getter_def( } } - let init_holders = holders.init_holders(ctx); - let check_gil_refs = holders.check_gil_refs(); - let associated_method = quote! { - #cfg_attrs - unsafe fn #wrapper_ident( - py: #pyo3_path::Python<'_>, - _slf: *mut #pyo3_path::ffi::PyObject - ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { - #init_holders - let result = #body; - #check_gil_refs - result + let mut holders = Holders::new(); + match property_type { + PropertyType::Descriptor { + field_index, field, .. + } => { + let ty = &field.ty; + let field = if let Some(ident) = &field.ident { + ident.to_token_stream() + } else { + syn::Index::from(field_index).to_token_stream() + }; + + // TODO: on MSRV 1.77+, we can use `::std::mem::offset_of!` here, and it should + // make it possible for the `MaybeRuntimePyMethodDef` to be a `Static` variant. + let method_def = quote_spanned! {ty.span()=> + #cfg_attrs + { + #[allow(unused_imports)] // might not be used if all probes are positve + use #pyo3_path::impl_::pyclass::Probe; + + struct Offset; + unsafe impl #pyo3_path::impl_::pyclass::OffsetCalculator<#cls, #ty> for Offset { + fn offset() -> usize { + #pyo3_path::impl_::pyclass::class_offset::<#cls>() + + #pyo3_path::impl_::pyclass::offset_of!(#cls, #field) + } + } + + const GENERATOR: #pyo3_path::impl_::pyclass::PyClassGetterGenerator::< + #cls, + #ty, + Offset, + { #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE }, + { #pyo3_path::impl_::pyclass::IsToPyObject::<#ty>::VALUE }, + > = unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::new() }; + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Runtime( + || GENERATOR.generate(#python_name, #doc) + ) + } + }; + + Ok(MethodAndMethodDef { + associated_method: quote! {}, + method_def, + }) } - }; + // Forward to `IntoPyCallbackOutput`, to handle `#[getter]`s returning results. + PropertyType::Function { + spec, self_type, .. + } => { + let wrapper_ident = format_ident!("__pymethod_get_{}__", spec.name); + let call = impl_call_getter(cls, spec, self_type, &mut holders, ctx)?; + let body = quote! { + #pyo3_path::callback::convert(py, #call) + }; - let method_def = quote! { - #cfg_attrs - #pyo3_path::class::PyMethodDefType::Getter( - #pyo3_path::class::PyGetterDef::new( - #python_name, - #cls::#wrapper_ident, - #doc - ) - ) - }; + let init_holders = holders.init_holders(ctx); + let check_gil_refs = holders.check_gil_refs(); + let associated_method = quote! { + #cfg_attrs + unsafe fn #wrapper_ident( + py: #pyo3_path::Python<'_>, + _slf: *mut #pyo3_path::ffi::PyObject + ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + #init_holders + let result = #body; + #check_gil_refs + result + } + }; - Ok(MethodAndMethodDef { - associated_method, - method_def, - }) + let method_def = quote! { + #cfg_attrs + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::Getter( + #pyo3_path::class::PyGetterDef::new( + #python_name, + #cls::#wrapper_ident, + #doc + ) + ) + ) + }; + + Ok(MethodAndMethodDef { + associated_method, + method_def, + }) + } + } } /// Split an argument of pyo3::Python from the front of the arg list, if present diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index acfce37e..391e96f5 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -6,9 +6,9 @@ use crate::{ impl_::freelist::FreeList, impl_::pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectLayout}, pyclass_init::PyObjectInit, - types::any::PyAnyMethods, - types::PyBool, - Borrowed, Py, PyAny, PyClass, PyErr, PyMethodDefType, PyResult, PyTypeInfo, Python, + types::{any::PyAnyMethods, PyBool}, + Borrowed, IntoPy, Py, PyAny, PyClass, PyErr, PyMethodDefType, PyResult, PyTypeInfo, Python, + ToPyObject, }; use std::{ borrow::Cow, @@ -131,8 +131,15 @@ impl Clone for PyClassImplCollector { impl Copy for PyClassImplCollector {} +pub enum MaybeRuntimePyMethodDef { + /// Used in cases where const functionality is not sufficient to define the method + /// purely at compile time. + Runtime(fn() -> PyMethodDefType), + Static(PyMethodDefType), +} + pub struct PyClassItems { - pub methods: &'static [PyMethodDefType], + pub methods: &'static [MaybeRuntimePyMethodDef], pub slots: &'static [ffi::PyType_Slot], } @@ -890,7 +897,7 @@ macro_rules! generate_pyclass_richcompare_slot { } pub use generate_pyclass_richcompare_slot; -use super::pycell::PyClassObject; +use super::{pycell::PyClassObject, pymethods::BoundRef}; /// Implements a freelist. /// @@ -1171,3 +1178,279 @@ pub(crate) unsafe extern "C" fn assign_sequence_item_from_mapping( ffi::Py_DECREF(index); result } + +/// Helper trait to locate field within a `#[pyclass]` for a `#[pyo3(get)]`. +/// +/// Below MSRV 1.77 we can't use `std::mem::offset_of!`, and the replacement in +/// `memoffset::offset_of` doesn't work in const contexts for types containing `UnsafeCell`. +/// +/// # Safety +/// +/// The trait is unsafe to implement because producing an incorrect offset will lead to UB. +pub unsafe trait OffsetCalculator { + /// Offset to the field within a `PyClassObject`, in bytes. + fn offset() -> usize; +} + +// Used in generated implementations of OffsetCalculator +pub fn class_offset() -> usize { + offset_of!(PyClassObject, contents) +} + +// Used in generated implementations of OffsetCalculator +pub use memoffset::offset_of; + +/// Type which uses specialization on impl blocks to determine how to read a field from a Rust pyclass +/// as part of a `#[pyo3(get)]` annotation. +pub struct PyClassGetterGenerator< + // structural information about the field: class type, field type, where the field is within the + // class struct + ClassT: PyClass, + FieldT, + Offset: OffsetCalculator, // on Rust 1.77+ this could be a const OFFSET: usize + // additional metadata about the field which is used to switch between different implementations + // at compile time + const IS_PY_T: bool, + const IMPLEMENTS_TOPYOBJECT: bool, +>(PhantomData<(ClassT, FieldT, Offset)>); + +impl< + ClassT: PyClass, + FieldT, + Offset: OffsetCalculator, + const IS_PY_T: bool, + const IMPLEMENTS_TOPYOBJECT: bool, + > PyClassGetterGenerator +{ + /// Safety: constructing this type requires that there exists a value of type FieldT + /// at the calculated offset within the type ClassT. + pub const unsafe fn new() -> Self { + Self(PhantomData) + } +} + +impl< + ClassT: PyClass, + U, + Offset: OffsetCalculator>, + const IMPLEMENTS_TOPYOBJECT: bool, + > PyClassGetterGenerator, Offset, true, IMPLEMENTS_TOPYOBJECT> +{ + /// `Py` fields have a potential optimization to use Python's "struct members" to read + /// the field directly from the struct, rather than using a getter function. + /// + /// This is the most efficient operation the Python interpreter could possibly do to + /// read a field, but it's only possible for us to allow this for frozen classes. + pub fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { + use crate::pyclass::boolean_struct::private::Boolean; + if ClassT::Frozen::VALUE { + PyMethodDefType::StructMember(ffi::PyMemberDef { + name: name.as_ptr(), + type_code: ffi::Py_T_OBJECT_EX, + offset: Offset::offset() as ffi::Py_ssize_t, + flags: ffi::Py_READONLY, + doc: doc.as_ptr(), + }) + } else { + PyMethodDefType::Getter(crate::PyGetterDef { + name, + meth: pyo3_get_value_topyobject::, Offset>, + doc, + }) + } + } +} + +/// Field is not `Py`; try to use `ToPyObject` to avoid potentially expensive clones of containers like `Vec` +impl> + PyClassGetterGenerator +{ + pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { + PyMethodDefType::Getter(crate::PyGetterDef { + name, + meth: pyo3_get_value_topyobject::, + doc, + }) + } +} + +#[cfg_attr( + diagnostic_namespace, + diagnostic::on_unimplemented( + message = "`{Self}` cannot be converted to a Python object", + label = "required by `#[pyo3(get)]` to create a readable property from a field of type `{Self}`", + note = "implement `ToPyObject` or `IntoPy + Clone` for `{Self}` to define the conversion", + ) +)] +pub trait PyO3GetField: IntoPy> + Clone {} +impl> + Clone> PyO3GetField for T {} + +/// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22. +impl> + PyClassGetterGenerator +{ + pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType + // The bound goes here rather than on the block so that this impl is always available + // if no specialization is used instead + where + FieldT: PyO3GetField, + { + PyMethodDefType::Getter(crate::PyGetterDef { + name, + meth: pyo3_get_value::, + doc, + }) + } +} + +/// Trait used to combine with zero-sized types to calculate at compile time +/// some property of a type. +/// +/// The trick uses the fact that an associated constant has higher priority +/// than a trait constant, so we can use the trait to define the false case. +/// +/// The true case is defined in the zero-sized type's impl block, which is +/// gated on some property like trait bound or only being implemented +/// for fixed concrete types. +pub trait Probe { + const VALUE: bool = false; +} + +macro_rules! probe { + ($name:ident) => { + pub struct $name(PhantomData); + impl Probe for $name {} + }; +} + +probe!(IsPyT); + +impl IsPyT> { + pub const VALUE: bool = true; +} + +probe!(IsToPyObject); + +impl IsToPyObject { + pub const VALUE: bool = true; +} + +fn pyo3_get_value_topyobject< + ClassT: PyClass, + FieldT: ToPyObject, + Offset: OffsetCalculator, +>( + py: Python<'_>, + obj: *mut ffi::PyObject, +) -> PyResult<*mut ffi::PyObject> { + // Check for mutable aliasing + let _holder = unsafe { + BoundRef::ref_from_ptr(py, &obj) + .downcast_unchecked::() + .try_borrow()? + }; + + let value = unsafe { obj.cast::().add(Offset::offset()).cast::() }; + + // SAFETY: Offset is known to describe the location of the value, and + // _holder is preventing mutable aliasing + Ok((unsafe { &*value }).to_object(py).into_ptr()) +} + +fn pyo3_get_value< + ClassT: PyClass, + FieldT: IntoPy> + Clone, + Offset: OffsetCalculator, +>( + py: Python<'_>, + obj: *mut ffi::PyObject, +) -> PyResult<*mut ffi::PyObject> { + // Check for mutable aliasing + let _holder = unsafe { + BoundRef::ref_from_ptr(py, &obj) + .downcast_unchecked::() + .try_borrow()? + }; + + let value = unsafe { obj.cast::().add(Offset::offset()).cast::() }; + + // SAFETY: Offset is known to describe the location of the value, and + // _holder is preventing mutable aliasing + Ok((unsafe { &*value }).clone().into_py(py).into_ptr()) +} + +#[cfg(test)] +#[cfg(feature = "macros")] +mod tests { + use super::*; + + #[test] + fn get_py_for_frozen_class() { + #[crate::pyclass(crate = "crate", frozen)] + struct FrozenClass { + #[pyo3(get)] + value: Py, + } + + let mut methods = Vec::new(); + let mut slots = Vec::new(); + + for items in FrozenClass::items_iter() { + methods.extend(items.methods.iter().map(|m| match m { + MaybeRuntimePyMethodDef::Static(m) => m.clone(), + MaybeRuntimePyMethodDef::Runtime(r) => r(), + })); + slots.extend_from_slice(items.slots); + } + + assert_eq!(methods.len(), 1); + assert!(slots.is_empty()); + + match methods.first() { + Some(PyMethodDefType::StructMember(member)) => { + assert_eq!(unsafe { CStr::from_ptr(member.name) }, ffi::c_str!("value")); + assert_eq!(member.type_code, ffi::Py_T_OBJECT_EX); + assert_eq!( + member.offset, + (memoffset::offset_of!(PyClassObject, contents) + + memoffset::offset_of!(FrozenClass, value)) + as ffi::Py_ssize_t + ); + assert_eq!(member.flags, ffi::Py_READONLY); + } + _ => panic!("Expected a StructMember"), + } + } + + #[test] + fn get_py_for_non_frozen_class() { + #[crate::pyclass(crate = "crate")] + struct FrozenClass { + #[pyo3(get)] + value: Py, + } + + let mut methods = Vec::new(); + let mut slots = Vec::new(); + + for items in FrozenClass::items_iter() { + methods.extend(items.methods.iter().map(|m| match m { + MaybeRuntimePyMethodDef::Static(m) => m.clone(), + MaybeRuntimePyMethodDef::Runtime(r) => r(), + })); + slots.extend_from_slice(items.slots); + } + + assert_eq!(methods.len(), 1); + assert!(slots.is_empty()); + + match methods.first() { + Some(PyMethodDefType::Getter(getter)) => { + assert_eq!(getter.name, ffi::c_str!("value")); + assert_eq!(getter.doc, ffi::c_str!("")); + // tests for the function pointer are in test_getter_setter.py + } + _ => panic!("Expected a StructMember"), + } + } +} diff --git a/src/impl_/pyclass/lazy_type_object.rs b/src/impl_/pyclass/lazy_type_object.rs index 7afaec8a..08a5f17d 100644 --- a/src/impl_/pyclass/lazy_type_object.rs +++ b/src/impl_/pyclass/lazy_type_object.rs @@ -8,6 +8,7 @@ use std::{ use crate::{ exceptions::PyRuntimeError, ffi, + impl_::pyclass::MaybeRuntimePyMethodDef, pyclass::{create_type_object, PyClassTypeObject}, sync::{GILOnceCell, GILProtected}, types::PyType, @@ -149,7 +150,15 @@ impl LazyTypeObjectInner { let mut items = vec![]; for class_items in items_iter { for def in class_items.methods { - if let PyMethodDefType::ClassAttribute(attr) = def { + let built_method; + let method = match def { + MaybeRuntimePyMethodDef::Runtime(builder) => { + built_method = builder(); + &built_method + } + MaybeRuntimePyMethodDef::Static(method) => method, + }; + if let PyMethodDefType::ClassAttribute(attr) = method { match (attr.meth)(py) { Ok(val) => items.push((attr.name, val)), Err(err) => { diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 2b63d1e4..60b655e5 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -52,6 +52,7 @@ impl IPowModulo { /// `PyMethodDefType` represents different types of Python callable objects. /// It is used by the `#[pymethods]` attribute. +#[cfg_attr(test, derive(Clone))] pub enum PyMethodDefType { /// Represents class method Class(PyMethodDef), @@ -65,6 +66,8 @@ pub enum PyMethodDefType { Getter(PyGetterDef), /// Represents setter descriptor, used by `#[setter]` Setter(PySetterDef), + /// Represents a struct member + StructMember(ffi::PyMemberDef), } #[derive(Copy, Clone, Debug)] diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs index 5404464c..efc057e7 100644 --- a/src/pycell/impl_.rs +++ b/src/pycell/impl_.rs @@ -253,7 +253,7 @@ where #[repr(C)] pub struct PyClassObject { pub(crate) ob_base: ::LayoutAsBase, - contents: PyClassObjectContents, + pub(crate) contents: PyClassObjectContents, } #[repr(C)] diff --git a/src/pyclass.rs b/src/pyclass.rs index 162ae0d3..29cd1251 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -213,10 +213,16 @@ pub mod boolean_struct { use super::*; /// A way to "seal" the boolean traits. - pub trait Boolean {} + pub trait Boolean { + const VALUE: bool; + } - impl Boolean for True {} - impl Boolean for False {} + impl Boolean for True { + const VALUE: bool = true; + } + impl Boolean for False { + const VALUE: bool = false; + } } pub struct True(()); diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index fd1d2b34..00dc7ee3 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -5,7 +5,7 @@ use crate::{ pycell::PyClassObject, pyclass::{ assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc, - tp_dealloc_with_gc, PyClassItemsIter, + tp_dealloc_with_gc, MaybeRuntimePyMethodDef, PyClassItemsIter, }, pymethods::{Getter, Setter}, trampoline::trampoline, @@ -52,6 +52,7 @@ where PyTypeBuilder { slots: Vec::new(), method_defs: Vec::new(), + member_defs: Vec::new(), getset_builders: HashMap::new(), cleanup: Vec::new(), tp_base: base, @@ -102,6 +103,7 @@ type PyTypeBuilderCleanup = Box; struct PyTypeBuilder { slots: Vec, method_defs: Vec, + member_defs: Vec, getset_builders: HashMap<&'static CStr, GetSetDefBuilder>, /// Used to patch the type objects for the things there's no /// PyType_FromSpec API for... there's no reason this should work, @@ -187,6 +189,7 @@ impl PyTypeBuilder { | PyMethodDefType::Static(def) => self.method_defs.push(def.as_method_def()), // These class attributes are added after the type gets created by LazyStaticType PyMethodDefType::ClassAttribute(_) => {} + PyMethodDefType::StructMember(def) => self.member_defs.push(*def), } } @@ -195,6 +198,10 @@ impl PyTypeBuilder { // Safety: Py_tp_methods expects a raw vec of PyMethodDef unsafe { self.push_raw_vec_slot(ffi::Py_tp_methods, method_defs) }; + let member_defs = std::mem::take(&mut self.member_defs); + // Safety: Py_tp_members expects a raw vec of PyMemberDef + unsafe { self.push_raw_vec_slot(ffi::Py_tp_members, member_defs) }; + let mut getset_destructors = Vec::with_capacity(self.getset_builders.len()); #[allow(unused_mut)] @@ -261,7 +268,7 @@ impl PyTypeBuilder { }); } - // Safety: Py_tp_members expects a raw vec of PyGetSetDef + // Safety: Py_tp_getset expects a raw vec of PyGetSetDef unsafe { self.push_raw_vec_slot(ffi::Py_tp_getset, property_defs) }; // If mapping methods implemented, define sequence methods get implemented too. @@ -310,6 +317,14 @@ impl PyTypeBuilder { self.push_slot(slot.slot, slot.pfunc); } for method in items.methods { + let built_method; + let method = match method { + MaybeRuntimePyMethodDef::Runtime(builder) => { + built_method = builder(); + &built_method + } + MaybeRuntimePyMethodDef::Static(method) => method, + }; self.pymethod_def(method); } } @@ -360,23 +375,19 @@ impl PyTypeBuilder { } } - let mut members = Vec::new(); - // __dict__ support if let Some(dict_offset) = dict_offset { - members.push(offset_def(ffi::c_str!("__dictoffset__"), dict_offset)); + self.member_defs + .push(offset_def(ffi::c_str!("__dictoffset__"), dict_offset)); } // weakref support if let Some(weaklist_offset) = weaklist_offset { - members.push(offset_def( + self.member_defs.push(offset_def( ffi::c_str!("__weaklistoffset__"), weaklist_offset, )); } - - // Safety: Py_tp_members expects a raw vec of PyMemberDef - unsafe { self.push_raw_vec_slot(ffi::Py_tp_members, members) }; } // Setting buffer protocols, tp_dictoffset and tp_weaklistoffset via slots doesn't work until diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index 19547cff..c1a4b923 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -202,7 +202,6 @@ fn empty_class_in_module() { }); } -#[cfg(feature = "py-clone")] #[pyclass] struct ClassWithObjectField { // It used to be that PyObject was not supported with (get, set) @@ -211,7 +210,6 @@ struct ClassWithObjectField { value: PyObject, } -#[cfg(feature = "py-clone")] #[pymethods] impl ClassWithObjectField { #[new] @@ -220,7 +218,6 @@ impl ClassWithObjectField { } } -#[cfg(feature = "py-clone")] #[test] fn class_with_object_field() { Python::with_gil(|py| { diff --git a/tests/test_class_conversion.rs b/tests/test_class_conversion.rs index a46132b9..ede8928f 100644 --- a/tests/test_class_conversion.rs +++ b/tests/test_class_conversion.rs @@ -54,14 +54,12 @@ impl SubClass { } } -#[cfg(feature = "py-clone")] #[pyclass] struct PolymorphicContainer { #[pyo3(get, set)] inner: Py, } -#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_stores_base_class() { Python::with_gil(|py| { @@ -78,7 +76,6 @@ fn test_polymorphic_container_stores_base_class() { }); } -#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_stores_sub_class() { Python::with_gil(|py| { @@ -106,7 +103,6 @@ fn test_polymorphic_container_stores_sub_class() { }); } -#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_does_not_accept_other_types() { Python::with_gil(|py| { diff --git a/tests/test_getter_setter.rs b/tests/test_getter_setter.rs index b0b15d78..e2b8307f 100644 --- a/tests/test_getter_setter.rs +++ b/tests/test_getter_setter.rs @@ -258,3 +258,25 @@ fn borrowed_value_with_lifetime_of_self() { py_run!(py, inst, "assert inst.value == 'value'"); }); } + +#[test] +fn frozen_py_field_get() { + #[pyclass(frozen)] + struct FrozenPyField { + #[pyo3(get)] + value: Py, + } + + Python::with_gil(|py| { + let inst = Py::new( + py, + FrozenPyField { + value: "value".into_py(py), + }, + ) + .unwrap() + .to_object(py); + + py_run!(py, inst, "assert inst.value == 'value'"); + }); +} diff --git a/tests/test_methods.rs b/tests/test_methods.rs index 615e2dba..ddb3c01b 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -874,7 +874,6 @@ fn test_from_sequence() { }); } -#[cfg(feature = "py-clone")] #[pyclass] struct r#RawIdents { #[pyo3(get, set)] @@ -883,7 +882,6 @@ struct r#RawIdents { r#subsubtype: PyObject, } -#[cfg(feature = "py-clone")] #[pymethods] impl r#RawIdents { #[new] @@ -901,8 +899,8 @@ impl r#RawIdents { } #[getter(r#subtype)] - pub fn r#get_subtype(&self) -> PyObject { - self.r#subtype.clone() + pub fn r#get_subtype(&self, py: Python<'_>) -> PyObject { + self.r#subtype.clone_ref(py) } #[setter(r#subtype)] @@ -911,8 +909,8 @@ impl r#RawIdents { } #[getter] - pub fn r#get_subsubtype(&self) -> PyObject { - self.r#subsubtype.clone() + pub fn r#get_subsubtype(&self, py: Python<'_>) -> PyObject { + self.r#subsubtype.clone_ref(py) } #[setter] @@ -948,7 +946,6 @@ impl r#RawIdents { } } -#[cfg(feature = "py-clone")] #[test] fn test_raw_idents() { Python::with_gil(|py| { diff --git a/tests/test_no_imports.rs b/tests/test_no_imports.rs index 89d54f4e..3509a11f 100644 --- a/tests/test_no_imports.rs +++ b/tests/test_no_imports.rs @@ -143,14 +143,12 @@ fn test_basic() { }); } -#[cfg(feature = "py-clone")] #[pyo3::pyclass] struct NewClassMethod { #[pyo3(get)] cls: pyo3::PyObject, } -#[cfg(feature = "py-clone")] #[pyo3::pymethods] impl NewClassMethod { #[new] @@ -162,7 +160,6 @@ impl NewClassMethod { } } -#[cfg(feature = "py-clone")] #[test] fn test_new_class_method() { pyo3::Python::with_gil(|py| { diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 8c29205c..8b1e3114 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -248,14 +248,12 @@ fn test_inplace_repeat() { // Check that #[pyo3(get, set)] works correctly for Vec -#[cfg(feature = "py-clone")] #[pyclass] struct GenericList { #[pyo3(get, set)] items: Vec, } -#[cfg(feature = "py-clone")] #[test] fn test_generic_list_get() { Python::with_gil(|py| { @@ -268,7 +266,6 @@ fn test_generic_list_get() { }); } -#[cfg(feature = "py-clone")] #[test] fn test_generic_list_set() { Python::with_gil(|py| { diff --git a/tests/ui/invalid_property_args.rs b/tests/ui/invalid_property_args.rs index b5eba27e..f35367df 100644 --- a/tests/ui/invalid_property_args.rs +++ b/tests/ui/invalid_property_args.rs @@ -39,4 +39,10 @@ struct MultipleName(#[pyo3(name = "foo", name = "bar")] i32); #[pyclass] struct NameWithoutGetSet(#[pyo3(name = "value")] i32); +#[pyclass] +struct InvalidGetterType { + #[pyo3(get)] + value: ::std::marker::PhantomData, +} + fn main() {} diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr index dea2e3fb..0ee00cc6 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -45,3 +45,21 @@ error: `name` is useless without `get` or `set` | 40 | struct NameWithoutGetSet(#[pyo3(name = "value")] i32); | ^^^^^^^^^^^^^^ + +error[E0277]: `PhantomData` cannot be converted to a Python object + --> tests/ui/invalid_property_args.rs:45:12 + | +45 | value: ::std::marker::PhantomData, + | ^ required by `#[pyo3(get)]` to create a readable property from a field of type `PhantomData` + | + = help: the trait `IntoPy>` is not implemented for `PhantomData`, which is required by `PhantomData: PyO3GetField` + = note: implement `ToPyObject` or `IntoPy + Clone` for `PhantomData` to define the conversion + = note: required for `PhantomData` to implement `PyO3GetField` +note: required by a bound in `PyClassGetterGenerator::::generate` + --> src/impl_/pyclass.rs + | + | pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType + | -------- required by a bound in this associated function +... + | FieldT: PyO3GetField, + | ^^^^^^^^^^^^ required by this bound in `PyClassGetterGenerator::::generate`