scroll icon indicating copy to clipboard operation
scroll copied to clipboard

derive(Pread) doesn't work with fields containing an array of structs

Open luser opened this issue 7 years ago • 6 comments

The code that handles array fields initializes its temporary array to [0; #size as usize], which doesn't work for an array of structs, like:

#[derive(Pread)]
struct A {
    a: u32,
}

#[derive(Pread)]
struct B {
    b: [A; 4],
}

You get:

error[E0308]: mismatched types
 --> src/main.rs:9:10
  |
9 | #[derive(Pread)]
  |          ^^^^^
  |          |
  |          expected struct `A`, found integral variable
  |          help: try using a variant of the expected type: `A(0)`
  |
  = note: expected type `A`
             found type `{integer}`

error[E0277]: the trait bound `A: std::marker::Copy` is not satisfied
 --> src/main.rs:9:10
  |
9 | #[derive(Pread)]
  |          ^^^^^ the trait `std::marker::Copy` is not implemented for `A`
  |
  = note: the `Copy` trait is required because the repeated element will be copied

error: aborting due to 2 previous errors

The additional error about Copy is because the [x; N] array initializer syntax will copy the x value.

I suppose the most straightforward way to fix both of these would be to change that code to just generate the array inline, like: [ src.gread_with::<#whatever>(offset, ctx)?, src.gread_with::<#whatever>(offset, ctx)? ... ]

luser avatar Sep 06 '18 17:09 luser

Here's a simple example project that demonstrates the error: https://github.com/luser/scroll-derive-struct-array .

I hit this in practice trying to convert my minidump crate to use scroll. The MDXmmSaveArea32AMD64 struct contains arrays of uint128_struct.

luser avatar Sep 06 '18 17:09 luser

Hmmm yea this 100% needs to be fixed. I wrote most of the derive code quickly for my needs (simple pods at first) and it isn’t super robust, and definitely needs some more love. Even error messages aren’t great.

Is requiring the type to be Default reasonable and we can just call default in the array perhaps another approach ? And then fill it up.

Or your method should work too I think. L

But yea I’ll merge anything fixing this :)

m4b avatar Sep 08 '18 00:09 m4b

I think this can easily be fixed by simply calling default and requiring anyone to also derive Copy and Default on their types, which kind of sucks?

E.g., in your use case, https://github.com/luser/rust-minidump/blob/db4a828873d68f513261d285d274feb9febc9733/minidump-common/src/format.rs#L95

I don't know if you're in control of those annotations? If so, it should probably be fine?

However, I would have to test that patch with goblin and other shit, because adding those new required bounds could break a bunch of stuff if someone hadn't implemented them.

Consequently, I like your suggestion better, but I don't see an easy way to generate a finite rust array in syn, inline, off the top of my head (I haven't done proc macro stuff in a while).

If anyone has any ideas, or PR which generates the inline array without Default (or unsafe like uninitialized) I'd be pretty happy, as this is definitely a deficiency :)

m4b avatar Sep 11 '18 04:09 m4b

I actually got myself unstuck on this by switching to u128 for those arrays, hence my other PR. I think it still makes sense to fix this at some point, but it's not currently blocking me on my minidump crate.

luser avatar Sep 11 '18 10:09 luser

I think the best way to fix this is to use MaybeUninit; if anyone feels like making a PR that would be appreciated :)

m4b avatar Dec 14 '21 05:12 m4b

I am running into that and considering first a TryFromCtx impl for &'a [T; N] or [T; N] ? then you don't need any macro magic because you are just reading a classical type right?

RaitoBezarius avatar May 28 '23 15:05 RaitoBezarius