Pyo3: __radd__ etc don't get called if __add__ is also defined

[davidhewitt]

If we make a class like this:

#[pyclass]
#[derive(Debug)]
struct RhsArithmetic {}

#[pymethods]
impl RhsArithmetic {
    #[new]
    fn new() -> RhsArithmetic {
        RhsArithmetic {}
    }
}

#[pyproto]
impl PyNumberProtocol for RhsArithmetic {
    fn __mul__(lhs: PyRef<'p, Self>, rhs: &PyAny) -> PyResult<String> {
        Ok(format!("MUL"))
    }
    fn __rmul__(&self, other: &PyAny) -> PyResult<String> {
        Ok(format!("RMUL"))
    }
}

Then both 1 * rhs and rhs * 1 call the function in the tp_as_number.nb_mul slot. As of #839 this will be our __mul__ implementation above (and never __rmul__).

And specifically it will call it with the arguments in the same order as invoked. So from above, we will have __mul__(1, rhs) and __mul__(rhs, 1).

This is problematic, because __mul__ as defined above will raise a TypeError if the first argument is not Self.

We saw this behaviour reported on Gitter. All three of the below will currently be calling __mul__, which is why the second will TypeError:

>>> rhs * 1.0
'MUL'
>>> 1.0 * rhs # should call __rmul__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError
>>> rhs * rhs
'MUL'

I am thinking that instead of doing this, if both __mul__ and __rmul__ are defined we should be creating a wrapper function which calls the correct function depending on the type of the first argument.

The implementation of PyNumber_Add suggests that we should return Py_NotImplemented from this wrapper function if there is a type error, so that sq_concat is given a chance to be called also.

[programmerjake]

Similar logic in PyNumber_Multiply

[davidhewitt]

From a quick search I think the equivalent logic for the wrapper function for classes defined in Python is in this macro

[kngwyu]

This is difficult for our current protocol implementation, but after rewriting it without specialization, I think we can implement this.

[davidhewitt]

I'm nominating this for 0.12.

[davidhewitt]

@kngwyu just a heads up; I'm assigning this to you after the discussion in #1072 so that others don't spend time taking a stab at this while you're working on an implementation :)