Comparisons with __eq__ should not raise TypeError

[davidhewitt]

Consider this implementation of __richcmp__:

#[pyproto]
impl<'p> PyObjectProtocol<'p> for MyClass {
    fn __richcmp__(
        &'p self,
        other: PyRef<'p, MyClass>,
        op: CompareOp,
    ) -> PyResult<PyObject> {
        match op {
            CompareOp::Eq => Ok(self.eq(&*other).into_py(other.py())),
            _ => Ok(other.py().NotImplemented()),
        }
    }
}

With this comparison, comparing MyClass() == 1 (for example) fails with TypeError: Can't convert 1 to MyClass.

This is fairly un-Pythonic; the usual situation for Python 3 is that == should return False if the types can't be compared.

[mvaled]

Hi @davidhewitt:

Minor correction on the issue: "should return False" should be edited to "should return NotImplemented".

[pganssle]

This is fairly un-Pythonic; the usual situation for Python 3 is that == should return False if the types can't be compared.

Just to clarify, __eq__(self, other) should return NotImplemented if self's class doesn't know how to compare itself to other's class. The == operator logic will then handle converting two NotImplemented responses (from a.__eq__(b) and b.__eq__(a) into False).

It seems like the problem may be in the definition for self.eq?

[davidhewitt]

Thanks for the clarifications! I think possibly the issue is in how we define __richcmp__. At the moment the proc-macro code can raise TypeError if argument conversion fails.

Potentially we want to instead let the proc-macro code return NotImplemented on argument conversion failure?

[mvaled]

Hi @davidhewitt

I'm willing to give this a fair try. I would need some mentoring, though. First I'm trying to produce a test, maybe in ./test_arithmetics.rs. Then I would try to change impl_proto_impl.

I don't know yet how the macro uses the PyObjectRichcmpProtocol to derive the implementation.

[mvaled]

I've created PR #1072 to address this issue. So far I've only create a simple test case; which, of course, fails.

[davidhewitt]

Many thanks @mvaled - I have some thoughts on the solution which I'll try to find time to write up later today!

[davidhewitt]

So here's my thoughts on this issue. It has some design questions which I think we should discuss to help us get the right design for the solution. Please challenge any or all of the below - it's only my ideas and could easily have issues I have not foreseen!

1. How far does this solution go?

So far I wrote only about __eq__ above, but really in the C-Api that is __richcmp__, which we already pointed out. However, I think __richcmp__ is not the only method that we should consider returning NotImplemented for. For example, if we look at the CPython set implementation (https://github.com/python/cpython/blob/master/Objects/setobject.c), we see it returns PY_RETURN_NOTIMPLEMENTED in many of the operator implementations.

We may want to start by making a list of all the protocol methods we think this behaviour is applicable for.

2. What's the intended outcome / API?

The equivalent concept in C++'s pybind11 is the py::is_operator marker. What that indicates is that during argument extraction errors should not be raised, and instead NotImplemented is returned. That seems reasonable to me, and it's roughly equivalent with what we see in the CPython set implementation linked above.

I don't think we need to add any marker here, and instead can do it under-the-hood for all methods identified in (1).

Because this'll add extra magic to #[pyproto] that swallows exceptions, we should make sure to carefully document the methods we add this behaviour to.

We might also want to consider adding an attribute e.g. #[pyo3(no_return_notimplemented)] which could be added to these methods to opt-out of this magic. Though I'm happy to punt opt-out until the future when someone asks for it!

3. What's the implementation?

Let's focus on __richcmp__ for the moment, which resembles the same structure that all the methods follow.

The #[pyproto] proc macro actually doesn't expand to the argument extraction (yet). Instead, if the #[pyproto] has __richcmp__, that leads to PyObjectMethods::set_richcompare() being called inside our setup code.

So you can follow the implementation from there. The function implementation that's instantiated is tp_richcompare which can be found at the bottom of src/class/basic.rs:

pyo3/src/class/basic.rs

Line 251 in 51171f7

fn tp_richcompare<T>() -> Option<ffi::richcmpfunc>

Inside that function, the line responsible for argument extraction is this one (currently 281):

pyo3/src/class/basic.rs

Line 281 in 51171f7

let arg = arg.extract()?;

So, if we go with the design I'm sketching out here, we could change that line so that instead of short-circuiting the error with ?, we could match on the argument extraction result and return py.NotImplemented() if necessary.

The rest of the protocol method implementations follow a very similar structure as above. The complication is that they are very uniform in structure so we often implement them using macro_rules! in src/class/macros.rs. You may find that depending on the methods we add this behaviour to, these macros need to be refactored slightly.

[mvaled]

Hi @davidhewitt

Thanks for your explanation. I have some study to do ;)

I'm allotting the next week end for this issue. I'll report back when I know more.