tidy up implementation

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)
                 }
             };
 

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<T: PyClass, X, const OFFSET: usize, const IS_PY_T: bool>(
-    PhantomData<T>,
-    PhantomData<X>,
-);
-
-impl<T: PyClass, X, const OFFSET: usize, const IS_PY_T: bool>
-    PyClassGetterGenerator<T, X, OFFSET, IS_PY_T>
+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<ClassT>, PhantomData<FieldT>);
+
+impl<
+        ClassT: PyClass,
+        FieldT,
+        const OFFSET: usize,
+        const IS_PY_T: bool,
+        const IMPLEMENTS_TOPYOBJECT: bool,
+    > PyClassGetterGenerator<ClassT, FieldT, OFFSET, IS_PY_T, IMPLEMENTS_TOPYOBJECT>
 {
     /// Safety: constructing this type requires that there exists a value of type X
     /// at offset OFFSET within the type T.
@@ -1189,43 +1201,89 @@ impl<T: PyClass, X, const OFFSET: usize, const IS_PY_T: bool>
     }
 }
 
-impl<T: PyClass, X, const OFFSET: usize> PyClassGetterGenerator<T, Py<X>, OFFSET, true> {
-    /// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22.
+impl<ClassT: PyClass, U, const OFFSET: usize, const IMPLEMENTS_TOPYOBJECT: bool>
+    PyClassGetterGenerator<ClassT, Py<U>, OFFSET, true, IMPLEMENTS_TOPYOBJECT>
+{
+    /// Py<T> 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::<T>, contents) + OFFSET)
+                offset: (std::mem::offset_of!(PyClassObject::<ClassT>, contents) + OFFSET)
                     as ffi::Py_ssize_t,
                 flags: ffi::Py_READONLY,
                 doc: doc.as_ptr(),
             })
         } else {
             PyMethodDefType::Getter(crate::PyGetterDef {
                 name,
-                meth: pyo3_get_py_t::<T, Py<X>, OFFSET>,
+                meth: pyo3_get_value_topyobject::<ClassT, Py<U>, OFFSET>,
                 doc,
             })
         }
     }
 }
 
-impl<T: PyClass, X: IntoPy<Py<PyAny>> + Clone, const OFFSET: usize>
-    PyClassGetterGenerator<T, X, OFFSET, false>
+/// Field is not Py<T>; try to use `ToPyObject` to avoid potentially expensive clones of containers like `Vec`
+impl<ClassT: PyClass, FieldT: ToPyObject, const OFFSET: usize>
+    PyClassGetterGenerator<ClassT, FieldT, OFFSET, false, 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 {
         PyMethodDefType::Getter(crate::PyGetterDef {
             // TODO: store &CStr in PyGetterDef etc
             name,
-            meth: pyo3_get_value::<T, X, OFFSET>,
+            meth: pyo3_get_value_topyobject::<ClassT, FieldT, OFFSET>,
+            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<T>` fields are always converible to Python objects",
+        note = "implement `ToPyObject` or `IntoPy<PyObject> + Clone` for `{Self}` to define the conversion",
+    )
+)]
+pub trait PyO3GetField: IntoPy<Py<PyAny>> + Clone {}
+impl<T: IntoPy<Py<PyAny>> + Clone> PyO3GetField for T {}
+
+/// Base case attempts to use IntoPy + Clone, which was the only behaviour before PyO3 0.22.
+impl<ClassT: PyClass, FieldT, const OFFSET: usize>
+    PyClassGetterGenerator<ClassT, FieldT, OFFSET, false, false>
+{
+    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::<ClassT, FieldT, OFFSET>,
             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<T> IsPyT<Py<T>> {
     pub const VALUE: bool = true;
 }
 
-macro_rules! trait_tester {
-    ($name:ident, $($trait:tt)+) => {
-        tester!($name);
+tester!(IsToPyObject);
 
-        impl<T: $($trait)+> $name<T> {
-            pub const VALUE: bool = true;
-        }
-    }
+impl<T: ToPyObject> IsToPyObject<T> {
+    pub const VALUE: bool = true;
 }
 
-trait_tester!(IsIntoPyAndClone, IntoPy<Py<PyAny>> + Clone);
-trait_tester!(IsToPyObject, ToPyObject);
-
-fn pyo3_get_py_t<T: PyClass, U, const OFFSET: usize>(
+fn pyo3_get_value_topyobject<ClassT: PyClass, FieldT: ToPyObject, const OFFSET: usize>(
     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::<T>()
+            .downcast_unchecked::<ClassT>()
             .try_borrow()?
     };
 
     let value = unsafe {
         obj.cast::<u8>()
-            .offset((std::mem::offset_of!(PyClassObject::<T>, contents) + OFFSET) as isize)
-            .cast::<Py<U>>()
+            .offset((std::mem::offset_of!(PyClassObject::<ClassT>, contents) + OFFSET) as isize)
+            .cast::<FieldT>()
     };
 
     // 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<T: PyClass, X: IntoPy<Py<PyAny>> + Clone, const OFFSET: usize>(
+fn pyo3_get_value<ClassT: PyClass, FieldT: IntoPy<Py<PyAny>> + Clone, const OFFSET: usize>(
     py: Python<'_>,
     obj: *mut ffi::PyObject,
 ) -> PyResult<*mut ffi::PyObject> {
-    assert!(IsIntoPyAndClone::<X>::VALUE);
     // Check for mutable aliasing
     let _holder = unsafe {
         BoundRef::ref_from_ptr(py, &obj)
-            .downcast_unchecked::<T>()
+            .downcast_unchecked::<ClassT>()
             .try_borrow()?
     };
 
     let value = unsafe {
         obj.cast::<u8>()
-            .offset((std::mem::offset_of!(PyClassObject::<T>, contents) + OFFSET) as isize)
-            .cast::<X>()
+            .offset((std::mem::offset_of!(PyClassObject::<ClassT>, contents) + OFFSET) as isize)
+            .cast::<FieldT>()
     };
 
     // SAFETY: OFFSET is known to describe the location of the value, and

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]

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<i32>,
+}
+
 fn main() {}

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<i32>` cannot be converted to a Python object
+  --> tests/ui/invalid_property_args.rs:45:12
+   |
+45 |     value: ::std::marker::PhantomData<i32>,
+   |            ^ required by `#[pyo3(get)]` to create a readable property from a field of type `PhantomData<i32>`
+   |
+   = help: the trait `IntoPy<Py<PyAny>>` is not implemented for `PhantomData<i32>`, which is required by `PhantomData<i32>: PyO3GetField`
+   = note: `Py<T>` fields are always converible to Python objects
+   = note: implement `ToPyObject` or `IntoPy<PyObject> + Clone` for `PhantomData<i32>` to define the conversion
+   = note: required for `PhantomData<i32>` to implement `PyO3GetField`
+note: required by a bound in `PyClassGetterGenerator::<ClassT, FieldT, OFFSET, false, false>::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::<ClassT, FieldT, OFFSET, false, false>::generate`