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

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

Open
luser opened this issue Sep 6, 2018 · 6 comments
Open

Comments

@luser
Copy link
Contributor

luser commented Sep 6, 2018

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

luser commented Sep 6, 2018

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.

@m4b
Copy link
Owner

m4b commented Sep 8, 2018

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
Copy link
Owner

m4b commented Sep 11, 2018

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 :)

@luser
Copy link
Contributor Author

luser commented Sep 11, 2018

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.

@m4b m4b transferred this issue from m4b/scroll_derive Sep 6, 2019
@m4b
Copy link
Owner

m4b commented Dec 14, 2021

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

@RaitoBezarius
Copy link
Contributor

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?

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

3 participants