Skip to content
This repository has been archived by the owner on Sep 6, 2019. It is now read-only.

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

Closed
luser opened this issue Sep 13, 2018 · 4 comments · Fixed by #17
Closed

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

luser opened this issue Sep 13, 2018 · 4 comments · Fixed by #17

Comments

@luser
Copy link
Contributor

luser commented Sep 13, 2018

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
Copy link
Contributor Author

luser commented Sep 13, 2018

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 added a commit to luser/scroll that referenced this issue Sep 13, 2018
luser added a commit to luser/scroll that referenced this issue Sep 13, 2018
@luser
Copy link
Contributor Author

luser commented Sep 13, 2018

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

@m4b
Copy link
Owner

m4b commented Sep 14, 2018

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
Copy link
Contributor Author

luser commented Sep 14, 2018

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.

luser added a commit to luser/scroll_derive that referenced this issue Sep 20, 2018
@m4b m4b closed this as completed in #17 Sep 22, 2018
willglynn pushed a commit to willglynn/scroll that referenced this issue Nov 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants