Fix derive in macro-expanded structs

[jan-auer]

This accommodates for an apparent change in behavior since Rust 1.47.0. When array-type struct fields are expanded in a macro, they are put in an invisible type group, which the derive needs to handle.

Minimal repro:

use scroll::Pread;

macro_rules! test {
    ( struct $name:ident { $( $field:ident: $t:ty, )* } ) => {
        #[derive(Pread)]
        pub struct $name {
            $( pub $field: $t, )*
        }
    };
}

test! {
    struct Test {
        field: [u16; 40],
    }
}

[jan-auer]

The change is here. I went with recursion to avoid duplication and I'm not sure if this can actually occur recursively.

[m4b]

maybe add a note about this, so it isn't refactored / changed in the future?

[m4b]

I assume this is backwards compatible with pre 1.47 ?

[m4b]

A comment like I suggested would be nice, about the 1.47 breakage; can we add your repro as a test to prevent this in the future?

And thank you for this! I would never have guessed from the changelog that a backwards incompatible change occurred in stable (nor that this would be affected...)

[m4b]

maybe add a note about this, so it isn't refactored / changed in the future?

[m4b]

I didn't really understand the discussion in the rust issue, it was stated that either the macro is unhygenic, or we didn't correctly handle Delimiter::None; since this change doesn't appear to handle Delimiter::None (afaics), that leads me to conclude the macro is unhygenic?

Edit: @mitsuhiko maybe you can shed some light on this?

[m4b]

I am even more confused right now; we do have derive(Pread) on arrays in scroll_derive/tests (https://github.com//m4b/scroll/blob/357df8d1f2680800ee4e30b83f04db50b7aa6243/scroll_derive/tests/tests.rs#L29), which for me compile without error with rustc 1.47

    Finished test [unoptimized + debuginfo] target(s) in 0.05s
     Running target/debug/deps/scroll_derive-309c8411296c9539

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/tests-952cf3cd5783b14d

running 10 tests
test test_array ... ok
test test_array_copy ... ok
test test_data ... ok
test test_ioread ... ok
test test_generics ... ok
test test_iowrite ... ok
test test_newtype ... ok
test test_nested_struct ... ok
test test_pread_arrays ... ok
test test_sizewith ... ok

test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests scroll_derive

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

rustc --version
rustc 1.47.0 (18bf6b4f0 2020-10-07)

[m4b]

Ohhhhh, gosh, sorry I totally missed the part that the derive pread on an array is inside a macro_rules!

[m4b]

Ok since this is kind of an emergency I'm going to merge it and push new version; we can follow up later on other details if needed

[m4b]

scroll_derive 0.10.3 is out with this fix, thanks @jan-auer et. al!

[jan-auer]

I should have probably provided more information. Would you like me to follow up with some code comments on this?

Effectively the change is that now the compiler creates more None delimiters, which syn parses into a syn::Type::Group wrapping the actual type. In this case, for some reason it does not do this when you just have a regular struct with an array field, but it does when you put that inside a macro. Hence the repro.

On the bottom line, I think this is actually something that this derive just has to handle. Since every type can (now) appear inside an invisible group, it has to account for that and traverse.

Thanks for merging this!