Implement PartialEq for PyBytes and [u8] #4259

codeguru42 · 2024-06-17T01:18:02Z

This is my attempt to implement PartialEq for PyBytes and [u8]. See #4250.

codeguru42

This is my first draft for this PR. At this point, I just want to be sure I'm heading in the right general direction. I realize the meat of the implementation is incomplete. I have a couple comments/questions to fill in some gaps for me.

src/types/bytes.rs

codeguru42 · 2024-06-17T01:24:34Z

src/types/bytes.rs

+impl PartialEq<[u8]> for Borrowed<'_, '_, PyBytes> {
+    #[inline]
+    fn eq(&self, other: &[u8]) -> bool {
+        self.to_cow().map_or(false, |s| s == other)


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:

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?

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?

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

@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?

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> { ... }

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

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.

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

codeguru42 · 2024-06-17T02:39:20Z

From what I can tell you have to specify the length of the array like [u8; 12]. Is there a way to make the length generic or should I use Vec<u8> instead?

Icxolu · 2024-06-17T17:08:19Z

src/types/bytes.rs

+impl PartialEq<[u8]> for Borrowed<'_, '_, PyBytes> {
+    #[inline]
+    fn eq(&self, other: &[u8]) -> bool {
+        self.to_cow().map_or(false, |s| s == other)


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

src/types/bytes.rs

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

codeguru42 · 2024-06-19T03:39:28Z

This is working for PartialEq for PyBytes and [u8]. Do we want to merge this as-is or should I also add impls for [u8; N] as suggested above by icxolu?

codeguru42 · 2024-06-20T02:54:25Z

I think I fixed one of the examples in the doc comment. I'm not sure how to fix the second one, though.

Icxolu

This is working for PartialEq for PyBytes and [u8]. Do we want to merge this as-is or should I also add impls for [u8; N] as suggested above by icxolu?

I would suggest we start with what we have here and if there is demand for the array comparisons, we can add them later as a followup.

src/types/bytes.rs

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

davidhewitt

Looks great, thanks! Will merge now and rebase the 0.22 release on it tomorrow 👍

alex · 2024-06-22T22:35:13Z

newsfragments/4250.added.md

@@ -0,0 +1 @@
+Implement `PartialEq<str>` for `Bound<'py, PyBytes>`.


Suggested change

Implement `PartialEq<str>` for `Bound<'py, PyBytes>`.

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

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

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

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

alex · 2024-06-22T22:35:31Z

Changelog is wrong, so let's remember to fix when we do the release!

codeguru42 added 4 commits June 16, 2024 18:58

Copy pasta implementation from types/string.rs

eaddbfc

changelog

a002fd8

I think I don't need a special implementation for 3.10 or ABI

af6d92f

Copy pasta tests

daf147c

codeguru42 commented Jun 17, 2024

View reviewed changes

Icxolu reviewed Jun 17, 2024

View reviewed changes

codeguru42 and others added 4 commits June 17, 2024 11:18

Fix comment with correct type

bd1c1b6

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

Fix implementation

c4f3d3a

Use slice in tests

edd87ab

Try renaming changelog file

b488470

Fix doc example

b8dc030

Icxolu reviewed Jun 20, 2024

View reviewed changes

src/types/bytes.rs Outdated Show resolved Hide resolved

Fix doc example

c90926e

Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>

davidhewitt approved these changes Jun 22, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Jun 22, 2024

alex reviewed Jun 22, 2024

View reviewed changes

Merged via the queue into PyO3:main with commit 908ef6a Jun 22, 2024
41 checks passed

codeguru42 mentioned this pull request Jun 23, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PartialEq for PyBytes and [u8] #4259

Implement PartialEq for PyBytes and [u8] #4259

codeguru42 commented Jun 17, 2024

codeguru42 left a comment •

edited

Loading

codeguru42 Jun 17, 2024

Icxolu Jun 17, 2024

codeguru42 Jun 17, 2024

Icxolu Jun 17, 2024

codeguru42 Jun 17, 2024

Icxolu Jun 17, 2024

codeguru42 Jun 17, 2024 •

edited

Loading

codeguru42 commented Jun 17, 2024

Icxolu Jun 17, 2024

codeguru42 commented Jun 19, 2024

codeguru42 commented Jun 20, 2024

Icxolu left a comment

davidhewitt left a comment

alex Jun 22, 2024

codeguru42 Jun 22, 2024

codeguru42 Jun 23, 2024

davidhewitt Jun 24, 2024 •

edited

Loading

alex commented Jun 22, 2024

		@@ -0,0 +1 @@
		Implement `PartialEq<str>` for `Bound<'py, PyBytes>`.

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

Implement PartialEq for PyBytes and [u8] #4259

Implement PartialEq for PyBytes and [u8] #4259

Conversation

codeguru42 commented Jun 17, 2024

codeguru42 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeguru42 Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

codeguru42 commented Jun 17, 2024

Choose a reason for hiding this comment

codeguru42 commented Jun 19, 2024

codeguru42 commented Jun 20, 2024

Icxolu left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

alex commented Jun 22, 2024

codeguru42 left a comment •

edited

Loading

codeguru42 Jun 17, 2024 •

edited

Loading

davidhewitt Jun 24, 2024 •

edited

Loading