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

add PyBytes::new_bound #3777

Merged
merged 1 commit into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion guide/src/conversions/traits.md
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ enum RustyEnum<'a> {
# }
#
# {
# let thing = PyBytes::new(py, b"text");
# let thing = PyBytes::new_bound(py, b"text");
# let rust_thing: RustyEnum<'_> = thing.extract()?;
#
# assert_eq!(
Expand Down
7 changes: 3 additions & 4 deletions pytests/src/buf_and_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,9 @@ impl BytesExtractor {
}

#[pyfunction]
fn return_memoryview(py: Python<'_>) -> PyResult<&PyMemoryView> {
let bytes: &PyAny = PyBytes::new(py, b"hello world").into();
let memoryview = TryInto::try_into(bytes)?;
Ok(memoryview)
fn return_memoryview(py: Python<'_>) -> PyResult<Bound<'_, PyMemoryView>> {
let bytes = PyBytes::new_bound(py, b"hello world");
PyMemoryView::from_bound(&bytes)
}

#[pymodule]
Expand Down
2 changes: 1 addition & 1 deletion src/conversions/anyhow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
//! let res = Python::with_gil(|py| {
//! let zlib = PyModule::import(py, "zlib")?;
//! let decompress = zlib.getattr("decompress")?;
//! let bytes = PyBytes::new(py, bytes);
//! let bytes = PyBytes::new_bound(py, bytes);
//! let value = decompress.call1((bytes,))?;
//! value.extract::<Vec<u8>>()
//! })?;
Expand Down
2 changes: 1 addition & 1 deletion src/conversions/eyre.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
//! let res = Python::with_gil(|py| {
//! let zlib = PyModule::import(py, "zlib")?;
//! let decompress = zlib.getattr("decompress")?;
//! let bytes = PyBytes::new(py, bytes);
//! let bytes = PyBytes::new_bound(py, bytes);
//! let value = decompress.call1((bytes,))?;
//! value.extract::<Vec<u8>>()
//! })?;
Expand Down
2 changes: 1 addition & 1 deletion src/conversions/num_bigint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ macro_rules! bigint_conversion {
#[cfg(Py_LIMITED_API)]
fn to_object(&self, py: Python<'_>) -> PyObject {
let bytes = $to_bytes(self);
let bytes_obj = PyBytes::new(py, &bytes);
let bytes_obj = PyBytes::new_bound(py, &bytes);
let kwargs = if $is_signed > 0 {
let kwargs = PyDict::new(py);
kwargs.set_item(crate::intern!(py, "signed"), true).unwrap();
Expand Down
4 changes: 2 additions & 2 deletions src/conversions/std/slice.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#[cfg(feature = "experimental-inspect")]
use crate::inspect::types::TypeInfo;
use crate::{types::PyBytes, FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python, ToPyObject};
use crate::{types::PyBytes, FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python};

impl<'a> IntoPy<PyObject> for &'a [u8] {
fn into_py(self, py: Python<'_>) -> PyObject {
PyBytes::new(py, self).to_object(py)
PyBytes::new_bound(py, self).unbind().into()
}

#[cfg(feature = "experimental-inspect")]
Expand Down
8 changes: 4 additions & 4 deletions src/tests/hygiene/pymethods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ impl Dummy {
"Dummy"
}

fn __bytes__<'py>(&self, py: crate::Python<'py>) -> &'py crate::types::PyBytes {
crate::types::PyBytes::new(py, &[0])
fn __bytes__<'py>(&self, py: crate::Python<'py>) -> crate::Bound<'py, crate::types::PyBytes> {
crate::types::PyBytes::new_bound(py, &[0])
}

fn __format__(&self, format_spec: ::std::string::String) -> ::std::string::String {
Expand Down Expand Up @@ -420,8 +420,8 @@ impl Dummy {
"Dummy"
}

fn __bytes__<'py>(&self, py: crate::Python<'py>) -> &'py crate::types::PyBytes {
crate::types::PyBytes::new(py, &[0])
fn __bytes__<'py>(&self, py: crate::Python<'py>) -> crate::Bound<'py, crate::types::PyBytes> {
crate::types::PyBytes::new_bound(py, &[0])
}

fn __format__(&self, format_spec: ::std::string::String) -> ::std::string::String {
Expand Down
94 changes: 81 additions & 13 deletions src/types/bytes.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::ffi_ptr_ext::FfiPtrExt;
use crate::instance::{Borrowed, Bound};
use crate::types::any::PyAnyMethods;
use crate::{ffi, FromPyObject, IntoPy, Py, PyAny, PyNativeType, PyResult, Python, ToPyObject};
use std::borrow::Cow;
use std::ops::Index;
Expand All @@ -17,14 +19,45 @@ pub struct PyBytes(PyAny);
pyobject_native_type_core!(PyBytes, pyobject_native_static_type_object!(ffi::PyBytes_Type), #checkfunction=ffi::PyBytes_Check);

impl PyBytes {
/// Deprecated form of [`PyBytes::new_bound`].
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyBytes::new` will be replaced by `PyBytes::new_bound` in a future PyO3 version"
)
)]
pub fn new<'p>(py: Python<'p>, s: &[u8]) -> &'p PyBytes {
Self::new_bound(py, s).into_gil_ref()
}

/// Creates a new Python bytestring object.
/// The bytestring is initialized by copying the data from the `&[u8]`.
///
/// Panics if out of memory.
pub fn new<'p>(py: Python<'p>, s: &[u8]) -> &'p PyBytes {
pub fn new_bound<'p>(py: Python<'p>, s: &[u8]) -> Bound<'p, PyBytes> {
let ptr = s.as_ptr() as *const c_char;
let len = s.len() as ffi::Py_ssize_t;
unsafe { py.from_owned_ptr(ffi::PyBytes_FromStringAndSize(ptr, len)) }
unsafe {
ffi::PyBytes_FromStringAndSize(ptr, len)
.assume_owned(py)
.downcast_into_unchecked()
}
}

/// Deprecated form of [`PyBytes::new_bound_with`].
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyBytes::new_with` will be replaced by `PyBytes::new_bound_with` in a future PyO3 version"
)
)]
pub fn new_with<F>(py: Python<'_>, len: usize, init: F) -> PyResult<&PyBytes>
where
F: FnOnce(&mut [u8]) -> PyResult<()>,
{
Self::new_bound_with(py, len, init).map(Bound::into_gil_ref)
}

/// Creates a new Python `bytes` object with an `init` closure to write its contents.
Expand All @@ -41,34 +74,49 @@ impl PyBytes {
///
/// # fn main() -> PyResult<()> {
/// Python::with_gil(|py| -> PyResult<()> {
/// let py_bytes = PyBytes::new_with(py, 10, |bytes: &mut [u8]| {
/// let py_bytes = PyBytes::new_bound_with(py, 10, |bytes: &mut [u8]| {
/// bytes.copy_from_slice(b"Hello Rust");
/// Ok(())
/// })?;
/// let bytes: &[u8] = FromPyObject::extract(py_bytes)?;
/// let bytes: &[u8] = py_bytes.extract()?;
/// assert_eq!(bytes, b"Hello Rust");
/// Ok(())
/// })
/// # }
/// ```
pub fn new_with<F>(py: Python<'_>, len: usize, init: F) -> PyResult<&PyBytes>
pub fn new_bound_with<F>(py: Python<'_>, len: usize, init: F) -> PyResult<Bound<'_, PyBytes>>
where
F: FnOnce(&mut [u8]) -> PyResult<()>,
{
unsafe {
let pyptr = ffi::PyBytes_FromStringAndSize(std::ptr::null(), len as ffi::Py_ssize_t);
// Check for an allocation error and return it
let pypybytes: Py<PyBytes> = Py::from_owned_ptr_or_err(py, pyptr)?;
let pybytes = pyptr.assume_owned_or_err(py)?.downcast_into_unchecked();
let buffer: *mut u8 = ffi::PyBytes_AsString(pyptr).cast();
debug_assert!(!buffer.is_null());
// Zero-initialise the uninitialised bytestring
std::ptr::write_bytes(buffer, 0u8, len);
// (Further) Initialise the bytestring in init
// If init returns an Err, pypybytearray will automatically deallocate the buffer
init(std::slice::from_raw_parts_mut(buffer, len)).map(|_| pypybytes.into_ref(py))
init(std::slice::from_raw_parts_mut(buffer, len)).map(|_| pybytes)
}
}

/// Deprecated form of [`PyBytes::bound_from_ptr`].
///
/// # Safety
/// See [`PyBytes::bound_from_ptr`].
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyBytes::from_ptr` will be replaced by `PyBytes::bound_from_ptr` in a future PyO3 version"
)
)]
pub unsafe fn from_ptr(py: Python<'_>, ptr: *const u8, len: usize) -> &PyBytes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this returns a gil-ref, should we deprecate it as well, in favor of bound_from_ptr()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, my mistake to miss this; that's the downside of the commits I've been cherry-picking from #3606 - they were mostly trying to get a complete compile with a good range of the types rather than a complete implementation 🙈

I'll add here 👍

Self::bound_from_ptr(py, ptr, len).into_gil_ref()
}

/// Creates a new Python byte string object from a raw pointer and length.
///
/// Panics if out of memory.
Expand All @@ -79,11 +127,10 @@ impl PyBytes {
/// leading pointer of a slice of length `len`. [As with
/// `std::slice::from_raw_parts`, this is
/// unsafe](https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#safety).
pub unsafe fn from_ptr(py: Python<'_>, ptr: *const u8, len: usize) -> &PyBytes {
py.from_owned_ptr(ffi::PyBytes_FromStringAndSize(
ptr as *const _,
len as isize,
))
pub unsafe fn bound_from_ptr(py: Python<'_>, ptr: *const u8, len: usize) -> Bound<'_, PyBytes> {
ffi::PyBytes_FromStringAndSize(ptr as *const _, len as isize)
.assume_owned(py)
.downcast_into_unchecked()
}

/// Gets the Python string as a byte slice.
Expand Down Expand Up @@ -142,6 +189,15 @@ impl<I: SliceIndex<[u8]>> Index<I> for PyBytes {
}
}

/// This is the same way [Vec] is indexed.
impl<I: SliceIndex<[u8]>> Index<I> for Bound<'_, PyBytes> {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could additionally implement it on &Bound as well, so it also works an a borrowed bound

Copy link
Member Author

@davidhewitt davidhewitt Jan 31, 2024

Choose a reason for hiding this comment

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

I think it's not needed due to the way bytes[i] gets desugared into something like *bytes.index(i).

I've pushed an additional part to test_bound_bytes_index which indexes on &Bound<PyBytes>, see if you agree with me that the one implementation is sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, that even better! 👍

type Output = I::Output;

fn index(&self, index: I) -> &Self::Output {
&self.as_bytes()[index]
}
}

/// Special-purpose trait impl to efficiently handle both `bytes` and `bytearray`
///
/// If the source object is a `bytes` object, the `Cow` will be borrowed and
Expand All @@ -160,7 +216,7 @@ impl<'source> FromPyObject<'source> for Cow<'source, [u8]> {

impl ToPyObject for Cow<'_, [u8]> {
fn to_object(&self, py: Python<'_>) -> Py<PyAny> {
PyBytes::new(py, self.as_ref()).into()
PyBytes::new_bound(py, self.as_ref()).into()
}
}

Expand All @@ -171,6 +227,7 @@ impl IntoPy<Py<PyAny>> for Cow<'_, [u8]> {
}

#[cfg(test)]
#[cfg_attr(not(feature = "gil-refs"), allow(deprecated))]
mod tests {
use super::*;

Expand All @@ -182,6 +239,17 @@ mod tests {
});
}

#[test]
fn test_bound_bytes_index() {
Python::with_gil(|py| {
let bytes = PyBytes::new_bound(py, b"Hello World");
assert_eq!(bytes[1], b'e');

let bytes = &bytes;
assert_eq!(bytes[1], b'e');
});
}

#[test]
fn test_bytes_new_with() -> super::PyResult<()> {
Python::with_gil(|py| -> super::PyResult<()> {
Expand Down
6 changes: 3 additions & 3 deletions tests/test_bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ fn test_pybytes_bytes_conversion() {
}

#[pyfunction]
fn bytes_vec_conversion(py: Python<'_>, bytes: Vec<u8>) -> &PyBytes {
PyBytes::new(py, bytes.as_slice())
fn bytes_vec_conversion(py: Python<'_>, bytes: Vec<u8>) -> Bound<'_, PyBytes> {
PyBytes::new_bound(py, bytes.as_slice())
}

#[test]
Expand All @@ -43,7 +43,7 @@ fn test_bytearray_vec_conversion() {
#[test]
fn test_py_as_bytes() {
let pyobj: pyo3::Py<pyo3::types::PyBytes> =
Python::with_gil(|py| pyo3::types::PyBytes::new(py, b"abc").into_py(py));
Python::with_gil(|py| pyo3::types::PyBytes::new_bound(py, b"abc").unbind());

let data = Python::with_gil(|py| pyobj.as_bytes(py));

Expand Down
Loading