Repository: sharksforarms/deku
Pull request #454 (2024-06-19 03:06:37)

Restore v0.16.0 id_pat
* When moving to the Read powered impl, I forced the user to use the same type as the id when reading into an id_pat With the use of Seek, we can now fix that and just seek back to the position before the id was read in the case of id_pat

github-actions[bot]: ## Benchmark for 22221b6
  <details>
    <summary>Click to view benchmark</summary>

| Test | Base         | PR               | % |
|------|--------------|------------------|---|
| deku_read_bits | 1281.4±11.79ns | **1247.9±26.80ns** | **-2.61%** |
| deku_read_byte | 3.4±0.07ns | 3.4±0.06ns | 0.00% |
| deku_read_enum | 2.6±0.04ns | 2.6±0.07ns | 0.00% |
| deku_read_vec | **35.5±2.16ns** | 36.8±0.95ns | **+3.66%** |
| deku_write_bits | 173.6±6.01ns | **167.2±9.96ns** | **-3.69%** |
| deku_write_byte | 24.0±0.36ns | 24.3±1.73ns | +1.25% |
| deku_write_enum | 22.6±0.37ns | 22.6±0.38ns | 0.00% |
| deku_write_vec | 415.8±6.26ns | 417.1±3.39ns | +0.31% |

  </details>
  
github-actions[bot]: ## Benchmark for 556c1e7
  <details>
    <summary>Click to view benchmark</summary>

| Test | Base         | PR               | % |
|------|--------------|------------------|---|
| deku_read_bits | 1273.3±19.06ns | **1201.5±15.42ns** | **-5.64%** |
| deku_read_byte | 3.4±0.09ns | 3.4±0.12ns | 0.00% |
| deku_read_enum | 2.6±0.04ns | 2.6±0.04ns | 0.00% |
| deku_read_vec | **34.9±0.57ns** | 36.4±0.60ns | **+4.30%** |
| deku_write_bits | 183.4±4.70ns | **165.6±4.03ns** | **-9.71%** |
| deku_write_byte | 24.0±0.24ns | 24.1±0.49ns | +0.42% |
| deku_write_enum | 22.7±0.45ns | 22.7±0.34ns | 0.00% |
| deku_write_vec | 420.2±7.31ns | 414.7±6.46ns | -1.31% |

  </details>
  
wcampbell0x2a: Waiting for #360 
github-actions[bot]: ## Benchmark for 397a8b8
  <details>
    <summary>Click to view benchmark</summary>

| Test | Base         | PR               | % |
|------|--------------|------------------|---|
| count | **3.6±0.02µs** | 5.0±0.01µs | **+38.89%** |
| deku_read_bits | **1162.0±11.01ns** | 1188.4±11.06ns | **+2.27%** |
| deku_read_byte | **3.4±0.07ns** | 3.6±0.06ns | **+5.88%** |
| deku_read_enum | **3.1±0.07ns** | 3.5±0.05ns | **+12.90%** |
| deku_read_vec | **31.0±0.70ns** | 43.2±0.34ns | **+39.35%** |
| deku_write_bits | 189.2±4.86ns | **176.0±2.64ns** | **-6.98%** |
| deku_write_byte | 24.5±0.46ns | 24.9±0.75ns | +1.63% |
| deku_write_enum | **22.6±0.36ns** | 24.0±0.65ns | **+6.19%** |
| deku_write_vec | 416.1±6.63ns | 411.2±5.85ns | -1.18% |
| read_all | 5.6±0.03µs | **3.8±0.06µs** | **-32.14%** |
| read_all_bytes | **3.9±0.06µs** | 5.5±0.01µs | **+41.03%** |

  </details>
  
github-actions[bot]: ## Benchmark for cae76f9
  <details>
    <summary>Click to view benchmark</summary>

| Test | Base         | PR               | % |
|------|--------------|------------------|---|
| count | **3.4±0.01µs** | 5.0±0.02µs | **+47.06%** |
| deku_read_bits | 1138.2±19.45ns | 1136.3±13.71ns | -0.17% |
| deku_read_byte | 3.6±0.13ns | **3.4±0.04ns** | **-5.56%** |
| deku_read_enum | 3.5±0.03ns | **3.1±0.05ns** | **-11.43%** |
| deku_read_vec | **32.3±0.38ns** | 42.4±0.61ns | **+31.27%** |
| deku_write_bits | 192.2±4.54ns | 195.5±5.03ns | +1.72% |
| deku_write_byte | **24.4±0.41ns** | 24.8±0.31ns | **+1.64%** |
| deku_write_enum | **22.6±0.37ns** | 24.1±0.48ns | **+6.64%** |
| deku_write_vec | **416.2±5.39ns** | 470.8±9.26ns | **+13.12%** |
| read_all | **3.8±0.05µs** | 5.5±0.06µs | **+44.74%** |
| read_all_bytes | 5.4±0.04µs | **3.8±0.22µs** | **-29.63%** |

  </details>
  
wcampbell0x2a: Ah, I'll need to fix the commits. Unfortunate side effect of squashing.

Patch:
diff --git a/deku-derive/src/lib.rs b/deku-derive/src/lib.rs
index fea289b..a686b3b 100644
--- a/deku-derive/src/lib.rs
+++ b/deku-derive/src/lib.rs
@@ -571,8 +571,8 @@ impl FieldData {
                 || (data.bits_read.is_some() || data.bytes_read.is_some()))
         {
             return Err(cerror(
-                data.bits.span(),
-                "conflicting: `read_all` cannot be used with `until`, count`, `bits_read`, or `bytes_read`",
+                data.read_all.span(),
+                "conflicting: `read_all` cannot be used with `until`, `count`, `bits_read`, or `bytes_read`",
             ));
         }
 
@@ -584,7 +584,7 @@ impl FieldData {
             > 1
         {
             return Err(cerror(
-                data.bits.span(),
+                data.ty.span(),
                 "conflicting: only one `seek` attribute can be used at one time",
             ));
         }
diff --git a/deku-derive/src/macros/deku_read.rs b/deku-derive/src/macros/deku_read.rs
index 287e03c..587c05a 100644
--- a/deku-derive/src/macros/deku_read.rs
+++ b/deku-derive/src/macros/deku_read.rs
@@ -227,21 +227,22 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> {
             .and_then(|v| v.ident.as_ref())
             .is_some();
 
-        let (use_id, variant_id) = if let Some(variant_id) = &variant.id {
+        let mut restore = false;
+        let variant_id = if let Some(variant_id) = &variant.id {
             match variant_id {
-                Id::TokenStream(v) => (false, quote! {&#v}.into_token_stream()),
-                Id::LitByteStr(v) => (false, v.into_token_stream()),
-                Id::Int(v) => (false, v.into_token_stream()),
+                Id::TokenStream(v) => quote! {&#v}.into_token_stream(),
+                Id::LitByteStr(v) => v.into_token_stream(),
+                Id::Int(v) => v.into_token_stream(),
             }
         } else if let Some(variant_id_pat) = &variant.id_pat {
             // If user has supplied an id, then we have an id_pat that and the id variant doesn't
             // need read into an id value
             if id.is_none() {
-                // if set, the first field read will not read from reader and instead
-                // be __deku_variant_id
-                (true, variant_id_pat.clone())
+                // if id_pat and !id, we need to restore after reading
+                restore = true;
+                variant_id_pat.clone()
             } else {
-                (false, variant_id_pat.clone())
+                variant_id_pat.clone()
             }
         } else if has_discriminant {
             let ident = &variant.ident;
@@ -249,7 +250,7 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> {
             pre_match_tokens.push(quote! {
                 let #internal_ident = <#id_type>::try_from(Self::#ident as isize)?;
             });
-            (true, quote! { _ if __deku_variant_id == #internal_ident })
+            quote! { _ if __deku_variant_id == #internal_ident }
         } else {
             return Err(syn::Error::new(
                 variant.ident.span(),
@@ -269,7 +270,7 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> {
             quote! { #variant_reader; }
         } else {
             let (field_idents, field_reads) =
-                emit_field_reads(input, &variant.fields.as_ref(), &ident, use_id)?;
+                emit_field_reads(input, &variant.fields.as_ref(), &ident, restore)?;
 
             // filter out temporary fields
             let field_idents = field_idents
@@ -589,7 +590,7 @@ fn emit_field_read(
     i: usize,
     f: &FieldData,
     ident: &TokenStream,
-    use_id: bool,
+    restore_pad_id: bool,
 ) -> Result<(TokenStream, TokenStream), syn::Error> {
     let crate_ = super::get_crate_name();
     let field_type = &f.ty;
@@ -726,9 +727,18 @@ fn emit_field_read(
             quote!(<#field_type as ::#crate_::DekuReader<'_, _>>)
         };
 
-        if use_id {
+        if restore_pad_id {
             quote! {
-                __deku_variant_id
+                {
+                    if let Err(e) = __deku_reader.seek_last_read() {
+                        return Err(DekuError::Io(e.kind()));
+                    }
+                    #type_as_deku_read::from_reader_with_ctx
+                    (
+                        __deku_reader,
+                        (#read_args)
+                    )?
+                }
             }
         } else if let Some(field_count) = &f.count {
             quote! {
diff --git a/src/reader.rs b/src/reader.rs
index 0fed9ba..d85414f 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -32,11 +32,14 @@ pub struct Reader<'a, R: Read + Seek> {
     inner: &'a mut R,
     /// bits stored from previous reads that didn't read to the end of a byte size
     leftover: Option<Leftover>,
-    /// Amount of bits read during the use of [read_bits](Reader::read_bits) and [read_bytes](Reader::read_bytes).
+    /// Amount of bits read during the last call to [read_bits](Reader::read_bits) and [read_bytes](Reader::read_bytes)
+    pub last_bits_read_amt: usize,
+    /// Amount of bits read during the use of [read_bits](Reader::read_bits) and [read_bytes](Reader::read_bytes)
     pub bits_read: usize,
 }
 
 impl<R: Read + Seek> Seek for Reader<'_, R> {
+    #[inline]
     fn seek(&mut self, pos: SeekFrom) -> no_std_io::io::Result<u64> {
         #[cfg(feature = "logging")]
         log::trace!("seek: {pos:?}");
@@ -48,6 +51,7 @@ impl<R: Read + Seek> Seek for Reader<'_, R> {
 }
 
 impl<R: Read + Seek> AsMut<R> for Reader<'_, R> {
+    #[inline]
     fn as_mut(&mut self) -> &mut R {
         self.inner
     }
@@ -60,10 +64,23 @@ impl<'a, R: Read + Seek> Reader<'a, R> {
         Self {
             inner,
             leftover: None,
+            last_bits_read_amt: 0,
             bits_read: 0,
         }
     }
 
+    /// Seek to previous previous before the last read, used for `id_pat`
+    #[inline]
+    pub fn seek_last_read(&mut self) -> no_std_io::io::Result<()> {
+        let number = self.last_bits_read_amt as i64;
+        let seek_amt = (number / 8).saturating_add((number % 8).signum());
+        self.seek(SeekFrom::Current(seek_amt.saturating_neg()))?;
+        self.bits_read -= self.last_bits_read_amt;
+        self.leftover = None;
+
+        Ok(())
+    }
+
     /// Consume self, returning inner Reader
     #[inline]
     pub fn into_inner(self) -> &'a mut R {
@@ -254,9 +271,13 @@ impl<'a, R: Read + Seek> Reader<'a, R> {
             }
         }
 
-        self.bits_read += ret.len();
+        let bits_read = ret.len();
+        self.last_bits_read_amt = bits_read;
+        self.bits_read += bits_read;
+
         #[cfg(feature = "logging")]
         log::trace!("read_bits: returning {ret}");
+
         Ok(Some(ret))
     }
 
@@ -280,7 +301,9 @@ impl<'a, R: Read + Seek> Reader<'a, R> {
                 return Err(DekuError::Io(e.kind()));
             }
 
-            self.bits_read += amt * 8;
+            let bits_read = amt * 8;
+            self.last_bits_read_amt += bits_read;
+            self.bits_read += bits_read;
 
             #[cfg(feature = "logging")]
             log::trace!("read_bytes: returning {:02x?}", &buf[..amt]);
@@ -362,6 +385,7 @@ impl<'a, R: Read + Seek> Reader<'a, R> {
                 return Err(DekuError::Io(e.kind()));
             }
 
+            self.last_bits_read_amt += N * 8;
             self.bits_read += N * 8;
 
             #[cfg(feature = "logging")]


Test patch:
diff --git a/tests/test_attributes/test_ctx.rs b/tests/test_attributes/test_ctx.rs
index 2decec6..d0bb7bb 100644
--- a/tests/test_attributes/test_ctx.rs
+++ b/tests/test_attributes/test_ctx.rs
@@ -125,6 +125,8 @@ fn test_struct_enum_ctx_id() {
         VarB,
         #[deku(id = "(2, 3)")]
         VarC(u8),
+        #[deku(id_pat = "_")]
+        VarAll(u8),
     }
 
     #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
@@ -198,6 +200,24 @@ fn test_struct_enum_ctx_id() {
 
     let ret_write: Vec<u8> = ret_read.try_into().unwrap();
     assert_eq!(ret_write, test_data);
+
+    // VarPat
+    let test_data = [0x02_u8, 0xff, 0xcc];
+    let mut cursor = Cursor::new(test_data);
+    let (_, ret_read) = StructEnumId::from_reader((&mut cursor, 0)).unwrap();
+
+    assert_eq!(
+        StructEnumId {
+            my_id: 0x02,
+            data: 0xff,
+            enum_from_id: EnumId::VarAll(0xcc),
+            enum_from_just_id: EnumJustId::VarB,
+        },
+        ret_read
+    );
+
+    let ret_write: Vec<u8> = ret_read.try_into().unwrap();
+    assert_eq!(ret_write, test_data);
 }
 
 #[test]
diff --git a/tests/test_compile/cases/count_read_conflict.stderr b/tests/test_compile/cases/count_read_conflict.stderr
index 19f29cf..9adaa4a 100644
--- a/tests/test_compile/cases/count_read_conflict.stderr
+++ b/tests/test_compile/cases/count_read_conflict.stderr
@@ -22,7 +22,7 @@ error: conflicting: both `count` and `bytes_read` specified on field
 23 |     #[deku(count = "1", bytes_read = "2")]
    |                    ^^^
 
-error: conflicting: `read_all` cannot be used with `until`, count`, `bits_read`, or `bytes_read`
+error: conflicting: `read_all` cannot be used with `until`, `count`, `bits_read`, or `bytes_read`
   --> $DIR/count_read_conflict.rs:27:10
    |
 27 | #[derive(DekuRead)]
@@ -30,7 +30,7 @@ error: conflicting: `read_all` cannot be used with `until`, count`, `bits_read`,
    |
    = note: this error originates in the derive macro `DekuRead` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error: conflicting: `read_all` cannot be used with `until`, count`, `bits_read`, or `bytes_read`
+error: conflicting: `read_all` cannot be used with `until`, `count`, `bits_read`, or `bytes_read`
   --> $DIR/count_read_conflict.rs:33:10
    |
 33 | #[derive(DekuRead)]
@@ -38,7 +38,7 @@ error: conflicting: `read_all` cannot be used with `until`, count`, `bits_read`,
    |
    = note: this error originates in the derive macro `DekuRead` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error: conflicting: `read_all` cannot be used with `until`, count`, `bits_read`, or `bytes_read`
+error: conflicting: `read_all` cannot be used with `until`, `count`, `bits_read`, or `bytes_read`
   --> $DIR/count_read_conflict.rs:39:10
    |
 39 | #[derive(DekuRead)]
diff --git a/tests/test_compile/cases/seek_conflict.stderr b/tests/test_compile/cases/seek_conflict.stderr
index 0666f5e..d16a136 100644
--- a/tests/test_compile/cases/seek_conflict.stderr
+++ b/tests/test_compile/cases/seek_conflict.stderr
@@ -1,7 +1,5 @@
 error: conflicting: only one `seek` attribute can be used at one time
- --> tests/test_compile/cases/seek_conflict.rs:3:10
+ --> tests/test_compile/cases/seek_conflict.rs:6:13
   |
-3 | #[derive(DekuRead, Debug, PartialEq, Eq)]
-  |          ^^^^^^^^
-  |
-  = note: this error originates in the derive macro `DekuRead` (in Nightly builds, run with -Z macro-backtrace for more info)
+6 |     byte_a: u8,
+  |             ^^
diff --git a/tests/test_enum.rs b/tests/test_enum.rs
index 4459ff0..56e61eb 100644
--- a/tests/test_enum.rs
+++ b/tests/test_enum.rs
@@ -99,6 +99,8 @@ fn test_enum_array_type() {
         VarA,
         #[deku(id = "[1,1,1]")]
         VarB,
+        #[deku(id_pat = "_")]
+        VarC((u8, u8, u8)),
     }
 
     let input = b"123".to_vec();
@@ -108,13 +110,18 @@ fn test_enum_array_type() {
 
     let ret_write: Vec<u8> = ret_read.try_into().unwrap();
     assert_eq!(input.to_vec(), ret_write);
+
+    let input = b"321".to_vec();
+
+    let ret_read = TestEnumArray::try_from(input.as_slice()).unwrap();
+    assert_eq!(TestEnumArray::VarC((b"3"[0], b"2"[0], b"1"[0])), ret_read);
+
+    let ret_write: Vec<u8> = ret_read.try_into().unwrap();
+    assert_eq!(input.to_vec(), ret_write);
 }
 
 #[test]
 fn test_id_pat_with_id() {
-    // In these tests, the id_pat is already stored in the previous read to `my_id`, so we don't
-    // use that for the next reading...
-
     #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
     pub struct DekuTest {
         my_id: u8,
@@ -127,6 +134,11 @@ fn test_id_pat_with_id() {
     pub enum MyEnum {
         #[deku(id_pat = "1..=2")]
         VariantA(u8),
+
+        // Id type can differ from id_type
+        #[deku(id_pat = "4..=5")]
+        VariantDiffType(u16),
+
         #[deku(id_pat = "_")]
         VariantB,
     }
@@ -143,15 +155,75 @@ fn test_id_pat_with_id() {
     );
     assert_eq!(input, &*v.to_bytes().unwrap());
 
-    let input = [0x05];
+    let input = [0x04, 0x02, 0xff];
+    let mut cursor = Cursor::new(input);
+    let (_, v) = DekuTest::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(
+        v,
+        DekuTest {
+            my_id: 0x04,
+            enum_from_id: MyEnum::VariantDiffType(0xff02)
+        }
+    );
+    assert_eq!(input, &*v.to_bytes().unwrap());
+
+    let input = [0x06];
     let mut cursor = Cursor::new(input);
     let (_, v) = DekuTest::from_reader((&mut cursor, 0)).unwrap();
     assert_eq!(
         v,
         DekuTest {
-            my_id: 0x05,
+            my_id: 0x06,
             enum_from_id: MyEnum::VariantB
         }
     );
     assert_eq!(input, &*v.to_bytes().unwrap());
+
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    #[deku(id_type = "u8", bits = "2")]
+    pub enum IdPatBits {
+        #[deku(id = 1)]
+        A(#[deku(bits = 6)] u8),
+
+        #[deku(id_pat = "_")]
+        B(#[deku(bits = 8)] u8),
+    }
+
+    let input = [0b1100_1111];
+    let mut cursor = Cursor::new(input);
+    let (_, v) = IdPatBits::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(v, IdPatBits::B(0b1100_1111));
+    assert_eq!(input, &*v.to_bytes().unwrap());
+
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    #[deku(id_type = "u8", bits = "7")]
+    pub enum IdPatBitsBigger {
+        #[deku(id = 1)]
+        A(#[deku(bits = 9)] u16),
+
+        #[deku(id_pat = "_")]
+        B(u16),
+    }
+
+    let input = [0b0000_0000, 0b0000_0001];
+    let mut cursor = Cursor::new(input);
+    let (_, v) = IdPatBitsBigger::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(v, IdPatBitsBigger::B(0b1_0000_0000));
+    assert_eq!(input, &*v.to_bytes().unwrap());
+
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    #[deku(id_type = "u8", bits = "7")]
+    pub enum IdPatBitsTuple {
+        #[deku(id = 1)]
+        A(#[deku(bits = 9)] u16),
+
+        #[deku(id_pat = "_")]
+        B((u8, u8)),
+    }
+
+    let input = [0b0000_0000, 0b0000_0001];
+    let mut cursor = Cursor::new(input);
+    let (_, v) = IdPatBitsTuple::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(v, IdPatBitsTuple::B((0, 1)));
+    assert_eq!(input, &*v.to_bytes().unwrap());
 }