Merge pull request #2187 from PyO3/getattribute

Protocols: implement __getattribute__
PyO3 · Feb 26, 2022 · 1ed9a73 · 1ed9a73
2 parents 03dc96b + dc4f114
commit 1ed9a73
Show file tree

Hide file tree

Showing 6 changed files with 162 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add support for `from_py_with` on struct tuples and enums to override the default from-Python conversion. [#2181](https://github.com/PyO3/pyo3/pull/2181)
 - Add `eq`, `ne`, `lt`, `le`, `gt`, `ge` methods to `PyAny` that wrap `rich_compare`. [#2175](https://github.com/PyO3/pyo3/pull/2175)
 - Add `Py::is` and `PyAny::is` methods to check for object identity. [#2183](https://github.com/PyO3/pyo3/pull/2183)
+- Add support for the `__getattribute__` magic method. [#2187](https://github.com/PyO3/pyo3/pull/2187)
 
 ### Changed
 

diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md
@@ -75,6 +75,16 @@ given signatures should be interpreted as follows:
     </details>
 
   - `__getattr__(<self>, object) -> object`
+  - `__getattribute__(<self>, object) -> object`
+    <details>
+    <summary>Differences between `__getattr__` and `__getattribute__`</summary>
+    As in Python, `__getattr__` is only called if the attribute is not found
+    by normal attribute lookup.  `__getattribute__`, on the other hand, is
+    called for *every* attribute access.  If it wants to access existing
+    attributes on `self`, it needs to be very careful not to introduce
+    infinite recursion, and use `baseclass.__getattribute__()`.
+    </details>
+
   - `__setattr__(<self>, value: object) -> ()`
   - `__delattr__(<self>, object) -> ()`
 

diff --git a/pyo3-macros-backend/src/pyimpl.rs b/pyo3-macros-backend/src/pyimpl.rs
@@ -242,6 +242,11 @@ fn add_shared_proto_slots(
         }};
     }
 
+    try_add_shared_slot!(
+        "__getattribute__",
+        "__getattr__",
+        generate_pyclass_getattro_slot
+    );
     try_add_shared_slot!("__setattr__", "__delattr__", generate_pyclass_setattr_slot);
     try_add_shared_slot!("__set__", "__delete__", generate_pyclass_setdescr_slot);
     try_add_shared_slot!("__setitem__", "__delitem__", generate_pyclass_setitem_slot);

diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs
@@ -38,7 +38,6 @@ impl PyMethodKind {
     fn from_name(name: &str) -> Self {
         match name {
             // Protocol implemented through slots
-            "__getattr__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__GETATTR__)),
             "__str__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__STR__)),
             "__repr__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__REPR__)),
             "__hash__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__HASH__)),
@@ -85,6 +84,10 @@ impl PyMethodKind {
             "__releasebuffer__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__RELEASEBUFFER__)),
             "__clear__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__CLEAR__)),
             // Protocols implemented through traits
+            "__getattribute__" => {
+                PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__GETATTRIBUTE__))
+            }
+            "__getattr__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__GETATTR__)),
             "__setattr__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__SETATTR__)),
             "__delattr__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__DELATTR__)),
             "__set__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__SET__)),
@@ -570,21 +573,6 @@ impl PropertyType<'_> {
     }
 }
 
-const __GETATTR__: SlotDef = SlotDef::new("Py_tp_getattro", "getattrofunc")
-    .arguments(&[Ty::Object])
-    .before_call_method(TokenGenerator(|| {
-        quote! {
-            // Behave like python's __getattr__ (as opposed to __getattribute__) and check
-            // for existing fields and methods first
-            let existing = _pyo3::ffi::PyObject_GenericGetAttr(_slf, arg0);
-            if existing.is_null() {
-                // PyObject_HasAttr also tries to get an object and clears the error if it fails
-                _pyo3::ffi::PyErr_Clear();
-            } else {
-                return existing;
-            }
-        }
-    }));
 const __STR__: SlotDef = SlotDef::new("Py_tp_str", "reprfunc");
 const __REPR__: SlotDef = SlotDef::new("Py_tp_repr", "reprfunc");
 const __HASH__: SlotDef = SlotDef::new("Py_tp_hash", "hashfunc")
@@ -870,7 +858,6 @@ struct SlotDef {
     func_ty: StaticIdent,
     arguments: &'static [Ty],
     ret_ty: Ty,
-    before_call_method: Option<TokenGenerator>,
     extract_error_mode: ExtractErrorMode,
     return_mode: Option<ReturnMode>,
     require_unsafe: bool,
@@ -885,7 +872,6 @@ impl SlotDef {
             func_ty: StaticIdent(func_ty),
             arguments: NO_ARGUMENTS,
             ret_ty: Ty::Object,
-            before_call_method: None,
             extract_error_mode: ExtractErrorMode::Raise,
             return_mode: None,
             require_unsafe: false,
@@ -902,11 +888,6 @@ impl SlotDef {
         self
     }
 
-    const fn before_call_method(mut self, before_call_method: TokenGenerator) -> Self {
-        self.before_call_method = Some(before_call_method);
-        self
-    }
-
     const fn return_conversion(mut self, return_conversion: TokenGenerator) -> Self {
         self.return_mode = Some(ReturnMode::Conversion(return_conversion));
         self
@@ -936,7 +917,6 @@ impl SlotDef {
         let SlotDef {
             slot,
             func_ty,
-            before_call_method,
             arguments,
             extract_error_mode,
             ret_ty,
@@ -963,7 +943,6 @@ impl SlotDef {
         Ok(quote!({
             unsafe extern "C" fn __wrap(_raw_slf: *mut _pyo3::ffi::PyObject, #(#method_arguments),*) -> #ret_ty {
                 let _slf = _raw_slf;
-                #before_call_method
                 let gil = _pyo3::GILPool::new();
                 let #py = gil.python();
                 _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> {
@@ -1074,6 +1053,10 @@ impl SlotFragmentDef {
     }
 }
 
+const __GETATTRIBUTE__: SlotFragmentDef =
+    SlotFragmentDef::new("__getattribute__", &[Ty::Object]).ret_ty(Ty::Object);
+const __GETATTR__: SlotFragmentDef =
+    SlotFragmentDef::new("__getattr__", &[Ty::Object]).ret_ty(Ty::Object);
 const __SETATTR__: SlotFragmentDef =
     SlotFragmentDef::new("__setattr__", &[Ty::Object, Ty::NonNullObject]);
 const __DELATTR__: SlotFragmentDef = SlotFragmentDef::new("__delattr__", &[Ty::Object]);

diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs
@@ -5,7 +5,7 @@ use crate::{
     pycell::PyCellLayout,
     pyclass_init::PyObjectInit,
     type_object::{PyLayout, PyTypeObject},
-    PyCell, PyClass, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python,
+    Py, PyAny, PyCell, PyClass, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python,
 };
 use std::{
     marker::PhantomData,
@@ -198,6 +198,84 @@ macro_rules! slot_fragment_trait {
     }
 }
 
+slot_fragment_trait! {
+    PyClass__getattribute__SlotFragment,
+
+    /// # Safety: _slf and _attr must be valid non-null Python objects
+    #[inline]
+    unsafe fn __getattribute__(
+        self,
+        py: Python,
+        slf: *mut ffi::PyObject,
+        attr: *mut ffi::PyObject,
+    ) -> PyResult<*mut ffi::PyObject> {
+        let res = ffi::PyObject_GenericGetAttr(slf, attr);
+        if res.is_null() {
+            Err(PyErr::fetch(py))
+        } else {
+            Ok(res)
+        }
+    }
+}
+
+slot_fragment_trait! {
+    PyClass__getattr__SlotFragment,
+
+    /// # Safety: _slf and _attr must be valid non-null Python objects
+    #[inline]
+    unsafe fn __getattr__(
+        self,
+        py: Python,
+        _slf: *mut ffi::PyObject,
+        attr: *mut ffi::PyObject,
+    ) -> PyResult<*mut ffi::PyObject> {
+        Err(PyErr::new::<PyAttributeError, _>(
+            (Py::<PyAny>::from_borrowed_ptr(py, attr),)
+        ))
+    }
+}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! generate_pyclass_getattro_slot {
+    ($cls:ty) => {{
+        unsafe extern "C" fn __wrap(
+            _slf: *mut $crate::ffi::PyObject,
+            attr: *mut $crate::ffi::PyObject,
+        ) -> *mut $crate::ffi::PyObject {
+            use ::std::result::Result::*;
+            use $crate::impl_::pyclass::*;
+            let gil = $crate::GILPool::new();
+            let py = gil.python();
+            $crate::callback::panic_result_into_callback_output(
+                py,
+                ::std::panic::catch_unwind(move || -> $crate::PyResult<_> {
+                    let collector = PyClassImplCollector::<$cls>::new();
+
+                    // Strategy:
+                    // - Try __getattribute__ first.  Its default is PyObject_GenericGetAttr.
+                    // - If it returns a result, use it.
+                    // - If it fails with AttributeError, try __getattr__.
+                    // - If it fails otherwise, reraise.
+                    match collector.__getattribute__(py, _slf, attr) {
+                        Ok(obj) => Ok(obj),
+                        Err(e) if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) => {
+                            collector.__getattr__(py, _slf, attr)
+                        }
+                        Err(e) => Err(e),
+                    }
+                }),
+            )
+        }
+        $crate::ffi::PyType_Slot {
+            slot: $crate::ffi::Py_tp_getattro,
+            pfunc: __wrap as $crate::ffi::getattrofunc as _,
+        }
+    }};
+}
+
+pub use generate_pyclass_getattro_slot;
+
 /// Macro which expands to three items
 /// - Trait for a __setitem__ dunder
 /// - Trait for the corresponding __delitem__ dunder

diff --git a/tests/test_proto_methods.rs b/tests/test_proto_methods.rs
@@ -1,9 +1,8 @@
 #![cfg(feature = "macros")]
 
-use pyo3::exceptions::{PyIndexError, PyValueError};
+use pyo3::exceptions::{PyAttributeError, PyIndexError, PyValueError};
 use pyo3::types::{PyDict, PyList, PyMapping, PySequence, PySlice, PyType};
-use pyo3::{exceptions::PyAttributeError, prelude::*};
-use pyo3::{py_run, PyCell};
+use pyo3::{prelude::*, py_run, PyCell};
 use std::{isize, iter};
 
 mod common;
@@ -606,6 +605,63 @@ fn getattr_doesnt_override_member() {
     py_assert!(py, inst, "inst.a == 8");
 }
 
+#[pyclass]
+struct ClassWithGetAttribute {
+    #[pyo3(get, set)]
+    data: u32,
+}
+
+#[pymethods]
+impl ClassWithGetAttribute {
+    fn __getattribute__(&self, _name: &str) -> u32 {
+        self.data * 2
+    }
+}
+
+#[test]
+fn getattribute_overrides_member() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let inst = PyCell::new(py, ClassWithGetAttribute { data: 4 }).unwrap();
+    py_assert!(py, inst, "inst.data == 8");
+    py_assert!(py, inst, "inst.y == 8");
+}
+
+#[pyclass]
+struct ClassWithGetAttrAndGetAttribute;
+
+#[pymethods]
+impl ClassWithGetAttrAndGetAttribute {
+    fn __getattribute__(&self, name: &str) -> PyResult<u32> {
+        if name == "exists" {
+            Ok(42)
+        } else if name == "error" {
+            Err(PyValueError::new_err("bad"))
+        } else {
+            Err(PyAttributeError::new_err("fallback"))
+        }
+    }
+
+    fn __getattr__(&self, name: &str) -> PyResult<u32> {
+        if name == "lucky" {
+            Ok(57)
+        } else {
+            Err(PyAttributeError::new_err("no chance"))
+        }
+    }
+}
+
+#[test]
+fn getattr_and_getattribute() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let inst = PyCell::new(py, ClassWithGetAttrAndGetAttribute).unwrap();
+    py_assert!(py, inst, "inst.exists == 42");
+    py_assert!(py, inst, "inst.lucky == 57");
+    py_expect_exception!(py, inst, "inst.error", PyValueError);
+    py_expect_exception!(py, inst, "inst.unlucky", PyAttributeError);
+}
+
 /// Wraps a Python future and yield it once.
 #[pyclass]
 #[derive(Debug)]