From 0678f112662ed42117737b69f80c55d39127a4fc Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Fri, 25 Feb 2022 18:52:36 +0100 Subject: [PATCH] Protocols: implement __getattribute__ converting tp_getattro to a shared slot Fixes #2186 --- CHANGELOG.md | 1 + guide/src/class/protocols.md | 10 ++++ pyo3-macros-backend/src/pyimpl.rs | 5 ++ pyo3-macros-backend/src/pymethod.rs | 33 +++---------- src/impl_/pyclass.rs | 76 ++++++++++++++++++++++++++++- tests/test_proto_methods.rs | 62 +++++++++++++++++++++-- 6 files changed, 158 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2ae4ce5..ef979445 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `wrap_pyfunction!` can now wrap a `#[pyfunction]` which is implemented in a different Rust module or crate. [#2091](https://github.com/PyO3/pyo3/pull/2091) - Add `PyAny::contains` method (`in` operator for `PyAny`). [#2115](https://github.com/PyO3/pyo3/pull/2115) - Add `PyMapping::contains` method (`in` operator for `PyMapping`). [#2133](https://github.com/PyO3/pyo3/pull/2133) +- Add support for the `__getattribute__` magic method. [#2187](https://github.com/PyO3/pyo3/pull/2187) - Add garbage collection magic methods `__traverse__` and `__clear__` to `#[pymethods]`. [#2159](https://github.com/PyO3/pyo3/pull/2159) - 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) diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md index c3428550..cacde232 100644 --- a/guide/src/class/protocols.md +++ b/guide/src/class/protocols.md @@ -75,6 +75,16 @@ given signatures should be interpreted as follows: - `__getattr__(, object) -> object` + - `__getattribute__(, object) -> object` +
+ Differences between `__getattr__` and `__getattribute__` + 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__()`. +
+ - `__setattr__(, value: object) -> ()` - `__delattr__(, object) -> ()` diff --git a/pyo3-macros-backend/src/pyimpl.rs b/pyo3-macros-backend/src/pyimpl.rs index e5270641..e8f23819 100644 --- 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 index 29aab459..78b01b5e 100644 --- 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, extract_error_mode: ExtractErrorMode, return_mode: Option, 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 index abf7fb26..3ec2c7ef 100644 --- 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,80 @@ 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::((Py::::from_owned_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::callback::IntoPyCallbackOutput; + 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) => obj.convert(py), + Err(e) if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) => { + collector.__getattr__(py, _slf, attr).convert(py) + } + 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 index e2c238fc..87035916 100644 --- 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 { + 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 { + 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)]