jam1garner / binrw

A Rust crate for helping parse and rebuild binary data using ✨macro magic✨.
https://binrw.rs
MIT License
585 stars 35 forks source link

Make correct error handling easier for manual implementations #155

Open csnover opened 2 years ago

csnover commented 2 years ago

Copied from #9:

Error recovery by rewinding the stream on parse failure is implemented only in the derive and the default impls. Requiring manual implementations to handle this themselves is error-prone, remembering to consistently implement it in BinRead itself is error-prone, and manually tracking and reporting the position of the field/struct/variant which failed to be read successfully is error-prone. Consider something like splitting the API so read_options becomes something like:

fn read_options<R: io::Read + io::Seek>(reader: &mut R, options: &ReadOptions, args: Self::Args) -> BinResult<Self> {
    let pos = reader.seek(SeekFrom::Current(0))?;
    read_options_impl(reader, options, args).map_err(|mut err| {
        if let Some(seek_error) = reader.seek(SeekFrom::Start(pos)) {
            err.append(seek_error);
        }
        err
    })
}

…and then read_options_impl (bikeshed name) is where the actual implementation goes, so any error recovery can be fully uniform (and more options could be made available if someone requests them, e.g. if someone prefers to have a stream end in an undefined state or receive no position information on error in exchange for fewer seeks).

This may actually be impossible to fully realise because it would require calls from inside another read_options to become read_options_impl or whatever and only callees that are not read_options would call through a wrapper? At the least some helper functions can exist to make this job easier, because I’ve already written them for myself.


The other part of this is to make it easier to actually generate errors, since right now it is a bit verbose and could probably be easier. A helper that records the position and makes it easy to call an error probably. Similar to syn::parse::Lookahead1 maybe?; any thing which checkpoints positions and has some easy method to call that bails with the error.