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`