Skip to content

FIx method resolution in number slots when a single method is a subclass #6228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MatthieuDartiailh wants to merge 7 commits into
base: main
Choose a base branch
from
Open

FIx method resolution in number slots when a single method is a subclass #6228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
775dded
rework binop slot dispatching to be type based and dispatch to parent…
MatthieuDartiailh Jul 23, 2026
8dc3388
tests: add tests demonstrating proper resolution of add/radd when sub…
MatthieuDartiailh Jul 23, 2026
9f19128
add newfragment
MatthieuDartiailh Jul 23, 2026
d84ec67
use SelfConversionPolicy::trusted now that we do not call blindly fra…
MatthieuDartiailh Jul 24, 2026
205d1b9
add safety comment
MatthieuDartiailh Jul 24, 2026
dbcd614
fix macro namespacing
MatthieuDartiailh Jul 24, 2026
268e604
do not return early if method lookup fails on LHS since RHS may work
MatthieuDartiailh Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/6228.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix reversible arithmetic operators don't respect subclassing if only one arm implemented
38 changes: 24 additions & 14 deletions pyo3-macros-backend/src/pyimpl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__",
Expand All @@ -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__",
Expand Down
16 changes: 7 additions & 9 deletions pyo3-macros-backend/src/pymethod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1655,20 +1655,18 @@ 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(),
// 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() },
}
}

Expand Down
256 changes: 240 additions & 16 deletions src/impl_/pyclass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use core::{
ffi::{c_int, c_void},
marker::PhantomData,
ptr::{self, NonNull},
result::Result,
};
use std::{sync::Mutex, thread};

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -563,21 +563,129 @@ 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,
_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_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) };
}

::core::result::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::*;
use $crate::types::PyAnyMethods;
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) {
::core::result::Result::Ok(s) => s,
::core::result::Result::Err(e) => return ::core::result::Result::Err(e),
};
Comment thread
MatthieuDartiailh marked this conversation as resolved.
match super_obj.call_method1(reflected_name, (&lhs_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 {
return ::core::result::Result::Err(e);
}
}
}
}
::core::result::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::*;
use $crate::types::PyAnyMethods;

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) {
::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,)) {
::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,
// 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);
}
}
}
}

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) };
}

::core::result::Result::Ok(py.NotImplemented().into_ptr())
}

$crate::ffi::PyType_Slot {
Expand Down Expand Up @@ -744,22 +852,138 @@ 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,
_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_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) };

@chirizxc chirizxc Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we can add a fast path so that direct type object comparisons avoid the creation of a Bound:

let lhs_type = unsafe { (*_slf).ob_type };
let type_ = <$cls as $crate::PyTypeInfo>::type_object_raw(py);
if lhs_type == type_ {
	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.__rpow__(py, _other, _slf, _mod) };
}

::core::result::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) => {{

@chirizxc chirizxc Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same type optimization as above:

                     let collector = PyClassImplCollector::<$cls>::new();
                       
-                    let lhs_obj = unsafe { $crate::Bound::from_borrowed_ptr(py, _slf) };
-                    if lhs_obj.is_instance_of::<$cls>() {
+                    let lhs_type = unsafe { (*_slf).ob_type };
+                    let type_ = <$cls as $crate::PyTypeInfo>::type_object_raw(py);
+                    if lhs_type == type_ || unsafe {
+                        $crate::Bound::from_borrowed_ptr(py, _slf).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>() {
+                    let rhs_type = unsafe { (*_other).ob_type };
+                    if rhs_type == type_ || unsafe {
+                        $crate::Bound::from_borrowed_ptr(py, _other).is_instance_of::<$cls>()
+                    } {
                         return unsafe { collector.$rhs(py, _other, _slf) };
                     }

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::*;
use $crate::types::PyAnyMethods;
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) {
::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)) {
::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 {
return ::core::result::Result::Err(e);
}
}
}
}
::core::result::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::*;
use $crate::types::PyAnyMethods;

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)) {
::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,
// 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);
}
}
}
}

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) };
}

::core::result::Result::Ok(py.NotImplemented().into_ptr())
}

$crate::ffi::PyType_Slot {
Expand Down
Loading