Skip to content
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.

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

Implement PartialEq for PyBytes and [u8] #4259

Merged
merged 10 commits into from
Jun 22, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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/4250.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement `PartialEq<str>` for `Bound<'py, PyBytes>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Implement `PartialEq<str>` for `Bound<'py, PyBytes>`.
Implement `PartialEq<[u8]>` for `Bound<'py, PyBytes>`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this but wasn't able to accept it in time before the merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it on my next PR for #4245

Copy link
Member

@davidhewitt davidhewitt Jun 24, 2024

Choose a reason for hiding this comment

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

No prob; I got it in #4266 thanks to @alex

153 changes: 153 additions & 0 deletions src/types/bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,29 @@ use std::str;
/// Represents a Python `bytes` object.
///
/// This type is immutable.
///
/// # Equality
///
/// For convenience, [`Bound<'py, PyBytes>`] implements [`PartialEq<str>`] to allow comparing the
codeguru42 marked this conversation as resolved.
Show resolved Hide resolved
/// data in the Python bytes to a Rust `[u8]`.
///
/// This is not always the most appropriate way to compare Python bytes, as Python bytes subclasses
/// may have different equality semantics. In situations where subclasses overriding equality might be
/// relevant, use [`PyAnyMethods::eq`], at cost of the additional overhead of a Python method call.
///
/// ```rust
/// # use pyo3::prelude::*;
/// use pyo3::types::PyBytes;
///
/// # Python::with_gil(|py| {
/// let py_bytes = PyBytes::new_bound(py, b"foo");
/// // via PartialEq<[u8]>
/// assert_eq!(py_bytes, b"foo");
///
/// // via Python equality
/// assert!(py_bytes.as_any().eq(b"foo").unwrap());
/// # });
/// ```
codeguru42 marked this conversation as resolved.
Show resolved Hide resolved
#[repr(transparent)]
pub struct PyBytes(PyAny);

Expand Down Expand Up @@ -191,6 +214,106 @@ impl<I: SliceIndex<[u8]>> Index<I> for Bound<'_, PyBytes> {
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<[u8]> for Bound<'_, PyBytes> {
#[inline]
fn eq(&self, other: &[u8]) -> bool {
self.as_borrowed() == *other
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<&'_ [u8]> for Bound<'_, PyBytes> {
#[inline]
fn eq(&self, other: &&[u8]) -> bool {
self.as_borrowed() == **other
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<Bound<'_, PyBytes>> for [u8] {
#[inline]
fn eq(&self, other: &Bound<'_, PyBytes>) -> bool {
*self == other.as_borrowed()
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<&'_ Bound<'_, PyBytes>> for [u8] {
#[inline]
fn eq(&self, other: &&Bound<'_, PyBytes>) -> bool {
*self == other.as_borrowed()
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<Bound<'_, PyBytes>> for &'_ [u8] {
#[inline]
fn eq(&self, other: &Bound<'_, PyBytes>) -> bool {
**self == other.as_borrowed()
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<[u8]> for &'_ Bound<'_, PyBytes> {
#[inline]
fn eq(&self, other: &[u8]) -> bool {
self.as_borrowed() == other
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<[u8]> for Borrowed<'_, '_, PyBytes> {
#[inline]
fn eq(&self, other: &[u8]) -> bool {
self.to_cow().map_or(false, |s| s == other)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell, all the other impls are just permutations of different types that ultimate call this impl. I have two questions here:

  1. I copied this from types/string.py. But I removed the special implementation for 3.10 or ABI. I assume that is specific to the string implementation. Is that correct? Or is there an analogous implementation for bytes that is special for 3.10 or ABI?
  2. I'm pretty sure to_cow() isn't a thing for PyBytes. I will work on an implementation here. Do I manually map (or loop) over the two parameters and compare each element? Or is there something more idiomatic in Rust?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use self.as_bytes() == other here (which should then use the PartialEq impl on slices on std)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Icxolu Thanks for the feedback. I tried your suggestion and tests fail with

no implementation for `instance::Bound<'_, bytes::PyBytes> == &[u8; 12]

I guess we need the length of the array as part of the type? Or should I use Vec<u8> instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is because let b = b"hello, world"; is creating an array (&[u8, 12]), not a slice (&[u8]). To test these impl you can turn that array into a slice like this: let b = b"hello, world".as_slice();

If we want we can also add implementations for arrays directly as well, using const generics, something like the following:

impl <const N: usize> PartialEq<[u8; N]> for Borrowed<'_, '_, PyString> { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...or are my tests wrong and need changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

They currently test comparison with arrays (which are nor implemented) instead of the comparison with slices. This can be a bit surprising, since often arrays can be automatically coerced to slices by the compiler. In this case the compiler is not smart enough to see that this is what we want.

If you change your test to use let b = b"hello, world".as_slice(); or let b: &[_] = b"hello, world";, we explicitly specify that we want b to be a slice as opposed to an array. This way it should then also use the PartialEq impl that you have implemented here.

Copy link
Contributor Author

@codeguru42 codeguru42 Jun 17, 2024

Choose a reason for hiding this comment

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

For context, your first response wasn't showing when I typed my previous comment.

}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<&[u8]> for Borrowed<'_, '_, PyBytes> {
#[inline]
fn eq(&self, other: &&[u8]) -> bool {
*self == **other
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<Borrowed<'_, '_, PyBytes>> for [u8] {
#[inline]
fn eq(&self, other: &Borrowed<'_, '_, PyBytes>) -> bool {
other == self
}
}

/// Compares whether the Python bytes object is equal to the [u8].
///
/// In some cases Python equality might be more appropriate; see the note on [`PyBytes`].
impl PartialEq<Borrowed<'_, '_, PyBytes>> for &'_ [u8] {
#[inline]
fn eq(&self, other: &Borrowed<'_, '_, PyBytes>) -> bool {
other == self
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -251,4 +374,34 @@ mod tests {
.is_instance_of::<PyValueError>(py));
});
}

#[test]
fn test_comparisons() {
Python::with_gil(|py| {
let b = b"hello, world";
let py_bytes = PyBytes::new_bound(py, b);

assert_eq!(py_bytes, b"hello, world");

assert_eq!(py_bytes, b);
assert_eq!(&py_bytes, b);
assert_eq!(b, py_bytes);
assert_eq!(b, &py_bytes);

assert_eq!(py_bytes, *b);
assert_eq!(&py_bytes, *b);
assert_eq!(*b, py_bytes);
assert_eq!(*b, &py_bytes);

let py_string = py_bytes.as_borrowed();

assert_eq!(py_string, b);
assert_eq!(&py_string, b);
assert_eq!(b, py_string);
assert_eq!(b, &py_string);

assert_eq!(py_string, *b);
assert_eq!(*b, py_string);
})
}
}
Loading