Pyo3: bytes should translate to &[u8]

[pganssle]

Per a discussion in the gitter chat, it seems that currently the proper traits are not implemented to allow for Python's bytes objects to be automatically translated into &[u8] parameters. Right now, in order to write a function that takes bytes, you need to use &PyBytes:

#[pyfunction]
fn print_bytes(_py: Python, x: &PyBytes) {
    println!("{:?}", x.as_bytes())
}

If you instead use &[u8] directly, there's an error at compile time because the pyo3::PyTypeInfo trait is not implemented for [u8]. I think it should be possible to pass both a bytes and bytearray object into a Rust function that takes &[u8].

I am using version 0.6.0-alpha.4, and with this code:

#[pyfunction]
fn print_bytes(_py: Python, x: &[u8]) {
    println!("{:?}", x.as_bytes())
}

I get this error message:

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
  --> src/lib.rs:36:1
   |
36 | #[pyfunction]
   | ^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `[u8]`
   = note: to learn more, visit <...>
   = note: required because of the requirements on the impl of
           `pyo3::FromPyObject<'_>` for `&[u8]`

error[E0277]: the trait bound `[u8]: pyo3::PyTypeInfo` is not satisfied
  --> src/lib.rs:36:1
   |
36 | #[pyfunction]
   | ^^^^^^^^^^^^^ the trait `pyo3::PyTypeInfo` is not implemented for `[u8]`
   |
   = note: required because of the requirements on the impl
           of `pyo3::PyTryFrom<'_>` for `[u8]`
   = note: required because of the requirements on the impl
           of `pyo3::FromPyObject<'_>` for `&[u8]`

error: aborting due to 2 previous errors

CC @thedrow

[konstin]

I think the second example was meant to contain a different snippet

[pganssle]

@konstin Oops, sorry about that, fixed.

[Hywan]

Would be a nice feature :-). Right now, the workaround I've is the following:

pub fn foo(bytes: &PyAny) -> PyResult<bool> {
    match <PyBytes as PyTryFrom>::try_from(bytes) {
        Ok(bytes) => /* do something */,
        _ => Ok(false)
    }
}

[konstin]

If someone wants to implement this,PyBytes_AsStringAndSize (docs, bindings) and PyString::as_bytes should be a good starting point: https://github.com/PyO3/pyo3/blob/fdeef7d67e6a526e20819f95a1212a34ae85b1b1/src/types/string.rs#L50-L61

[ExpHP]

You can use a Python to modify a bytearray at any time, so &[u8] is incorrect for this type. The best you can do is &[Cell<u8>].

[konstin]

Indeed, only bytes should be convertible to &[u8].

[pganssle]

You can use a Python to modify a bytearray at any time, so &[u8] is incorrect for this type. The best you can do is &[Cell<u8>].

@konstin @ExpHP I am not sure I agree with this, can you clarify exactly what the objection is? In both cases you get an immutable view into a memory array, so it's the difference between &mut [u8] (bytearray) and &[u8] (bytes). In Rust you are allowed to coerce mutable references to immutable references:

fn some_func(y: &[u8]) {
    println!("{:?}", y);
}

fn main() {
    let mut x : [u8; 4] = [0, 0x55, 0x10, 0xff];
    some_func(&mut x);
}

So it makes sense to allow conversion from bytearray to &[u8]. I agree that you should be able to pass a bytearray to a function that takes &[Cell<u8>], though, but that you should not be able to pass a bytes object to that function.

[birkenfeld]

@pganssle Rust can and will assume that memory referenced by &[u8] does not change while you hold the reference. Code like this

let x: &[u8] = python_bytearray.as_bytes();
modify(&python_bytearray);
println!("{:?}", x);

is now problematic, for example because the compiler might decide to do the read from x before the call to modify.

[pganssle]

@birkenfeld I suppose it's a fair point, though it depends heavily on how the implementation works. If it's indeed just passing a reference to some memory that Python can modify while Rust holds a reference to it, then it's a soundness problem. If it's copying the memory into some intermediate representation managed by Rust, it's not a problem.

I haven't looked enough into the implementation of #[pyfunction] to know how easy this would be, but presumably it's also possible to have bytearray automatically converted to some Rust-managed array even if bytes just returns a reference.

[ExpHP]

&mut in rust does not mean mutable, it means unique. This is inherently false for anything obtained from python. (I think it's more or less trivial to show that pyo3's ability to define &mut self methods on python classes (likewise it's choice to use &mut self for __iadd__, etc.) is unsound)

---

Here is proof that even the existing fn data(&PyByteArray) -> &[u8] method invokes undefined behavior:

extern crate pyo3;
use pyo3::prelude::{PyResult, Python};
use pyo3::types::{PyDict, PyByteArray};

fn main() -> Result<(), ()> {
    let gil = Python::acquire_gil();
    show_ub(gil.python()).map_err(|e| {
        eprintln!("error! :{:?}", e);
        e.print_and_set_sys_last_vars(gil.python());
    })
}

// a function that behaves differently in dev and release builds
fn show_ub(py: Python<'_>) -> PyResult<()> {
    let dict = PyDict::new(py);
    let array = PyByteArray::new(py, &[2]);
    dict.set_item("b", array)?;

    let bytes: &[u8] = array.data();
    lol(&bytes[0], &|| py.run("b[0] = 3", None, Some(&dict)).unwrap());

    // we should never make it here, because 2 != 3
    unreachable!("uh oh, how did we get here!?");
}

#[inline(never)]
fn lol(x: &u8, func: &dyn Fn()) {
    let old = *x;
    func();
    let new = *x;
    assert_eq!(old, new);
}

Results on debug:

   Compiling bbb v0.1.0 (/home/lampam/cpp/throwaway/bbb)
    Finished dev [unoptimized + debuginfo] target(s) in 0.43s
     Running `target/debug/bbb`
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `2`,
 right: `3`', src/main.rs:31:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Results on release: The assert_eq! gets optimized away:

   Compiling bbb v0.1.0 (/home/lampam/cpp/throwaway/bbb)
    Finished release [optimized] target(s) in 0.02s
     Running `target/release/bbb`
thread 'main' panicked at 'internal error: entered unreachable code: uh oh, how did we get here!?', src/main.rs:23:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

[konstin]

(I think it's more or less trivial to show that pyo3's ability to define &mut self methods on python classes (likewise it's choice to use &mut self for __iadd__, etc.) is unsound)

Yep, #342 should fix that. On looking at #342 again I've realized that @athre0z had already pointed that out over there. I'm sorry for missing that, I initially ignored the comments because I thought they were only more evidence for the &mut self situation.