Fix DekuRead for #[repr(...)] enums constructed using id via ctx and whose variants assign discriminant values (sharksforarms#577)

* deku-derive: Derive Clone, Copy for ReprType

ReprType is a simple enum; copy and clone operations have the same cost
as forming pointers, so let's allow them to reduce some source noise.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

* deku-derive: Prune discriminant error paths with let-else

Reduce indentation and improve readability by eliminating the match
expression. Also, in my opinion, describing the constraints on the goal
in a linear fashion makes the code easier to reason about.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

* deku-derive: Drop redundant <T>::try_from() for discriminant access

The unsafe expression already dereferences the pointer cast to the
target type.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

* deku-derive: emit_enum(): Prefer repr over id_type for conversions

Use of the `id` attribute in enum contexts precludes use of
`id_type`, under which condition id_type is None. A None id_type
breaks the generated token stream at the point of the cast in the unsafe
expression accessing the discriminant value.

However, we must have a valid `repr` and that repr must match id_type
where it's present, by elimination of paths to the contrary in the
existing error handling. Emit the token for the repr type in place of
id_type for the purpose of the type conversion logic.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

---------

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Co-authored-by: Emmanuel Thompson <ethompson@fastly.com>

Author: amboar

deku-derive/src/lib.rs

@@ -184,7 +184,7 @@ struct DekuData {
     bit_order: Option<syn::LitStr>,
 }
 
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 enum ReprType {
     U8,
     U16,
@@ -198,6 +198,23 @@ enum ReprType {
     I128,
 }
 
+impl From<ReprType> for TokenStream {
+    fn from(value: ReprType) -> Self {
+        match value {
+            ReprType::U8 => quote! { u8 },
+            ReprType::U16 => quote! { u16 },
+            ReprType::U32 => quote! { u32 },
+            ReprType::U64 => quote! { u64 },
+            ReprType::U128 => quote! { u128 },
+            ReprType::I8 => quote! { i8 },
+            ReprType::I16 => quote! { i16 },
+            ReprType::I32 => quote! { i32 },
+            ReprType::I64 => quote! { i64 },
+            ReprType::I128 => quote! { i128 },
+        }
+    }
+}
+
 fn from_token(ts: TokenStream) -> Option<ReprType> {
     let mut repr_value = None;
 

deku-derive/src/macros/deku_read.rs

@@ -257,36 +257,32 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> {
                 variant_id_pat.clone()
             }
         } else if has_discriminant {
-            match &input.repr {
-                None => {
+            let Some(repr) = input.repr else {
+                return Err(syn::Error::new(
+                    variant.ident.span(),
+                    "DekuRead: `id_type` must be specified on non-unit variants",
+                ));
+            };
+            if let Some(id_type) = id_type {
+                let Some(id_type_repr) = from_token(id_type.clone()) else {
                     return Err(syn::Error::new(
                         variant.ident.span(),
-                        "DekuRead: `id_type` must be specified on non-unit variants",
+                        "DekuRead: `repr` must be specified on non-unit variants",
+                    ));
+                };
+                if id_type_repr != repr {
+                    return Err(syn::Error::new(
+                        variant.ident.span(),
+                        "DekuRead: `repr` must match `id_type`",
                     ));
-                }
-                Some(repr) => {
-                    if let Some(id_type) = id_type {
-                        if let Some(id_type_repr) = from_token(id_type.clone()) {
-                            if id_type_repr != *repr {
-                                return Err(syn::Error::new(
-                                    variant.ident.span(),
-                                    "DekuRead: `repr` must match `id_type`",
-                                ));
-                            }
-                        } else {
-                            return Err(syn::Error::new(
-                                variant.ident.span(),
-                                "DekuRead: `repr` must be specified on non-unit variants",
-                            ));
-                        }
-                    }
                 }
             }
+            let repr_type: TokenStream = repr.into();
             let ident = &variant.ident;
             let internal_ident = gen_internal_field_ident(&quote!(#ident));
             pre_match_tokens.push(quote! {
                 // https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.access-memory
-                let #internal_ident = <#id_type>::try_from(unsafe { *(&Self::#ident as *const Self as *const #id_type) })?;
+                let #internal_ident = unsafe { *(&Self::#ident as *const Self as *const #repr_type) };
             });
             quote! { _ if __deku_variant_id == #internal_ident }
         } else {

deku-derive/src/macros/deku_write.rs

@@ -267,7 +267,7 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> {
                 // if the variant has fields, the first must be storing the id
                 quote! {}
             } else if has_discriminant {
-                match &input.repr {
+                match input.repr {
                     None => {
                         return Err(syn::Error::new(
                             variant.ident.span(),
@@ -277,7 +277,7 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> {
                     Some(repr) => {
                         if let Some(id_type) = id_type {
                             if let Some(id_type_repr) = from_token(id_type.clone()) {
-                                if id_type_repr != *repr {
+                                if id_type_repr != repr {
                                     return Err(syn::Error::new(
                                         variant.ident.span(),
                                         "DekuWrite: `repr` must match `id_type`",

tests/test_enum.rs

@@ -362,3 +362,48 @@ fn test_variable_endian_enum(input: &[u8], expected: VariableEndian) {
     let ret_write: Vec<u8> = ret_read.try_into().unwrap();
     assert_eq!(input.to_vec(), ret_write);
 }
+
+#[test]
+fn test_repr_assignment_with_id_via_ctx() {
+    use deku::ctx::Endian;
+
+    #[derive(Debug, DekuRead, DekuWrite, Eq, PartialEq)]
+    #[deku(ctx = "endian: Endian, mid: u8", id = "mid", endian = "endian")]
+    #[repr(u8)]
+    enum Body {
+        First = 0x00,
+        #[deku(id = "0x01")]
+        Second(u8),
+    }
+
+    #[derive(Debug, DekuRead, DekuWrite, Eq, PartialEq)]
+    #[deku(endian = "little")]
+    struct Message {
+        id: u8,
+        header: u16,
+        #[deku(ctx = "*id")]
+        body: Body,
+    }
+
+    let input = [0u8, 1u8, 0u8];
+    let mut cursor = Cursor::new(input);
+    assert_eq!(
+        Message {
+            id: 0,
+            header: 1,
+            body: Body::First,
+        },
+        Message::from_reader((&mut cursor, 0)).unwrap().1
+    );
+
+    let input = [1u8, 2u8, 0u8, 3u8];
+    let mut cursor = Cursor::new(input);
+    assert_eq!(
+        Message {
+            id: 1,
+            header: 2,
+            body: Body::Second(3),
+        },
+        Message::from_reader((&mut cursor, 0)).unwrap().1
+    );
+}