From 775dded3ad5305bd6490a842546847df00d2e13f Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Thu, 23 Jul 2026 22:34:32 +0200 Subject: [PATCH 1/7] rework binop slot dispatching to be type based and dispatch to parent class when necessary --- pyo3-macros-backend/src/pyimpl.rs | 38 +++-- src/impl_/pyclass.rs | 250 ++++++++++++++++++++++++++++-- 2 files changed, 258 insertions(+), 30 deletions(-) diff --git a/pyo3-macros-backend/src/pyimpl.rs b/pyo3-macros-backend/src/pyimpl.rs index b29c8344e3d..5a8d287ef36 100644 --- a/pyo3-macros-backend/src/pyimpl.rs +++ b/pyo3-macros-backend/src/pyimpl.rs @@ -319,6 +319,16 @@ fn add_shared_proto_slots( }}; } + macro_rules! try_add_binop_slot { + ($slot:ident, $forward:literal, $reflected:literal) => {{ + let has_forward = implemented_proto_fragments.remove($forward); + let has_reflected = implemented_proto_fragments.remove($reflected); + if has_forward || has_reflected { + proto_impls.push(quote! { #pyo3_path::impl_::pyclass::$slot!(#ty, #has_forward, #has_reflected) }); + } + }}; + } + try_add_shared_slot!( generate_pyclass_getattro_slot, "__getattribute__", @@ -327,24 +337,24 @@ fn add_shared_proto_slots( try_add_shared_slot!(generate_pyclass_setattr_slot, "__setattr__", "__delattr__"); try_add_shared_slot!(generate_pyclass_setdescr_slot, "__set__", "__delete__"); try_add_shared_slot!(generate_pyclass_setitem_slot, "__setitem__", "__delitem__"); - try_add_shared_slot!(generate_pyclass_add_slot, "__add__", "__radd__"); - try_add_shared_slot!(generate_pyclass_sub_slot, "__sub__", "__rsub__"); - try_add_shared_slot!(generate_pyclass_mul_slot, "__mul__", "__rmul__"); - try_add_shared_slot!(generate_pyclass_mod_slot, "__mod__", "__rmod__"); - try_add_shared_slot!(generate_pyclass_divmod_slot, "__divmod__", "__rdivmod__"); - try_add_shared_slot!(generate_pyclass_lshift_slot, "__lshift__", "__rlshift__"); - try_add_shared_slot!(generate_pyclass_rshift_slot, "__rshift__", "__rrshift__"); - try_add_shared_slot!(generate_pyclass_and_slot, "__and__", "__rand__"); - try_add_shared_slot!(generate_pyclass_or_slot, "__or__", "__ror__"); - try_add_shared_slot!(generate_pyclass_xor_slot, "__xor__", "__rxor__"); - try_add_shared_slot!(generate_pyclass_matmul_slot, "__matmul__", "__rmatmul__"); - try_add_shared_slot!(generate_pyclass_truediv_slot, "__truediv__", "__rtruediv__"); - try_add_shared_slot!( + try_add_binop_slot!(generate_pyclass_add_slot, "__add__", "__radd__"); + try_add_binop_slot!(generate_pyclass_sub_slot, "__sub__", "__rsub__"); + try_add_binop_slot!(generate_pyclass_mul_slot, "__mul__", "__rmul__"); + try_add_binop_slot!(generate_pyclass_mod_slot, "__mod__", "__rmod__"); + try_add_binop_slot!(generate_pyclass_divmod_slot, "__divmod__", "__rdivmod__"); + try_add_binop_slot!(generate_pyclass_lshift_slot, "__lshift__", "__rlshift__"); + try_add_binop_slot!(generate_pyclass_rshift_slot, "__rshift__", "__rrshift__"); + try_add_binop_slot!(generate_pyclass_and_slot, "__and__", "__rand__"); + try_add_binop_slot!(generate_pyclass_or_slot, "__or__", "__ror__"); + try_add_binop_slot!(generate_pyclass_xor_slot, "__xor__", "__rxor__"); + try_add_binop_slot!(generate_pyclass_matmul_slot, "__matmul__", "__rmatmul__"); + try_add_binop_slot!(generate_pyclass_truediv_slot, "__truediv__", "__rtruediv__"); + try_add_binop_slot!( generate_pyclass_floordiv_slot, "__floordiv__", "__rfloordiv__" ); - try_add_shared_slot!(generate_pyclass_pow_slot, "__pow__", "__rpow__"); + try_add_binop_slot!(generate_pyclass_pow_slot, "__pow__", "__rpow__"); try_add_shared_slot!( generate_pyclass_richcompare_slot, "__lt__", diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index acd8c2a3cda..676fcb835de 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -23,6 +23,7 @@ use core::{ ffi::{c_int, c_void}, marker::PhantomData, ptr::{self, NonNull}, + result::Result, }; use std::{sync::Mutex, thread}; @@ -517,7 +518,6 @@ define_pyclass_setattr_slot! { objobjargproc, } -/// Macro which expands to three items /// - Trait for a lhs dunder e.g. __add__ /// - Trait for the corresponding rhs e.g. __radd__ /// - A macro which will use dtolnay specialisation to generate the shared slot for the two dunders @@ -563,7 +563,8 @@ macro_rules! define_pyclass_binary_operator_slot { #[doc(hidden)] #[macro_export] macro_rules! $generate_macro { - ($cls:ty) => {{ + // Both forward and reflected available + ($cls:ty, true, true) => {{ unsafe fn slot_impl( py: $crate::Python<'_>, _slf: *mut $crate::ffi::PyObject, @@ -571,13 +572,117 @@ macro_rules! define_pyclass_binary_operator_slot { ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; let collector = PyClassImplCollector::<$cls>::new(); - let lhs_result = unsafe { collector.$lhs(py, _slf, _other) }?; - if lhs_result == unsafe { $crate::ffi::Py_NotImplemented() } { - unsafe { $crate::ffi::Py_DECREF(lhs_result) }; - unsafe { collector.$rhs(py, _other, _slf) } - } else { - ::core::result::Result::Ok(lhs_result) + + let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; + if lhs_obj.is_instance_of::<$cls>() { + return unsafe { collector.$lhs(py, _slf, _other) }; + } + + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + if rhs_obj.is_instance_of::<$cls>() { + return unsafe { collector.$rhs(py, _other, _slf) }; + } + + Result::Ok(py.NotImplemented().into_ptr()) + } + + $crate::ffi::PyType_Slot { + slot: $crate::ffi::$slot, + pfunc: $crate::impl_::trampoline::get_trampoline_function!( + binaryfunc, slot_impl + ) as $crate::ffi::binaryfunc as _, + } + }}; + + // Only forward available + ($cls:ty, true, false) => {{ + unsafe fn slot_impl( + py: $crate::Python<'_>, + _slf: *mut $crate::ffi::PyObject, + _other: *mut $crate::ffi::PyObject, + ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { + use $crate::impl_::pyclass::*; + let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; + + if lhs_obj.is_instance_of::<$cls>() { + let collector = PyClassImplCollector::<$cls>::new(); + return unsafe { collector.$lhs(py, _slf, _other) }; + } + + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + if rhs_obj.is_instance_of::<$cls>() { + // RHS is our class, try parent's reflected method via super() + // This ensures parent knows it's being called for reflection + let py_type = <$cls as $crate::PyTypeInfo>::type_object(py); + let reflected_name = $crate::intern!(py, concat!(stringify!($rhs))); + let super_obj = match $crate::types::PySuper::new(&py_type, &rhs_obj) { + Ok(s) => s, + Err(e) => return Err(e), + }; + match super_obj.call_method1(reflected_name, (&lhs_obj,)) { + Ok(result) => return Ok(result.into_ptr()), + Err(e) => { + // If parent doesn't implement the method, return NotImplemented instead of AttributeError + if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { + return Ok(py.NotImplemented().into_ptr()); + } else { + return Err(e); + } + } + } + } + Result::Ok(py.NotImplemented().into_ptr()) + } + + $crate::ffi::PyType_Slot { + slot: $crate::ffi::$slot, + pfunc: $crate::impl_::trampoline::get_trampoline_function!( + binaryfunc, slot_impl + ) as $crate::ffi::binaryfunc as _, + } + }}; + + // Only reflected available + ($cls:ty, false, true) => {{ + unsafe fn slot_impl( + py: $crate::Python<'_>, + _slf: *mut $crate::ffi::PyObject, + _other: *mut $crate::ffi::PyObject, + ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { + use $crate::impl_::pyclass::*; + + let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; + if lhs_obj.is_instance_of::<$cls>() { + // LHS is our class, try parent's forward method via super() + // This ensures parent knows it's being called for forward dispatch + let py_type = <$cls as $crate::PyTypeInfo>::type_object(py); + let forward_name = $crate::intern!(py, concat!(stringify!($lhs))); + let other_bound = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + let super_obj = match $crate::types::PySuper::new(&py_type, &lhs_obj) { + Ok(s) => s, + Err(e) => return Err(e), + }; + match super_obj.call_method1(forward_name, (&other_bound,)) { + Ok(result) => return Ok(result.into_ptr()), + Err(e) => { + // If parent doesn't implement the method, return NotImplemented instead of AttributeError + if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { + return Ok(py.NotImplemented().into_ptr()); + } else { + return Err(e); + } + } + } } + + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + let rhs_is_instance = rhs_obj.is_instance_of::<$cls>(); + if rhs_is_instance { + let collector = PyClassImplCollector::<$cls>::new(); + return unsafe { collector.$rhs(py, _other, _slf) }; + } + + Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { @@ -744,8 +849,9 @@ slot_fragment_trait! { #[doc(hidden)] #[macro_export] macro_rules! generate_pyclass_pow_slot { - ($cls:ty) => {{ - fn slot_impl( + // Both forward and reflected available + ($cls:ty, true, true) => {{ + unsafe fn slot_impl( py: $crate::Python<'_>, _slf: *mut $crate::ffi::PyObject, _other: *mut $crate::ffi::PyObject, @@ -753,13 +859,125 @@ macro_rules! generate_pyclass_pow_slot { ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; let collector = PyClassImplCollector::<$cls>::new(); - let lhs_result = unsafe { collector.__pow__(py, _slf, _other, _mod) }?; - if lhs_result == unsafe { $crate::ffi::Py_NotImplemented() } { - unsafe { $crate::ffi::Py_DECREF(lhs_result) }; - unsafe { collector.__rpow__(py, _other, _slf, _mod) } - } else { - ::core::result::Result::Ok(lhs_result) + + let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; + if lhs_obj.is_instance_of::<$cls>() { + return unsafe { collector.__pow__(py, _slf, _other, _mod) }; } + + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + if rhs_obj.is_instance_of::<$cls>() { + return unsafe { collector.__rpow__(py, _other, _slf, _mod) }; + } + + Result::Ok(py.NotImplemented().into_ptr()) + } + + $crate::ffi::PyType_Slot { + slot: $crate::ffi::Py_nb_power, + pfunc: $crate::impl_::trampoline::get_trampoline_function!(ternaryfunc, slot_impl) + as $crate::ffi::ternaryfunc as _, + } + }}; + + // Only forward available + ($cls:ty, true, false) => {{ + unsafe fn slot_impl( + py: $crate::Python<'_>, + _slf: *mut $crate::ffi::PyObject, + _other: *mut $crate::ffi::PyObject, + _mod: *mut $crate::ffi::PyObject, + ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { + use $crate::impl_::pyclass::*; + let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; + + if lhs_obj.is_instance_of::<$cls>() { + let collector = PyClassImplCollector::<$cls>::new(); + return unsafe { collector.__pow__(py, _slf, _other, _mod) }; + } + + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + if rhs_obj.is_instance_of::<$cls>() { + // RHS is our class, try parent's reflected method via super() + // This ensures parent knows it's being called for reflection + let py_type = <$cls as $crate::PyTypeInfo>::type_object(py); + let mod_obj = if _mod.is_null() { + py.None().into_bound(py) + } else { + unsafe { $crate::Bound::from_borrowed_ptr(py, _mod) } + }; + let super_obj = match $crate::types::PySuper::new(&py_type, &rhs_obj) { + Ok(s) => s, + Err(e) => return Err(e), + }; + match super_obj.call_method1($crate::intern!(py, "__rpow__"), (&lhs_obj, mod_obj)) { + Ok(result) => return Ok(result.into_ptr()), + Err(e) => { + // If parent doesn't implement the method, return NotImplemented instead of AttributeError + if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { + return Ok(py.NotImplemented().into_ptr()); + } else { + return Err(e); + } + } + } + } + Result::Ok(py.NotImplemented().into_ptr()) + } + + $crate::ffi::PyType_Slot { + slot: $crate::ffi::Py_nb_power, + pfunc: $crate::impl_::trampoline::get_trampoline_function!(ternaryfunc, slot_impl) + as $crate::ffi::ternaryfunc as _, + } + }}; + + // Only reflected available + ($cls:ty, false, true) => {{ + unsafe fn slot_impl( + py: $crate::Python<'_>, + _slf: *mut $crate::ffi::PyObject, + _other: *mut $crate::ffi::PyObject, + _mod: *mut $crate::ffi::PyObject, + ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { + use $crate::impl_::pyclass::*; + + let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; + if lhs_obj.is_instance_of::<$cls>() { + // LHS is our class, try parent's forward method via super() + // This ensures parent knows it's being called for forward dispatch + let py_type = <$cls as $crate::PyTypeInfo>::type_object(py); + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + let mod_obj = if _mod.is_null() { + py.None().into_bound(py) + } else { + unsafe { $crate::Bound::from_borrowed_ptr(py, _mod) } + }; + let super_obj = match $crate::types::PySuper::new(&py_type, &lhs_obj) { + Ok(s) => s, + Err(e) => return Err(e), + }; + match super_obj.call_method1($crate::intern!(py, "__pow__"), (&rhs_obj, mod_obj)) { + Ok(result) => return Ok(result.into_ptr()), + Err(e) => { + // If parent doesn't implement the method, return NotImplemented instead of AttributeError + if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { + return Ok(py.NotImplemented().into_ptr()); + } else { + return Err(e); + } + } + } + } + + let rhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; + let rhs_is_instance = rhs_obj.is_instance_of::<$cls>(); + if rhs_is_instance { + let collector = PyClassImplCollector::<$cls>::new(); + return unsafe { collector.__rpow__(py, _other, _slf, _mod) }; + } + + Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { From 8dc3388ed8d70c469362d18f2997e7b2c7b6a652 Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Fri, 24 Jul 2026 00:22:52 +0200 Subject: [PATCH 2/7] tests: add tests demonstrating proper resolution of add/radd when subclass implement a single method --- tests/test_arithmetics.rs | 243 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index 2cf21e59601..707b9576160 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -521,6 +521,249 @@ fn lhs_fellback_to_rhs() { }); } +// ============================================================================ +// Binary Operator Inheritance Tests +// Tests super() delegation with parent classes and multiple inheritance +// ============================================================================ + +/// Test 1: Parent Forward → Child Reflected (Rust only) +/// Parent implements __add__, child implements __radd__ +#[pyclass(subclass)] +struct AddBase; + +#[pymethods] +impl AddBase { + fn __repr__(&self) -> &'static str { + "AddBase" + } + + fn __add__(&self, other: &Bound<'_, PyAny>) -> String { + format!("AddBase.__add__({other:?})") + } +} + +#[pyclass(extends=AddBase)] +struct AddChild; + +#[pymethods] +impl AddChild { + fn __repr__(&self) -> &'static str { + "AddChild" + } + + fn __radd__(&self, other: &Bound<'_, PyAny>) -> String { + format!("AddChild.__radd__({other:?})") + } +} + +#[test] +fn binary_inheritance_forward_to_reflected() { + Python::attach(|py| { + let child = Py::new(py, (AddChild, AddBase)).unwrap(); + + py_run!( + py, + child, + r#" +assert child + 5 == "AddBase.__add__(5)", f"Forward (child + 5): expected 'AddBase.__add__(5)', got {child + 5!r}" +assert 5 + child == "AddChild.__radd__(5)", f"Reflected (5 + child): expected 'AddChild.__radd__(5)', got {5 + child!r}" + "# + ); + }); +} + +/// Test 2: Parent Reflected → Child Forward (Rust only) +/// Parent implements __radd__, child implements __add__ +#[pyclass(subclass)] +struct RAddBase; + +#[pymethods] +impl RAddBase { + fn __repr__(&self) -> &'static str { + "RAddBase" + } + + fn __radd__(&self, other: &Bound<'_, PyAny>) -> String { + format!("RAddBase.__radd__({other:?})") + } +} + +#[pyclass(extends=RAddBase)] +struct RAddChild; + +#[pymethods] +impl RAddChild { + fn __repr__(&self) -> &'static str { + "RAddChild" + } + + fn __add__(&self, other: &Bound<'_, PyAny>) -> String { + format!("RAddChild.__add__({other:?})") + } +} + +#[test] +fn binary_inheritance_reflected_to_forward() { + Python::attach(|py| { + let child = Py::new(py, (RAddChild, RAddBase)).unwrap(); + + py_run!( + py, + child, + r#" +assert child + 5 == "RAddChild.__add__(5)", f"Forward (child + 5): expected 'RAddChild.__add__(5)', got {child + 5!r}" +assert 5 + child == "RAddBase.__radd__(5)", f"Reflected (5 + child): expected 'RAddBase.__radd__(5)', got {5 + child!r}" + "# + ); + }); +} + +/// Test 3: Rust Child Forward + Pure Python Mixin Reflected +/// Tests both MRO orders: (RustImpl, PyMixin) and (PyMixin, RustImpl) +#[pyclass(subclass)] +struct ForwardOnly; + +#[pymethods] +impl ForwardOnly { + #[new] + fn new() -> Self { + ForwardOnly + } + + fn __repr__(&self) -> &'static str { + "ForwardOnly" + } + + fn __add__(&self, other: &Bound<'_, PyAny>) -> String { + format!("ForwardOnly.__add__({other:?})") + } +} + +#[test] +fn binary_inheritance_rust_forward_py_reflected_mro_rust_first() { + Python::attach(|py| { + // Create ForwardOnly instance to get its type, then use in Python inheritance + let forward_only = Bound::new(py, ForwardOnly).unwrap(); + let forward_type = forward_only.get_type(); + + py_run!( + py, + forward_type, + r#" +class PyReflectedMixin: + def __radd__(self, other): + return f"PyReflectedMixin.__radd__({other!r})" + +class ChildRustFirst(forward_type, PyReflectedMixin): + pass + +child = ChildRustFirst() +assert child + 5 == "ForwardOnly.__add__(5)", f"Forward (child + 5): expected 'ForwardOnly.__add__(5)', got {child + 5!r}" +assert 5 + child == "PyReflectedMixin.__radd__(5)", f"Reflected (5 + child): expected 'PyReflectedMixin.__radd__(5)', got {5 + child!r}" + "# + ); + }); +} + +#[test] +fn binary_inheritance_rust_forward_py_reflected_mro_py_first() { + Python::attach(|py| { + // Create ForwardOnly instance to get its type, then use in Python inheritance + let forward_only = Bound::new(py, ForwardOnly).unwrap(); + let forward_type = forward_only.get_type(); + + py_run!( + py, + forward_type, + r#" +class PyReflectedMixin: + def __radd__(self, other): + return f"PyReflectedMixin.__radd__({other!r})" + +class ChildPyFirst(PyReflectedMixin, forward_type): + pass + +child = ChildPyFirst() +assert child + 5 == "ForwardOnly.__add__(5)", f"Forward (child + 5): expected 'ForwardOnly.__add__(5)', got {child + 5!r}" +assert 5 + child == "PyReflectedMixin.__radd__(5)", f"Reflected (5 + child): expected 'PyReflectedMixin.__radd__(5)', got {5 + child!r}" + "# + ); + }); +} + +/// Test 4: Rust Child Reflected + Pure Python Mixin Forward +/// Tests both MRO orders: (RustImpl, PyMixin) and (PyMixin, RustImpl) +#[pyclass(subclass)] +struct ReflectedOnly; + +#[pymethods] +impl ReflectedOnly { + #[new] + fn new() -> Self { + ReflectedOnly + } + + fn __repr__(&self) -> &'static str { + "ReflectedOnly" + } + + fn __radd__(&self, other: &Bound<'_, PyAny>) -> String { + format!("ReflectedOnly.__radd__({other:?})") + } +} + +#[test] +fn binary_inheritance_rust_reflected_py_forward_mro_rust_first() { + Python::attach(|py| { + // Create ReflectedOnly instance to get its type, then use in Python inheritance + let reflected_only = Bound::new(py, ReflectedOnly).unwrap(); + let reflected_type = reflected_only.get_type(); + + py_run!( + py, + reflected_type, + r#" +class PyForwardMixin: + def __add__(self, other): + return f"PyForwardMixin.__add__({other!r})" + +class ChildRustFirst(reflected_type, PyForwardMixin): + pass + +child = ChildRustFirst() +assert child + 5 == "PyForwardMixin.__add__(5)", f"Forward (child + 5): expected 'PyForwardMixin.__add__(5)', got {child + 5!r}" +assert 5 + child == "ReflectedOnly.__radd__(5)", f"Reflected (5 + child): expected 'ReflectedOnly.__radd__(5)', got {5 + child!r}" + "# + ); + }); +} + +#[test] +fn binary_inheritance_rust_reflected_py_forward_mro_py_first() { + Python::attach(|py| { + // Create ReflectedOnly instance to get its type, then use in Python inheritance + let reflected_only = Bound::new(py, ReflectedOnly).unwrap(); + let reflected_type = reflected_only.get_type(); + + py_run!( + py, + reflected_type, + r#" +class PyForwardMixin: + def __add__(self, other): + return f"PyForwardMixin.__add__({other!r})" + +class ChildPyFirst(PyForwardMixin, reflected_type): + pass + +child = ChildPyFirst() +assert child + 5 == "PyForwardMixin.__add__(5)", f"Forward (child + 5): expected 'PyForwardMixin.__add__(5)', got {child + 5!r}" +assert 5 + child == "ReflectedOnly.__radd__(5)", f"Reflected (5 + child): expected 'ReflectedOnly.__radd__(5)', got {5 + child!r}" + "# + ); + }); +} + #[pyclass] struct RichComparisons {} From 9f19128909b2455e9ddc0ebbead9f895757d1268 Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Fri, 24 Jul 2026 00:28:05 +0200 Subject: [PATCH 3/7] add newfragment --- newsfragments/6228.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/6228.fixed.md diff --git a/newsfragments/6228.fixed.md b/newsfragments/6228.fixed.md new file mode 100644 index 00000000000..bf8ffc11a01 --- /dev/null +++ b/newsfragments/6228.fixed.md @@ -0,0 +1 @@ +Fix reversible arithmetic operators don't respect subclassing if only one arm implemented Closes #6024 From d84ec67d780b84ed3606c02f554316c66d7b2ce0 Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Fri, 24 Jul 2026 19:06:26 +0200 Subject: [PATCH 4/7] use SelfConversionPolicy::trusted now that we do not call blindly fragments anymore --- pyo3-macros-backend/src/pymethod.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index c529c40f4ff..fa9e83d5544 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1655,20 +1655,16 @@ impl SlotFragmentDef { const fn binary_operator(fragment: &'static str) -> Self { // Binary operator fragments (`__add__`, `__radd__`, etc.) are combined - // into a shared slot (e.g. `nb_add`) that may call the forward fragment - // with a non-class receiver (e.g. `1 + MyClass()` → `nb_add(1, c)`). - // The runtime helper then tries the reflected fragment with the operands - // swapped, which can also produce a non-class `_slf`. Both cases require - // a checked type conversion so that a mismatch gracefully returns - // `NotImplemented` rather than causing undefined behaviour. - // TODO: addressing #6024 could allow to simplify this by using type - // checks to directly dispatch to the right fragment. + // into a shared slot (e.g. `nb_add`) that dispatch to the appropriate + // fragment based on the receiver type. + // Direct calls to the fragment through method wrappers are guaranteed + // to have a receiver of the correct type. SlotFragmentDef { fragment, arguments: &[Ty::Object], extract_error_mode: ExtractErrorMode::NotImplemented, ret_ty: Ty::Object, - self_conversion: SelfConversionPolicy::checked(), + self_conversion: unsafe { SelfConversionPolicy::trusted() }, } } From 205d1b9b36ee0f8bce4bc8807414dde052fd89e8 Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Fri, 24 Jul 2026 20:33:55 +0200 Subject: [PATCH 5/7] add safety comment --- pyo3-macros-backend/src/pymethod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index fa9e83d5544..8811e03aa4a 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1664,6 +1664,8 @@ impl SlotFragmentDef { arguments: &[Ty::Object], extract_error_mode: ExtractErrorMode::NotImplemented, ret_ty: Ty::Object, + // SAFETY: Combined slot dispatch based on type which guarantees + // the receiver is of the correct type for direct calls to the fragment. self_conversion: unsafe { SelfConversionPolicy::trusted() }, } } From dbcd614922f52742abc2133cbaffb217598a3cc3 Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Fri, 24 Jul 2026 23:06:05 +0200 Subject: [PATCH 6/7] fix macro namespacing --- newsfragments/6228.fixed.md | 2 +- src/impl_/pyclass.rs | 62 ++++++++++++++++++++----------------- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/newsfragments/6228.fixed.md b/newsfragments/6228.fixed.md index bf8ffc11a01..5e07d333d2f 100644 --- a/newsfragments/6228.fixed.md +++ b/newsfragments/6228.fixed.md @@ -1 +1 @@ -Fix reversible arithmetic operators don't respect subclassing if only one arm implemented Closes #6024 +Fix reversible arithmetic operators don't respect subclassing if only one arm implemented diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 676fcb835de..1f501fbffbe 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -571,6 +571,7 @@ macro_rules! define_pyclass_binary_operator_slot { _other: *mut $crate::ffi::PyObject, ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; + use $crate::types::PyAnyMethods; let collector = PyClassImplCollector::<$cls>::new(); let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; @@ -583,7 +584,7 @@ macro_rules! define_pyclass_binary_operator_slot { return unsafe { collector.$rhs(py, _other, _slf) }; } - Result::Ok(py.NotImplemented().into_ptr()) + ::core::result::Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { @@ -602,6 +603,7 @@ macro_rules! define_pyclass_binary_operator_slot { _other: *mut $crate::ffi::PyObject, ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; + use $crate::types::PyAnyMethods; let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; if lhs_obj.is_instance_of::<$cls>() { @@ -616,22 +618,22 @@ macro_rules! define_pyclass_binary_operator_slot { let py_type = <$cls as $crate::PyTypeInfo>::type_object(py); let reflected_name = $crate::intern!(py, concat!(stringify!($rhs))); let super_obj = match $crate::types::PySuper::new(&py_type, &rhs_obj) { - Ok(s) => s, - Err(e) => return Err(e), + ::core::result::Result::Ok(s) => s, + ::core::result::Result::Err(e) => return ::core::result::Result::Err(e), }; match super_obj.call_method1(reflected_name, (&lhs_obj,)) { - Ok(result) => return Ok(result.into_ptr()), - Err(e) => { + ::core::result::Result::Ok(result) => return ::core::result::Result::Ok(result.into_ptr()), + ::core::result::Result::Err(e) => { // If parent doesn't implement the method, return NotImplemented instead of AttributeError if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { - return Ok(py.NotImplemented().into_ptr()); + return ::core::result::Result::Ok(py.NotImplemented().into_ptr()); } else { - return Err(e); + return ::core::result::Result::Err(e); } } } } - Result::Ok(py.NotImplemented().into_ptr()) + ::core::result::Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { @@ -650,6 +652,7 @@ macro_rules! define_pyclass_binary_operator_slot { _other: *mut $crate::ffi::PyObject, ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; + use $crate::types::PyAnyMethods; let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; if lhs_obj.is_instance_of::<$cls>() { @@ -659,17 +662,17 @@ macro_rules! define_pyclass_binary_operator_slot { let forward_name = $crate::intern!(py, concat!(stringify!($lhs))); let other_bound = unsafe { $crate::Bound::from_borrowed_ptr(py, _other) }; let super_obj = match $crate::types::PySuper::new(&py_type, &lhs_obj) { - Ok(s) => s, - Err(e) => return Err(e), + ::core::result::Result::Ok(s) => s, + ::core::result::Result::Err(e) => return ::core::result::Result::Err(e), }; match super_obj.call_method1(forward_name, (&other_bound,)) { - Ok(result) => return Ok(result.into_ptr()), - Err(e) => { + ::core::result::Result::Ok(result) => return ::core::result::Result::Ok(result.into_ptr()), + ::core::result::Result::Err(e) => { // If parent doesn't implement the method, return NotImplemented instead of AttributeError if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { - return Ok(py.NotImplemented().into_ptr()); + return ::core::result::Result::Ok(py.NotImplemented().into_ptr()); } else { - return Err(e); + return ::core::result::Result::Err(e); } } } @@ -682,7 +685,7 @@ macro_rules! define_pyclass_binary_operator_slot { return unsafe { collector.$rhs(py, _other, _slf) }; } - Result::Ok(py.NotImplemented().into_ptr()) + ::core::result::Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { @@ -858,6 +861,7 @@ macro_rules! generate_pyclass_pow_slot { _mod: *mut $crate::ffi::PyObject, ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; + use $crate::types::PyAnyMethods; let collector = PyClassImplCollector::<$cls>::new(); let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; @@ -870,7 +874,7 @@ macro_rules! generate_pyclass_pow_slot { return unsafe { collector.__rpow__(py, _other, _slf, _mod) }; } - Result::Ok(py.NotImplemented().into_ptr()) + ::core::result::Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { @@ -889,6 +893,7 @@ macro_rules! generate_pyclass_pow_slot { _mod: *mut $crate::ffi::PyObject, ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; + use $crate::types::PyAnyMethods; let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; if lhs_obj.is_instance_of::<$cls>() { @@ -907,22 +912,22 @@ macro_rules! generate_pyclass_pow_slot { unsafe { $crate::Bound::from_borrowed_ptr(py, _mod) } }; let super_obj = match $crate::types::PySuper::new(&py_type, &rhs_obj) { - Ok(s) => s, - Err(e) => return Err(e), + ::core::result::Result::Ok(s) => s, + ::core::result::Result::Err(e) => return ::core::result::Result::Err(e), }; match super_obj.call_method1($crate::intern!(py, "__rpow__"), (&lhs_obj, mod_obj)) { - Ok(result) => return Ok(result.into_ptr()), - Err(e) => { + ::core::result::Result::Ok(result) => return ::core::result::Result::Ok(result.into_ptr()), + ::core::result::Result::Err(e) => { // If parent doesn't implement the method, return NotImplemented instead of AttributeError if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { - return Ok(py.NotImplemented().into_ptr()); + return ::core::result::Result::Ok(py.NotImplemented().into_ptr()); } else { - return Err(e); + return ::core::result::Result::Err(e); } } } } - Result::Ok(py.NotImplemented().into_ptr()) + ::core::result::Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { @@ -941,6 +946,7 @@ macro_rules! generate_pyclass_pow_slot { _mod: *mut $crate::ffi::PyObject, ) -> $crate::PyResult<*mut $crate::ffi::PyObject> { use $crate::impl_::pyclass::*; + use $crate::types::PyAnyMethods; let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) }; if lhs_obj.is_instance_of::<$cls>() { @@ -958,13 +964,13 @@ macro_rules! generate_pyclass_pow_slot { Err(e) => return Err(e), }; match super_obj.call_method1($crate::intern!(py, "__pow__"), (&rhs_obj, mod_obj)) { - Ok(result) => return Ok(result.into_ptr()), - Err(e) => { + ::core::result::Result::Ok(result) => return ::core::result::Result::Ok(result.into_ptr()), + ::core::result::Result::Err(e) => { // If parent doesn't implement the method, return NotImplemented instead of AttributeError if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { - return Ok(py.NotImplemented().into_ptr()); + return ::core::result::Result::Ok(py.NotImplemented().into_ptr()); } else { - return Err(e); + return ::core::result::Result::Err(e); } } } @@ -977,7 +983,7 @@ macro_rules! generate_pyclass_pow_slot { return unsafe { collector.__rpow__(py, _other, _slf, _mod) }; } - Result::Ok(py.NotImplemented().into_ptr()) + ::core::result::Result::Ok(py.NotImplemented().into_ptr()) } $crate::ffi::PyType_Slot { From 268e60487ba92e0f7c2d8b611456abcd6e9b66c1 Mon Sep 17 00:00:00 2001 From: mdartiailh Date: Fri, 24 Jul 2026 23:52:18 +0200 Subject: [PATCH 7/7] do not return early if method lookup fails on LHS since RHS may work Expand test_arithmetics to cover this case --- src/impl_/pyclass.rs | 16 +++---- tests/test_arithmetics.rs | 99 +++++++++++++++++++++++++++++++++++---- 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 1f501fbffbe..066792e6685 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -668,10 +668,10 @@ macro_rules! define_pyclass_binary_operator_slot { match super_obj.call_method1(forward_name, (&other_bound,)) { ::core::result::Result::Ok(result) => return ::core::result::Result::Ok(result.into_ptr()), ::core::result::Result::Err(e) => { - // If parent doesn't implement the method, return NotImplemented instead of AttributeError - if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { - return ::core::result::Result::Ok(py.NotImplemented().into_ptr()); - } else { + // If parent doesn't implement the method, + // try the reflected method on our class instead + // of returning NotImplemented for AttributeError + if !e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { return ::core::result::Result::Err(e); } } @@ -966,10 +966,10 @@ macro_rules! generate_pyclass_pow_slot { match super_obj.call_method1($crate::intern!(py, "__pow__"), (&rhs_obj, mod_obj)) { ::core::result::Result::Ok(result) => return ::core::result::Result::Ok(result.into_ptr()), ::core::result::Result::Err(e) => { - // If parent doesn't implement the method, return NotImplemented instead of AttributeError - if e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { - return ::core::result::Result::Ok(py.NotImplemented().into_ptr()); - } else { + // If parent doesn't implement the method, + // try the reflected method on our class instead + // of returning NotImplemented for AttributeError + if !e.is_instance_of::<$crate::exceptions::PyAttributeError>(py) { return ::core::result::Result::Err(e); } } diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index 707b9576160..a0116a833cb 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -327,39 +327,111 @@ struct RhsArithmetic(String); #[pymethods] impl RhsArithmetic { fn __radd__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} + {}", self.0) + if other.is_instance_of::() { + format!( + "{} + {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} + {}", self.0) + } } fn __rsub__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} - {}", self.0) + if other.is_instance_of::() { + format!( + "{} - {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} - {}", self.0) + } } fn __rmul__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} * {}", self.0) + if other.is_instance_of::() { + format!( + "{} * {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} * {}", self.0) + } } fn __rlshift__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} << {}", self.0) + if other.is_instance_of::() { + format!( + "{} << {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} << {}", self.0) + } } fn __rrshift__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} >> {}", self.0) + if other.is_instance_of::() { + format!( + "{} >> {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} >> {}", self.0) + } } fn __rand__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} & {}", self.0) + if other.is_instance_of::() { + format!( + "{} & {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} & {}", self.0) + } } fn __rxor__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} ^ {}", self.0) + if other.is_instance_of::() { + format!( + "{} ^ {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} ^ {}", self.0) + } } fn __ror__(&self, other: &Bound<'_, PyAny>) -> String { - format!("{other:?} | {}", self.0) + if other.is_instance_of::() { + format!( + "{} | {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} | {}", self.0) + } } fn __rpow__(&self, other: &Bound<'_, PyAny>, _mod: Option<&Bound<'_, PyAny>>) -> String { - format!("{other:?} ** {}", self.0) + if other.is_instance_of::() { + format!( + "{} ** {}", + other.cast::().unwrap().borrow().0, + self.0 + ) + } else { + format!("{other:?} ** {}", self.0) + } } } @@ -369,22 +441,31 @@ fn rhs_arithmetic() { let c = Py::new(py, RhsArithmetic("RA".to_string())).unwrap(); py_run!(py, c, "assert c.__radd__(1) == '1 + RA'"); py_run!(py, c, "assert 1 + c == '1 + RA'"); + py_run!(py, c, "assert c + c == 'RA + RA'"); py_run!(py, c, "assert c.__rsub__(1) == '1 - RA'"); py_run!(py, c, "assert 1 - c == '1 - RA'"); + py_run!(py, c, "assert c - c == 'RA - RA'"); py_run!(py, c, "assert c.__rmul__(1) == '1 * RA'"); py_run!(py, c, "assert 1 * c == '1 * RA'"); + py_run!(py, c, "assert c * c == 'RA * RA'"); py_run!(py, c, "assert c.__rlshift__(1) == '1 << RA'"); py_run!(py, c, "assert 1 << c == '1 << RA'"); + py_run!(py, c, "assert c << c == 'RA << RA'"); py_run!(py, c, "assert c.__rrshift__(1) == '1 >> RA'"); py_run!(py, c, "assert 1 >> c == '1 >> RA'"); + py_run!(py, c, "assert c >> c == 'RA >> RA'"); py_run!(py, c, "assert c.__rand__(1) == '1 & RA'"); py_run!(py, c, "assert 1 & c == '1 & RA'"); + py_run!(py, c, "assert c & c == 'RA & RA'"); py_run!(py, c, "assert c.__rxor__(1) == '1 ^ RA'"); py_run!(py, c, "assert 1 ^ c == '1 ^ RA'"); + py_run!(py, c, "assert c ^ c == 'RA ^ RA'"); py_run!(py, c, "assert c.__ror__(1) == '1 | RA'"); py_run!(py, c, "assert 1 | c == '1 | RA'"); + py_run!(py, c, "assert c | c == 'RA | RA'"); py_run!(py, c, "assert c.__rpow__(1) == '1 ** RA'"); py_run!(py, c, "assert 1 ** c == '1 ** RA'"); + py_run!(py, c, "assert c ** c == 'RA ** RA'"); }); }