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

added various std traits for PyBackedStr and PyBackedBytes #4020

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Mar 29, 2024

Adds Clone, Debug, PartialEq, Eq, PartialOrd, Ord and Hash implementation for PyBackedBytes and PyBackedStr, and Display for PyBackedStr. The implementations just delegate to &[u8] and &str respectively. PartialEq and PartialOrd are also implemented between Self and Deref::Target respectively.

Copy link
Contributor

@LilyFoote LilyFoote left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do we need tests?

@Icxolu
Copy link
Contributor Author

Icxolu commented Mar 30, 2024

Do we need tests?

I was wondering about that as well. I'll try to come up with a few later.

@davidhewitt
Copy link
Member

I wonder, can we ship these in 0.21.1?

Theoretically adding trait implementations can be breaking, but these types are so new and the traits so basic I can't really see them being a problem.

I would definitely like to ship the Send / Sync implementations in 0.21.1.

@@ -79,13 +88,15 @@ impl FromPyObject<'_> for PyBackedStr {
/// A wrapper around `[u8]` where the storage is either owned by a Python `bytes` object, or a Rust `Box<[u8]>`.
///
/// This type gives access to the underlying data via a `Deref` implementation.
#[derive(Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the derived Clone impl incorrect for the Rust variant? Cloning Box<[u8]> will yield a new allocation for data will still point to the old one meaning this is a use-after-free in waiting?

So we either need to manually fix up the data pointer or use a shared ownership pointer for the Rust side as well, e.g. Arc<[u8]>.

Copy link
Member

Choose a reason for hiding this comment

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

(And so yes, these do need tests. ;-))

Copy link
Member

Choose a reason for hiding this comment

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

Yes I was thinking similarly about the Box, an Arc seems reasonable to me.

I also half wonder if it was a mistake to have the Rust variant here given the name PyBackedBytes. I added it to support bytearrays in the same way we already do for Cow<[u8]> but on further reflection it does seem a bit add odds with the name that we do the copy.

I think probably it's ok as I would expect most uses to be with bytes, but we might want to document this copy-from-bytearray more clearly and justify for sake of safety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the derived Clone impl incorrect for the Rust variant? Cloning Box<[u8]> will yield a new allocation for data will still point to the old one meaning this is a use-after-free in waiting?

Big oops, thanks for catching.

Using Arc<[u8]> sounds good to me aswell, that way this stays a cheap clone either way.

@Icxolu
Copy link
Contributor Author

Icxolu commented Mar 30, 2024

I've added the change to Arc<[u8]> and tests for the traits impls. I also added a few more PartialEq impls that came useful while writing the tests.

I wonder, can we ship these in 0.21.1? Theoretically adding trait implementations can be breaking, but these types are so new and the traits so basic I can't really see them being a problem.

I agree, since these types are so new, I think the chances of anyone relying on these impls not to be there are very low. Adding them in 0.21.1 should be fine I would say.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me! I'll start preparing 0.21.1 a bit later today 👍

@davidhewitt davidhewitt added this pull request to the merge queue Apr 1, 2024
Merged via the queue into PyO3:main with commit 63ba371 Apr 1, 2024
42 of 43 checks passed
@Icxolu Icxolu deleted the pybacked-traits branch April 1, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants