m4b / scroll

Scroll - making scrolling through buffers fun since 2016
MIT License
151 stars 36 forks source link

Pread, Pwrite with struct containing [u8; 8] do not work anymore with Rust 1.47.0 #76

Closed andresv closed 3 years ago

andresv commented 3 years ago

I have a struct which has [u8; 8] field and with Rust 1.47.0 it does not build anymore. I am using scroll 0.10.1.

56  |                   #[derive(Debug, Default, Pread, Pwrite)]
    |                                            ^^^^^ the trait `scroll::ctx::TryFromCtx<'_, _>` is not implemented for `[u8; 8]`
m4b commented 3 years ago

yea unfortunately it looks like 1.47 broke backwards compat for custom derives in some cases (https://github.com/rust-lang/rust/issues/77718, and somehow a crater run missed one of the crates using it, I'm not sure exactly), #75 is fixing this

m4b commented 3 years ago

@andresv is your derive inside a macro_rules!? That was the breakage mentioned in #75; if it is breaking outside a macro then that's not good.

m4b commented 3 years ago

ok 0.10.3 was released, cargo update should fix you; if not, we'll have to figure out what's going on :)

andresv commented 3 years ago

Yes it was inside macro_rules!. Hmm scroll 0.10.3 is not in crates: https://crates.io/crates/scroll?

m4b commented 3 years ago

Sorry, should have clarified, scroll_derive got bumped to 0.10.3; cargo update should get you the right stuff, unless you are pinning it to e.g. 0.10.2

andresv commented 3 years ago

I have it like this: scroll = { version = "0.10", features = ["derive"], default-features = false } And I did: cargo update -p scroll

m4b commented 3 years ago

you'll have to do cargo update -p scroll_derive unless I'm mistaken

andresv commented 3 years ago

I guess the problem is that I do not use scroll_derive crate directly. Basically I am using the version that scroll itself uses as dependency.

Currently I have:

Cargo.toml

scroll = { version = "0.10", features = ["derive"], default-features = false }

some lib.rs

use scroll::{self, Pread, Pwrite, BE};
...
// this inside some macro_rules!
#[derive(Debug, Default, Pread, Pwrite)]
pub struct Payload {
    $(pub $write_fields: $write_ty),*
}
...              

However I guess I should somehow use scroll_derive directly. I tried that one:

scroll = { version = "0.10", features = ["derive"], default-features = false }
scroll_derive = "0.10.3"
use scroll::{self, BE};
use scroll_derive::{Pread, Pwrite};
...
// this inside some macro_rules!
#[derive(Debug, Default, Pread, Pwrite)]
pub struct Payload {
    $(pub $write_fields: $write_ty),*
}
...

It seems after that it is happy with Pread, but barks at Pwrite:

error[E0277]: the trait bound `&[u8; 8]: scroll::ctx::TryIntoCtx<_>` is not satisfied
    |
57  |                   #[derive(Debug, Default, Pread, Pwrite)]
    |                                                   ^^^^^^ the trait `scroll::ctx::TryIntoCtx<_>` is not implemented for `&[u8; 8]`

164 |         let len = buf.pwrite_with(frame, 0, BE)?;
    |                       ^^^^^^^^^^^ method not found in `&mut [u8]`
    |
    = help: items from traits can only be used if the trait is in scope
    = note: the following trait is implemented but not in scope; perhaps add a `use` for it:
            `use scroll::Pwrite;`
andresv commented 3 years ago

This is minimal macro that introduces issues with rust >= 1.47.

Cargo.toml

[dependencies]
scroll = {version = "0.10", features = ["derive"] }
use scroll::{Pread, Pwrite};
macro_rules! message {
    ($message:ident, $write_ty: ty) => {
        #[derive(Debug, Default, Pread, Pwrite)]
        struct Data {
            $message: $write_ty,
        }
    };
}

// this works
message!(payload, u8);
// this does not work anymore with rust >= 1.47
// works fine with 1.46
message!(payload, [u8; 8]);
45 |         #[derive(Debug, Default, Pread, Pwrite)]
   |                                         ^^^^^^ the trait `TryIntoCtx<_>` is not implemented for `&[u8; 8]`
...
53 | message!(payload, [u8; 8]);
   | --------------------------- in this macro invocation
   |
   = help: the following implementations were found:
             <&'a [u8] as TryIntoCtx>
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

No problem If [u8; 8] is directly in macro:

macro_rules! message {
    ($message:ident, $write_ty: ty) => {
        #[derive(Debug, Default, Pread, Pwrite)]
        struct Data {
            $message: [u8; 8],
        }
    };
}
m4b commented 3 years ago

Ok I’m sorry I didn’t see the follow up that pwrite also doesn’t work. Thank you for repro. @jan-auer any idea what might be going on here ?

m4b commented 3 years ago

Ok i can repro on 1.48; this is weird; it's definitely only inside the macro; if i add Pwrite to the macro test in tests:

  --> tests/tests.rs:10:25
   |
10 |           #[derive(Pread, Pwrite)]

the test fails to compile, with same error.

m4b commented 3 years ago

so expanding the macro yields the following for pread and pwrite:

impl <'a> ::scroll::ctx::TryFromCtx<'a, ::scroll::Endian> for Message where
 Message: '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{payload:
                     {
                         let mut __tmp: [u8; 8] = [0u8.into(); 8usize];
                         src.gread_inout_with(offset, &mut __tmp, ctx)?;
                         __tmp
                     },};
        Ok((data, *offset))
    }
}
impl <'a> ::scroll::ctx::TryIntoCtx<::scroll::Endian> for &'a Message {
    type Error = ::scroll::Error;
    #[inline]
    fn try_into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian)
     -> ::scroll::export::result::Result<usize, Self::Error> {
        use ::scroll::Pwrite;
        let offset = &mut 0;
        dst.gwrite_with(&self.payload, offset, ctx)?;
        ;
        Ok(*offset)
    }
}

but gets the following when outside the macro:

impl <'a> ::scroll::ctx::TryIntoCtx<::scroll::Endian> for &'a Message {
    type Error = ::scroll::Error;
    #[inline]
    fn try_into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian)
     -> ::scroll::export::result::Result<usize, Self::Error> {
        use ::scroll::Pwrite;
        let offset = &mut 0;
        for i in 0..self.payload.len() {
            dst.gwrite_with(&self.payload[i], offset, ctx)?;
        };
        ;
        Ok(*offset)
    }
}

so it gets treated as a slice iirc. it's quite strange :thinking:

m4b commented 3 years ago

ok i see what's going on; one has to do basically what was done for pread, taking into account the invisible groups; i'm playing with it right now, may have something up soon

m4b commented 3 years ago

ok i believe i've fixed it; the codegen is more optimal now as well, it probably is the same, but relied on array len being const which was only recently? anyway, it now emits the exact len so the compiler should have no trouble unrolling now:

        for i in 0..8usize {
            dst.gwrite_with(&self.payload[i], offset, ctx)?;
        };
m4b commented 3 years ago

@andresv i believe this is fixed, and released in scroll_derive 0.10.5; you will need to update as per instructions above, i.e., cargo update -p scroll_derive; please let me know if this does not fix your issues, and reopen :)

andresv commented 3 years ago

Excellent, I can confirm it works.

m4b commented 3 years ago

wonderful! apologies for the late fix, i didn't see that your pwrite derive had the same issue, thanks for pinging again with the repro :)

jan-auer commented 3 years ago

Looks like this is the same issue as in https://github.com/m4b/scroll/pull/75, except we forgot to check Pwrite :)