#derive (Pread) requires From<u8> for arrays?

[joschock]

Perhaps I'm not understanding some elements of scroll_derive and using it wrong, but the following won't compile:

use scroll::{Pread, Pwrite};

#[derive(Pread, Pwrite)]
struct A {
    b_array: [B; 5]
}

#[derive(Pread, Pwrite, Clone, Copy)]
struct B {
    field:u32
}

the error is:

error[E0277]: the trait bound `B: From<u8>` is not satisfied
 --> src/main.rs:4:10
  |
4 | #[derive(Pread, Pwrite)]
  |          ^^^^^ the trait `From<u8>` is not implemented for `B`, which is required by `u8: Into<_>`
  |
  = note: required for `u8` to implement `Into<B>`
  = note: this error originates in the derive macro `Pread` (in Nightly builds, run with -Z macro-backtrace for more info)

Adding a From<u8> impl on B does allow it to compile, but that seems like maybe not the right thing to do, since there isn't an actual real conversion from u8?

impl From<u8> for B {
    fn from(_value: u8) -> Self {
        Self { field: 0}
    }
}

Is there something I should do differently here to make this work as expected?

[m4b]

This should work; seems like a bug, iiuc? We have a test for array with generic value, but that value only appears to be u8's:

scroll/scroll_derive/tests/tests.rs

Lines 179 to 196 in f711272

	struct Data8<T, Y> {
	ids: [T; 3],
	xyz: Y,
	}
	#[test]
	fn test_generics() {
	let mut bytes = [0xde, 0xed, 0xef, 0x10, 0x10];
	let data: Data8<u8, u16> = bytes.pread_with(0, LE).unwrap();
	assert_eq!(data.ids, [0xde, 0xed, 0xef]);
	assert_eq!(data.xyz, 0x1010);
	let data: Data8<u8, u16> = bytes.cread_with(0, LE);
	assert_eq!(data.ids, [0xde, 0xed, 0xef]);
	assert_eq!(data.xyz, 0x1010);
	let size = Data8::<u8, u16>::size_with(&LE);
	let written = bytes.pwrite_with(&data, 0, LE).unwrap();
	assert_eq!(written, size);
	}

[m4b]

Yea can repro with this git apply:

diff --git a/scroll_derive/tests/tests.rs b/scroll_derive/tests/tests.rs
index 14dec4e..f04268f 100644
--- a/scroll_derive/tests/tests.rs
+++ b/scroll_derive/tests/tests.rs
@@ -269,3 +269,15 @@ fn test_custom_ctx_derive() {
     };
     assert_eq!(data3, bytes.ioread_with(LE).unwrap());
 }
+
+#[derive(Debug, Pread, Pwrite, SizeWith)]
+struct Data12 {
+    ids: [Data11; 1],
+}
+
+#[test]
+fn test_array_with_pread_data() {
+    let bytes = [0xde, 0xed, 0xef, 0x10, 0x10, 0x01];
+    let data: Data12 = bytes.pread_with(0, LE).unwrap();
+    assert_eq!(data.ids[0].a, 0xedde);
+}

[m4b]

@Systemcluster i see ea70080 added the generics support in the derive, and there's been some additional work by @Easyoakland on the derive side, I think this is probably just not correct implemented for nested types in arrays.

[m4b]

The bug on the derive side is here:

impl<'a> ::scroll::ctx::TryFromCtx<'a, ::scroll::Endian> for Data12
where
    Data12: 'a,
{
    type Error = ::scroll::Error;
    #[inline]
    fn try_from_ctx(
        src: &'a [u8],
        ctx: ::scroll::Endian,
    ) -> ::scroll::export::result::Result<(Self, usize), Self::Error> {
        use ::scroll::Pread;
        let offset = &mut 0;
        let data = Self {
            ids: {
                let mut __tmp: [Data11; 1] = [0u8.into(); 1usize];
                src.gread_inout_with(offset, &mut __tmp, ctx)?;
                __tmp
            },
        };
        Ok((data, *offset))
    }
}

right here:

                let mut __tmp: [Data11; 1] = [0u8.into(); 1usize];

We have a safe and sound array impl in rust using MaybeUninit so i think we can just use that, e.g. above simply should emit:

impl<'a> ::scroll::ctx::TryFromCtx<'a, ::scroll::Endian> for Data12
where
    Data12: 'a,
{
    type Error = ::scroll::Error;
    #[inline]
    fn try_from_ctx(
        src: &'a [u8],
        ctx: ::scroll::Endian,
    ) -> ::scroll::export::result::Result<(Self, usize), Self::Error> {
        use ::scroll::Pread;
        let offset = &mut 0;
        let data = Self {
            ids: src.gread_with(offset, ctx)?,
        };
        Ok((data, *offset))
    }
}

Historically, I think the derive with arrays significantly predated existence of MaybeUninit, and it working with any arrays, and it never came up, so I think I probably just stuck u8 as a temporary workaround in that case :D

[m4b]

cread with also broken in same manner:

scroll/scroll_derive/src/lib.rs

Lines 434 to 438 in f711272

	let mut __tmp: #ty = [0u8.into(); #size];
	for i in 0..__tmp.len() {
	__tmp[i] = src.cread_with(*offset, #ctx);
	*offset += #incr;
	}

unfortunately, something like:

diff --git a/scroll_derive/src/lib.rs b/scroll_derive/src/lib.rs
index a5714dd..846e356 100644
--- a/scroll_derive/src/lib.rs
+++ b/scroll_derive/src/lib.rs
@@ -14,18 +14,6 @@ fn impl_field(
     let default_ctx = syn::Ident::new("ctx", proc_macro2::Span::call_site()).into_token_stream();
     let ctx = custom_ctx.unwrap_or(&default_ctx);
     match *ty {
-        syn::Type::Array(ref array) => match array.len {
-            syn::Expr::Lit(syn::ExprLit {
-                lit: syn::Lit::Int(ref int),
-                ..
-            }) => {
-                let size = int.base10_parse::<usize>().unwrap();
-                quote! {
-                    #ident: { let mut __tmp: #ty = [0u8.into(); #size]; src.gread_inout_with(offset, &mut __tmp, #ctx)?; __tmp }
-                }
-            }
-            _ => panic!("Pread derive with bad array constexpr"),
-        },
         syn::Type::Group(ref group) => impl_field(ident, &group.elem, custom_ctx),
         _ => {
             quote! {

while it fixes the issue for non-generic structs, it unfortunately is broken for generics, seems like the error type diverges?

error[E0271]: type mismatch resolving `<T as TryFromCtx<'_, Endian>>::Error == Error`
   --> tests/tests.rs:177:32
    |
177 | #[derive(Debug, PartialEq, Eq, Pread, Pwrite, IOread, IOwrite, SizeWith)]
    |                                ^^^^^ expected `Error`, found associated type
    |
    = note:         expected enum `scroll::Error`
            found associated type `<T as TryFromCtx<'_, Endian>>::Error`
    = note: required for `[T; 3]` to implement `TryFromCtx<'_, Endian>`
note: required by a bound in `gread_with`
   --> /home/m4b/projects/scroll/src/pread.rs:113:26
    |
113 |     fn gread_with<'a, N: TryFromCtx<'a, Ctx, Self, Error = E>>(
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Pread::gread_with`
    = note: this error originates in the derive macro `Pread` (in Nightly builds, run with -Z macro-backtrace for more info)

i think there is probably an easy fix, but it's not hitting me at the moment and i didn't add the generics part so somewhat unsure about those code paths :)

[m4b]

I have a patch fixing this but:

impl<'a, T, Y> ::scroll::ctx::TryFromCtx<'a, ::scroll::Endian> for Data8<T, Y>
where
    T: ::scroll::ctx::TryFromCtx<'a, ::scroll::Endian, Error = ::scroll::Error>
        + ::std::marker::Copy,

I don't like how the T error is fixed to ::scroll::Error, I feel like i should be able to remove that and do:

            ids: src
                .gread_with::<[T; 3]>(offset, ctx)
                .map_err(::scroll::Error::from)?,

on all the greads but it doesn't like this., with this (to my mind, very odd) error message:

error[E0271]: type mismatch resolving `<T as TryFromCtx<'_, Endian>>::Error == Error`
   --> tests/tests.rs:208:47
    |
208 |                 .gread_with::<[T; 3]>(offset, ctx)
    |                  ----------                   ^^^ expected `Error`, found associated type
    |                  |
    |                  required by a bound introduced by this call
    |
    = note:         expected enum `scroll::Error`
            found associated type `<T as TryFromCtx<'_, Endian>>::Error`
    = note: required for `[T; 3]` to implement `TryFromCtx<'_, Endian>`

unsure why it thinks ctx all of sudden should be an Error, but is instead an associated type? 🤔

[m4b]

tl;dr we can do generics and fix this bug but it makes the generic type require it's error at the moment to be scroll::Error; @Systemcluster you added the generics portion, will this be ok for your usecases (for now)?

[m4b]

@joschock if possible, would you mind testing this branch to see if it fixes issues for you?

[m4b]

unfortunately this wouldn't be a minor patch release since in theory it changes the generics parameters for the generics portion of derive. I could make it a patch release by preserving the same broken behavior that presumably no one is using. alternatively I could just take the risk and push it as a minor patch since i can't imagine anyone was relying on generic bounds from scroll requiring their generic type be From<u8> which makes little sense, and meant all generic types were always constrained to T == u8, iiuc what I removed in the generic bounds in the derive

[m4b]

For anyone passively reading this and wondering wtf i'm talking about, e.g., this code compiled before in our tests:

#[derive(Debug, PartialEq, Eq, Pread, Pwrite, IOread, IOwrite, SizeWith)]
#[repr(C)]
struct Data8<T, Y> {
    ids: [T; 3],
    xyz: Y,
}
#[test]
fn test_generics() {
    let mut bytes = [0xde, 0xed, 0xef, 0x10, 0x10];
    let data: Data8<u8, u16> = bytes.pread_with(0, LE).unwrap();
    assert_eq!(data.ids, [0xde, 0xed, 0xef]);
    assert_eq!(data.xyz, 0x1010);

so one might wonder how was Y: From<u8> if what I'm saying was true (it is, all the generic bounds had From<u8> which was the source of john shock's confusion and their adding nonsense From<u8> to satisfy the bound; this existed solely to satisfy the array condition where we had the broken [0u8; #size].

And indeed, in the code above, u16: From<u8> , so our bound is satisfied and the derive will work. But as soon as a user adds a custom type, which doesn't implement From<u8> unless if they add that, compile will fail in the unexpected manner above.

Whew 😅

I think on hindsight it's probably safe to remove this bound in a minor patch, because while it's technically breaking, no one could really have relied on the behavior realistically, though it's possible if they have hyper generic code they may have had to forward that bound, but in this case, removing the bound won't break them, it's just one less thing we require the generic T to implement.

[joschock]

@m4b - fast turnaround 🥇

Yes, the above PR branch makes it work without requiring the unexpected From<u8> impl.

[m4b]

ok it's merged into master/main; @joschock do you mind waiting a bit, I'd like to understand how best to fix the similarly identical bug in cread derive; i may end up just emitting derive error that it doesn't work on fixed size arrays, but today the cread implementation (similar to pread) is broken on multiple levels; adding the From impl just lets it compile; if you were to pread it out of real data after pwriting, roundtripping would fail, e.g,. it doesn't work at all :D

[m4b]

(waiting a bit for a crates release that is)

[joschock]

@m4b, no rush on my end; I'll just peg to the merge commit for the interim.