From dda52e28ba2862d58023d15cd584691e5a26aa83 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 15 Jun 2024 11:13:30 +0100 Subject: [PATCH 1/7] use `ffi::MemberGef` for `#[pyo3(get)]` fields of `Py` --- pyo3-macros-backend/src/pymethod.rs | 147 +++++++++++++--------------- src/impl_/pyclass.rs | 144 +++++++++++++++++++++++++-- src/impl_/pymethods.rs | 2 + src/pycell/impl_.rs | 2 +- src/pyclass.rs | 12 ++- src/pyclass/create_type_object.rs | 20 ++-- tests/test_class_basics.rs | 3 - tests/test_class_conversion.rs | 4 - tests/test_methods.rs | 3 - tests/test_no_imports.rs | 3 - tests/test_sequence.rs | 3 - 11 files changed, 232 insertions(+), 111 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 013b15010bf..cebd7bb7be5 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -749,102 +749,95 @@ pub fn impl_py_getter_def( let python_name = property_type.null_terminated_python_name()?; let doc = property_type.doc(); + let mut cfg_attrs = TokenStream::new(); + if let PropertyType::Descriptor { field, .. } = &property_type { + for attr in field + .attrs + .iter() + .filter(|attr| attr.path().is_ident("cfg")) + { + attr.to_tokens(&mut cfg_attrs); + } + } + let mut holders = Holders::new(); - let body = match property_type { + 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 + let ty = &field.ty; + let field = if let Some(ident) = &field.ident { 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, - ) + + let method_def = quote! { + #cfg_attrs + { + const OFFSET: usize = ::std::mem::offset_of!(#cls, #field); + unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::< + #cls, + #ty, + OFFSET, + { + use #pyo3_path::impl_::pyclass::Tester; + #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE + } + >::new() }.generate( + unsafe { ::std::ffi::CStr::from_bytes_with_nul_unchecked(#python_name.as_bytes()) }, + unsafe { ::std::ffi::CStr::from_bytes_with_nul_unchecked(#doc.as_bytes()) }, + ) + } + }; + + 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)?; - quote! { + let body = 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 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 cfg_attrs = TokenStream::new(); - if let PropertyType::Descriptor { field, .. } = &property_type { - for attr in field - .attrs - .iter() - .filter(|attr| attr.path().is_ident("cfg")) - { - attr.to_tokens(&mut cfg_attrs); - } - } + 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::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 84c00acdd74..ca69ed53eab 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -3,13 +3,15 @@ use crate::PyNativeType; use crate::{ exceptions::{PyAttributeError, PyNotImplementedError, PyRuntimeError, PyValueError}, ffi, - impl_::freelist::FreeList, - impl_::pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectLayout}, + impl_::{ + freelist::FreeList, + pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectLayout}, + }, internal_tricks::extract_c_string, 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, @@ -891,7 +893,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. /// @@ -1172,3 +1174,133 @@ pub(crate) unsafe extern "C" fn assign_sequence_item_from_mapping( ffi::Py_DECREF(index); result } + +/// Type which uses deref specialization to determine how to read a value from a Rust pyclass +/// as part of a `#[pyo3(get)]` annotation. +pub struct PyClassGetterGenerator( + PhantomData, + PhantomData, +); + +impl + PyClassGetterGenerator +{ + /// Safety: constructing this type requires that there exists a value of type X + /// at offset OFFSET within the type T. + pub const unsafe fn new() -> Self { + Self(PhantomData, PhantomData) + } +} + +impl PyClassGetterGenerator, OFFSET, true> { + /// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22. + pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { + use crate::pyclass::boolean_struct::private::Boolean; + if T::Frozen::VALUE { + PyMethodDefType::StructMember(ffi::PyMemberDef { + name: name.as_ptr(), + type_code: ffi::Py_T_OBJECT_EX, + offset: (std::mem::offset_of!(PyClassObject::, contents) + OFFSET) + as ffi::Py_ssize_t, + flags: ffi::Py_READONLY, + doc: doc.as_ptr(), + }) + } else { + PyMethodDefType::Getter(crate::PyGetterDef { + // TODO: store &CStr in PyGetterDef etc + name: unsafe { std::str::from_utf8_unchecked(name.to_bytes_with_nul()) }, + meth: pyo3_get_py_t::, OFFSET>, + doc: unsafe { std::str::from_utf8_unchecked(doc.to_bytes_with_nul()) }, + }) + } + } +} + +impl> + Clone, const OFFSET: usize> + PyClassGetterGenerator +{ + /// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22. + pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { + PyMethodDefType::Getter(crate::PyGetterDef { + // TODO: store &CStr in PyGetterDef etc + name: unsafe { std::str::from_utf8_unchecked(name.to_bytes_with_nul()) }, + meth: pyo3_get_value::, + doc: unsafe { std::str::from_utf8_unchecked(doc.to_bytes_with_nul()) }, + }) + } +} + +pub trait Tester { + const VALUE: bool = false; +} + +macro_rules! tester { + ($name:ident) => { + pub struct $name(PhantomData); + impl Tester for $name {} + }; +} + +tester!(IsPyT); + +impl IsPyT> { + pub const VALUE: bool = true; +} + +macro_rules! trait_tester { + ($name:ident, $($trait:tt)+) => { + tester!($name); + + impl $name { + pub const VALUE: bool = true; + } + } +} + +trait_tester!(IsIntoPyAndClone, IntoPy> + Clone); +trait_tester!(IsToPyObject, ToPyObject); + +fn pyo3_get_py_t( + 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::() + .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) + .cast::>() + }; + + // SAFETY: OFFSET is known to describe the location of the value, and + // _holder is preventing mutable aliasing + Ok((unsafe { &*value }).clone_ref(py).into_py(py).into_ptr()) +} + +fn pyo3_get_value> + Clone, const OFFSET: usize>( + py: Python<'_>, + obj: *mut ffi::PyObject, +) -> PyResult<*mut ffi::PyObject> { + assert!(IsIntoPyAndClone::::VALUE); + // Check for mutable aliasing + let _holder = unsafe { + BoundRef::ref_from_ptr(py, &obj) + .downcast_unchecked::() + .try_borrow()? + }; + + let value = unsafe { + obj.cast::() + .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) + .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()) +} diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 44b2af25650..f8bf87965ab 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -67,6 +67,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 5404464caba..efc057e74f7 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 162ae0d3119..29cd1251974 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 262d1e8ffc7..dff3560806b 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -55,6 +55,7 @@ where PyTypeBuilder { slots: Vec::new(), method_defs: Vec::new(), + member_defs: Vec::new(), getset_builders: HashMap::new(), cleanup: Vec::new(), tp_base: base, @@ -105,6 +106,7 @@ type PyTypeBuilderCleanup = Box; struct PyTypeBuilder { slots: Vec, method_defs: Vec, + member_defs: Vec, getset_builders: HashMap<&'static str, GetSetDefBuilder>, /// Used to patch the type objects for the things there's no /// PyType_FromSpec API for... there's no reason this should work, @@ -197,6 +199,7 @@ impl PyTypeBuilder { } // These class attributes are added after the type gets created by LazyStaticType PyMethodDefType::ClassAttribute(_) => {} + PyMethodDefType::StructMember(def) => self.member_defs.push(*def), } } @@ -205,6 +208,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)] @@ -231,7 +238,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. @@ -333,20 +340,17 @@ impl PyTypeBuilder { } } - let mut members = Vec::new(); - // __dict__ support if let Some(dict_offset) = dict_offset { - members.push(offset_def("__dictoffset__\0", dict_offset)); + self.member_defs + .push(offset_def("__dictoffset__\0", dict_offset)); } // weakref support if let Some(weaklist_offset) = weaklist_offset { - members.push(offset_def("__weaklistoffset__\0", weaklist_offset)); + self.member_defs + .push(offset_def("__weaklistoffset__\0", 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 325b3d52c3d..38088fde18f 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 a46132b9586..ede8928f865 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_methods.rs b/tests/test_methods.rs index 615e2dba0af..37f3b2d8bd6 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] @@ -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 89d54f4e057..3509a11f4be 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 8c29205cc18..8b1e3114797 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| { From 9ea2842081053864a32cecee7ebc059dfd5981ef Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 18 Jun 2024 22:56:08 +0100 Subject: [PATCH 2/7] tidy up implementation --- pyo3-macros-backend/src/pymethod.rs | 14 +-- src/impl_/pyclass.rs | 124 ++++++++++++++++++-------- tests/test_methods.rs | 8 +- tests/ui/invalid_property_args.rs | 6 ++ tests/ui/invalid_property_args.stderr | 19 ++++ 5 files changed, 123 insertions(+), 48 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index e22bdebc76c..06db0f8d410 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -772,19 +772,19 @@ pub fn impl_py_getter_def( syn::Index::from(field_index).to_token_stream() }; - let method_def = quote! { + let method_def = quote_spanned! {ty.span()=> #cfg_attrs { + use #pyo3_path::impl_::pyclass::Tester; const OFFSET: usize = ::std::mem::offset_of!(#cls, #field); - unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::< + const GENERATOR: #pyo3_path::impl_::pyclass::PyClassGetterGenerator::< #cls, #ty, OFFSET, - { - use #pyo3_path::impl_::pyclass::Tester; - #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE - } - >::new() }.generate(#python_name, #doc) + { #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE }, + { #pyo3_path::impl_::pyclass::IsToPyObject::<#ty>::VALUE }, + > = unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::new() }; + GENERATOR.generate(#python_name, #doc) } }; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 57ee96b5f65..3c682f28a72 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1172,15 +1172,27 @@ pub(crate) unsafe extern "C" fn assign_sequence_item_from_mapping( result } -/// Type which uses deref specialization to determine how to read a value from a Rust pyclass +/// 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( - PhantomData, - PhantomData, -); - -impl - PyClassGetterGenerator +pub struct PyClassGetterGenerator< + // structural information about the field: class type, field type, where the field is within the + // class struct + ClassT: PyClass, + FieldT, + 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, PhantomData); + +impl< + ClassT: PyClass, + FieldT, + const OFFSET: usize, + const IS_PY_T: bool, + const IMPLEMENTS_TOPYOBJECT: bool, + > PyClassGetterGenerator { /// Safety: constructing this type requires that there exists a value of type X /// at offset OFFSET within the type T. @@ -1189,15 +1201,21 @@ impl } } -impl PyClassGetterGenerator, OFFSET, true> { - /// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22. +impl + 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 const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { use crate::pyclass::boolean_struct::private::Boolean; - if T::Frozen::VALUE { + if ClassT::Frozen::VALUE { PyMethodDefType::StructMember(ffi::PyMemberDef { name: name.as_ptr(), type_code: ffi::Py_T_OBJECT_EX, - offset: (std::mem::offset_of!(PyClassObject::, contents) + OFFSET) + offset: (std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as ffi::Py_ssize_t, flags: ffi::Py_READONLY, doc: doc.as_ptr(), @@ -1205,27 +1223,67 @@ impl PyClassGetterGenerator, OFFSET } else { PyMethodDefType::Getter(crate::PyGetterDef { name, - meth: pyo3_get_py_t::, OFFSET>, + meth: pyo3_get_value_topyobject::, OFFSET>, doc, }) } } } -impl> + Clone, const OFFSET: usize> - PyClassGetterGenerator +/// Field is not Py; try to use `ToPyObject` to avoid potentially expensive clones of containers like `Vec` +impl + PyClassGetterGenerator { - /// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22. pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { PyMethodDefType::Getter(crate::PyGetterDef { // TODO: store &CStr in PyGetterDef etc name, - meth: pyo3_get_value::, + 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 = "`Py` fields are always converible to Python objects", + 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 { + // TODO: store &CStr in PyGetterDef etc + 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 Tester { const VALUE: bool = false; } @@ -1243,57 +1301,49 @@ impl IsPyT> { pub const VALUE: bool = true; } -macro_rules! trait_tester { - ($name:ident, $($trait:tt)+) => { - tester!($name); +tester!(IsToPyObject); - impl $name { - pub const VALUE: bool = true; - } - } +impl IsToPyObject { + pub const VALUE: bool = true; } -trait_tester!(IsIntoPyAndClone, IntoPy> + Clone); -trait_tester!(IsToPyObject, ToPyObject); - -fn pyo3_get_py_t( +fn pyo3_get_value_topyobject( 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::() + .downcast_unchecked::() .try_borrow()? }; let value = unsafe { obj.cast::() - .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) - .cast::>() + .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) + .cast::() }; // SAFETY: OFFSET is known to describe the location of the value, and // _holder is preventing mutable aliasing - Ok((unsafe { &*value }).clone_ref(py).into_py(py).into_ptr()) + Ok((unsafe { &*value }).to_object(py).into_ptr()) } -fn pyo3_get_value> + Clone, const OFFSET: usize>( +fn pyo3_get_value> + Clone, const OFFSET: usize>( py: Python<'_>, obj: *mut ffi::PyObject, ) -> PyResult<*mut ffi::PyObject> { - assert!(IsIntoPyAndClone::::VALUE); // Check for mutable aliasing let _holder = unsafe { BoundRef::ref_from_ptr(py, &obj) - .downcast_unchecked::() + .downcast_unchecked::() .try_borrow()? }; let value = unsafe { obj.cast::() - .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) - .cast::() + .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) + .cast::() }; // SAFETY: OFFSET is known to describe the location of the value, and diff --git a/tests/test_methods.rs b/tests/test_methods.rs index 37f3b2d8bd6..ddb3c01b6b8 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -899,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)] @@ -909,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] diff --git a/tests/ui/invalid_property_args.rs b/tests/ui/invalid_property_args.rs index b5eba27eb60..f35367df7aa 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 dea2e3fb2b4..201a0d5fa88 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -45,3 +45,22 @@ 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: `Py` fields are always converible to Python objects + = 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` From 547e18fcc627bc4d3fd0ee92c58f831df5fb2b1a Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 19 Jun 2024 09:37:47 +0100 Subject: [PATCH 3/7] make it work on MSRV :( --- pyo3-macros-backend/src/pyclass.rs | 14 ++-- pyo3-macros-backend/src/pyimpl.rs | 14 ++-- pyo3-macros-backend/src/pymethod.rs | 60 +++++++++++------ src/impl_/pyclass.rs | 97 +++++++++++++++++---------- src/impl_/pyclass/lazy_type_object.rs | 11 ++- src/pyclass/create_type_object.rs | 10 ++- tests/ui/invalid_property_args.stderr | 4 +- 7 files changed, 138 insertions(+), 72 deletions(-) diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 3e40977e4af..1e70f387d75 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 a1242d49f10..c131fc05131 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 06db0f8d410..bf19ee3f0eb 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -329,7 +329,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, @@ -510,12 +512,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 { @@ -700,11 +704,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 + ) ) ) }; @@ -772,19 +778,31 @@ pub fn impl_py_getter_def( 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 { use #pyo3_path::impl_::pyclass::Tester; - const OFFSET: usize = ::std::mem::offset_of!(#cls, #field); + + 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, + Offset, { #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE }, { #pyo3_path::impl_::pyclass::IsToPyObject::<#ty>::VALUE }, > = unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::new() }; - GENERATOR.generate(#python_name, #doc) + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Runtime( + || GENERATOR.generate(#python_name, #doc) + ) } }; @@ -820,11 +838,13 @@ pub fn impl_py_getter_def( let method_def = quote! { #cfg_attrs - #pyo3_path::class::PyMethodDefType::Getter( - #pyo3_path::class::PyGetterDef::new( - #python_name, - #cls::#wrapper_ident, - #doc + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Static( + #pyo3_path::class::PyMethodDefType::Getter( + #pyo3_path::class::PyGetterDef::new( + #python_name, + #cls::#wrapper_ident, + #doc + ) ) ) }; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 3c682f28a72..01981e4630d 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -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], } @@ -1172,6 +1179,23 @@ pub(crate) unsafe extern "C" fn assign_sequence_item_from_mapping( result } +// 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`. +pub unsafe trait OffsetCalculator { + /// Offset to the field within a PyClassObject, in bytes. + /// + /// The trait is unsafe to implement because producing an incorrect offset will lead to UB. + 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< @@ -1179,51 +1203,54 @@ pub struct PyClassGetterGenerator< // class struct ClassT: PyClass, FieldT, - const OFFSET: usize, + 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, PhantomData); +>(PhantomData<(ClassT, FieldT, Offset)>); impl< ClassT: PyClass, FieldT, - const OFFSET: usize, + Offset: OffsetCalculator, const IS_PY_T: bool, const IMPLEMENTS_TOPYOBJECT: bool, - > PyClassGetterGenerator + > PyClassGetterGenerator { - /// Safety: constructing this type requires that there exists a value of type X - /// at offset OFFSET within the type T. + /// 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, PhantomData) + Self(PhantomData) } } -impl - PyClassGetterGenerator, OFFSET, true, IMPLEMENTS_TOPYOBJECT> +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 const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { + 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: (std::mem::offset_of!(PyClassObject::, contents) + OFFSET) - as ffi::Py_ssize_t, + 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>, + meth: pyo3_get_value_topyobject::, Offset>, doc, }) } @@ -1231,14 +1258,13 @@ impl } /// Field is not Py; try to use `ToPyObject` to avoid potentially expensive clones of containers like `Vec` -impl - PyClassGetterGenerator +impl> + PyClassGetterGenerator { pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { PyMethodDefType::Getter(crate::PyGetterDef { - // TODO: store &CStr in PyGetterDef etc name, - meth: pyo3_get_value_topyobject::, + meth: pyo3_get_value_topyobject::, doc, }) } @@ -1257,8 +1283,8 @@ 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 +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 @@ -1267,9 +1293,8 @@ impl FieldT: PyO3GetField, { PyMethodDefType::Getter(crate::PyGetterDef { - // TODO: store &CStr in PyGetterDef etc name, - meth: pyo3_get_value::, + meth: pyo3_get_value::, doc, }) } @@ -1307,7 +1332,11 @@ impl IsToPyObject { pub const VALUE: bool = true; } -fn pyo3_get_value_topyobject( +fn pyo3_get_value_topyobject< + ClassT: PyClass, + FieldT: ToPyObject, + Offset: OffsetCalculator, +>( py: Python<'_>, obj: *mut ffi::PyObject, ) -> PyResult<*mut ffi::PyObject> { @@ -1318,18 +1347,18 @@ fn pyo3_get_value_topyobject() - .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) - .cast::() - }; + let value = unsafe { obj.cast::().add(Offset::offset()).cast::() }; - // SAFETY: OFFSET is known to describe the location of the value, and + // 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> + Clone, const OFFSET: usize>( +fn pyo3_get_value< + ClassT: PyClass, + FieldT: IntoPy> + Clone, + Offset: OffsetCalculator, +>( py: Python<'_>, obj: *mut ffi::PyObject, ) -> PyResult<*mut ffi::PyObject> { @@ -1340,13 +1369,9 @@ fn pyo3_get_value> + Clone, const OFFS .try_borrow()? }; - let value = unsafe { - obj.cast::() - .offset((std::mem::offset_of!(PyClassObject::, contents) + OFFSET) as isize) - .cast::() - }; + let value = unsafe { obj.cast::().add(Offset::offset()).cast::() }; - // SAFETY: OFFSET is known to describe the location of the value, and + // 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()) } diff --git a/src/impl_/pyclass/lazy_type_object.rs b/src/impl_/pyclass/lazy_type_object.rs index 7afaec8a99b..08a5f17d4bd 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/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index cdc25f6e433..00dc7ee36f8 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, @@ -317,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); } } diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr index 201a0d5fa88..64f5b62d57c 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -56,11 +56,11 @@ error[E0277]: `PhantomData` cannot be converted to a Python object = note: `Py` fields are always converible to Python objects = 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` +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` + | ^^^^^^^^^^^^ required by this bound in `PyClassGetterGenerator::::generate` From 7aa721b8cafa7e55b7d5ad397bedac9a8f0b6c7e Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 20 Jun 2024 04:04:22 +0100 Subject: [PATCH 4/7] fix docs and newsfragment --- newsfragments/4254.changed.md | 1 + src/impl_/pyclass.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 newsfragments/4254.changed.md diff --git a/newsfragments/4254.changed.md b/newsfragments/4254.changed.md new file mode 100644 index 00000000000..e58e0345696 --- /dev/null +++ b/newsfragments/4254.changed.md @@ -0,0 +1 @@ +Optimize code generated by `#[pyo3(get)]` on `#[pyclass]` fields. diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 01981e4630d..e2547f28af2 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1182,7 +1182,7 @@ pub(crate) unsafe extern "C" fn assign_sequence_item_from_mapping( // 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`. pub unsafe trait OffsetCalculator { - /// Offset to the field within a PyClassObject, in bytes. + /// Offset to the field within a `PyClassObject`, in bytes. /// /// The trait is unsafe to implement because producing an incorrect offset will lead to UB. fn offset() -> usize; From fec85ec304f71369a1d4e1fd6ad0e9356ee54e6f Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 20 Jun 2024 04:38:34 +0100 Subject: [PATCH 5/7] clippy --- src/impl_/pyclass.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index e2547f28af2..c5db9795e85 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1179,12 +1179,16 @@ pub(crate) unsafe extern "C" fn assign_sequence_item_from_mapping( result } -// 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`. +/// 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. - /// - /// The trait is unsafe to implement because producing an incorrect offset will lead to UB. fn offset() -> usize; } From 49aad6c5188ff309a96bbd238e90a3e80ecf98af Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 20 Jun 2024 05:04:42 +0100 Subject: [PATCH 6/7] internal docs and coverage --- src/impl_/pyclass.rs | 4 ++-- tests/test_getter_setter.rs | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index c5db9795e85..23e3ed9be38 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1236,7 +1236,7 @@ impl< const IMPLEMENTS_TOPYOBJECT: bool, > PyClassGetterGenerator, Offset, true, IMPLEMENTS_TOPYOBJECT> { - /// Py fields have a potential optimization to use Python's "struct members" to read + /// `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 @@ -1261,7 +1261,7 @@ impl< } } -/// Field is not Py; try to use `ToPyObject` to avoid potentially expensive clones of containers like `Vec` +/// Field is not `Py`; try to use `ToPyObject` to avoid potentially expensive clones of containers like `Vec` impl> PyClassGetterGenerator { diff --git a/tests/test_getter_setter.rs b/tests/test_getter_setter.rs index b0b15d78cd0..e2b8307fd32 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'"); + }); +} From b862d1b4e7951f4b22e1c4792245d3ac8b70ab07 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 22 Jun 2024 00:06:10 +0100 Subject: [PATCH 7/7] review: mejrs --- pyo3-macros-backend/src/pymethod.rs | 3 +- src/impl_/pyclass.rs | 87 +++++++++++++++++++++++++-- src/impl_/pymethods.rs | 1 + tests/ui/invalid_property_args.stderr | 1 - 4 files changed, 84 insertions(+), 8 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index bf19ee3f0eb..c842fb87428 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -783,7 +783,8 @@ pub fn impl_py_getter_def( let method_def = quote_spanned! {ty.span()=> #cfg_attrs { - use #pyo3_path::impl_::pyclass::Tester; + #[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 { diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 23e3ed9be38..391e96f5f43 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1279,7 +1279,6 @@ impl> /// 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 Tester { +pub trait Probe { const VALUE: bool = false; } -macro_rules! tester { +macro_rules! probe { ($name:ident) => { pub struct $name(PhantomData); - impl Tester for $name {} + impl Probe for $name {} }; } -tester!(IsPyT); +probe!(IsPyT); impl IsPyT> { pub const VALUE: bool = true; } -tester!(IsToPyObject); +probe!(IsToPyObject); impl IsToPyObject { pub const VALUE: bool = true; @@ -1379,3 +1378,79 @@ fn pyo3_get_value< // _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_/pymethods.rs b/src/impl_/pymethods.rs index 8ef315b7007..60b655e5647 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), diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr index 64f5b62d57c..0ee00cc6430 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -53,7 +53,6 @@ error[E0277]: `PhantomData` cannot be converted to a Python object | ^ 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: `Py` fields are always converible to Python objects = 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`