m4b/scroll_derive

derive(Pwrite) doesn't compile for structs with non-Copy fields

luser opened this issue · 4 comments

luser commented

I ran into this while trying to test something else, but it's definitely going to bite me in the future. Here's a simple example:

#[derive(Pwrite)]
struct A {
    pub y: u64,
}

#[derive(Pwrite)]
struct B {
    pub a: A,
}

This code fails to compile with this error:

error[E0507]: cannot move out of borrowed content
  --> src/main.rs:12:10
   |
12 | #[derive(Pwrite)]
   |          ^^^^^^ cannot move out of borrowed content

The expanded impls for B look like:

impl <'a> ::scroll::ctx::TryIntoCtx<::scroll::Endian> for &'a B {
    type
    Error
    =
    ::scroll::Error;
    type
    Size
    =
    usize;
    #[inline]
    fn try_into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian)
     -> ::scroll::export::result::Result<Self::Size, Self::Error> {
        use ::scroll::Pwrite;
        let offset = &mut 0;
        dst.gwrite_with(self.a, offset, ctx)?;
        Ok(*offset)
    }
}
impl ::scroll::ctx::TryIntoCtx<::scroll::Endian> for B {
    type
    Error
    =
    ::scroll::Error;
    type
    Size
    =
    usize;
    #[inline]
    fn try_into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian)
     -> ::scroll::export::result::Result<Self::Size, Self::Error> {
        (&self).try_into_ctx(dst, ctx)
    }
}

Removing the #[derive(Pwrite)] and pasting that code inline gives a more precise error:

error[E0507]: cannot move out of borrowed content
  --> src/main.rs:30:25
   |
30 |         dst.gwrite_with(self.a, offset, ctx)?;
   |                         ^^^^ cannot move out of borrowed content

Changing that line to borrow instead fixes this: dst.gwrite_with(&self.a, offset, ctx)?;. I haven't checked to see if that has a negative impact on the generated code, but since the TryIntoCtx impl for T already forwards to &T I can't imagine it'd make a difference.

luser commented

Unfortunately changing the proc macro implementations to use &self.#ident fails because we don't have implementations of TryIntoCtx for references-to-primitives:

error[E0277]: the trait bound `&u32: scroll::ctx::TryIntoCtx<_>` is not satisfied
 --> tests/tests.rs:5:35
  |
5 | #[derive(Debug, PartialEq, Pread, Pwrite)]
  |                                   ^^^^^^ the trait `scroll::ctx::TryIntoCtx<_>` is not implemented for `&u32`
  |
  = help: the following implementations were found:
            <u32 as scroll::ctx::TryIntoCtx<scroll::Endian>>

This probably isn't hard to fix, though.

luser commented

I have a simple patch that uses references, and atop m4b/scroll#35 it works.

m4b commented

Man, what a horrible bug :/

Maybe we/I should have just made pwrite take a reference to self instead of the owned value? That would technically be a breaking change, but also perhaps it's better to be flexible and allow both?

In meantime, merging your reference patch should be backwards compatible and will be fine if decide to only take reference to self in pwrite, so probably just merge that?

luser commented

Maybe we/I should have just made pwrite take a reference to self instead of the owned value? That would technically be a breaking change, but also perhaps it's better to be flexible and allow both?

I dunno! Obviously I'm trying to do things that are sort of outside of your original design. For structs composed of just primitive fields this all works fine!

In meantime, merging your reference patch should be backwards compatible and will be fine if decide to only take reference to self in pwrite, so probably just merge that?

I don't think it should harm anything, no.