Cannot return NotImplemented from in-place arithmetic operators

[jakelishman]

Bug Description

The in-place arithmetic magic methods (e.g. __iadd__) can return NotImplemented in Python space, and this is used to delegate a += b to a = type(b).__radd__(b, a) if a's __iadd__ returns NotImplemented. For example (pardon the terribly behaved objects):

class NoAdd:
    def __iadd__(self, other):
        return NotImplemented

class AllowsRightAdd:
    def __radd__(self, other):
        return "hello, world"

x = NoAdd()
x += AllowsRightAdd()
assert x == "hello, world"

If I understand the code/error message correctly, PyO3 requires the happy-path return from __iadd__ and other in-place operators to be () such that it can fill in the self return that Python wants. This makes it not possible to return NotImplemented to allow delegation in these methods.

Steps to Reproduce

Define a Rust #[pyclass] that implements one of the in-place arithmetic operators, attempting to return NotImplemented with any signature, including ones where the self object is returned. Something like this is probably minimal:

use pyo3::prelude::*;

#[pyclass]
struct MyStruct;

#[pymethods]
impl MyStruct {
    fn __iadd__(slf_: Bound<Self>, _other: Bound<PyAny>) -> Py<PyAny> {
        slf_.py().NotImplemented()
    }
}

#[pymodule]
fn core(m: &Bound<PyModule>) -> PyResult<()> {
    m.add_class::<MyStruct>()?;
    Ok(())
}

Backtrace

Compiling the above file gives something like

error[E0277]: the trait bound `pyo3::Py<pyo3::PyAny>: IntoPyCallbackOutput<()>` is not satisfied
   --> src/lib.rs:6:1
    |
6   | #[pymethods]
    | ^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<()>` is not implemented for `pyo3::Py<pyo3::PyAny>`
    |
    = help: the following other types implement trait `IntoPyCallbackOutput<Target>`:
              `()` implements `IntoPyCallbackOutput<()>`
              `()` implements `IntoPyCallbackOutput<i32>`
              `*mut PyObject` implements `IntoPyCallbackOutput<*mut PyObject>`
              `HashCallbackOutput` implements `IntoPyCallbackOutput<isize>`
              `Result<T, E>` implements `IntoPyCallbackOutput<U>`
              `bool` implements `IntoPyCallbackOutput<bool>`
              `bool` implements `IntoPyCallbackOutput<i32>`
              `usize` implements `IntoPyCallbackOutput<isize>`
              `usize` implements `IntoPyCallbackOutput<usize>`
note: required by a bound in `pyo3::callback::convert`
   --> /Users/jake/code/pyo3/pyo3/src/callback.rs:170:8
    |
168 | pub fn convert<T, U>(py: Python<'_>, value: T) -> PyResult<U>
    |        ------- required by a bound in this function
169 | where
170 |     T: IntoPyCallbackOutput<U>,
    |        ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `convert`
    = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `pyo3_test` (lib) due to 1 previous error

Your operating system and version

macOS 14

Your Python version (python --version)

Python 3.10.6

Your Rust version (rustc --version)

rustc 1.81.0 (eeb90cda1 2024-09-04)

Your PyO3 version

0.22.3 or commit 4cbf6e0

How did you install python? Did you use a virtualenv?

Built from source, run from a venv.

Additional Info

Fwiw, I don't think it's a good idea to write classes that do this, I just found this while trying to make a new type in Rust that matches some questionable Python-space behaviour of a pre-existing related class. I'm not actually going to match the behaviour in the end, so this isn't a high priority for me at all.

This feels like a case that could degrade the user experience to resolve if not careful - if the in-place arithmetic methods are updated to allow signatures that return arbitrary Python objects, we'd have to have a way of teaching users that they're supposed to return the self object from in-place operations in the happy path, which previously they've not had to do. It feels clear to me that the default and suggested form should continue to be to have the method return impl IntoPyCallbackOutput<()>, and anything that allows support for NotImplemented should be something that's explicitly opted into.

[jakelishman]

For example with the potential confusion, #4507 shows that there's definitely some expectations around the return types, and people who've tried to return self. I'm slightly worried that making it possible to return NotImplemented makes it quite easy to accidentally start doing the wrong thing.

Maybe a #[pyo3(arbitrary_inplace_return)] annotation or something like that to enable a different mode for the callback generation, so the user has to be way more explicit about doing something non-standard?

[davidhewitt]

I think there's definitely scope to improve here, and we can probably do it backwards-compatibly. Ideally we should match more closely to the Python behaviour, because that's most intuitive for users (as you yourself suggest).

Related to these operators is also #4026 where my main concern was the __radd__ operators and friends and the complexity we have internally there. Perhaps both of these problems can be solved at the same time.