sharksforarms / deku

Declarative binary reading and writing: bit-level, symmetric, serialization/deserialization
Apache License 2.0
1.15k stars 55 forks source link

Enum discriminant re/mapping #328

Closed IniterWorker closed 1 year ago

IniterWorker commented 1 year ago

Let's say you want to map an enum that's not the perfect mirror of the data.

Is there any way to do it with the current proc-macros?

Example

pub enum TestDesMapping {
    /// A
    A = 0,
    /// B
    B = 1,
    /// C everything in range (C_desc + 1)..
    C = 2,

Current behaviour

If we use id_pat or id like below then we will raise an error on DekuRead method try_from : Too much data.

#[derive(DekuWrite, DekuRead)]
pub enum TestDesMapping {
    /// A
    A = 0,
    /// B
    B = 1,
    /// C everything after
    #[deku(id_pat = "_")]
    C = 2,

Expected Behaviour

In this scenario, I should expect to have C for the range (C_desc + 1)...

sharksforarms commented 1 year ago

Can you use id_pat like so? https://github.com/sharksforarms/deku/blob/13fc4e9b084042ac77f9a3f6aa366f791ddac646/examples/enums.rs#L20-L21

Are you able to provide a minimal example to showcase what you're trying to do?

IniterWorker commented 1 year ago

I would like to use a pattern match such as id_pat. But currently it doesn't fit my use case.

Failures

failures:
    test::test_basic_c
    test::test_basic_pattern

Example

#[cfg(test)]
mod test {
    use deku::prelude::*;

    #[derive(Clone, Copy, PartialEq, Eq, Debug, DekuWrite, DekuRead)]
    #[deku(type = "u8")]
    #[non_exhaustive]
    #[repr(u8)]
    pub enum TestDesMapping {
        /// A
        A = 0,
        /// B
        B = 1,
        /// C everything after
        #[deku(id_pat = "_")]
        C = 2,
    }

    #[test]
    fn test_basic_a() {
        let input = [0u8];
        let ret_read = TestDesMapping::try_from(input.as_slice()).unwrap();
        assert_eq!(TestDesMapping::A, ret_read);
        let ret_write: Vec<u8> = ret_read.try_into().unwrap();
        assert_eq!(input.to_vec(), ret_write);
    }

    #[test]
    fn test_basic_c() {
        let input = [2u8];
        let ret_read = TestDesMapping::try_from(input.as_slice()).unwrap();
        assert_eq!(TestDesMapping::C, ret_read);
        let ret_write: Vec<u8> = ret_read.try_into().unwrap();
        assert_eq!(input.to_vec(), ret_write);
    }

    #[test]
    fn test_basic_pattern() {
        let input = [10u8];
        let ret_read = TestDesMapping::try_from(input.as_slice()).unwrap();
        assert_eq!(TestDesMapping::C, ret_read);
        let ret_write: Vec<u8> = ret_read.try_into().unwrap();
        assert_eq!(input.to_vec(), ret_write);
    }
}
sharksforarms commented 1 year ago

I think you're hitting a rough/edge case here where when id_pat is used, the value which is read is not consumed, and so you're getting Too much data

As can be shown in the following example:

use deku::prelude::*;

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(type = "u8")]
enum DekuTest {
    A = 0,
    #[deku(id_pat = "_")]
    B = 1,
}

fn main() {
    let test_data = vec![0, 1];

    let (rest, deku_test) = DekuTest::from_bytes((test_data.as_ref(), 0)).unwrap();
    assert_eq!(
        DekuTest::A,
        deku_test
    );
    // 0 got consumed
    assert_eq!(rest.0, &[1]);

    let (rest, deku_test) = DekuTest::from_bytes(rest).unwrap();
    assert_eq!(
        DekuTest::B,
        deku_test
    );
    // 1 did not get consumed
    assert_eq!(rest.0, &[1]);
}

As explained in the documentation of id_pat,

The enum variant must have space to store the identifier for proper writing.

But in your case, you don't want symetric, so you don't need to store the identifier for writing.

Things get funky when using both discrimiant values and id/id_pat

The solution which comes to mind is using id, id_pat and map

use deku::prelude::*;
use std::convert::TryInto;

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(type = "u8")]
enum DekuTest {
    #[deku(id = "0")]
    A,
    #[deku(id_pat = "_")]
    B(
        #[deku(map = "|_: u8| -> Result<_, DekuError> { Ok(2) }")]
        u8
    ),
}

fn main() {
    let test_data = vec![0, 1, 2];

    let (rest, deku_test) = DekuTest::from_bytes((test_data.as_ref(), 0)).unwrap();
    assert_eq!(
        DekuTest::A,
        deku_test
    );
    // 0 got consumed
    assert_eq!(rest.0, &[1, 2]);
    let output: Vec<u8> = deku_test.try_into().unwrap();
    assert_eq!(output, vec![0]);

    // ---

    let (rest, deku_test) = DekuTest::from_bytes(rest).unwrap();
    assert_eq!(
        DekuTest::B(2),
        deku_test
    );
    // 1 got consumed
    assert_eq!(rest.0, &[2]);
    let output: Vec<u8> = deku_test.try_into().unwrap();
    assert_eq!(output, vec![2]);

    // ---

    let (rest, deku_test) = DekuTest::from_bytes(rest).unwrap();
    assert_eq!(
        DekuTest::B(2),
        deku_test
    );
    // 2 got consumed
    assert_eq!(rest.0, &[]);
    let output: Vec<u8> = deku_test.try_into().unwrap();
    assert_eq!(output, vec![2]);
}

cc: @wcampbell0x2a maybe has another idea

sharksforarms commented 1 year ago

For your tests, @IniterWorker

#[cfg(test)]
mod test {
    use deku::prelude::*;
    #[derive(Clone, Copy, PartialEq, Eq, Debug, DekuWrite, DekuRead)]
    #[deku(type = "u8")]
    #[non_exhaustive]
    #[repr(u8)]
    pub enum TestDesMapping {
        /// A
+       #[deku(id = "0")]
+       A,
-        A = 0,
        /// B
+        #[deku(id = "1")]
+        B,
-        B = 1,
        /// C everything after
+        #[deku(id_pat = "_")]
+        C(#[deku(map = "|_: u8| -> Result<_, DekuError> { Ok(2) }")] u8),
-        C = 2,
    }

    #[test]
    fn test_basic_a() {
        let input = [0u8];
        let ret_read = TestDesMapping::try_from(input.as_slice()).unwrap();
        assert_eq!(TestDesMapping::A, ret_read);
        let ret_write: Vec<u8> = ret_read.try_into().unwrap();
        assert_eq!(input.to_vec(), ret_write);
    }

    #[test]
    fn test_basic_c() {
        let input = [2u8];
        let ret_read = TestDesMapping::try_from(input.as_slice()).unwrap();
+       assert_eq!(TestDesMapping::C(2), ret_read);
-        assert_eq!(TestDesMapping::C, ret_read);
        let ret_write: Vec<u8> = ret_read.try_into().unwrap();
        assert_eq!(input.to_vec(), ret_write);
    }

    #[test]
    fn test_basic_pattern() {
        let input = [10u8];
        let ret_read = TestDesMapping::try_from(input.as_slice()).unwrap();
+       assert_eq!(TestDesMapping::C(2), ret_read);
-        assert_eq!(TestDesMapping::C, ret_read);
        let ret_write: Vec<u8> = ret_read.try_into().unwrap();
+        assert_eq!(vec![2u8], ret_write);
-        assert_eq!(input.to_vec(), ret_write);
    }
}
IniterWorker commented 1 year ago

Hi @sharksforarms,

Thank you for the quick answer,

This approach doesn't cover the primitive requirement from the enum. Because, in this example, we are using a variant with a parameter, the enum can't be represented by its own primitive.

This will reduce a lot of feature/impl from here, and we will get an error on DekuRead and/or DekuWrite that using as keyword to cast from deku(type = "u8").

#[derive(DekuWrite, DekuRead))]
         ^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

I am wondering if the best way for us is to allow match a range in the deku derive id parameter.

PS: I have done some work on my code that uses the crate deku to fit with your current approach. Thanks!

IniterWorker commented 1 year ago

@sharksforarms, could you give me your input?

#[derive(Clone, Copy, PartialEq, Eq, Debug, DekuWrite, DekuRead)]
#[deku(type = "u8")]
#[non_exhaustive]
#[repr(u8)]
pub enum TestDesMapping {
    /// A
    A = 0,
    /// B
    B = 1,
    /// C everything after
    #[deku(catch_all)]
    C = 2,
}

At the first glance, I think, I should put the logic in this section: https://github.com/sharksforarms/deku/blob/dccf88a97bdae9f71f0c300850c0b8ed15d8eac3/deku-derive/src/macros/deku_read.rs#L185