Merge pull request #505 from PyO3/fix_getattr

Use existing fields and methods before calling custom __getattr__

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
  * `module` argument to `pyclass` macro. [#499](https://github.com/PyO3/pyo3/pull/499)
  * `py_run!` macro [#512](https://github.com/PyO3/pyo3/pull/512)
+ * Use existing fields and methods before calling custom __getattr__. [#505](https://github.com/PyO3/pyo3/pull/512)
 
 ### Fixed
 

src/class/basic.rs

@@ -207,12 +207,37 @@ where
     T: for<'p> PyObjectGetAttrProtocol<'p>,
 {
     fn tp_getattro() -> Option<ffi::binaryfunc> {
-        py_binary_func!(
-            PyObjectGetAttrProtocol,
-            T::__getattr__,
-            T::Success,
-            PyObjectCallbackConverter
-        )
+        #[allow(unused_mut)]
+        unsafe extern "C" fn wrap<T>(
+            slf: *mut ffi::PyObject,
+            arg: *mut ffi::PyObject,
+        ) -> *mut ffi::PyObject
+        where
+            T: for<'p> PyObjectGetAttrProtocol<'p>,
+        {
+            let _pool = crate::GILPool::new();
+            let py = Python::assume_gil_acquired();
+
+            // Behave like python's __getattr__ (as opposed to __getattribute__) and check
+            // for existing fields and methods first
+            let existing = ffi::PyObject_GenericGetAttr(slf, arg);
+            if existing == std::ptr::null_mut() {
+                // PyObject_HasAttr also tries to get an object and clears the error if it fails
+                ffi::PyErr_Clear();
+            } else {
+                return existing;
+            }
+
+            let slf = py.mut_from_borrowed_ptr::<T>(slf);
+            let arg = py.from_borrowed_ptr::<crate::types::PyAny>(arg);
+
+            let result = match arg.extract() {
+                Ok(arg) => slf.__getattr__(arg).into(),
+                Err(e) => Err(e.into()),
+            };
+            crate::callback::cb_convert(PyObjectCallbackConverter, py, result)
+        }
+        Some(wrap::<T>)
     }
 }
 

tests/test_dunder.rs

@@ -462,3 +462,25 @@ fn weakref_dunder_dict_support() {
         "import weakref; assert weakref.ref(inst)() is inst; inst.a = 1; assert inst.a == 1"
     );
 }
+
+#[pyclass]
+struct ClassWithGetAttr {
+    #[pyo3(get, set)]
+    data: u32,
+}
+
+#[pyproto]
+impl PyObjectProtocol for ClassWithGetAttr {
+    fn __getattr__(&self, _name: &str) -> PyResult<u32> {
+        Ok(self.data * 2)
+    }
+}
+
+#[test]
+fn getattr_doesnt_override_member() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let inst = PyRef::new(py, ClassWithGetAttr { data: 4 }).unwrap();
+    py_assert!(py, inst, "inst.data == 4");
+    py_assert!(py, inst, "inst.a == 8");
+}