From 4f8ee968812977c1ecc9909143ecea75b114870d Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 22 Feb 2024 23:38:42 +0100 Subject: [PATCH] fix `AsRef` and `Deref` impls on `Bound` (#3879) * fix `AsRef` and `Deref` of `Bound` to `Bound` * cleanup unnessesary `.as_any()` calls * remove trait bound on `AsRef` impl * add comment for `Deref` trait bound * rename marker trait --- guide/src/class/object.md | 4 ++-- pyo3-macros-backend/src/pyclass.rs | 2 ++ src/instance.rs | 12 +++++----- src/types/boolobject.rs | 4 ++-- src/types/mod.rs | 27 ++++++++++++++++++++++ tests/test_buffer.rs | 12 +++++----- tests/test_class_conversion.rs | 6 ++--- tests/test_field_cfg.rs | 10 ++------ tests/test_inheritance.rs | 2 +- tests/test_mapping.rs | 4 ++-- tests/test_proto_methods.rs | 37 +++++++++++------------------- 11 files changed, 65 insertions(+), 55 deletions(-) diff --git a/guide/src/class/object.md b/guide/src/class/object.md index 47188939..10867976 100644 --- a/guide/src/class/object.md +++ b/guide/src/class/object.md @@ -217,8 +217,8 @@ impl Number { # fn main() -> PyResult<()> { # Python::with_gil(|py| { -# let x = &Bound::new(py, Number(4))?.into_any(); -# let y = &Bound::new(py, Number(4))?.into_any(); +# let x = &Bound::new(py, Number(4))?; +# let y = &Bound::new(py, Number(4))?; # assert!(x.eq(y)?); # assert!(!x.ne(y)?); # Ok(()) diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 806df88e..ce39cb01 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -365,6 +365,8 @@ fn impl_class( const _: () = { use #krate as _pyo3; + impl _pyo3::types::DerefToPyAny for #cls {} + #pytypeinfo_impl #py_class_impl diff --git a/src/instance.rs b/src/instance.rs index f35ab4bb..ecf12811 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -4,7 +4,7 @@ use crate::pycell::{PyBorrowError, PyBorrowMutError, PyCell}; use crate::pyclass::boolean_struct::{False, True}; use crate::type_object::HasPyGilRef; use crate::types::{any::PyAnyMethods, string::PyStringMethods, typeobject::PyTypeMethods}; -use crate::types::{PyDict, PyString, PyTuple}; +use crate::types::{DerefToPyAny, PyDict, PyString, PyTuple}; use crate::{ ffi, AsPyPointer, DowncastError, FromPyObject, IntoPy, PyAny, PyClass, PyClassInitializer, PyRef, PyRefMut, PyTypeInfo, Python, ToPyObject, @@ -366,9 +366,12 @@ fn python_format( } } +// The trait bound is needed to avoid running into the auto-deref recursion +// limit (error[E0055]), because `Bound` would deref into itself. See: +// https://github.com/rust-lang/rust/issues/19509 impl<'py, T> Deref for Bound<'py, T> where - T: AsRef, + T: DerefToPyAny, { type Target = Bound<'py, PyAny>; @@ -378,10 +381,7 @@ where } } -impl<'py, T> AsRef> for Bound<'py, T> -where - T: AsRef, -{ +impl<'py, T> AsRef> for Bound<'py, T> { #[inline] fn as_ref(&self) -> &Bound<'py, PyAny> { self.as_any() diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index 01bc14b4..906a9670 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -168,7 +168,7 @@ mod tests { Python::with_gil(|py| { assert!(PyBool::new_bound(py, true).is_true()); let t = PyBool::new_bound(py, true); - assert!(t.as_any().extract::().unwrap()); + assert!(t.extract::().unwrap()); assert!(true.to_object(py).is(&*PyBool::new_bound(py, true))); }); } @@ -178,7 +178,7 @@ mod tests { Python::with_gil(|py| { assert!(!PyBool::new_bound(py, false).is_true()); let t = PyBool::new_bound(py, false); - assert!(!t.as_any().extract::().unwrap()); + assert!(!t.extract::().unwrap()); assert!(false.to_object(py).is(&*PyBool::new_bound(py, false))); }); } diff --git a/src/types/mod.rs b/src/types/mod.rs index 4ebf050b..217e2401 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -86,6 +86,31 @@ pub mod iter { pub use super::tuple::{BorrowedTupleIterator, BoundTupleIterator, PyTupleIterator}; } +/// Python objects that have a base type. +/// +/// This marks types that can be upcast into a [`PyAny`] and used in its place. +/// This essentially includes every Python object except [`PyAny`] itself. +/// +/// This is used to provide the [`Deref>`](std::ops::Deref) +/// implementations for [`Bound<'_, T>`](crate::Bound). +/// +/// Users should not need to implement this trait directly. It's implementation +/// is provided by the [`#[pyclass]`](macro@crate::pyclass) attribute. +/// +/// ## Note +/// This is needed because the compiler currently tries to figure out all the +/// types in a deref-chain before starting to look for applicable method calls. +/// So we need to prevent [`Bound<'_, PyAny`](crate::Bound) dereferencing to +/// itself in order to avoid running into the recursion limit. This trait is +/// used to exclude this from our blanket implementation. See [this Rust +/// issue][1] for more details. If the compiler limitation gets resolved, this +/// trait will be removed. +/// +/// [1]: https://github.com/rust-lang/rust/issues/19509 +pub trait DerefToPyAny { + // Empty. +} + // Implementations core to all native types #[doc(hidden)] #[macro_export] @@ -183,6 +208,8 @@ macro_rules! pyobject_native_type_named ( unsafe{&*(ob as *const $name as *const $crate::PyAny)} } } + + impl $crate::types::DerefToPyAny for $name {} }; ); diff --git a/tests/test_buffer.rs b/tests/test_buffer.rs index 72f94b3b..0b3da881 100644 --- a/tests/test_buffer.rs +++ b/tests/test_buffer.rs @@ -96,11 +96,11 @@ fn test_get_buffer_errors() { ) .unwrap(); - assert!(PyBuffer::::get_bound(instance.bind(py).as_any()).is_ok()); + assert!(PyBuffer::::get_bound(instance.bind(py)).is_ok()); instance.borrow_mut(py).error = Some(TestGetBufferError::NullShape); assert_eq!( - PyBuffer::::get_bound(instance.bind(py).as_any()) + PyBuffer::::get_bound(instance.bind(py)) .unwrap_err() .to_string(), "BufferError: shape is null" @@ -108,7 +108,7 @@ fn test_get_buffer_errors() { instance.borrow_mut(py).error = Some(TestGetBufferError::NullStrides); assert_eq!( - PyBuffer::::get_bound(instance.bind(py).as_any()) + PyBuffer::::get_bound(instance.bind(py)) .unwrap_err() .to_string(), "BufferError: strides is null" @@ -116,7 +116,7 @@ fn test_get_buffer_errors() { instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectItemSize); assert_eq!( - PyBuffer::::get_bound(instance.bind(py).as_any()) + PyBuffer::::get_bound(instance.bind(py)) .unwrap_err() .to_string(), "BufferError: buffer contents are not compatible with u32" @@ -124,7 +124,7 @@ fn test_get_buffer_errors() { instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectFormat); assert_eq!( - PyBuffer::::get_bound(instance.bind(py).as_any()) + PyBuffer::::get_bound(instance.bind(py)) .unwrap_err() .to_string(), "BufferError: buffer contents are not compatible with u32" @@ -132,7 +132,7 @@ fn test_get_buffer_errors() { instance.borrow_mut(py).error = Some(TestGetBufferError::IncorrectAlignment); assert_eq!( - PyBuffer::::get_bound(instance.bind(py).as_any()) + PyBuffer::::get_bound(instance.bind(py)) .unwrap_err() .to_string(), "BufferError: buffer contents are insufficiently aligned for u32" diff --git a/tests/test_class_conversion.rs b/tests/test_class_conversion.rs index ccb97409..a0919527 100644 --- a/tests/test_class_conversion.rs +++ b/tests/test_class_conversion.rs @@ -146,13 +146,11 @@ fn test_pyref_as_base() { #[test] fn test_pycell_deref() { Python::with_gil(|py| { - let cell = Bound::new(py, (SubClass {}, BaseClass { value: 120 })).unwrap(); + let obj = Bound::new(py, (SubClass {}, BaseClass { value: 120 })).unwrap(); // Should be able to deref as PyAny - // FIXME: This deref does _not_ work assert_eq!( - cell.as_any() - .call_method0("foo") + obj.call_method0("foo") .and_then(|e| e.extract::<&str>()) .unwrap(), "SubClass" diff --git a/tests/test_field_cfg.rs b/tests/test_field_cfg.rs index be400f2d..c5fc4958 100644 --- a/tests/test_field_cfg.rs +++ b/tests/test_field_cfg.rs @@ -22,14 +22,8 @@ fn test_cfg() { Python::with_gil(|py| { let cfg = CfgClass { b: 3 }; let py_cfg = Py::new(py, cfg).unwrap(); - assert!(py_cfg.bind(py).as_any().getattr("a").is_err()); - let b: u32 = py_cfg - .bind(py) - .as_any() - .getattr("b") - .unwrap() - .extract() - .unwrap(); + assert!(py_cfg.bind(py).getattr("a").is_err()); + let b: u32 = py_cfg.bind(py).getattr("b").unwrap().extract().unwrap(); assert_eq!(b, 3); }); } diff --git a/tests/test_inheritance.rs b/tests/test_inheritance.rs index 7a465f1a..1209713e 100644 --- a/tests/test_inheritance.rs +++ b/tests/test_inheritance.rs @@ -247,7 +247,7 @@ mod inheriting_native_type { let item = &py.eval_bound("object()", None, None).unwrap(); assert_eq!(item.get_refcnt(), 1); - dict_sub.bind(py).as_any().set_item("foo", item).unwrap(); + dict_sub.bind(py).set_item("foo", item).unwrap(); assert_eq!(item.get_refcnt(), 2); drop(dict_sub); diff --git a/tests/test_mapping.rs b/tests/test_mapping.rs index 06928e74..4e24db97 100644 --- a/tests/test_mapping.rs +++ b/tests/test_mapping.rs @@ -123,7 +123,7 @@ fn mapping_is_not_sequence() { PyMapping::register::(py).unwrap(); - assert!(m.bind(py).as_any().downcast::().is_ok()); - assert!(m.bind(py).as_any().downcast::().is_err()); + assert!(m.bind(py).downcast::().is_ok()); + assert!(m.bind(py).downcast::().is_err()); }); } diff --git a/tests/test_proto_methods.rs b/tests/test_proto_methods.rs index 61b91243..a1cfde2b 100644 --- a/tests/test_proto_methods.rs +++ b/tests/test_proto_methods.rs @@ -81,7 +81,6 @@ fn test_getattr() { let example_py = make_example(py); assert_eq!( example_py - .as_any() .getattr("value") .unwrap() .extract::() @@ -90,7 +89,6 @@ fn test_getattr() { ); assert_eq!( example_py - .as_any() .getattr("special_custom_attr") .unwrap() .extract::() @@ -98,7 +96,6 @@ fn test_getattr() { 20, ); assert!(example_py - .as_any() .getattr("other_attr") .unwrap_err() .is_instance_of::(py)); @@ -109,13 +106,9 @@ fn test_getattr() { fn test_setattr() { Python::with_gil(|py| { let example_py = make_example(py); - example_py - .as_any() - .setattr("special_custom_attr", 15) - .unwrap(); + example_py.setattr("special_custom_attr", 15).unwrap(); assert_eq!( example_py - .as_any() .getattr("special_custom_attr") .unwrap() .extract::() @@ -129,12 +122,8 @@ fn test_setattr() { fn test_delattr() { Python::with_gil(|py| { let example_py = make_example(py); - example_py.as_any().delattr("special_custom_attr").unwrap(); - assert!(example_py - .as_any() - .getattr("special_custom_attr") - .unwrap() - .is_none()); + example_py.delattr("special_custom_attr").unwrap(); + assert!(example_py.getattr("special_custom_attr").unwrap().is_none()); }) } @@ -142,7 +131,7 @@ fn test_delattr() { fn test_str() { Python::with_gil(|py| { let example_py = make_example(py); - assert_eq!(example_py.as_any().str().unwrap().to_cow().unwrap(), "5"); + assert_eq!(example_py.str().unwrap().to_cow().unwrap(), "5"); }) } @@ -151,7 +140,7 @@ fn test_repr() { Python::with_gil(|py| { let example_py = make_example(py); assert_eq!( - example_py.as_any().repr().unwrap().to_cow().unwrap(), + example_py.repr().unwrap().to_cow().unwrap(), "ExampleClass(value=5)" ); }) @@ -161,7 +150,7 @@ fn test_repr() { fn test_hash() { Python::with_gil(|py| { let example_py = make_example(py); - assert_eq!(example_py.as_any().hash().unwrap(), 5); + assert_eq!(example_py.hash().unwrap(), 5); }) } @@ -169,9 +158,9 @@ fn test_hash() { fn test_bool() { Python::with_gil(|py| { let example_py = make_example(py); - assert!(example_py.as_any().is_truthy().unwrap()); + assert!(example_py.is_truthy().unwrap()); example_py.borrow_mut().value = 0; - assert!(!example_py.as_any().is_truthy().unwrap()); + assert!(!example_py.is_truthy().unwrap()); }) } @@ -231,7 +220,7 @@ fn mapping() { ) .unwrap(); - let mapping: &Bound<'_, PyMapping> = inst.bind(py).as_any().downcast().unwrap(); + let mapping: &Bound<'_, PyMapping> = inst.bind(py).downcast().unwrap(); py_assert!(py, inst, "len(inst) == 0"); @@ -333,7 +322,7 @@ fn sequence() { let inst = Py::new(py, Sequence { values: vec![] }).unwrap(); - let sequence: &Bound<'_, PySequence> = inst.bind(py).as_any().downcast().unwrap(); + let sequence: &Bound<'_, PySequence> = inst.bind(py).downcast().unwrap(); py_assert!(py, inst, "len(inst) == 0"); @@ -360,16 +349,16 @@ fn sequence() { // indices. assert!(sequence.len().is_err()); // however regular python len() works thanks to mp_len slot - assert_eq!(inst.bind(py).as_any().len().unwrap(), 0); + assert_eq!(inst.bind(py).len().unwrap(), 0); py_run!(py, inst, "inst.append(0)"); sequence.set_item(0, 5).unwrap(); - assert_eq!(inst.bind(py).as_any().len().unwrap(), 1); + assert_eq!(inst.bind(py).len().unwrap(), 1); assert_eq!(sequence.get_item(0).unwrap().extract::().unwrap(), 5); sequence.del_item(0).unwrap(); - assert_eq!(inst.bind(py).as_any().len().unwrap(), 0); + assert_eq!(inst.bind(py).len().unwrap(), 0); }); }