Fix id write endianness and sub-width signed sign-extension

deku-derive/src/macros/deku_write.rs

@@ -616,7 +616,9 @@ fn emit_field_write(
         // TODO: This should error on some attributes that don't make sense aren't used?
         // Such as magic, seek*
         let crate_ = super::get_crate_name();
-        let field_endian = input.id_endian.as_ref();
+        // Mirror gen_id_args' read-side precedence (id_endian -> enum endian) so a
+        // hardcoded `endian="big"` or a bound `endian="endian"` reaches the write.
+        let field_endian = input.id_endian.as_ref().or(input.endian.as_ref());
         #[cfg(feature = "bits")]
         let field_bits = input.bits.as_ref();
         #[cfg(not(feature = "bits"))]

src/impls/primitive.rs

@@ -653,7 +653,7 @@ macro_rules! ImplDekuReadSignExtend {
                 let ret = reader.read_bytes(size.0, &mut buf, order)?;
                 let a = match ret {
                     ReaderRet::Bytes => {
-                        if endian.is_le() {
+                        let value = if endian.is_le() {
                             <$typ>::from_le_bytes(buf.try_into().unwrap())
                         } else {
                             if size.0 != core::mem::size_of::<$typ>() {
@@ -662,6 +662,17 @@ macro_rules! ImplDekuReadSignExtend {
                                 buf[..padding].fill(0x00);
                             }
                             <$typ>::from_be_bytes(buf.try_into().unwrap())
+                        };
+                        // Sign-extend: a sub-width signed read zero-pads above, so the
+                        // sign bit of the N-byte value must be propagated to the full type.
+                        // A zero-byte read has no sign bit to propagate, and shifting by
+                        // the full type width would overflow, so it is left as-is.
+                        const MAX_TYPE_BITS: usize = BitSize::of::<$typ>().0;
+                        if size.0 == 0 {
+                            value
+                        } else {
+                            let shift = MAX_TYPE_BITS - (size.0 * 8);
+                            (value << shift) >> shift
                         }
                     }
                     #[cfg(all(feature = "bits", feature = "alloc"))]
@@ -1712,4 +1723,82 @@ mod tests {
     TestSignExtendingPanic!(test_sign_extend_i64_panic, i64, 64);
     #[cfg(feature = "bits")]
     TestSignExtendingPanic!(test_sign_extend_i128_panic, i128, 128);
+
+    // Regression: sub-width byte-aligned signed reads must sign-extend (ByteSize path).
+    #[cfg(feature = "bits")]
+    #[test]
+    fn test_sign_extend_bytesize_i32_negative() {
+        // 3-byte LE encoding of -100
+        let mut cursor = std::io::Cursor::new([0x9C_u8, 0xFF, 0xFF]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            -100_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(3))).unwrap()
+        );
+
+        // 3-byte BE encoding of -100
+        let mut cursor = std::io::Cursor::new([0xFF_u8, 0xFF, 0x9C]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            -100_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(3))).unwrap()
+        );
+    }
+
+    #[cfg(feature = "bits")]
+    #[test]
+    fn test_sign_extend_bytesize_i32_min() {
+        // 3-byte LE encoding of -8388608 (0x800000 sign-extended)
+        let mut cursor = std::io::Cursor::new([0x00_u8, 0x00, 0x80]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            -8388608_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(3))).unwrap()
+        );
+
+        // 3-byte BE encoding of -8388608
+        let mut cursor = std::io::Cursor::new([0x80_u8, 0x00, 0x00]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            -8388608_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(3))).unwrap()
+        );
+    }
+
+    // A runtime-computed `bytes` of 0 must keep reading nothing and yield 0,
+    // not overflow the sign-extension shift.
+    #[test]
+    fn test_sign_extend_bytesize_zero() {
+        let mut cursor = std::io::Cursor::new([0x01_u8, 0x02]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            0_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(0))).unwrap()
+        );
+
+        let mut cursor = std::io::Cursor::new([0x01_u8, 0x02]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            0_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(0))).unwrap()
+        );
+    }
+
+    // Full-width reads take shift == 0 and must round-trip unchanged.
+    #[test]
+    fn test_sign_extend_bytesize_full_width() {
+        let mut cursor = std::io::Cursor::new([0x9C_u8, 0xFF, 0xFF, 0xFF]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            -100_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(4))).unwrap()
+        );
+
+        let mut cursor = std::io::Cursor::new([0xFF_u8, 0xFF, 0xFF, 0x9C]);
+        let mut reader = Reader::new(&mut cursor);
+        assert_eq!(
+            -100_i32,
+            i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(4))).unwrap()
+        );
+    }
 }

tests/test_enum.rs

@@ -440,3 +440,83 @@ fn test_repr_assignment_with_id_via_ctx() {
         Message::from_reader((&mut cursor, 0)).unwrap().1
     );
 }
+
+/// Regression: the id stored in an id_pat variant must be written with the enum's declared
+/// endian. Before the fix it was always written little-endian regardless of endian="big".
+#[test]
+fn test_id_pat_big_endian_write() {
+    // The id_pat variant is what triggers the previously-buggy write path.
+    // A fixed-id variant (like Known) uses a different path that was already correct.
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    #[deku(id_type = "u16", endian = "big")]
+    enum E {
+        #[deku(id = "65535")]
+        Known,
+        #[deku(id_pat = "0..=65534")]
+        Other(u16),
+    }
+
+    // id_pat variant: 0x0102 big-endian → [0x01, 0x02]; was [0x02, 0x01] before the fix
+    assert_eq!(E::Other(0x0102).to_bytes().unwrap(), [0x01, 0x02]);
+
+    // round-trip
+    let mut cursor = Cursor::new([0x01_u8, 0x02]);
+    let (_, decoded) = E::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(decoded, E::Other(0x0102));
+}
+
+/// Regression: signed 24-bit id_pat enum must round-trip negative values on little-endian.
+/// A fixed negative id (-8388608) and a negative range value (-100) must both
+/// encode and decode correctly.
+#[test]
+#[cfg(feature = "bits")]
+fn test_id_pat_signed_i24_negative_roundtrip_le() {
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    #[deku(id_type = "i32", bits = "24", endian = "little")]
+    enum Signed24LE {
+        #[deku(id = "-8388608")]
+        Fixed,
+        #[deku(id_pat = "-8388607..=8388607")]
+        Other(i32),
+    }
+
+    let cases = [
+        Signed24LE::Fixed,
+        Signed24LE::Other(-100),
+        Signed24LE::Other(100),
+    ];
+    for val in &cases {
+        let bytes = val.to_bytes().unwrap();
+        assert_eq!(bytes.len(), 3, "24-bit enum should encode to 3 bytes");
+        let mut cursor = Cursor::new(&bytes[..]);
+        let (_, decoded) = Signed24LE::from_reader((&mut cursor, 0)).unwrap();
+        assert_eq!(val, &decoded, "LE round-trip failed for {val:?}");
+    }
+}
+
+/// Same as above but big-endian — exercises both endian threading and sign-extension.
+#[test]
+#[cfg(feature = "bits")]
+fn test_id_pat_signed_i24_negative_roundtrip_be() {
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    #[deku(id_type = "i32", bits = "24", endian = "big")]
+    enum Signed24BE {
+        #[deku(id = "-8388608")]
+        Fixed,
+        #[deku(id_pat = "-8388607..=8388607")]
+        Other(i32),
+    }
+
+    let cases = [
+        Signed24BE::Fixed,
+        Signed24BE::Other(-100),
+        Signed24BE::Other(100),
+    ];
+    for val in &cases {
+        let bytes = val.to_bytes().unwrap();
+        assert_eq!(bytes.len(), 3, "24-bit enum should encode to 3 bytes");
+        let mut cursor = Cursor::new(&bytes[..]);
+        let (_, decoded) = Signed24BE::from_reader((&mut cursor, 0)).unwrap();
+        assert_eq!(val, &decoded, "BE round-trip failed for {val:?}");
+    }
+}

tests/test_regression.rs

@@ -451,3 +451,29 @@ fn issue_525() {
     assert_eq!(test.0, (ret, 0));
     assert_eq!(test.1, TestSeek { a: 0, b: 2, c: 3 });
 }
+
+// A `bytes` expression evaluating to 0 at runtime must read nothing and yield 0,
+// rather than overflowing the sign-extension shift.
+//
+// https://github.com/sharksforarms/deku/pull/664#issuecomment-5162310570
+#[test]
+fn pr_664_runtime_zero_bytes() {
+    #[derive(Debug, PartialEq, DekuRead)]
+    struct Dynamic {
+        n: u8,
+        #[deku(bytes = "*n as usize")]
+        a: i32,
+    }
+
+    // n = 0: no bytes are read for `a`, the trailing bytes are left untouched
+    let mut cursor = std::io::Cursor::new([0x00_u8, 0x01, 0x02]);
+    let (amt_read, val) = Dynamic::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(8, amt_read);
+    assert_eq!(Dynamic { n: 0, a: 0 }, val);
+
+    // n = 1: the single byte read is still sign-extended
+    let mut cursor = std::io::Cursor::new([0x01_u8, 0x9C]);
+    let (amt_read, val) = Dynamic::from_reader((&mut cursor, 0)).unwrap();
+    assert_eq!(16, amt_read);
+    assert_eq!(Dynamic { n: 1, a: -100 }, val);
+}