Fix for PyObject with `#[pyo3(get)]`

CHANGELOG.md

@@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 * Fixed unsoundness of subclassing. [#683](https://github.com/PyO3/pyo3/pull/683).
 * Clear error indicator when the exception is handled on the Rust side. [#719](https://github.com/PyO3/pyo3/pull/719)
 * Usage of raw identifiers with `#[pyo3(set)]`. [#745](https://github.com/PyO3/pyo3/pull/745)
+* Usage of `PyObject` with `#[pyo3(get)]` [#XXX](https://github.com/PyO3/pyo3/pull/XXX)
 
 ### Removed
 

pyo3-derive-backend/src/pyclass.rs

@@ -421,42 +421,6 @@ fn impl_descriptors(
     cls: &syn::Type,
     descriptors: Vec<(syn::Field, Vec<FnType>)>,
 ) -> syn::Result<TokenStream> {
-    let methods: Vec<TokenStream> = descriptors
-        .iter()
-        .flat_map(|&(ref field, ref fns)| {
-            fns.iter()
-                .map(|desc| {
-                    let name = field.ident.as_ref().unwrap();
-                    let field_ty = &field.ty;
-                    match *desc {
-                        FnType::Getter => {
-                            quote! {
-                                impl #cls {
-                                    fn #name(&self) -> pyo3::PyResult<#field_ty> {
-                                        Ok(self.#name.clone())
-                                    }
-                                }
-                            }
-                        }
-                        FnType::Setter => {
-                            let setter_name =
-                                syn::Ident::new(&format!("set_{}", name.unraw()), Span::call_site());
-                            quote! {
-                                impl #cls {
-                                    fn #setter_name(&mut self, value: #field_ty) -> pyo3::PyResult<()> {
-                                        self.#name = value;
-                                        Ok(())
-                                    }
-                                }
-                            }
-                        }
-                        _ => unreachable!(),
-                    }
-                })
-                .collect::<Vec<TokenStream>>()
-        })
-        .collect();
-
     let py_methods: Vec<TokenStream> = descriptors
         .iter()
         .flat_map(|&(ref field, ref fns)| {
@@ -479,7 +443,17 @@ fn impl_descriptors(
                                 output: parse_quote!(PyResult<#field_ty>),
                                 doc,
                             };
-                            Ok(impl_py_getter_def(&spec, &impl_wrap_getter(&cls, &spec)?))
+                            Ok(impl_py_getter_def(
+                                &spec,
+                                &impl_wrap_getter(
+                                    &cls,
+                                    &spec,
+                                    quote!({
+                                        use pyo3::derive_utils::GetPropertyValue;
+                                        (&_slf.#name).get_property_value(_py)
+                                    }),
+                                ),
+                            ))
                         }
                         FnType::Setter => {
                             let setter_name = syn::Ident::new(
@@ -503,7 +477,14 @@ fn impl_descriptors(
                                 output: parse_quote!(PyResult<()>),
                                 doc,
                             };
-                            Ok(impl_py_setter_def(&spec, &impl_wrap_setter(&cls, &spec)?))
+                            Ok(impl_py_setter_def(
+                                &spec,
+                                &impl_wrap_setter(
+                                    &cls,
+                                    &spec,
+                                    quote!({ _slf.#name = _val; Ok(()) }),
+                                ),
+                            ))
                         }
                         _ => unreachable!(),
                     }
@@ -513,7 +494,6 @@ fn impl_descriptors(
         .collect::<syn::Result<_>>()?;
 
     Ok(quote! {
-        #(#methods)*
 
         pyo3::inventory::submit! {
             #![crate = pyo3] {

pyo3-derive-backend/src/pymethod.rs

@@ -21,8 +21,14 @@ pub fn gen_py_method(
         FnType::FnCall => impl_py_method_def_call(&spec, &impl_wrap(cls, &spec, false)),
         FnType::FnClass => impl_py_method_def_class(&spec, &impl_wrap_class(cls, &spec)),
         FnType::FnStatic => impl_py_method_def_static(&spec, &impl_wrap_static(cls, &spec)),
-        FnType::Getter => impl_py_getter_def(&spec, &impl_wrap_getter(cls, &spec)?),
+        FnType::Getter => impl_py_getter_def(
-        FnType::Setter => impl_py_setter_def(&spec, &impl_wrap_setter(cls, &spec)?),
+            &spec,
+            &impl_wrap_getter(cls, &spec, impl_call_getter(&spec)?),
+        ),
+        FnType::Setter => impl_py_setter_def(
+            &spec,
+            &impl_wrap_setter(cls, &spec, impl_call_setter(&spec)?),
+        ),
     })
 }
 
@@ -244,8 +250,7 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
     }
 }
 
-/// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
+fn impl_call_getter(spec: &FnSpec) -> syn::Result<TokenStream> {
-pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<TokenStream> {
     let takes_py = match &*spec.args {
         [] => false,
         [arg] if utils::if_type_is_python(arg.ty) => true,
@@ -258,7 +263,6 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<To
     };
 
     let name = &spec.name;
-    let python_name = &spec.python_name;
 
     let fncall = if takes_py {
         quote! { _slf.#name(_py) }
@@ -266,7 +270,13 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<To
         quote! { _slf.#name() }
     };
 
-    Ok(quote! {
+    Ok(fncall)
+}
+
+/// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
+pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec, fncall: TokenStream) -> TokenStream {
+    let python_name = &spec.python_name;
+    quote! {
         unsafe extern "C" fn __wrap(
             _slf: *mut pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void) -> *mut pyo3::ffi::PyObject
         {
@@ -288,31 +298,41 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<To
                 }
             }
         }
-    })
+    }
+}
+
+fn impl_call_setter(spec: &FnSpec) -> syn::Result<TokenStream> {
+    let name = &spec.name;
+    // let python_name = &spec.python_name;
+
+    // let val_ty = match &*spec.args {
+    //     [] => {
+    //         return Err(syn::Error::new_spanned(
+    //             &spec.name,
+    //             "Not enough arguments for setter {}::{}",
+    //         ))
+    //     }
+    //     [arg] => &arg.ty,
+    //     _ => {
+    //         return Err(syn::Error::new_spanned(
+    //             spec.args[0].ty,
+    //             "Setter function must have exactly one argument",
+    //         ))
+    //     }
+    // };
+
+    Ok(quote!(pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name(_val))))
 }
 
 /// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
-pub(crate) fn impl_wrap_setter(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Result<TokenStream> {
+pub(crate) fn impl_wrap_setter(
-    let name = &spec.name;
+    cls: &syn::Type,
+    spec: &FnSpec<'_>,
+    fncall: TokenStream,
+) -> TokenStream {
     let python_name = &spec.python_name;
 
-    let val_ty = match &*spec.args {
+    quote! {
-        [] => {
-            return Err(syn::Error::new_spanned(
-                &spec.name,
-                "Not enough arguments for setter {}::{}",
-            ))
-        }
-        [arg] => &arg.ty,
-        _ => {
-            return Err(syn::Error::new_spanned(
-                spec.args[0].ty,
-                "Setter function must have exactly one argument",
-            ))
-        }
-    };
-
-    Ok(quote! {
         #[allow(unused_mut)]
         unsafe extern "C" fn __wrap(
             _slf: *mut pyo3::ffi::PyObject,
@@ -324,9 +344,9 @@ pub(crate) fn impl_wrap_setter(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Resul
             let _slf = _py.mut_from_borrowed_ptr::<#cls>(_slf);
             let _value = _py.from_borrowed_ptr(_value);
 
-            let _result = match <#val_ty as pyo3::FromPyObject>::extract(_value) {
+            let _result = match pyo3::FromPyObject::extract(_value) {
                 Ok(_val) => {
-                    pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name(_val))
+                    #fncall
                 }
                 Err(e) => Err(e)
             };
@@ -338,7 +358,7 @@ pub(crate) fn impl_wrap_setter(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Resul
                 }
             }
         }
-    })
+    }
 }
 
 /// This function abstracts away some copied code and can propably be simplified itself

src/derive_utils.rs

@@ -11,7 +11,7 @@ use crate::instance::PyNativeType;
 use crate::pyclass::PyClass;
 use crate::pyclass_init::PyClassInitializer;
 use crate::types::{PyAny, PyDict, PyModule, PyTuple};
-use crate::{ffi, GILPool, IntoPy, PyObject, Python};
+use crate::{ffi, GILPool, IntoPy, PyObject, Python, ToPyObject};
 use std::ptr;
 
 /// Description of a python parameter; used for `parse_args()`.
@@ -199,3 +199,22 @@ impl<T: PyClass, I: Into<PyClassInitializer<T>>> IntoPyNewResult<T, I> for PyRes
         self
     }
 }
+
+pub trait GetPropertyValue {
+    fn get_property_value(&self, py: Python) -> PyObject;
+}
+
+impl<T> GetPropertyValue for &T
+where
+    T: IntoPy<PyObject> + Clone,
+{
+    fn get_property_value(&self, py: Python) -> PyObject {
+        (*self).clone().into_py(py)
+    }
+}
+
+impl GetPropertyValue for PyObject {
+    fn get_property_value(&self, py: Python) -> PyObject {
+        self.to_object(py)
+    }
+}

tests/test_class_basics.rs

@@ -138,3 +138,28 @@ fn empty_class_in_module() {
     // than using whatever calls init first.
     assert_eq!(module, "builtins");
 }
+
+#[pyclass]
+struct ClassWithObjectField {
+    // PyObject has special case for derive_utils::GetPropertyValue,
+    // so this test is making sure it compiles!
+    #[pyo3(get, set)]
+    value: PyObject,
+}
+
+#[pymethods]
+impl ClassWithObjectField {
+    #[new]
+    fn new(value: PyObject) -> ClassWithObjectField {
+        Self { value }
+    }
+}
+
+#[test]
+fn class_with_object_field() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let ty = py.get_type::<ClassWithObjectField>();
+    py_assert!(py, ty, "ty(5).value == 5");
+    py_assert!(py, ty, "ty(None).value == None");
+}