derive(Pwrite) doesn't compile for structs with non-Copy fields
luser opened this issue · 4 comments
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.
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.
I have a simple patch that uses references, and atop m4b/scroll#35 it works.
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?
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.