Revert repr on structs, enums, and fields

This feature does not work and fundamentally cannot work because the write-side always receives references to values, but (1) there are no `From<&T>` conversions for any std type that anyone would use as a repr, and (2) this would cause a complete copy of the converted object to be retained in memory just to write the object. The `with` feature offers a more appropriate approach to custom serialisations that does not suffer from these limitations. This reverts: "Implement top-level `repr` attribute for structs and enums." commit 6f32003 "Fix nightly tests" commit 38b48d4 "Implement field-level `repr` for conversion." commit 8353699 ...with the exception of portions of those commits which fixed a bug where arbitrary errors from `try_map` in BinWrite were not allowed due to overly-restrictive type hints.
jam1garner · Nov 17, 2022 · 2111381 · 2111381
1 parent aed92e3
commit 2111381
Show file tree

Hide file tree

Showing 20 changed files with 43 additions and 236 deletions.
diff --git a/binrw/tests/derive/struct_map.rs b/binrw/tests/derive/struct_map.rs
@@ -33,86 +33,6 @@ fn map_expr() {
     assert_eq!(result.a, 2);
 }
 
-#[test]
-fn map_repr_enum() {
-    #[derive(BinRead, Debug, PartialEq)]
-    #[br(repr = u8)]
-    enum Test {
-        SubTest(u8),
-    }
-
-    impl From<u8> for Test {
-        fn from(u: u8) -> Self {
-            Self::SubTest(u)
-        }
-    }
-
-    let result = Test::read(&mut Cursor::new("\x01")).unwrap();
-    assert_eq!(result, Test::SubTest(1));
-}
-
-#[test]
-fn map_repr_enum_variant() {
-    #[derive(BinRead, Debug, PartialEq)]
-    enum Test {
-        SubTest(#[br(repr = u8)] SubTest),
-    }
-
-    #[derive(Debug, PartialEq)]
-    struct SubTest(u8);
-
-    impl From<u8> for SubTest {
-        fn from(u: u8) -> Self {
-            Self(u)
-        }
-    }
-
-    let result = Test::read_le(&mut Cursor::new("\x01")).unwrap();
-    assert_eq!(result, Test::SubTest(SubTest(1)));
-}
-
-#[test]
-fn map_repr_struct() {
-    #[derive(BinRead, Debug)]
-    #[br(repr = u8)]
-    struct Test {
-        a: u8,
-    }
-
-    impl From<u8> for Test {
-        fn from(a: u8) -> Self {
-            Self { a }
-        }
-    }
-
-    let result = Test::read(&mut Cursor::new("\x01")).unwrap();
-    assert_eq!(result.a, 1);
-}
-
-#[test]
-fn map_repr_struct_field() {
-    #[derive(BinRead, Debug)]
-    #[br(big)]
-    struct Test {
-        #[br(repr = u8)]
-        a: SubTest,
-    }
-
-    #[derive(Debug)]
-    struct SubTest {
-        a: u8,
-    }
-
-    impl From<u8> for SubTest {
-        fn from(a: u8) -> Self {
-            Self { a }
-        }
-    }
-
-    let result = Test::read(&mut Cursor::new("\x01")).unwrap();
-    assert_eq!(result.a.a, 1);
-}
-
 #[test]
 fn map_struct() {
     #[derive(BinRead, Debug)]

diff --git a/binrw/tests/derive/write/map.rs b/binrw/tests/derive/write/map.rs
@@ -39,78 +39,6 @@ fn map_field_code_coverage() {
     }
 }
 
-#[test]
-fn map_repr_enum() {
-    #[allow(dead_code)]
-    #[derive(BinWrite, Debug)]
-    #[bw(repr = u8)]
-    enum Test {
-        SubTest(u8),
-    }
-
-    impl From<&Test> for u8 {
-        fn from(t: &Test) -> Self {
-            match t {
-                Test::SubTest(u) => *u,
-            }
-        }
-    }
-}
-
-#[test]
-fn map_repr_enum_variant() {
-    #[allow(dead_code)]
-    #[derive(BinWrite, Debug)]
-    enum Test {
-        SubTest(#[bw(repr = u8)] SubTest),
-    }
-
-    #[derive(Debug)]
-    struct SubTest(u8);
-
-    impl From<&SubTest> for u8 {
-        fn from(s: &SubTest) -> Self {
-            s.0
-        }
-    }
-}
-
-#[test]
-fn map_repr_struct() {
-    #[derive(BinWrite, Debug)]
-    #[bw(repr = u8)]
-    struct Test {
-        a: u8,
-    }
-
-    impl From<&Test> for u8 {
-        fn from(t: &Test) -> Self {
-            t.a
-        }
-    }
-}
-
-#[test]
-fn map_repr_struct_field() {
-    #[derive(BinWrite, Debug)]
-    #[bw(big)]
-    struct Test {
-        #[bw(repr = u8)]
-        a: SubTest,
-    }
-
-    #[derive(Debug)]
-    struct SubTest {
-        a: u8,
-    }
-
-    impl From<&SubTest> for u8 {
-        fn from(s: &SubTest) -> Self {
-            s.a
-        }
-    }
-}
-
 #[test]
 fn try_map() {
     use binrw::prelude::*;

diff --git a/binrw/tests/ui/invalid_keyword_enum.stderr b/binrw/tests/ui/invalid_keyword_enum.stderr
@@ -1,4 +1,4 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`, `return_all_errors`, `return_unexpected_error`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`, `return_all_errors`, `return_unexpected_error`
  --> $DIR/invalid_keyword_enum.rs:4:6
   |
 4 | #[br(invalid_enum_keyword)]

diff --git a/binrw/tests/ui/invalid_keyword_enum_variant.stderr b/binrw/tests/ui/invalid_keyword_enum_variant.stderr
@@ -1,4 +1,4 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
  --> $DIR/invalid_keyword_enum_variant.rs:5:10
   |
 5 |     #[br(invalid_enum_variant_keyword)]

diff --git a/binrw/tests/ui/invalid_keyword_struct.stderr b/binrw/tests/ui/invalid_keyword_struct.stderr
@@ -1,4 +1,4 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
  --> $DIR/invalid_keyword_struct.rs:4:6
   |
 4 | #[br(invalid_struct_keyword)]

diff --git a/binrw/tests/ui/invalid_keyword_struct_field.stderr b/binrw/tests/ui/invalid_keyword_struct_field.stderr
@@ -1,4 +1,4 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `args`, `args_raw`, `calc`, `default`, `ignore`, `with`, `parse_with`, `count`, `offset`, `offset_after`, `if`, `deref_now`, `postprocess_now`, `restore_position`, `try`, `temp`, `assert`, `err_context`, `pad_before`, `pad_after`, `align_before`, `align_after`, `seek_before`, `pad_size_to`, `dbg`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `args`, `args_raw`, `calc`, `default`, `ignore`, `with`, `parse_with`, `count`, `offset`, `offset_after`, `if`, `deref_now`, `postprocess_now`, `restore_position`, `try`, `temp`, `assert`, `err_context`, `pad_before`, `pad_after`, `align_before`, `align_after`, `seek_before`, `pad_size_to`, `dbg`
  --> tests/ui/invalid_keyword_struct_field.rs:5:10
   |
 5 |     #[br(invalid_struct_field_keyword)]

diff --git a/binrw/tests/ui/invalid_keyword_unit_enum.stderr b/binrw/tests/ui/invalid_keyword_unit_enum.stderr
@@ -1,4 +1,4 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `import`, `import_raw`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `import`, `import_raw`, `repr`
  --> tests/ui/invalid_keyword_unit_enum.rs:4:6
   |
 4 | #[br(invalid_unit_enum_keyword)]

diff --git a/binrw/tests/ui/invalid_keyword_with_imports.stderr b/binrw/tests/ui/invalid_keyword_with_imports.stderr
@@ -1,4 +1,4 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
  --> $DIR/invalid_keyword_with_imports.rs:5:6
   |
 5 | #[br(invalid_struct_keyword)]

diff --git a/binrw/tests/ui/non_blocking_errors.stderr b/binrw/tests/ui/non_blocking_errors.stderr
@@ -1,16 +1,16 @@
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `import`, `import_raw`, `assert`, `pre_assert`
  --> tests/ui/non_blocking_errors.rs:6:6
   |
 6 | #[br(invalid_keyword_struct)]
   |      ^^^^^^^^^^^^^^^^^^^^^^
 
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `args`, `args_raw`, `calc`, `default`, `ignore`, `with`, `parse_with`, `count`, `offset`, `offset_after`, `if`, `deref_now`, `postprocess_now`, `restore_position`, `try`, `temp`, `assert`, `err_context`, `pad_before`, `pad_after`, `align_before`, `align_after`, `seek_before`, `pad_size_to`, `dbg`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `args`, `args_raw`, `calc`, `default`, `ignore`, `with`, `parse_with`, `count`, `offset`, `offset_after`, `if`, `deref_now`, `postprocess_now`, `restore_position`, `try`, `temp`, `assert`, `err_context`, `pad_before`, `pad_after`, `align_before`, `align_after`, `seek_before`, `pad_size_to`, `dbg`
  --> tests/ui/non_blocking_errors.rs:8:10
   |
 8 |     #[br(invalid_keyword_struct_field_a)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `magic`, `args`, `args_raw`, `calc`, `default`, `ignore`, `with`, `parse_with`, `count`, `offset`, `offset_after`, `if`, `deref_now`, `postprocess_now`, `restore_position`, `try`, `temp`, `assert`, `err_context`, `pad_before`, `pad_after`, `align_before`, `align_after`, `seek_before`, `pad_size_to`, `dbg`
+error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `magic`, `args`, `args_raw`, `calc`, `default`, `ignore`, `with`, `parse_with`, `count`, `offset`, `offset_after`, `if`, `deref_now`, `postprocess_now`, `restore_position`, `try`, `temp`, `assert`, `err_context`, `pad_before`, `pad_after`, `align_before`, `align_after`, `seek_before`, `pad_size_to`, `dbg`
   --> tests/ui/non_blocking_errors.rs:10:10
    |
 10 |     #[br(invalid_keyword_struct_field_b)]

diff --git a/binrw/tests/ui/repr_magic_conflict.stderr b/binrw/tests/ui/repr_magic_conflict.stderr
@@ -1,8 +1,8 @@
 error: `repr` and `magic` are mutually exclusive
- --> tests/ui/repr_magic_conflict.rs:4:13
+ --> tests/ui/repr_magic_conflict.rs:4:6
   |
 4 |   #[br(repr = u8)]
-  |  _____________^
+  |  ______^
 5 | | enum Foo {
 6 | |     #[br(magic = 0u8)] A,
   | |______________^
diff --git a/binrw_derive/src/binrw/codegen/meta.rs b/binrw_derive/src/binrw/codegen/meta.rs
@@ -1,5 +1,5 @@
 use super::sanitization::{META_ENDIAN_KIND, READ_ENDIAN, READ_MAGIC, WRITE_ENDIAN, WRITE_MAGIC};
-use crate::binrw::parser::{CondEndian, Input, Map};
+use crate::binrw::parser::{CondEndian, Input};
 use proc_macro2::TokenStream;
 use quote::quote;
 
@@ -25,19 +25,11 @@ pub(crate) fn generate<const WRITE: bool>(
     let endian_meta = if WRITE { WRITE_ENDIAN } else { READ_ENDIAN };
 
     let endian = match input.endian() {
-        CondEndian::Inherited => match input.map() {
-            Map::None => input.is_empty().then(|| {
-                quote! {
-                    #META_ENDIAN_KIND::None
-                }
-            }),
-            Map::Map(_) | Map::Try(_) => Some(quote! {
+        CondEndian::Inherited => input.is_endian_agnostic().then(|| {
+            quote! {
                 #META_ENDIAN_KIND::None
-            }),
-            Map::Repr(repr) => ["i8", "u8"].contains(&repr.to_string().as_str()).then(|| {
-                quote! { <(#repr) as #endian_meta>::ENDIAN }
-            }),
-        },
+            }
+        }),
         CondEndian::Fixed(endian) => Some(quote! {
             #META_ENDIAN_KIND::Endian(#endian)
         }),

diff --git a/binrw_derive/src/binrw/codegen/read_options.rs b/binrw_derive/src/binrw/codegen/read_options.rs
@@ -32,14 +32,6 @@ pub(crate) fn generate(input: &Input, derive_input: &syn::DeriveInput) -> TokenS
         },
         Map::Try(map) => map::generate_try_map(input, name, map),
         Map::Map(map) => map::generate_map(input, name, map),
-        Map::Repr(ty) => match input {
-            Input::UnitOnlyEnum(e) => generate_unit_enum(input, name, e),
-            _ => map::generate_try_map(
-                input,
-                name,
-                &quote! { <#ty as core::convert::TryInto<_>>::try_into },
-            ),
-        },
     };
 
     quote! {

diff --git a/binrw_derive/src/binrw/codegen/read_options/enum.rs b/binrw_derive/src/binrw/codegen/read_options/enum.rs
@@ -27,7 +27,7 @@ pub(super) fn generate_unit_enum(
         .add_magic_pre_assertion()
         .finish();
 
-    let read = match en.map.as_repr() {
+    let read = match &en.repr {
         Some(repr) => generate_unit_enum_repr(repr, &en.fields),
         None => generate_unit_enum_magic(&en.fields),
     };

diff --git a/binrw_derive/src/binrw/codegen/read_options/struct.rs b/binrw_derive/src/binrw/codegen/read_options/struct.rs
@@ -317,7 +317,7 @@ impl<'field> FieldGenerator<'field> {
                 let value = self.out;
                 quote! { #map_func(#value) }
             }
-            Map::Try(_) | Map::Repr(_) => {
+            Map::Try(_) => {
                 // TODO: Position should always just be saved once for a field if used
                 let value = self.out;
                 let map_err = super::get_map_err(SAVED_POSITION);
@@ -343,15 +343,7 @@ impl<'field> FieldGenerator<'field> {
                     let #map_func = (#COERCE_FN::<#ty, _, _>(#map));
                 }
             }
-            Map::Try(try_map) | Map::Repr(try_map) => {
-                let try_map = if matches!(self.field.map, Map::Repr(_)) {
-                    quote! {
-                        <#try_map as core::convert::TryInto<_>>::try_into
-                    }
-                } else {
-                    try_map.clone()
-                };
-
+            Map::Try(try_map) => {
                 // TODO: Position should always just be saved once for a field if used
                 quote! {
                     let #map_func = (#COERCE_FN::<::core::result::Result<#ty, _>, _, _>(#try_map));
@@ -411,7 +403,7 @@ impl<'field> FieldGenerator<'field> {
                 }
             } else {
                 match &self.field.map {
-                    Map::Map(_) | Map::Try(_) | Map::Repr(_) => {
+                    Map::Map(_) | Map::Try(_) => {
                         quote_spanned! {ty.span()=>
                             let #args_var = #MAP_ARGS_TYPE_HINT(&#map_func, #args);
                         }

diff --git a/binrw_derive/src/binrw/codegen/write_options.rs b/binrw_derive/src/binrw/codegen/write_options.rs
@@ -25,10 +25,6 @@ pub(crate) fn generate(input: &Input, derive_input: &syn::DeriveInput) -> TokenS
             Input::UnitOnlyEnum(e) => generate_unit_enum(input, name, e),
         },
         Map::Try(map) | Map::Map(map) => generate_map(input, name, map),
-        Map::Repr(map) => match input {
-            Input::UnitOnlyEnum(e) => generate_unit_enum(input, name, e),
-            _ => generate_map(input, name, map),
-        },
     };
 
     quote! {
@@ -44,11 +40,6 @@ fn generate_map(input: &Input, name: Option<&Ident>, map: &TokenStream) -> Token
         let map_err = get_map_err(POS);
         quote! { #map_err? }
     });
-    let map = if matches!(input.map(), Map::Repr(_)) {
-        quote! { <#map as core::convert::TryFrom<_>>::try_from }
-    } else {
-        map.clone()
-    };
     let write_data = quote! {
         #WRITE_METHOD(
             &((#map)(self) #map_try),

diff --git a/binrw_derive/src/binrw/codegen/write_options/enum.rs b/binrw_derive/src/binrw/codegen/write_options/enum.rs
@@ -11,7 +11,7 @@ pub(crate) fn generate_unit_enum(
     name: Option<&Ident>,
     en: &UnitOnlyEnum,
 ) -> TokenStream {
-    let write = match en.map.as_repr() {
+    let write = match &en.repr {
         Some(repr) => generate_unit_enum_repr(repr, &en.fields),
         None => generate_unit_enum_magic(&en.fields),
     };

diff --git a/binrw_derive/src/binrw/codegen/write_options/struct_field.rs b/binrw_derive/src/binrw/codegen/write_options/struct_field.rs
@@ -224,7 +224,7 @@ impl<'a> StructFieldGenerator<'a> {
                     let #args = #WRITE_MAP_ARGS_TYPE_HINT(&#map_fn, #args_val);
                     #out
                 },
-                Map::Try(_) | Map::Repr(_) => quote! {
+                Map::Try(_) => quote! {
                     let #args = #WRITE_TRY_MAP_ARGS_TYPE_HINT(&#map_fn, #args_val);
                     #out
                 },
@@ -287,7 +287,6 @@ fn args_ident(ident: &Ident) -> Ident {
 fn field_mapping(map: &Map) -> Option<TokenStream> {
     match map {
         Map::Try(map_fn) | Map::Map(map_fn) => Some(quote! { (#map_fn) }),
-        Map::Repr(ty) => Some(quote! { (<#ty as core::convert::TryFrom<_>>::try_from) }),
         Map::None => None,
     }
 }

diff --git a/binrw_derive/src/binrw/parser/field_level_attrs.rs b/binrw_derive/src/binrw/parser/field_level_attrs.rs
@@ -18,7 +18,7 @@ attr_struct! {
         pub(crate) field: syn::Field,
         #[from(RW:Big, RW:Little, RW:IsBig, RW:IsLittle)]
         pub(crate) endian: CondEndian,
-        #[from(RW:Map, RW:TryMap, RW:Repr)]
+        #[from(RW:Map, RW:TryMap)]
         pub(crate) map: Map,
         #[from(RW:Magic)]
         pub(crate) magic: Magic,