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

Add const generic support #52

Open
v1gnesh opened this issue Jul 12, 2023 · 14 comments
Open

Add const generic support #52

v1gnesh opened this issue Jul 12, 2023 · 14 comments
Labels
enhancement New feature or request

Comments

@v1gnesh
Copy link

v1gnesh commented Jul 12, 2023

Hello @Licenser!

Thank you for sharing your excellent work.
I'm delighted to see that this crate shaves off about half the run time when parsing a huge binary file into structs, then into JSON.

As a novice programmer in general, I'm quite stuck by the below error.

When you have time, could you please help?

Code to reproduce:

use std::io::Cursor;
use binrw::BinRead;
use simd_json_derive::Serialize;

pub fn funk(bytes: &[u8]) -> String {
    String::from_utf8(Vec::from(bytes)).unwrap()
}

#[derive(Debug, PartialEq, BinRead, Serialize)]
pub struct EString<const N: usize>(
    #[br(map = |bytes: [u8; N]| funk(&bytes))]
    String
);

#[derive(Debug, PartialEq, BinRead, Serialize)]
#[br(big)]
pub struct Header {
    len: u16,
    _field: [u8; 12],
    // want: EString<4>,
    want: u32,
}

fn main() {

    let rec = vec![0x01, 0xB0, 0x00, 0x00, 0x1e, 0x0e, 0x00, 0x37, 0x93, 0xfa, 0x01, 0x21,
                   0x23, 0x0f, 0xe2, 0xf0, 0xe6, 0xf1];

    let value = Header::read(&mut Cursor::new(rec)).unwrap();
    // value.json_string().unwrap()
    println!("\n{}", value.json_string().unwrap());
}

Happy output:
When the EString struct is commented out, and when Header.want of u32 is used instead of EString<4>.

{"len":432,"_field":[0,0,30,14,0,55,147,250,1,33,35,15],"want":3807438577}

Sad output:

error: unexpected `const` parameter declaration
  --> src/main.rs:10:20
   |
10 | pub struct EString<const N: usize>(
   |                    ^^^^^^^^^^^^^^ expected a `const` expression, not a parameter declaration

error: proc-macro derive produced unparsable tokens
 --> src/main.rs:9:37
  |
9 | #[derive(Debug, PartialEq, BinRead, Serialize)]
  |                                     ^^^^^^^^^

error[E0207]: the const parameter `N` is not constrained by the impl trait, self type, or predicates
  --> src/main.rs:10:20
   |
10 | pub struct EString<const N: usize>(
   |                    ^^^^^^^^^^^^^^ unconstrained const parameter
   |
   = note: expressions using a const parameter must map each value to a distinct output value
   = note: proving the result of expressions other than the parameter are unique is not supported

error: aborting due to 3 previous errors
@v1gnesh v1gnesh changed the title Trouble with const Trouble with const generic Jul 12, 2023
@Licenser
Copy link
Member

Licenser commented Jul 14, 2023

Heya,

sorry for the late reply. Const generics are not implemented in the derive but the EString is fairly simple you can implement Serialize like this:

impl<const N: usize> Serialize for EString<N> {
    fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
    where
        W: std::io::Write,
    {
        self.0.json_write(writer)
    }
}

Adding it to the derive, while not impossible I think, might be a bit tricky and I don't have tons of time at the moment :(

NOTE:

I ran this example and it gives UTF8 Error, is the string valid UTF8, it fails while decoding from what I can tell?

@v1gnesh
Copy link
Author

v1gnesh commented Jul 14, 2023

Thanks for getting back!
While I'm excited at the prospect of having a workaround for this, I'm getting the same error as before 😕.

EDIT:
I've changed the body of the funk function, and it now prints a valid String.
Also replaced _field with

    _field1: u64,
    _field2: u32,

.. to just print some val, and not an array.

Header {
    len: 432,
    _field1: 33045482017786,
    _field2: 18948879,
    want: EString(
        "S0W1",
    ),
}

@Licenser
Copy link
Member

This is the whole code that compiles for me, but the inout seems to be invalid as the funk function crashes:

use binrw::BinRead;
use simd_json_derive::Serialize;
use std::io::Cursor;

pub fn funk(bytes: &[u8]) -> String {
    String::from_utf8(Vec::from(bytes)).unwrap()
}

#[derive(Debug, PartialEq, BinRead)]
pub struct EString<const N: usize>(#[br(map = |bytes: [u8; N]| funk(&bytes))] String);

impl<const N: usize> Serialize for EString<N> {
    fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
    where
        W: std::io::Write,
    {
        self.0.json_write(writer)
    }
}

#[derive(Debug, PartialEq, BinRead, Serialize)]
#[br(big)]
pub struct Header {
    len: u16,
    _field: [u8; 12],
    want: EString<4>,
    // want: u32,
}

fn main() {
    let rec = vec![
        0x01, 0xB0, 0x00, 0x00, 0x1e, 0x0e, 0x00, 0x37, 0x93, 0xfa, 0x01, 0x21, 0x23, 0x0f, 0xe2,
        0xf0, 0xe6, 0xf1,
    ];

    let value = Header::read(&mut Cursor::new(rec)).unwrap();
    dbg!(&value);
    // value.json_string().unwrap()
    println!("\n{}", value.json_string().unwrap());
}

@v1gnesh
Copy link
Author

v1gnesh commented Jul 14, 2023

Oh, didn't notice that EString shouldn't have a derive(Serialize) at the top.

#[derive(Debug, PartialEq, BinRead)]
pub struct EString<const N: usize>(#[br(map = |bytes: [u8; N]| funk(&bytes))] String);

Thank you, this is amazing!

@Licenser
Copy link
Member

nice glad we got that working and it's really cool to hear this could cut the execution time in half :D

@v1gnesh
Copy link
Author

v1gnesh commented Jul 14, 2023

Now for a dumb question - say I have loads of these -
impl Serialize for CustomField {

Can I do something like - impl Serialize for CustomField1, CustomField2 { to avoid repetition... if they're all just unit structs where I want to print out the String?

@Licenser
Copy link
Member

Licenser commented Jul 14, 2023

if they're all the same do they need to be different sturcts? (could you give examples of how CustmField1 and 2 are looking?)

@v1gnesh
Copy link
Author

v1gnesh commented Jul 14, 2023

I'm invoking different map functions on them though:

#[derive(Debug, PartialEq, BinRead)]
pub struct CustomType1 (
    #[br(map = yyyyddd)]
    String,
);

#[derive(Debug, PartialEq, BinRead)]
pub struct CustomType2(
    #[br(map = hhmmss)]
    String
);

... and in the main Header struct, they end up like this:

    tme: CustomType1,
    dte: CustomType2,

... so the below is repetitive:

impl Serialize for CustomType1 {
    fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
        where
            W: std::io::Write,
    {
        self.0.json_write(writer)
    }
}

impl Serialize for CustomType2 {
    fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
        where
            W: std::io::Write,
    {
        self.0.json_write(writer)
    }
}

@Licenser
Copy link
Member

Ah I see, one way to deal with that is making a macro for it something like (the syntax might be slightly off):

macro_rules! string_type {
    ( $x:ident ) => {
      impl Serialize for $x {
          fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
              where
                  W: std::io::Write,
          {
              self.0.json_write(writer)
          }
      }
    };
}
string_type!(CustomType1);
string_type!(CustomType2);

@v1gnesh
Copy link
Author

v1gnesh commented Jul 14, 2023

If I implement a wrapper struct like this:

pub struct CustomStuff {
    #[br(map=funk1)
    tme: Custom1,
    #[br(map=funk2)
    dte: Custom2,
}

and then in Header can I refer to them as

    tme: CustomStuff.Custom1,
    dte: CustomStuff.Custom2,

so I can

impl Serialize for CustomStuff {
    fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
        where
            W: std::io::Write,
    {
        self.tme.0.json_write(writer)
    }
}

... note that it has to have tme.0, dte.0 etc

@Licenser
Copy link
Member

that would be something that needs flattening and be implemented in the upper level, so you'd either need to hand roll the entire serializer to not use the wrapper

@v1gnesh
Copy link
Author

v1gnesh commented Jul 14, 2023

Can't say I understand fully but I'm glad the macro you've shared works.
Thanks again for your excellent, excellent work with Rust in general (incl. tremor).

@Licenser
Copy link
Member

Thank you :)! I'll rename the issue if you don't mind to better capture that support or const generics would be nice.

@Licenser Licenser changed the title Trouble with const generic Add const generic support Jul 15, 2023
@Licenser Licenser added the enhancement New feature or request label Jul 15, 2023
@v1gnesh
Copy link
Author

v1gnesh commented Jul 23, 2023

Const generics are not implemented in the derive but the EString is fairly simple you can implement Serialize like this:

impl<const N: usize> Serialize for EString<N> {
    fn json_write<W>(&self, writer: &mut W) -> simd_json_derive::Result
    where
        W: std::io::Write,
    {
        self.0.json_write(writer)
    }
}

Adding it to the derive, while not impossible I think, might be a bit tricky and I don't have tons of time at the moment :(

@Licenser Similar to the Serialize impl in this comment, I attempted to impl for a unit struct with a field that uses const generic like so:

pub struct OString<const L: usize>(
    #[br(map = fenk)]
    String<L>
);

... where String type is from the heapless crate.

And when I try to impl, it says Only traits defined in the current crate can be implemented for arbitrary types [E0117].

So... I think this comment is still relevant to this new issue name (const generic support).
This one, I can't work around though :(

PS: Another item in the wishlist - Serialize impl for types from the speedate crate.
Can I open another issue for this one, if you will get time to spare, at least sometime this year? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants