From 065fcf0c5e148f1e728d8830ebf605bb0846c4d9 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Thu, 29 Jun 2023 03:50:03 -0500 Subject: [PATCH 1/3] Remove `BinRead::after_parse` This trait method exists pretty much exclusively to support deferred reads of `FilePtr` values but causes ongoing trouble elsewhere and the benefits do not seem to outweight the problems: 1. It requires `T::Args` to be cloneable in many cases where it should not be the case, which causes confusion, has a strong chance of causing accidental slowness, and makes it unnecessarily hard to move from imports; 2. An analysis I ran of binrw users on GitHub showed that pretty much all cases of `FilePtr` were using `FilePtr::parse` or `deref_now`, so any potential performance benefit does not seem to be realised by real-world projects; 3. Because there is no hard requirement to call `after_parse` and it mostly will not break anything, it is too easy for users to write custom implementations that do not do this and so are subtly broken. From the same GH analysis, there was only one case where I found someone who wrote a custom implementation that correctly called `after_parse`; 4. Since `after_parse` does not build a stack of objects to revisit later, its ability to avoid non-linear reads of data is limited to at most one struct or enum at a time anyway. Given these things (and probably others that I forget), IMO the existence of this feature is not justified. Instead, I think that a design that reads offsets into a `Vec<{integer}>` and then iterates over them later to convert into `Vec` is preferable; a subsequent patch includes some helper functions to do this, but also right now it can be done (with some verbosity) using the `args_iter` helper. Closes jam1garner/binrw#17, jam1garner/binrw#119. Fixes jam1garner/binrw#185, jam1garner/binrw#197. --- binrw/doc/attribute.md | 70 +------- binrw/src/binread/impls.rs | 60 ------- binrw/src/binread/mod.rs | 41 +---- binrw/src/file_ptr.rs | 170 ++++-------------- binrw/src/helpers.rs | 20 +-- binrw/src/pos_value.rs | 9 - binrw/tests/after_parse_test.rs | 29 --- binrw/tests/derive/struct.rs | 84 +++++++-- .../ui/deref_now_offset_after_conflict.rs | 9 - .../ui/deref_now_offset_after_conflict.stderr | 5 - .../ui/invalid_keyword_struct_field.stderr | 2 +- binrw/tests/ui/invalid_offset_type.rs | 3 - binrw/tests/ui/invalid_offset_type.stderr | 19 -- binrw/tests/ui/non_blocking_errors.stderr | 4 +- .../binrw/backtrace/syntax_highlighting.rs | 10 +- .../src/binrw/codegen/read_options/struct.rs | 155 +--------------- .../src/binrw/codegen/sanitization.rs | 1 - binrw_derive/src/binrw/parser/attrs.rs | 3 - .../src/binrw/parser/field_level_attrs.rs | 37 +--- binrw_derive/src/binrw/parser/keywords.rs | 3 - binrw_derive/src/binrw/parser/mod.rs | 7 - 21 files changed, 134 insertions(+), 607 deletions(-) delete mode 100644 binrw/tests/after_parse_test.rs delete mode 100644 binrw/tests/ui/deref_now_offset_after_conflict.rs delete mode 100644 binrw/tests/ui/deref_now_offset_after_conflict.stderr diff --git a/binrw/doc/attribute.md b/binrw/doc/attribute.md index 7b7641ef..464f5d21 100644 --- a/binrw/doc/attribute.md +++ b/binrw/doc/attribute.md @@ -87,7 +87,6 @@ Glossary of directives in binrw attributes (`#[br]`, `#[bw]`, `#[brw]`). | r | [`count`](#count) | field | Sets the length of a vector. | r | [`dbg`](#debug) | field | Prints the value and offset of a field to `stderr`. | r | [`default`](#ignore) | field | An alias for `ignore`. -| r | [`deref_now`](#postprocessing) | field | An alias for `postprocess_now`. | r | [`err_context`](#backtrace) | field | Adds additional context to errors. | rw | [`if`](#conditional-values) | field | Reads or writesReadsWrites data only if a condition is true. | rw | [`ignore`](#ignore) | field | For `BinRead`, uses the [`default`](core::default::Default) value for a field instead of reading data. For `BinWrite`, skips writing the field.Uses the [`default`](core::default::Default) value for a field instead of reading data.Skips writing the field. @@ -100,12 +99,10 @@ Glossary of directives in binrw attributes (`#[br]`, `#[bw]`, `#[brw]`). | rw | [`map`](#map) | all except unit variant | Maps an object or value to a new value. | rw | [`map_stream`](#stream-access-and-manipulation) | all except unit variant | Maps the readwrite stream to a new stream. | r | [`offset`](#offset) | field | Modifies the offset used by a [`FilePtr`](crate::FilePtr) while parsing. -| r | [`offset_after`](#offset) | field | Modifies the offset used by a [`FilePtr`](crate::FilePtr) after parsing. | rw | [`pad_after`](#padding-and-alignment) | field | Skips N bytes after readingwriting a field. | rw | [`pad_before`](#padding-and-alignment) | field | Skips N bytes before readingwriting a field. | rw | [`pad_size_to`](#padding-and-alignment) | field | Ensures the readerwriter is always advanced at least N bytes. | r | [`parse_with`](#custom-parserswriters) | field | Specifies a custom function for reading a field. -| r | [`postprocess_now`](#postprocessing) | field | Calls [`after_parse`](crate::BinRead::after_parse) immediately after reading data instead of after all fields have been read. | r | [`pre_assert`](#pre-assert) | struct, non-unit enum, unit variant | Like `assert`, but checks the condition before parsing. | rw | [`repr`](#repr) | unit-like enum | Specifies the underlying type for a unit-like (C-style) enum. | rw | [`restore_position`](#restore-position) | field | Restores the reader’swriter’s position after readingwriting a field. @@ -1760,13 +1757,11 @@ reset to where it was before parsing started. # Offset -The `offset` and `offset_after` directives are shorthands for passing -`offset` and `offset_after` arguments to a parser that operates like -[`FilePtr`](crate::FilePtr): +The `offset` directive is shorthand for passing `offset` to a parser that +operates like [`FilePtr`](crate::FilePtr): ```text #[br(offset = $offset:expr)] or #[br(offset($offset:expr))] -#[br(offset_after = $offset:expr)] or #[br(offset_after($offset:expr))] ``` When manually implementing @@ -1774,12 +1769,9 @@ When manually implementing [custom parser function](#custom-parserswriters), the offset is accessible from a named argument named `offset`. -For `offset`, any earlier field or [import](#arguments) can be referenced by -the expression in the directive. - -For `offset_after`, *all* fields and imports can be referenced by the -expression in the directive, but [`deref_now`](#postprocessing) cannot be -used. +Any (earlier only, when reading)earlier +field or [import](#arguments) can be +referenced by the expression in the directive. ## Examples @@ -1988,58 +1980,6 @@ started.
-# Postprocessing - -The `deref_now` directive, and its alias `postprocess_now`, cause a -field’s [`after_parse`](crate::BinRead::after_parse) function to be called -immediately after the field is parsed, instead of deferring the call until -the entire parent object has been parsed: - -```text -#[br(deref_now)] or #[br(postprocess_now)] -``` - -The [`BinRead::after_parse`](crate::BinRead::after_parse) function is -normally used to perform additional work after the whole parent object has -been parsed. For example, the [`FilePtr`](crate::FilePtr) type reads an -object offset during parsing with -[`read_options`](crate::BinRead::read_options), then actually seeks to and -parses the pointed-to object in `after_parse`. This improves read -performance by reading the whole parent object sequentially before seeking -to read the pointed-to object. - -However, if another field in the parent object needs to access data from the -pointed-to object, `after_parse` needs to be called earlier. Adding -`deref_now` (or its alias, `postprocess_now`) to the earlier field causes -this to happen. - -## Examples - -``` -# use binrw::{prelude::*, FilePtr32, NullString, io::Cursor}; -#[derive(BinRead, Debug)] -#[br(big, magic = b"TEST")] -struct TestFile { - #[br(deref_now)] - ptr: FilePtr32, - - value: i32, - - // Notice how `ptr` can be used as it has already been postprocessed - #[br(calc = ptr.len())] - ptr_len: usize, -} - -# let test_contents = b"\x54\x45\x53\x54\x00\x00\x00\x10\xFF\xFF\xFF\xFF\x00\x00\x00\x00\x54\x65\x73\x74\x20\x73\x74\x72\x69\x6E\x67\x00\x00\x00\x00\x69"; -# let test = Cursor::new(test_contents).read_be::().unwrap(); -# assert_eq!(test.ptr_len, 11); -# assert_eq!(test.value, -1); -# assert_eq!(test.ptr.to_string(), "Test string"); -``` -
- -
- # Pre-assert `pre_assert` works like [`assert`](#assert), but checks the condition before diff --git a/binrw/src/binread/impls.rs b/binrw/src/binread/impls.rs index 54a88ae3..82080002 100644 --- a/binrw/src/binread/impls.rs +++ b/binrw/src/binread/impls.rs @@ -147,22 +147,6 @@ where ) -> BinResult { crate::helpers::count_with(args.count, B::read_options)(reader, endian, args.inner) } - - fn after_parse( - &mut self, - reader: &mut R, - endian: Endian, - args: Self::Args<'_>, - ) -> BinResult<()> - where - R: Read + Seek, - { - for val in self.iter_mut() { - val.after_parse(reader, endian, args.inner.clone())?; - } - - Ok(()) - } } impl BinRead for [B; N] @@ -179,22 +163,6 @@ where ) -> BinResult { array_init::try_array_init(|_| BinRead::read_options(reader, endian, args.clone())) } - - fn after_parse( - &mut self, - reader: &mut R, - endian: Endian, - args: Self::Args<'_>, - ) -> BinResult<()> - where - R: Read + Seek, - { - for val in self.iter_mut() { - val.after_parse(reader, endian, args.clone())?; - } - - Ok(()) - } } macro_rules! binread_tuple_impl { @@ -211,19 +179,6 @@ macro_rules! binread_tuple_impl { ),* )) } - - fn after_parse(&mut self, reader: &mut R, endian: Endian, args: Self::Args<'_>) -> BinResult<()> { - let ($type1, $( - $types - ),*) = self; - - $type1.after_parse(reader, endian, args.clone())?; - $( - $types.after_parse(reader, endian, args.clone())?; - )* - - Ok(()) - } } binread_tuple_impl!($($types),*); @@ -267,21 +222,6 @@ impl BinRead for Option { ) -> BinResult { Ok(Some(T::read_options(reader, endian, args)?)) } - - fn after_parse( - &mut self, - reader: &mut R, - endian: Endian, - args: Self::Args<'_>, - ) -> BinResult<()> - where - R: Read + Seek, - { - match self { - Some(val) => val.after_parse(reader, endian, args), - None => Ok(()), - } - } } impl BinRead for core::marker::PhantomData { diff --git a/binrw/src/binread/mod.rs b/binrw/src/binread/mod.rs index 572063ad..f2270640 100644 --- a/binrw/src/binread/mod.rs +++ b/binrw/src/binread/mod.rs @@ -160,22 +160,6 @@ pub trait BinRead: Sized { endian: Endian, args: Self::Args<'_>, ) -> BinResult; - - /// Runs any post-processing steps required to finalize construction of the - /// object. - /// - /// # Errors - /// - /// If post-processing fails, an [`Error`](crate::Error) variant will be - /// returned. - fn after_parse( - &mut self, - _: &mut R, - _: Endian, - _: Self::Args<'_>, - ) -> BinResult<()> { - Ok(()) - } } /// Extension methods for reading [`BinRead`] objects directly from a reader. @@ -202,7 +186,7 @@ pub trait BinReaderExt: Read + Seek + Sized { fn read_type<'a, T>(&mut self, endian: Endian) -> BinResult where T: BinRead, - T::Args<'a>: Required + Clone, + T::Args<'a>: Required, { self.read_type_args(endian, T::Args::args()) } @@ -216,7 +200,7 @@ pub trait BinReaderExt: Read + Seek + Sized { fn read_be<'a, T>(&mut self) -> BinResult where T: BinRead, - T::Args<'a>: Required + Clone, + T::Args<'a>: Required, { self.read_type(Endian::Big) } @@ -230,7 +214,7 @@ pub trait BinReaderExt: Read + Seek + Sized { fn read_le<'a, T>(&mut self) -> BinResult where T: BinRead, - T::Args<'a>: Required + Clone, + T::Args<'a>: Required, { self.read_type(Endian::Little) } @@ -244,7 +228,7 @@ pub trait BinReaderExt: Read + Seek + Sized { fn read_ne<'a, T>(&mut self) -> BinResult where T: BinRead, - T::Args<'a>: Required + Clone, + T::Args<'a>: Required, { self.read_type(Endian::NATIVE) } @@ -254,15 +238,11 @@ pub trait BinReaderExt: Read + Seek + Sized { /// # Errors /// /// If reading fails, an [`Error`](crate::Error) variant will be returned. - fn read_type_args<'a, T>(&mut self, endian: Endian, args: T::Args<'a>) -> BinResult + fn read_type_args(&mut self, endian: Endian, args: T::Args<'_>) -> BinResult where T: BinRead, - T::Args<'a>: Clone, { - let mut res = T::read_options(self, endian, args.clone())?; - res.after_parse(self, endian, args)?; - - Ok(res) + T::read_options(self, endian, args) } /// Read `T` from the reader, assuming big-endian byte order, using the @@ -272,10 +252,9 @@ pub trait BinReaderExt: Read + Seek + Sized { /// /// If reading fails, an [`Error`](crate::Error) variant will be returned. #[inline] - fn read_be_args<'a, T>(&mut self, args: T::Args<'a>) -> BinResult + fn read_be_args(&mut self, args: T::Args<'_>) -> BinResult where T: BinRead, - T::Args<'a>: Clone, { self.read_type_args(Endian::Big, args) } @@ -287,10 +266,9 @@ pub trait BinReaderExt: Read + Seek + Sized { /// /// If reading fails, an [`Error`](crate::Error) variant will be returned. #[inline] - fn read_le_args<'a, T>(&mut self, args: T::Args<'a>) -> BinResult + fn read_le_args(&mut self, args: T::Args<'_>) -> BinResult where T: BinRead, - T::Args<'a>: Clone, { self.read_type_args(Endian::Little, args) } @@ -302,10 +280,9 @@ pub trait BinReaderExt: Read + Seek + Sized { /// /// If reading fails, an [`Error`](crate::Error) variant will be returned. #[inline] - fn read_ne_args<'a, T>(&mut self, args: T::Args<'a>) -> BinResult + fn read_ne_args(&mut self, args: T::Args<'_>) -> BinResult where T: BinRead, - T::Args<'a>: Clone, { self.read_type_args(Endian::NATIVE, args) } diff --git a/binrw/src/file_ptr.rs b/binrw/src/file_ptr.rs index ff8074b7..fa976d2a 100644 --- a/binrw/src/file_ptr.rs +++ b/binrw/src/file_ptr.rs @@ -39,14 +39,14 @@ pub type NonZeroFilePtr128 = FilePtr; /// /// `FilePtr` is composed of two types. The pointer type `P` is the /// absolute offset to a value within the data, and the value type `T` is -/// the actual pointed-to value. Once a `FilePtr` has been -/// [finalized](crate::BinRead::after_parse), [dereferencing] it will yield the +/// the actual pointed-to value. [Dereferencing] it will yield the /// pointed-to value. /// -/// When deriving `BinRead`, [offset](crate::docs::attribute#offset) directives -/// can be used to adjust the offset before the pointed-to value is read. +/// When deriving `BinRead`, the [offset](crate::docs::attribute#offset) +/// directive can be used to adjust the offset before the pointed-to value is +/// read. /// -/// [dereferencing]: core::ops::Deref +/// [Dereferencing]: core::ops::Deref /// /// # Examples /// @@ -69,54 +69,31 @@ pub type NonZeroFilePtr128 = FilePtr; /// [pointer] [value] /// 00000000: 0000 0008 0000 0000 ff ............ /// ``` +#[derive(Debug, Eq)] pub struct FilePtr { /// The raw offset to the value. pub ptr: Ptr, /// The pointed-to value. - pub value: Option, + pub value: T, } impl BinRead for FilePtr where Ptr: for<'a> BinRead = ()> + IntoSeekFrom, Value: BinRead, - for<'a> Value::Args<'a>: Clone, { type Args<'a> = FilePtrArgs>; /// Reads the offset of the value from the reader. - /// - /// The actual value will not be read until - /// [`after_parse()`](Self::after_parse) is called. fn read_options( - reader: &mut R, - endian: Endian, - _: Self::Args<'_>, - ) -> BinResult { - Ok(FilePtr { - ptr: Ptr::read_options(reader, endian, ())?, - value: None, - }) - } - - /// Finalizes the `FilePtr` by seeking to and reading the pointed-to value. - fn after_parse( - &mut self, reader: &mut R, endian: Endian, args: Self::Args<'_>, - ) -> BinResult<()> - where - R: Read + Seek, - { - self.after_parse_with_parser( - Value::read_options, - Value::after_parse, - reader, - endian, - args, - ) + ) -> BinResult { + let ptr = Ptr::read_options(reader, endian, ())?; + let value = Self::after_parse_with_parser(ptr, Value::read_options, reader, endian, args)?; + Ok(FilePtr { ptr, value }) } } @@ -124,59 +101,29 @@ impl FilePtr where Ptr: for<'a> BinRead = ()> + IntoSeekFrom, { - fn read_with_parser( - parser: Parser, - after_parse: AfterParse, - reader: &mut R, - endian: Endian, - args: FilePtrArgs, - ) -> BinResult - where - R: Read + Seek, - Args: Clone, - Parser: Fn(&mut R, Endian, Args) -> BinResult, - AfterParse: Fn(&mut Value, &mut R, Endian, Args) -> BinResult<()>, - { - let mut file_ptr = Self { - ptr: Ptr::read_options(reader, endian, ())?, - value: None, - }; - file_ptr.after_parse_with_parser(parser, after_parse, reader, endian, args)?; - Ok(file_ptr) - } - - fn after_parse_with_parser( - &mut self, + fn after_parse_with_parser( + ptr: Ptr, parser: Parser, - after_parse: AfterParse, reader: &mut R, endian: Endian, args: FilePtrArgs, - ) -> BinResult<()> + ) -> BinResult where R: Read + Seek, - Args: Clone, - Parser: Fn(&mut R, Endian, Args) -> BinResult, - AfterParse: Fn(&mut Value, &mut R, Endian, Args) -> BinResult<()>, + Parser: FnOnce(&mut R, Endian, Args) -> BinResult, { let relative_to = args.offset; let before = reader.stream_position()?; reader.seek(SeekFrom::Start(relative_to))?; - reader.seek(self.ptr.into_seek_from())?; - - let mut inner: Value = parser(reader, endian, args.inner.clone())?; - - after_parse(&mut inner, reader, endian, args.inner)?; + reader.seek(ptr.into_seek_from())?; + let value = parser(reader, endian, args.inner); reader.seek(SeekFrom::Start(before))?; - - self.value = Some(inner); - Ok(()) + value } /// Custom parser for use with the - /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive that reads - /// and then immediately finalizes a [`FilePtr`], returning the pointed-to - /// value as the result. + /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive + /// that reads a [`FilePtr`] and returns the pointed-to value as the result. /// /// # Errors /// @@ -184,23 +131,15 @@ where #[binrw::parser(reader, endian)] pub fn parse(args: FilePtrArgs, ...) -> BinResult where - Args: Clone, Value: for<'a> BinRead = Args>, { - Ok(Self::read_with_parser( - Value::read_options, - Value::after_parse, - reader, - endian, - args, - )? - .into_inner()) + Self::read_options(reader, endian, args).map(Self::into_inner) } /// Custom parser for use with the - /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive that reads and then - /// immediately finalizes a [`FilePtr`] using the specified parser, returning the pointed-to - /// value as the result. + /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive + /// that reads a [`FilePtr`] using the specified parser, returning the + /// pointed-to value as the result. /// /// # Errors /// @@ -210,19 +149,18 @@ where ) -> impl Fn(&mut R, Endian, FilePtrArgs) -> BinResult where R: Read + Seek, - Args: Clone, F: Fn(&mut R, Endian, Args) -> BinResult, { move |reader, endian, args| { - let after_parse = |_: &mut Value, _: &mut R, _: Endian, _: Args| Ok(()); - Ok(Self::read_with_parser(&parser, after_parse, reader, endian, args)?.into_inner()) + let ptr = Ptr::read_options(reader, endian, ())?; + Self::after_parse_with_parser(ptr, &parser, reader, endian, args) } } /// Custom parser for use with the - /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive that reads and then - /// immediately finalizes a [`FilePtr`] using the specified parser, returning the [`FilePtr`] - /// as the result. + /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive + /// that reads a [`FilePtr`] using the specified parser, returning the + /// [`FilePtr`] as the result. /// /// # Errors /// @@ -232,70 +170,32 @@ where ) -> impl Fn(&mut R, Endian, FilePtrArgs) -> BinResult where R: Read + Seek, - Args: Clone, F: Fn(&mut R, Endian, Args) -> BinResult, { move |reader, endian, args| { - let after_parse = |_: &mut Value, _: &mut R, _: Endian, _: Args| Ok(()); - Self::read_with_parser(&parser, after_parse, reader, endian, args) + let ptr = Ptr::read_options(reader, endian, ())?; + Self::after_parse_with_parser(ptr, &parser, reader, endian, args) + .map(|value| Self { ptr, value }) } } /// Consumes this object, returning the pointed-to value. - /// - /// # Panics - /// - /// Will panic if `FilePtr` hasn’t been finalized by calling - /// [`after_parse()`](Self::after_parse). pub fn into_inner(self) -> Value { - self.value.unwrap() + self.value } } -/// Dereferences the value. -/// -/// # Panics -/// -/// Will panic if `FilePtr` hasn’t been finalized by calling -/// [`after_parse()`](Self::after_parse). impl Deref for FilePtr { type Target = Value; fn deref(&self) -> &Self::Target { - match self.value.as_ref() { - Some(x) => x, - None => panic!( - "Deref'd FilePtr before reading (make sure to use FilePtr::after_parse first)" - ), - } + &self.value } } -/// # Panics -/// Will panic if the `FilePtr` has not been read yet using -/// [`BinRead::after_parse`](BinRead::after_parse) impl DerefMut for FilePtr { fn deref_mut(&mut self) -> &mut Value { - match self.value.as_mut() { - Some(x) => x, - None => panic!( - "Deref'd FilePtr before reading (make sure to use FilePtr::after_parse first)" - ), - } - } -} - -impl fmt::Debug for FilePtr -where - Ptr: fmt::Debug + IntoSeekFrom, - Value: fmt::Debug, -{ - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if let Some(ref value) = self.value { - fmt::Debug::fmt(value, f) - } else { - f.debug_tuple("UnreadPointer").field(&self.ptr).finish() - } + &mut self.value } } diff --git a/binrw/src/helpers.rs b/binrw/src/helpers.rs index 525410ef..819b3e8e 100644 --- a/binrw/src/helpers.rs +++ b/binrw/src/helpers.rs @@ -37,7 +37,7 @@ where Arg: Clone, Ret: FromIterator, { - until_with(cond, default_reader) + until_with(cond, T::read_options) } /// Creates a parser that uses a given function to read items into a collection @@ -129,7 +129,7 @@ where Arg: Clone, Ret: FromIterator, { - until_exclusive_with(cond, default_reader) + until_exclusive_with(cond, T::read_options) } /// Creates a parser that uses a given function to read items into a collection @@ -221,7 +221,7 @@ where Arg: Clone, Ret: FromIterator, { - until_eof_with(default_reader)(reader, endian, args) + until_eof_with(T::read_options)(reader, endian, args) } /// Creates a parser that uses a given function to read items into a collection @@ -318,7 +318,7 @@ where Ret: FromIterator, It: IntoIterator, { - args_iter_with(it, default_reader) + args_iter_with(it, T::read_options) } /// Creates a parser that uses a given function to build a collection, using @@ -406,7 +406,7 @@ where Arg: Clone, Ret: FromIterator + 'static, { - count_with(n, default_reader) + count_with(n, T::read_options) } /// Creates a parser that uses a given function to read N items into a @@ -477,16 +477,6 @@ where } } -#[binrw::parser(reader, endian)] -fn default_reader<'a, T: BinRead>(args: T::Args<'a>, ...) -> BinResult -where - T::Args<'a>: Clone, -{ - let mut value = T::read_options(reader, endian, args.clone())?; - value.after_parse(reader, endian, args)?; - Ok(value) -} - fn not_enough_bytes(_: T) -> Error { Error::Io(io::Error::new( io::ErrorKind::UnexpectedEof, diff --git a/binrw/src/pos_value.rs b/binrw/src/pos_value.rs index 02b4404e..299c64ed 100644 --- a/binrw/src/pos_value.rs +++ b/binrw/src/pos_value.rs @@ -44,15 +44,6 @@ impl BinRead for PosValue { val: T::read_options(reader, endian, args)?, }) } - - fn after_parse( - &mut self, - reader: &mut R, - endian: Endian, - args: Self::Args<'_>, - ) -> BinResult<()> { - self.val.after_parse(reader, endian, args) - } } impl core::ops::Deref for PosValue { diff --git a/binrw/tests/after_parse_test.rs b/binrw/tests/after_parse_test.rs deleted file mode 100644 index 3dee7e76..00000000 --- a/binrw/tests/after_parse_test.rs +++ /dev/null @@ -1,29 +0,0 @@ -use binrw::{io::Cursor, BinRead, BinReaderExt, FilePtr8}; - -#[test] -#[allow(non_snake_case)] -fn BinReaderExt_calls_after_parse() { - let test: FilePtr8 = Cursor::new([0x01, 0xFF]).read_be().unwrap(); - - assert_eq!(*test, 0xFF); -} - -#[test] -fn try_calls_after_parse() { - #[derive(BinRead)] - struct Try
(#[br(try)] Option
) - where - BR: BinRead, - for<'a> BR::Args<'a>: Default + 'static; - - let test: Try> = Cursor::new([0x01, 0xFF]).read_be().unwrap(); - - assert_eq!(*test.0.unwrap(), 0xFF) -} - -#[test] -fn tuple_calls_after_parse() { - let test: (FilePtr8, FilePtr8) = Cursor::new([2, 3, 0xFF, 0xEE]).read_be().unwrap(); - assert_eq!(*test.0, 0xFF); - assert_eq!(*test.1, 0xEE); -} diff --git a/binrw/tests/derive/struct.rs b/binrw/tests/derive/struct.rs index 87cf32de..69440540 100644 --- a/binrw/tests/derive/struct.rs +++ b/binrw/tests/derive/struct.rs @@ -190,9 +190,6 @@ fn deref_now() { #[derive(BinRead, Debug, PartialEq)] #[br(big, magic = b"TEST")] struct Test { - // deref_now on the first field tests that the reader position is correctly - // restored before reading the second field - #[br(deref_now)] a: FilePtr, b: i32, } @@ -203,20 +200,83 @@ fn deref_now() { Test { a: FilePtr { ptr: 0x10, - value: Some(NullString(b"Test string".to_vec())) + value: NullString(b"Test string".to_vec()) }, b: -1, } ); } +#[test] +fn move_args() { + #[derive(Debug, PartialEq)] + struct NonCopyArg; + + #[derive(BinRead, Debug, PartialEq)] + #[br(import(v: NonCopyArg))] + struct Inner(#[br(calc = v)] NonCopyArg); + + #[derive(BinRead, Debug, PartialEq)] + #[br(import(v: NonCopyArg))] + struct Test { + #[br(args(v))] + inner: Inner, + } + + assert_eq!( + Test::read_le_args(&mut Cursor::new(b""), (NonCopyArg,)).unwrap(), + Test { + inner: Inner(NonCopyArg) + } + ); +} + +#[test] +fn move_parser() { + struct A; + impl A { + fn accept(&self, x: u32) -> bool { + x == 0 + } + } + + #[derive(BinRead, Debug, PartialEq)] + #[br(import(a: A))] + struct Test { + #[br(parse_with = binrw::helpers::until(|x| a.accept(*x)))] + using_until: Vec, + } + + assert_eq!( + Test::read_le_args(&mut Cursor::new(b"\x01\0\0\0\x02\0\0\0\0\0\0\0"), (A,)).unwrap(), + Test { + using_until: vec![1, 2, 0], + } + ); +} + +#[test] +fn move_stream() { + #[binread] + #[derive(Debug, PartialEq)] + struct Test { + #[br(map_stream = |r| r)] + flags: u32, + } + + assert_eq!( + Test::read_le(&mut Cursor::new(b"\x01\0\0\0")).unwrap(), + Test { flags: 1 } + ); +} + // See https://github.com/jam1garner/binrw/issues/118 #[test] fn move_temp_field() { #[binread] #[derive(Debug, Eq, PartialEq)] struct Foo { - #[br(temp, postprocess_now)] + #[br(temp)] foo: binrw::NullString, #[br(calc = foo)] @@ -607,20 +667,6 @@ fn mixed_attrs() { assert_eq!(output.into_inner(), b"\x2a"); } -#[test] -fn offset_after() { - #[allow(dead_code)] - #[derive(BinRead, Debug)] - struct Test { - #[br(offset_after = b.into())] - a: FilePtr, - b: u8, - } - - let result = Test::read_le(&mut Cursor::new(b"\x01\x03\xff\xff\x04")).unwrap(); - assert_eq!(*result.a, 4); -} - #[test] fn raw_ident() { #[allow(dead_code)] diff --git a/binrw/tests/ui/deref_now_offset_after_conflict.rs b/binrw/tests/ui/deref_now_offset_after_conflict.rs deleted file mode 100644 index e477c686..00000000 --- a/binrw/tests/ui/deref_now_offset_after_conflict.rs +++ /dev/null @@ -1,9 +0,0 @@ -use binrw::BinRead; - -#[derive(BinRead)] -struct Foo { - #[br(deref_now, offset_after(1))] - a: u8, -} - -fn main() {} diff --git a/binrw/tests/ui/deref_now_offset_after_conflict.stderr b/binrw/tests/ui/deref_now_offset_after_conflict.stderr deleted file mode 100644 index 7c934747..00000000 --- a/binrw/tests/ui/deref_now_offset_after_conflict.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: `deref_now` and `offset_after` are mutually exclusive - --> $DIR/deref_now_offset_after_conflict.rs:5:10 - | -5 | #[br(deref_now, offset_after(1))] - | ^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/binrw/tests/ui/invalid_keyword_struct_field.stderr b/binrw/tests/ui/invalid_keyword_struct_field.stderr index db4c9fbe..aff77145 100644 --- 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`, `map_stream`, `magic`, `args`, `args_raw`, `calc`, `try_calc`, `default`, `ignore`, `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`, `repr`, `map_stream`, `magic`, `args`, `args_raw`, `calc`, `try_calc`, `default`, `ignore`, `parse_with`, `count`, `offset`, `if`, `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_offset_type.rs b/binrw/tests/ui/invalid_offset_type.rs index fe88f705..926004e5 100644 --- a/binrw/tests/ui/invalid_offset_type.rs +++ b/binrw/tests/ui/invalid_offset_type.rs @@ -5,9 +5,6 @@ struct Test { a: u8, #[br(offset = a)] b: FilePtr, - #[br(offset_after = d)] - c: FilePtr, - d: u8, } fn main() {} diff --git a/binrw/tests/ui/invalid_offset_type.stderr b/binrw/tests/ui/invalid_offset_type.stderr index c9bbfffa..409c6c63 100644 --- a/binrw/tests/ui/invalid_offset_type.stderr +++ b/binrw/tests/ui/invalid_offset_type.stderr @@ -16,22 +16,3 @@ help: you can convert a `u8` to a `u64` | 6 | #[br(offset = a.into())] | +++++++ - -error[E0308]: mismatched types - --> tests/ui/invalid_offset_type.rs:8:25 - | -8 | #[br(offset_after = d)] - | ^ - | | - | expected `u64`, found `u8` - | arguments to this method are incorrect - | -note: method defined here - --> src/file_ptr.rs - | - | pub offset: u64, - | ^^^^^^ -help: you can convert a `u8` to a `u64` - | -8 | #[br(offset_after = d.into())] - | +++++++ diff --git a/binrw/tests/ui/non_blocking_errors.stderr b/binrw/tests/ui/non_blocking_errors.stderr index 0321efa4..606ec8b5 100644 --- a/binrw/tests/ui/non_blocking_errors.stderr +++ b/binrw/tests/ui/non_blocking_errors.stderr @@ -4,13 +4,13 @@ error: expected one of: `stream`, `big`, `little`, `is_big`, `is_little`, `map`, 6 | #[br(invalid_keyword_struct)] | ^^^^^^^^^^^^^^^^^^^^^^ -error: expected one of: `big`, `little`, `is_big`, `is_little`, `map`, `try_map`, `repr`, `map_stream`, `magic`, `args`, `args_raw`, `calc`, `try_calc`, `default`, `ignore`, `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`, `repr`, `map_stream`, `magic`, `args`, `args_raw`, `calc`, `try_calc`, `default`, `ignore`, `parse_with`, `count`, `offset`, `if`, `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`, `map_stream`, `magic`, `args`, `args_raw`, `calc`, `try_calc`, `default`, `ignore`, `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`, `repr`, `map_stream`, `magic`, `args`, `args_raw`, `calc`, `try_calc`, `default`, `ignore`, `parse_with`, `count`, `offset`, `if`, `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_derive/src/binrw/backtrace/syntax_highlighting.rs b/binrw_derive/src/binrw/backtrace/syntax_highlighting.rs index f9f6c4e9..e42ba153 100644 --- a/binrw_derive/src/binrw/backtrace/syntax_highlighting.rs +++ b/binrw_derive/src/binrw/backtrace/syntax_highlighting.rs @@ -201,10 +201,6 @@ fn visit_expr_attributes(field: &StructField, visitor: &mut Visitor) { pad_size_to ); - if let Some(tokens) = field.offset_after.clone() { - visit!((*tokens).clone()); - } - if let Some(condition) = field.if_cond.clone() { let Condition { condition, @@ -408,9 +404,9 @@ fn is_keyword_ident(ident: &syn::Ident) -> bool { // binrw 'keywords' align_after, align_before, args, args_raw, assert, big, binread, br, brw, binwrite, - bw, calc, count, default, deref_now, ignore, import, import_raw, is_big, is_little, - little, magic, map, offset, offset_after, pad_after, pad_before, pad_size_to, parse_with, - postprocess_now, pre_assert, repr, restore_position, return_all_errors, + bw, calc, count, default, ignore, import, import_raw, is_big, is_little, + little, magic, map, offset, pad_after, pad_before, pad_size_to, parse_with, + pre_assert, repr, restore_position, return_all_errors, return_unexpected_error, seek_before, temp, try_map, write_with ); diff --git a/binrw_derive/src/binrw/codegen/read_options/struct.rs b/binrw_derive/src/binrw/codegen/read_options/struct.rs index 08664489..533fece8 100644 --- a/binrw_derive/src/binrw/codegen/read_options/struct.rs +++ b/binrw_derive/src/binrw/codegen/read_options/struct.rs @@ -6,15 +6,15 @@ use crate::{ codegen::{ get_assertions, get_endian, get_map_err, get_passed_args, get_try_calc, sanitization::{ - make_ident, AFTER_PARSE, ARGS_MACRO, ARGS_TYPE_HINT, BACKTRACE_FRAME, - BINREAD_TRAIT, COERCE_FN, DBG_EPRINTLN, MAP_ARGS_TYPE_HINT, MAP_READER_TYPE_HINT, - OPT, PARSE_FN_TYPE_HINT, POS, READER, READ_FUNCTION, READ_METHOD, - REQUIRED_ARG_TRAIT, SAVED_POSITION, SEEK_FROM, SEEK_TRAIT, TEMP, WITH_CONTEXT, + make_ident, ARGS_TYPE_HINT, BACKTRACE_FRAME, BINREAD_TRAIT, COERCE_FN, + DBG_EPRINTLN, MAP_ARGS_TYPE_HINT, MAP_READER_TYPE_HINT, OPT, PARSE_FN_TYPE_HINT, + POS, READER, READ_FUNCTION, READ_METHOD, REQUIRED_ARG_TRAIT, SAVED_POSITION, + SEEK_FROM, SEEK_TRAIT, TEMP, WITH_CONTEXT, }, }, parser::{ErrContext, FieldMode, Input, Map, Struct, StructField}, }, - util::{quote_spanned_any, IdentStr}, + util::quote_spanned_any, }; use alloc::borrow::Cow; use proc_macro2::TokenStream; @@ -82,21 +82,9 @@ impl<'input> StructGenerator<'input> { .fields .iter() .map(|field| generate_field(self.input, field, name, variant_name)); - let after_parse = { - let after_parse = self - .st - .fields - .iter() - .map(|field| generate_after_parse(self.input, field)); - wrap_save_restore( - &self.input.stream_ident_or(READER), - quote!(#(#after_parse)*), - ) - }; self.out = quote! { #prelude #(#read_fields)* - #after_parse }; self @@ -121,20 +109,6 @@ impl<'input> StructGenerator<'input> { } } -fn generate_after_parse(input: &Input, field: &StructField) -> Option { - if field.deref_now.is_none() { - get_after_parse_handler(field).map(|after_parse_fn| { - let (reader_var, endian_var, args_var) = make_field_vars(input, field); - AfterParseCallGenerator::new(field) - .get_value_from_ident() - .call_after_parse(after_parse_fn, &reader_var, &endian_var, &args_var) - .finish() - }) - } else { - None - } -} - fn generate_field( input: &Input, field: &StructField, @@ -150,7 +124,6 @@ fn generate_field( .read_value() .try_conversion(name, variant_name) .map_value() - .deref_now() .wrap_debug() .wrap_seek() .wrap_condition() @@ -165,69 +138,6 @@ fn generate_field( .finish() } -struct AfterParseCallGenerator<'field> { - field: &'field StructField, - out: TokenStream, -} - -impl<'field> AfterParseCallGenerator<'field> { - fn new(field: &'field StructField) -> Self { - Self { - field, - out: TokenStream::new(), - } - } - - fn call_after_parse( - mut self, - after_parse_fn: IdentStr, - reader_var: &TokenStream, - endian_var: &TokenStream, - args_var: &Option, - ) -> Self { - let value = self.out; - let args_arg = if let Some(offset) = &self.field.offset_after { - let offset = offset.as_ref(); - if let Some(args_var) = args_var { - quote_spanned_any! { offset.span()=> { - let mut #TEMP = #args_var; - #TEMP.offset = #offset; - #TEMP - }} - } else { - quote_spanned_any! { offset.span()=> - #ARGS_MACRO! { offset: #offset } - } - } - } else { - get_args_argument(self.field, args_var, false) - }; - - self.out = quote! { - #after_parse_fn(#value, #reader_var, #endian_var, #args_arg)?; - }; - - self - } - - fn finish(self) -> TokenStream { - self.out - } - - fn get_value_from_ident(mut self) -> Self { - let ident = &self.field.ident; - self.out = quote! { &mut #ident }; - - self - } - - fn get_value_from_temp(mut self) -> Self { - self.out = quote! { &mut #TEMP }; - - self - } -} - struct FieldGenerator<'field> { field: &'field StructField, out: TokenStream, @@ -310,33 +220,6 @@ impl<'field> FieldGenerator<'field> { self } - fn deref_now(mut self) -> Self { - if self.field.should_use_after_parse() { - return self; - } - - if let Some(after_parse) = get_after_parse_handler(self.field) { - let after_parse = AfterParseCallGenerator::new(self.field) - .get_value_from_temp() - .call_after_parse( - after_parse, - &self.reader_var, - &self.endian_var, - &self.args_var, - ) - .finish(); - - let value = self.out; - self.out = quote! {{ - let mut #TEMP = #value; - #after_parse - #TEMP - }}; - } - - self - } - fn finish(self) -> TokenStream { self.out } @@ -499,10 +382,9 @@ impl<'field> FieldGenerator<'field> { FieldMode::Calc(calc) => quote! { #calc }, FieldMode::TryCalc(calc) => get_try_calc(POS, &self.field.ty, calc), read_mode @ (FieldMode::Normal | FieldMode::Function(_)) => { - let args_arg = get_args_argument( - self.field, - &self.args_var, - matches!(read_mode, FieldMode::Normal), + let args_arg = self.args_var.as_ref().map_or_else( + || quote_spanned! {self.field.ty.span()=> <_ as #REQUIRED_ARG_TRAIT>::args() }, + ToTokens::to_token_stream, ); let reader_var = &self.reader_var; let endian_var = &self.endian_var; @@ -588,23 +470,6 @@ impl<'field> FieldGenerator<'field> { } } -fn get_args_argument( - field: &StructField, - args_var: &Option, - need_clone: bool, -) -> TokenStream { - args_var.as_ref().map_or_else( - || quote_spanned! {field.ty.span()=> <_ as #REQUIRED_ARG_TRAIT>::args() }, - |args_var| { - if need_clone { - quote! { #args_var.clone() } - } else { - quote! { #args_var } - } - }, - ) -} - fn get_err_context( field: &StructField, name: Option<&Ident>, @@ -725,10 +590,6 @@ fn generate_seek_before(reader_var: &TokenStream, field: &StructField) -> TokenS } } -fn get_after_parse_handler(field: &StructField) -> Option { - field.can_call_after_parse().then_some(AFTER_PARSE) -} - fn get_return_type(variant_ident: Option<&Ident>) -> TokenStream { variant_ident.map_or_else(|| quote! { Self }, |ident| quote! { Self::#ident }) } diff --git a/binrw_derive/src/binrw/codegen/sanitization.rs b/binrw_derive/src/binrw/codegen/sanitization.rs index c0e3dd38..544bc4fe 100644 --- a/binrw_derive/src/binrw/codegen/sanitization.rs +++ b/binrw_derive/src/binrw/codegen/sanitization.rs @@ -33,7 +33,6 @@ ident_str! { pub(crate) BIN_RESULT = from_crate!(BinResult); pub(crate) ENDIAN_ENUM = from_crate!(Endian); pub(crate) READ_METHOD = from_read_trait!(read_options); - pub(crate) AFTER_PARSE = from_read_trait!(after_parse); pub(crate) WRITE_METHOD = from_write_trait!(write_options); pub(crate) READER = "__binrw_generated_var_reader"; pub(crate) WRITER = "__binrw_generated_var_writer"; diff --git a/binrw_derive/src/binrw/parser/attrs.rs b/binrw_derive/src/binrw/parser/attrs.rs index 8fb68fcc..d53615be 100644 --- a/binrw_derive/src/binrw/parser/attrs.rs +++ b/binrw_derive/src/binrw/parser/attrs.rs @@ -16,7 +16,6 @@ pub(super) type Calc = MetaExpr; pub(super) type Count = MetaExpr; pub(super) type Debug = MetaVoid; pub(super) type Default = MetaVoid; -pub(super) type DerefNow = MetaVoid; pub(super) type ErrContext = MetaList; pub(super) type If = MetaList; pub(super) type Ignore = MetaVoid; @@ -29,12 +28,10 @@ pub(super) type Magic = MetaLit; pub(super) type Map = MetaExpr; pub(super) type MapStream = MetaExpr; pub(super) type Offset = MetaExpr; -pub(super) type OffsetAfter = MetaExpr; pub(super) type PadAfter = MetaExpr; pub(super) type PadBefore = MetaExpr; pub(super) type PadSizeTo = MetaExpr; pub(super) type ParseWith = MetaExpr; -pub(super) type PostProcessNow = MetaVoid; pub(super) type PreAssert = AssertLike; pub(super) type Repr = MetaType; pub(super) type RestorePosition = MetaVoid; diff --git a/binrw_derive/src/binrw/parser/field_level_attrs.rs b/binrw_derive/src/binrw/parser/field_level_attrs.rs index 09df066c..6dfc53ca 100644 --- a/binrw_derive/src/binrw/parser/field_level_attrs.rs +++ b/binrw_derive/src/binrw/parser/field_level_attrs.rs @@ -32,12 +32,8 @@ attr_struct! { pub(crate) count: Option, #[from(RO:Offset)] pub(crate) offset: Option, - #[from(RO:OffsetAfter)] - pub(crate) offset_after: Option>, #[from(RW:If)] pub(crate) if_cond: Option, - #[from(RO:DerefNow, RO:PostProcessNow)] - pub(crate) deref_now: Option>, #[from(RW:RestorePosition)] pub(crate) restore_position: Option<()>, #[from(RO:Try)] @@ -66,18 +62,6 @@ attr_struct! { } impl StructField { - /// Returns true if this field is read from a parser with an `after_parse` - /// method. - pub(crate) fn can_call_after_parse(&self) -> bool { - matches!(self.field_mode, FieldMode::Normal) && self.map.is_none() - } - - /// Returns true if the code generator should emit `BinRead::after_parse()` - /// after all fields have been read. - pub(crate) fn should_use_after_parse(&self) -> bool { - self.deref_now.is_none() && self.map.is_none() - } - /// Returns true if this field is generated using a calculated value instead /// of a parser. pub(crate) fn generated_value(&self) -> bool { @@ -112,7 +96,7 @@ impl StructField { /// Returns true if the field is using shorthand directives that are /// converted into named arguments. pub(crate) fn has_named_arg_directives(&self) -> bool { - self.count.is_some() || self.offset.is_some() || self.offset_after.is_some() + self.count.is_some() || self.offset.is_some() } /// Returns true if the only field-level attributes are asserts @@ -134,9 +118,7 @@ impl StructField { && all_fields_none!( count, offset, - offset_after, if_cond, - deref_now, restore_position, do_try, temp, @@ -163,20 +145,6 @@ impl StructField { fn validate(&self, _: Options) -> syn::Result<()> { let mut all_errors = None::; - if let (Some(offset_after), Some(deref_now)) = (&self.offset_after, &self.deref_now) { - let offset_after_span = offset_after.span(); - let span = offset_after_span - .join(deref_now.span()) - .unwrap_or(offset_after_span); - combine_error( - &mut all_errors, - syn::Error::new( - span, - "`deref_now` and `offset_after` are mutually exclusive", - ), - ); - } - if self.do_try.is_some() && self.generated_value() { //TODO: join with span of read mode somehow let span = self.do_try.as_ref().unwrap().span(); @@ -221,7 +189,6 @@ impl StructField { for (used, name) in [ (self.count.is_some(), "count"), (self.offset.is_some(), "offset"), - (self.offset_after.is_some(), "offset_after"), ] { if used { combine_error(&mut all_errors, syn::Error::new( @@ -260,9 +227,7 @@ impl FromField for StructField { field_mode: <_>::default(), count: <_>::default(), offset: <_>::default(), - offset_after: <_>::default(), if_cond: <_>::default(), - deref_now: <_>::default(), restore_position: <_>::default(), do_try: <_>::default(), temp: <_>::default(), diff --git a/binrw_derive/src/binrw/parser/keywords.rs b/binrw_derive/src/binrw/parser/keywords.rs index 5b1d19b5..4a4af2ba 100644 --- a/binrw_derive/src/binrw/parser/keywords.rs +++ b/binrw_derive/src/binrw/parser/keywords.rs @@ -22,7 +22,6 @@ define_keywords! { count, dbg, default, - deref_now, err_context, ignore, import, @@ -34,12 +33,10 @@ define_keywords! { map, map_stream, offset, - offset_after, pad_after, pad_before, pad_size_to, parse_with, - postprocess_now, pre_assert, repr, restore_position, diff --git a/binrw_derive/src/binrw/parser/mod.rs b/binrw_derive/src/binrw/parser/mod.rs index 248702b4..24b20c0e 100644 --- a/binrw_derive/src/binrw/parser/mod.rs +++ b/binrw_derive/src/binrw/parser/mod.rs @@ -388,13 +388,6 @@ mod tests { } }); - try_error!(deref_now_offset_after_conflict: "mutually exclusive" { - struct Foo { - #[br(deref_now, offset_after(1))] - a: u8, - } - }); - try_error!(unsupported_type_enum: "null enums are not supported" { enum Foo {} }); From 924e09435fc2103518b8f2416ead51d65cd6c165 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Sat, 1 Jul 2023 21:41:48 -0500 Subject: [PATCH 2/3] Allow mutable borrows in map functions --- binrw/src/private.rs | 2 +- binrw/tests/derive/struct.rs | 17 +++++++++++++++++ .../src/binrw/codegen/read_options/struct.rs | 4 ++-- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/binrw/src/private.rs b/binrw/src/private.rs index 8ee659e4..a2c533ce 100644 --- a/binrw/src/private.rs +++ b/binrw/src/private.rs @@ -71,7 +71,7 @@ where // capturing closures since they are not compatible with that type. pub fn coerce_fn(f: F) -> F where - F: Fn(T) -> R, + F: FnMut(T) -> R, { f } diff --git a/binrw/tests/derive/struct.rs b/binrw/tests/derive/struct.rs index 69440540..f6db4c68 100644 --- a/binrw/tests/derive/struct.rs +++ b/binrw/tests/derive/struct.rs @@ -291,6 +291,23 @@ fn move_temp_field() { ); } +#[test] +fn mut_map() { + #[derive(BinRead, Debug, PartialEq)] + #[br(import(v: &mut Vec))] + struct Test { + #[br(map = |a: u8| a + v.pop().unwrap())] + a: u8, + #[br(map = |b: u8| b + v.pop().unwrap())] + b: u8, + } + + assert_eq!( + Test::read_le_args(&mut Cursor::new(b"\x01\x02"), (&mut vec![1, 2],)).unwrap(), + Test { a: 3, b: 3 } + ); +} + #[test] fn empty_imports() { #[derive(BinRead, Debug, PartialEq)] diff --git a/binrw_derive/src/binrw/codegen/read_options/struct.rs b/binrw_derive/src/binrw/codegen/read_options/struct.rs index 533fece8..2758e10c 100644 --- a/binrw_derive/src/binrw/codegen/read_options/struct.rs +++ b/binrw_derive/src/binrw/codegen/read_options/struct.rs @@ -257,7 +257,7 @@ impl<'field> FieldGenerator<'field> { Map::None => return self, Map::Map(map) => { quote! { - let #map_func = (#COERCE_FN::<#ty, _, _>(#map)); + let mut #map_func = (#COERCE_FN::<#ty, _, _>(#map)); } } Map::Try(try_map) | Map::Repr(try_map) => { @@ -271,7 +271,7 @@ impl<'field> FieldGenerator<'field> { // 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)); + let mut #map_func = (#COERCE_FN::<::core::result::Result<#ty, _>, _, _>(#try_map)); } } }; From f823c31ff68d7a222a2ae47be5bca31f6d3cb53c Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Sat, 1 Jul 2023 20:23:18 -0500 Subject: [PATCH 3/3] Add helper functions for reading offset tables --- binrw/src/file_ptr.rs | 274 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 226 insertions(+), 48 deletions(-) diff --git a/binrw/src/file_ptr.rs b/binrw/src/file_ptr.rs index fa976d2a..7b1a2617 100644 --- a/binrw/src/file_ptr.rs +++ b/binrw/src/file_ptr.rs @@ -1,12 +1,10 @@ -//! Type definitions for wrappers which represent a layer of indirection within -//! a file. +//! Types definitions and helpers for handling indirection within a file. use crate::NamedArgs; use crate::{ io::{Read, Seek, SeekFrom}, BinRead, BinResult, Endian, }; -use core::fmt; use core::num::{ NonZeroI128, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI8, NonZeroU128, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, @@ -37,10 +35,10 @@ pub type NonZeroFilePtr128 = FilePtr; /// A wrapper type which represents a layer of indirection within a file. /// -/// `FilePtr` is composed of two types. The pointer type `P` is the -/// absolute offset to a value within the data, and the value type `T` is -/// the actual pointed-to value. [Dereferencing] it will yield the -/// pointed-to value. +/// The pointer type `Ptr` is an offset to a value within the data stream, and +/// the value type `T` is the value at that offset. +/// +/// [Dereferencing] a `FilePtr` yields the pointed-to value. /// /// When deriving `BinRead`, the [offset](crate::docs::attribute#offset) /// directive can be used to adjust the offset before the pointed-to value is @@ -48,6 +46,58 @@ pub type NonZeroFilePtr128 = FilePtr; /// /// [Dereferencing]: core::ops::Deref /// +/// # Performance +/// +/// Using `FilePtr` directly is not efficient when reading offset tables because +/// it immediately seeks to read the pointed-to value. Instead: +/// +/// 1. Read the offset table as a `Vec<{integer}>`, then use +/// [`parse_from_iter`] to create a collection of `Vec`. +/// +/// 2. Read the offset table as a `Vec<{integer}>`, then add a function +/// to your type that lazily reads the pointed-to value: +/// +/// ``` +/// # use binrw::{args, BinRead, BinResult, BinReaderExt, helpers::until_eof, io::{Cursor, Read, Seek, SeekFrom}}; +/// #[derive(BinRead)] +/// #[br(big)] +/// struct Header { +/// count: u16, +/// +/// #[br(args { count: count.into() })] +/// offsets: Vec, +/// } +/// +/// #[derive(BinRead)] +/// # #[derive(Debug, Eq, PartialEq)] +/// #[br(big)] +/// struct Item(u8); +/// +/// #[derive(BinRead)] +/// #[br(big, stream = s)] +/// struct Object { +/// header: Header, +/// #[br(try_calc = s.stream_position())] +/// data_offset: u64, +/// } +/// +/// impl Object { +/// pub fn get(&self, source: &mut R, index: usize) -> Option> { +/// self.header.offsets.get(index).map(|offset| { +/// let offset = self.data_offset + u64::from(*offset); +/// source.seek(SeekFrom::Start(offset))?; +/// Item::read(source) +/// }) +/// } +/// } +/// +/// # let mut s = Cursor::new(b"\0\x02\0\x01\0\0\x03\x04"); +/// # let x = Object::read(&mut s).unwrap(); +/// # assert!(matches!(x.get(&mut s, 0), Some(Ok(Item(4))))); +/// # assert!(matches!(x.get(&mut s, 1), Some(Ok(Item(3))))); +/// # assert!(matches!(x.get(&mut s, 2), None)); +/// ``` +/// /// # Examples /// /// ``` @@ -85,45 +135,23 @@ where { type Args<'a> = FilePtrArgs>; - /// Reads the offset of the value from the reader. fn read_options( reader: &mut R, endian: Endian, args: Self::Args<'_>, ) -> BinResult { let ptr = Ptr::read_options(reader, endian, ())?; - let value = Self::after_parse_with_parser(ptr, Value::read_options, reader, endian, args)?; + let value = Self::read_value(ptr, Value::read_options, reader, endian, args)?; Ok(FilePtr { ptr, value }) } } impl FilePtr where - Ptr: for<'a> BinRead = ()> + IntoSeekFrom, + Ptr: IntoSeekFrom, { - fn after_parse_with_parser( - ptr: Ptr, - parser: Parser, - reader: &mut R, - endian: Endian, - args: FilePtrArgs, - ) -> BinResult - where - R: Read + Seek, - Parser: FnOnce(&mut R, Endian, Args) -> BinResult, - { - let relative_to = args.offset; - let before = reader.stream_position()?; - reader.seek(SeekFrom::Start(relative_to))?; - reader.seek(ptr.into_seek_from())?; - let value = parser(reader, endian, args.inner); - reader.seek(SeekFrom::Start(before))?; - value - } - - /// Custom parser for use with the - /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive - /// that reads a [`FilePtr`] and returns the pointed-to value as the result. + /// Reads an offset, then seeks to and parses the pointed-to value using the + /// [`BinRead`] implementation for `Value`. Returns the pointed-to value. /// /// # Errors /// @@ -131,15 +159,15 @@ where #[binrw::parser(reader, endian)] pub fn parse(args: FilePtrArgs, ...) -> BinResult where + Ptr: for<'a> BinRead = ()> + IntoSeekFrom, Value: for<'a> BinRead = Args>, { Self::read_options(reader, endian, args).map(Self::into_inner) } - /// Custom parser for use with the - /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive - /// that reads a [`FilePtr`] using the specified parser, returning the - /// pointed-to value as the result. + /// Creates a parser that reads an offset, then seeks to and parses the + /// pointed-to value using the given `parser` function. Returns the + /// pointed-to value. /// /// # Errors /// @@ -150,17 +178,15 @@ where where R: Read + Seek, F: Fn(&mut R, Endian, Args) -> BinResult, + Ptr: for<'a> BinRead = ()> + IntoSeekFrom, { - move |reader, endian, args| { - let ptr = Ptr::read_options(reader, endian, ())?; - Self::after_parse_with_parser(ptr, &parser, reader, endian, args) - } + let parser = Self::with(parser); + move |reader, endian, args| parser(reader, endian, args).map(Self::into_inner) } - /// Custom parser for use with the - /// [`parse_with`](crate::docs::attribute#custom-parserswriters) directive - /// that reads a [`FilePtr`] using the specified parser, returning the - /// [`FilePtr`] as the result. + /// Creates a parser that reads an offset, then seeks to and parses the + /// pointed-to value using the given `parser` function. Returns a + /// [`FilePtr`] containing the offset and value. /// /// # Errors /// @@ -171,11 +197,12 @@ where where R: Read + Seek, F: Fn(&mut R, Endian, Args) -> BinResult, + Ptr: for<'a> BinRead = ()> + IntoSeekFrom, { move |reader, endian, args| { let ptr = Ptr::read_options(reader, endian, ())?; - Self::after_parse_with_parser(ptr, &parser, reader, endian, args) - .map(|value| Self { ptr, value }) + let value = Self::read_value(ptr, &parser, reader, endian, args)?; + Ok(Self { ptr, value }) } } @@ -183,6 +210,26 @@ where pub fn into_inner(self) -> Value { self.value } + + fn read_value( + ptr: Ptr, + parser: Parser, + reader: &mut R, + endian: Endian, + args: FilePtrArgs, + ) -> BinResult + where + R: Read + Seek, + Parser: FnOnce(&mut R, Endian, Args) -> BinResult, + { + let relative_to = args.offset; + let before = reader.stream_position()?; + reader.seek(SeekFrom::Start(relative_to))?; + reader.seek(ptr.into_seek_from())?; + let value = parser(reader, endian, args.inner); + reader.seek(SeekFrom::Start(before))?; + value + } } impl Deref for FilePtr { @@ -209,9 +256,140 @@ where } } +/// Creates a parser that reads a collection of values from an iterator of +/// file offsets using the [`BinRead`] implementation of `Value`. +/// +/// Offsets are treated as relative to the position of the reader when +/// parsing begins. Use the [`seek_before`] directive to reposition the +/// stream in this case. +/// +/// [`seek_before`]: crate::docs::attribute#padding-and-alignment +/// +/// # Examples +/// +/// ``` +/// # use binrw::{args, BinRead, BinReaderExt, io::Cursor}; +/// #[derive(BinRead)] +/// #[br(big)] +/// struct Header { +/// count: u16, +/// +/// #[br(args { count: count.into() })] +/// offsets: Vec, +/// } +/// +/// #[derive(BinRead)] +/// #[br(big)] +/// struct Object { +/// header: Header, +/// #[br(parse_with = binrw::file_ptr::parse_from_iter(header.offsets.iter().copied()))] +/// values: Vec, +/// } +/// +/// # let mut x = Cursor::new(b"\0\x02\0\x01\0\0\x03\x04"); +/// # let x = Object::read(&mut x).unwrap(); +/// # assert_eq!(x.values, &[4, 3]); +/// ``` +pub fn parse_from_iter( + it: It, +) -> impl FnOnce(&mut Reader, Endian, Args) -> BinResult +where + Ptr: IntoSeekFrom, + Value: for<'a> BinRead = Args>, + Ret: FromIterator, + Args: Clone, + It: IntoIterator, + Reader: Read + Seek, +{ + parse_from_iter_with(it, Value::read_options) +} + +/// Creates a parser that reads a collection of values from an iterator of +/// file offsets using the given `parser` function. +/// +/// Offsets are treated as relative to the position of the reader when +/// parsing begins. Use the [`seek_before`] directive to reposition the +/// stream in this case. +/// +/// [`seek_before`]: crate::docs::attribute#padding-and-alignment +/// +/// # Examples +/// +/// ``` +/// # use binrw::{args, BinRead, BinReaderExt, io::Cursor}; +/// #[derive(BinRead)] +/// #[br(big)] +/// struct Header { +/// count: u16, +/// +/// #[br(args { count: count.into() })] +/// offsets: Vec, +/// } +/// +/// # #[derive(Debug, Eq, PartialEq)] +/// struct Item(u8); +/// +/// #[derive(BinRead)] +/// #[br(big)] +/// struct Object { +/// header: Header, +/// #[br(parse_with = binrw::file_ptr::parse_from_iter_with(header.offsets.iter().copied(), |reader, endian, args| { +/// u8::read_options(reader, endian, args).map(Item) +/// }))] +/// values: Vec, +/// } +/// +/// # let mut x = Cursor::new(b"\0\x02\0\x01\0\0\x03\x04"); +/// # let x = Object::read(&mut x).unwrap(); +/// # assert_eq!(x.values, &[Item(4), Item(3)]); +/// ``` +pub fn parse_from_iter_with( + it: It, + parser: F, +) -> impl FnOnce(&mut Reader, Endian, Args) -> BinResult +where + Ptr: IntoSeekFrom, + Ret: FromIterator, + Args: Clone, + It: IntoIterator, + F: Fn(&mut Reader, Endian, Args) -> BinResult, + Reader: Read + Seek, +{ + move |reader, endian, args| { + let base_pos = reader.stream_position()?; + it.into_iter() + .map(move |ptr| { + // Avoid unnecessary seeks: + // 1. Unnecessary seeking backwards to the base position + // will cause forward-only readers to fail always even if + // the offsets are ordered; + // 2. Seeks that change the position when it does not need + // to change may unnecessarily flush a buffered reader + // cache. + match ptr.into_seek_from() { + seek @ SeekFrom::Current(offset) => { + if let Some(new_pos) = base_pos.checked_add_signed(offset) { + if new_pos != reader.stream_position()? { + reader.seek(SeekFrom::Start(new_pos))?; + } + } else { + reader.seek(seek)?; + } + } + seek => { + reader.seek(seek)?; + } + } + + parser(reader, endian, args.clone()) + }) + .collect() + } +} + /// A trait to convert from an integer into /// [`SeekFrom::Current`](crate::io::SeekFrom::Current). -pub trait IntoSeekFrom: Copy + fmt::Debug { +pub trait IntoSeekFrom: Copy { /// Converts the value. fn into_seek_from(self) -> SeekFrom; }