Implement `PartialEq` for more `Bound<'py, T>` types

[davidhewitt]

After #4245 we have equality between Python str and Rust str, which I think is a nice user-facing feature.

There's a possibility to go further here and implement this for more types:

• PartialEq<[u8]> for Bound<'py, PyBytes> seems to fit the same category of behaviour
• PartialEq<i32> for Bound<'py, PyLong>, maybe? (And other int types)
• similar for floats?
• PartialEq<bool> for Bound<'py, PyBool>?

I think there's a reasonable case to be made that all of these can be implemented without risk of exception.

The main concern I have with these implementations is what to do with Python subclasses - as per #4245 (comment)

[davidhewitt]

In #4245 we decided that (at least for strings) it was appropriate to proceed in the face of subclasses and document this case. It might be for the other types that we have to make a decision whether the subclass risk is too high, but I think probably it's fine.

Also, labelling this as "good first issue" as each type can be implemented as a fairly small and straightforward PR.

[codeguru42]

@davidhewitt I'm starting to look at this

[davidhewitt]

Super, thanks!

[JRRudy1]

@davidhewitt I had an idea related to this...

What if we added a public pyo3::ops module containing traits analogous to those in std::ops, but defined something like this:

pub trait PyPartialEq<'py, Other: ?Sized = Bound<'py, Self>>: Sized {
    // users can impl this since `Bound` is more familiar
    fn bound_eq(slf: &Bound<'py, Self>, other: &Other) -> bool;
    
    // they could also override this if there's a better way
    #[inline]
    fn borrowed_eq(slf: &Borrowed<'_, 'py, Self>, other: &Other) -> bool {
        Self::bound_eq(slf, other)
    }
}

and then we provide blanket impls such as:

impl<'py, T, Other> PartialEq<Other> for Bound<'py, T> 
where
    T: PyPartialEq<'py, Other>
{
    #[inline]
    fn eq(&self, other: &Other) -> bool {
        T::bound_eq(self, other)
    }
}

impl<'a, 'py, T, Other> PartialEq<Other> for Borrowed<'a, 'py, T> 
where
    T: PyPartialEq<'py, Other>
{
    #[inline]
    fn eq(&self, other: &Other) -> bool {
        T::borrowed_eq(self, other)
    }
}

This can be used internally to define the impls from #4245 and this issue, while also letting users implement PyPartialEq for their types to get the impl PartialEq for Bound<MyType> that they currently can't implement themselves due to orphan rules.

For example the following would give us the PartialEq<str> for Bound<PyString> and PartialEq<str> for Borrowed<PyString> impls from #4245:

impl<'py> PyPartialEq<'py, str> for PyString {
    #[inline]
    fn bound_eq(slf: &Bound<'py, Self>, other: &str) -> bool {
        Self::borrowed_eq(&slf.as_borrowed(), other)
    }
    #[inline]
    fn borrowed_eq(slf: &Borrowed<'_, 'py, Self>, other: &str) -> bool {
        slf.to_cow().map_or(false, |s| s == *other)
    }
}

And the following would give a user PartialEq<Self> for Bound<MyClass> and PartialEq<Self> for Borrowed<MyClass>:

impl<'py> PyPartialEq<'py> for MyClass {
    fn bound_eq(slf: &Bound<'py, Self>, other: &Bound<'py, Self>) -> bool {
        slf.borrow().value == other.borrow().value
    }
}

Another place this could be useful is to get an implementation of PartialEq for Bound<numpy::PyArray<T, D>> by adding a PyPartialEq for PyArray<T, D> impl in the numpy crate.

Some details about this would need to be ironed out, but I think the general concept would be pretty useful.

[mejrs]

What if we added a public pyo3::ops module containing traits analogous to those in std::ops,

I'm reluctant to add more traits as we have a lot already. I don't think this trait is good or useful enough for that. (also, there are a lot of traits in std::ops and std::cmp. Do we make a corresponding trait for all of those?)

I wonder if we can we just have a blanket impl? Something like:

impl<T: PartialEq> PartialEq for Bound<T> {
    #[inline]
    fn eq(&self, other: &Self) -> bool {
        PartialEq::eq(self.get(), other)
    }
}

Regardless, I'd prefer we do this just for bytes and str and see how it works out in practice. We can consider more if there's a compelling use case or demand for them.

[JRRudy1]

I'm reluctant to add more traits as we have a lot already

IMO it shouldn't be an issue to add self-contained utility traits like this this that would give users more options without complicating existing mechanisms, but that's just me.

Do we make a corresponding trait for all of those?

I think there's a small handful of obvious candidates (PartialEq, Add, Sub, Mul, Div), it certainly doesn't need to be exhaustive and wouldn't even need to include all 5 at first. But if a contributor has a use for BitXorAssign and wants to submit a PR adding it, then sure why not. The traits would be very simple, well-defined, and independent.

I wonder if we can we just have a blanket impl? Something like:

That impl would need a T: PyClass<Frozen = False> bound to call the get method, which would be pretty limiting. No non-frozen pyclasses, no non-pyclasses like PyArray, and no reusing it for internal types like PyString. It would also couple it with the pyclass implementation which isn't ideal.

Regardless, I'd prefer we do this just for bytes and str and see how it works out in practice.

Fair enough, I wasn't trying to imply that this needs to happen now. I just thought it was an interesting concept to propose while on the topic of expanding the collection of PartialEq impls for Bound types, since I have personally seen use cases for it in my own work.

[davidhewitt]

IMO it shouldn't be an issue to add self-contained utility traits like this this that would give users more options without complicating existing mechanisms, but that's just me.

I'm somewhat wary of Rust traits for Python ops for a couple of reasons:

• Subclassing. For the cases here we've already discussed these and decided it's ok, but the further the types in question move out of builtins and into userland the higher the risk I percieve that subclasses will interact with Python equality in necessary ways.
• Fallibility. Python protocol operations may freely fail at runtime.

Given these two risks, it seems to me like the most reasonable best practice we can offer is the existing conveniences like PyAnyMethods::eq and PyAnyMethods::hash etc., which route all operations through the correct Python machinery and allow for fallibility.

[codeguru42]

PartialEq<[u8]> for Bound<'py, PyBytes> seems to fit the same category of behaviour

Done in #4259

[Owen-CH-Leung]

Hi there, can I get a review for my PR here ? Thanks

[davidhewitt]

Looks like all the original suggestions were now done.