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

Is there a way to fully replace &'l [u8] with &'l mut dyn Buf? #395

Open
xoac opened this issue May 29, 2020 · 9 comments
Open

Is there a way to fully replace &'l [u8] with &'l mut dyn Buf? #395

xoac opened this issue May 29, 2020 · 9 comments

Comments

@xoac
Copy link

xoac commented May 29, 2020

I was trying to replace &'a [u8] with &'a mut dyn Buf but I fail with lifetime problem

use bytes; // 0.5.4
use bytes::*;

#[derive(Debug)]
struct Frame<'p> {
    x: u8,
    payload: &'p [u8],
}

impl<'p> Frame<'p> {
    fn decode<'a: 'p>(buf: &'a mut dyn Buf) -> Self {
        let x = buf.get_u8();
        Self {
            x,
            payload: buf.bytes(),
        }
    }
}

fn main() {
    let x = [0u8; 3];
    let mut y = &x[..];
    let f = Frame::decode(&mut y);
    println!("{:?}", f);
}

(Playground)

As far as I understand - the lifetime returned by buf.bytes() should be equal to 'a defined on Buf and should be valid at least as long as 'p. (buf takes &'a self as a reference).

I think about Buf as a view or coursor over bytes. But as long as this view is valid data will be not moved. It's work when I implement similar to bytes() method directly

Why I don't call to_bytes()? this decode function is no_std and I want give an end user full control of allocation.

@xoac
Copy link
Author

xoac commented May 29, 2020

Here is a proposition of OwnedBuf trait that is build on top of Buf. That could be used to resolve my issue. The name is probably not best (BorrowedBuf (?)).

use bytes; // 0.5.4
use bytes::*;

trait OwnedBuf<'a>: Buf {
    fn owned_bytes(&'a self) -> &'a [u8];
}

impl<'a> OwnedBuf<'a> for &'a [u8] {
    fn owned_bytes(&'a self) -> &'a [u8] {
        self
    }
}

#[derive(Debug)]
struct Frame<'p> {
    x: u8,
    payload: &'p [u8],
}

impl<'p> Frame<'p> {
    fn decode<'a: 'p>(buf: &'a mut dyn OwnedBuf<'a>) -> Self {
        let x = buf.get_u8();
        Self {
            x,
            payload: buf.owned_bytes()
        }
    }
}

fn main() {
    let x = [0u8; 3];
    let mut y = &x[..];
    let f = Frame::decode(&mut y);
    println!("{:?}", f);
}

(Playground)

@samlich
Copy link

samlich commented Jun 1, 2020

I used something like this, but with self taking the anonymous lifetime as it doesn't need to live as long as the buffer. It seems to work pretty well, except Option<[u8; 1]> only works when your do restrict the lifetime. I don't think I ever tried the vectored method, and it looks suspicious to me that it has the 'b lifetime.

pub trait BufBorrow<'a>: bytes::Buf {
        fn bytes_borrow(&self) -> &'a [u8];
        fn bytes_vectored_borrow<'b>(&'b self, dst: &mut [IoSlice<'b>]) -> usize {
                if dst.is_empty() {
                        return 0;
                }

                if self.has_remaining() {
                        dst[0] = IoSlice::new(self.bytes());
                        1
                } else {
                        0
                }
        }
}
impl
macro_rules! deref_forward_buf {
    () => (
    fn bytes_borrow(&self) -> &'a [u8] {
        (**self).bytes_borrow()
    }

    #[cfg(feature = "std")]
    fn bytes_vectored_borrow<'b>(&'b self, dst: &mut [IoSlice<'b>]) -> usize {
        (**self).bytes_vectored_borrow(dst)
    }
    )
}

impl<'a, T: BufBorrow<'a> + ?Sized> BufBorrow<'a> for &mut T {
        deref_forward_buf!();
}

impl<'a, T: BufBorrow<'a> + ?Sized> BufBorrow<'a> for Box<T> {
        deref_forward_buf!();
}

impl<'a> BufBorrow<'a> for &'a [u8] {
        #[inline]
        fn bytes_borrow(&self) -> &'a [u8] { self }
}

/*impl<'a> BufBorrow<'a> for Option<[u8; 1]> {
        fn bytes_borrow(&self) -> &'a [u8] {
                self.as_ref().map(AsRef::as_ref).unwrap_or(Default::default())
        }
}*/

#[cfg(feature = "std")]
impl<'a, T: AsRef<[u8]>> BufBorrow<'a> for std::io::Cursor<T> {
        fn bytes_borrow(&self) -> &[u8] {
                let len = self.get_ref().as_ref().len();
                let pos = self.position();

                if pos >= len as u64 {
                        return &[];
                }

                &self.get_ref().as_ref()[pos as usize..]
        }
}

// The existence of this function makes the compiler catch if the BufBorrow
// trait is "object-safe" or not.
fn _assert_trait_object(_b: &dyn BufBorrow) {}

Here is an abbreviation of my use case that should demonstrate the flexibility. Essentially being able to take some input and store references to it in the output, and being able to nest such functions.

impl<'x, 'i: 'x, I: 'i + BufBorrow<'i>> Tr<'x, 'i, T<'x>, I> for A {
  fn f(&self, i: &mut I, x: *mut T<'x>) {
    unsafe { x.write(T::new()) };
    B.f(i, x);
    x.append(T::new(core::str::from_utf8(i.bytes_borrow()).unwrap()));
  }
}

I hadn't gotten far enough to be confident enough to propose it, and I don't know what the pros/cons of adding this to Buf itself would be, but this would be useful even as an additional trait.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 1, 2020

For the record, the original playground can be modified to compile like this:

impl<'p> Frame<'p> {
    fn decode(buf: &'p mut dyn Buf) -> Self {
        let x = buf.get_u8();
        Self {
            x,
            payload: (&*buf).bytes(),
        }
    }
}

The issue is that bytes call is using the impl<T: Buf + ?Sized> Buf for &mut T, so the self argument given to bytes is a reference to the local variable buf, and not what buf points to. The snippet above sidesteps this by first creating a &'p dyn Buf, on which it can call bytes directly.

@xoac
Copy link
Author

xoac commented Jun 1, 2020

@Darksonn
Thanks - this has a significant disadvantage. You can't call buf.advance(self.remaining()):

error[E0502]: cannot borrow *buf as mutable because it is also borrowed as immutable

Do you have any idea how to workaround also this?

Here is probably what I am looking for

pub trait BorrowedBuf<'a>: Buf {
    fn advance_borrow(&'a mut self, cnt: usize) -> &'a [u8];
}

impl<'a> BorrowedBuf<'a> for &'a [u8] {
    fn advance_borrow(&'a mut self, cnt: usize) -> &'a [u8] {
        let to_ret = &self[..cnt];
        *self = &self[..cnt];
        to_ret
    }
}
/// This should be implemented inside bytes to work.
impl<'a> BorrowedBuf<'a> for Bytes {
    fn advance_borrow(&'a mut self, cnt: usize) -> &'a [u8] {
        let to_ret = &self.bytes()[..cnt];
        self.advance(cnt);
        to_ret
    }
}

@Ralith
Copy link

Ralith commented Jun 2, 2020

Do you have any idea how to workaround also this?

Does the usual

let remaining = self.remaining();
buf.advance(remaining);

trick not apply here for some reason?

@xoac
Copy link
Author

xoac commented Jun 2, 2020

@Ralith
clarified

impl<'p> Frame<'p> {
    fn decode(buf: &'p mut dyn Buf) -> Self {
        let x = buf.get_u8();
        let r = Self {
            x,
            payload: (&*buf).bytes(),
        };
        buf.advance(buf.remaining());
        r
    }
}

(Playground)

@Ralith
Copy link

Ralith commented Jun 2, 2020

Ah, I see. I think what you want is reasonable, and is basically a more generic version of Bytes::split_to. The proposed signature has a drawback though: the returned &[u8] will keep the mutable borrow alive.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 2, 2020

The issue here is that you can't do what you are trying to do with every implementor of Buf. Calling advance might invalidate the pointer returned by bytes, so to keep those bytes around after advancing the pointer, you must e.g. copy them into a vector. Let me quote from the Buf trait:

Buf can be thought of as an efficient Iterator for collections of bytes.

There's no guarantee that older bytes remain available as you advance the iterator. Sure, it works with &[u8] because immutable slices are Copy, but if you want your code to work with any Buf, there's no longer any guarantee that this is valid.

A Buf is not required to be splitable.

@xoac
Copy link
Author

xoac commented Jun 2, 2020

Yes I know that. This is why I proposed additional trait that allow you to do this. As far as I understand it Bytes and &[u8] could implement this trait. And I think this is about 9x% of use-cases.

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

No branches or pull requests

4 participants