[Help] How can I implement this parser?

[DJDuque]

Hi all,

I am starting a small project where I am parsing some simple binary structures. I have read through all the tutorials and documentation (which are excellent), but I am still getting stuck.

I want to parse (given a slice of bytes) the following format:

Byte Offset	Size (in bytes)	Description
0	4	Name (ASCII alphanumeric)
4	2	Data type
6	2	Data size (n)
8	n	Raw data byte slice

Assume that I have a struct DataType that implements TryFrom<u16>.

Then I want to parse from &'a [u8] to:

#[derive(Clone, Copy, Debug)]
pub struct Bank16View<'a> {
    name: &'a str,
    data_type: DataType,
    data_slice: &'a [u8],
}

I am trying to get something like (which obviously is wrong, but I haven't been able to understand all the things that I am doing wrong):

fn bank_16_view<'a, E: ParserError<&'a [u8]>>(
    endian: Endianness,
) -> impl Parser<&'a [u8], Bank16View<'a>, E> {
    (move |input: &mut &'a [u8]| {
        let (data_slice, (name, data_type, data_size)) = (
            take(4).try_map(std::str::from_utf8).verify(|s: &str| s.len() == 4),
            u16(endian).try_map(DataType::try_from),
            u16(endian),
        )
            .parse_peek(input)?;

        Bank16View {
            name,
            data_type,
            data_slice: &data_slice[..usize::from(data_size)],
        }
    })(input)
}

Once I get the above working, I would use it to have 2 public methods: try_from_le_bytes and try_from_be_bytes (simply calling the above under the hood). Is this a good plan?

Can I get some guidance on how to get the above working?

Thanks

[epage]

It'd help to know what problems you are having with it so we can have an idea of how to steer things. I'm goin to make some guesses

The first problem I'm noticing is your return type is impl Parser but you are trying to return PResult<Bank16View, E> because you have (input) at the end. If you remove that, you'll be returning a closure (FnMut) which, when the signature is correct, will match impl Parser and work.

take(4).try_map(std::str::from_utf8).verify(|s: &str| s.len() == 4)

The last verify should be redundant as take guarantees that only 4 tokens were consumed.

You mentioned the name should be alphanumeric which this doesn't check. You could do take_while(4, (b'a'..='z', b'A'..='Z', b'0'..=b'9').

parse_peek

This parser never commits to the parse, making it so whatever runs next will process the same
bytes all over again.

For more, see the next one

data_slice: &data_slice[..usize::from(data_size)],

This isn't doing any validation that there is enough data to slice, causing a panic if the size field is larger than remaining input.

Instead try using length_data(u16(endian)) for the last parser and use parse_next instead of parse_peek.

Hmm, I think I should rename that to length_take to be more consistent with the other slicing parsers.

[DJDuque]

Thanks, this was a very helpful reply.

I have some follow-up comments:

1. Feedback/suggestions (I will add more to this discussion or open an issue once I make more progress with my project):
   It would be good to enhance the discoverability of the length_count, length_data, and length_value functions. I think that the names themselves are fine, I wouldn't really change them (maybe length_take is more consistent, but that was not the issue for me).

   My issue (as someone with zero experience with this) was that looking at the binary module they don't pop-up as being different from the other 40 functions. I just assumed that all functions here where just all the combinations for parsing integers/floats with given endianness.

   Maybe it would be good to have a short trivial example in the description of that module using one of these. Also it would be good if the TLV link in this page points to this example rather than that wikipedia page.

2. Question: How should I expose error type in the public method of my library?
   I got things working as (I appreciate any improvements):

   fn bank_16_view<'a>(endian: Endianness) -> impl Parser<&'a [u8], Bank16View<'a>, ContextError> {
       move |input: &mut &'a [u8]| {
           let (name, data_type, data_slice) = (
               take_while(4, AsChar::is_alphanum)
                   // SAFETY: All 4 bytes are ASCII alphanumeric.
                   .map(|b: &[u8]| unsafe { std::str::from_utf8_unchecked(b) }),
               u16(endian).try_map(DataType::try_from),
               length_data(u16(endian)),
           )
               .parse_next(input)?;
   
           Ok(Bank16View {
               name,
               data_type,
               data_slice,
           })
       }
   }
   
   impl<'a> Bank16View<'a> {
       pub fn try_from_le_bytes(bytes: &'a [u8]) -> Result<Self, ParseError<&'a [u8], ContextError>> {
           bank_16_view(Endianness::Little).parse(bytes)
       }
   }
   

   The issue here is that ParseError<I, E> doesn't implement Error, so I cannot use e.g. the ? operator.

   After reading again through the Error Management section, I feel that CustomError is not what I want. To me, CustomError feels like the kind of thing I need if I want to change the default ContextError and handle the inner parsing error stuff (like context, ErrorKind, etc.) in a different way (am I right?).

   I tried to do stuff like (using thiserror):

   // This fails because the lifetime is not &'static
   #[derive(Debug, Error)]
   pub enum TryBankViewFromSliceError {
       #[error("parser error")]
       ParseError(#[from] ParseError<&'a [u8], ContextError>),
   }
   
   // This fails because ContextError doesn't implement Error
   #[derive(Debug, Error)]
   pub enum TryBankViewFromSliceError {
       #[error("parser error")]
       ParseError(#[from] ContextError),
   }
   

   Can you point me to some library code that does something similar?

[epage]

I think that the names themselves are fine, I wouldn't really change them (maybe length_take is more consistent, but that was not the issue for me).

My concern with the naming was not as much direct discoverability but that I had to look too closely to remember which was which. Being consistent in verbage within the API makes intent clearer when browsing the docs (the problem i had) and makes it more predictable so you are less likely to need the docs in the first place.

• take is a term already a verb we use when getting a slice
• count was a former name from nom that we made` repeat

My issue (as someone with zero experience with this) was that looking at the binary module they don't pop-up as being different from the other 40 functions. I just assumed that all functions here where just all the combinations for parsing integers/floats with given endianness.

#396 makes some further improvements

Note also that length_ functions are the first things called out in the binary section of our list of combinators.

Maybe it would be good to have a short trivial example in the description of that module using one of these. A

I've frequently seen people have their specific problem in using a library and jump to the conclusion that an example for their specific case should be front and center that I'm always a bit hesitant when I see cases that look like that. I've not dealt enough with binary parsing to say whether there makes sense a single representative example to highlight within winnow::binarys documentation and if that example should use a length_ function.

[epage]

Question: How should I expose error type in the public method of my library?
I got things working as (I appreciate any improvements):

winnows errors are designed for being used in the hot loop of a parser. I would recommend extracting what data you want from them and putting that within a custom error message.

[DJDuque]

I have been making good progress with my project. I want to thank you again for your work with winnow. The documentation and tutorials are excellent, and writing a parser is very pleasant. Some times I do struggle a little bit, and I most likely make some non-optimal choices, but I blame my lack of experience writing this kind of code.

I have encountered a use case that is probably general enough for a new binary parser (?).

Imagine the format:

0x8000         -----> Used to determine endianness
u16
u16
// the rest of the structure

I am doing basically something like:

fn parse(input: &mut &[u8]) -> PResult<(u16, u16)> {
    let endianness = match u16(Endianness::Little).parse_next(input)? {
        0x8000 => Endianness::Little,
        0x0080 => Endianness::Big,
        _ => return fail.parse_next(input),
    };

    (u16(endianness), u16(endianness)).parse_next(input)
}

Does the above make sense? Or is there a cleaner way of doing this? Maybe an endianness_and_then parser (although I have no idea how that could make things "cleaner". Just throwing out an idea.

[epage]

Some times I do struggle a little bit, and I most likely make some non-optimal choices, but I blame my lack of experience writing this kind of code.

Always appreciate hearing where people stub their toes. Even if one report isn't enough to act on, as we hear from others we can see what patterns develop to see how we might be able to better guide people.

Does the above make sense? Or is there a cleaner way of doing this? Maybe an endianness_and_then parser (although I have no idea how that could make things "cleaner". Just throwing out an idea.

I think that makes sense. If you wanted to make it a bit more declarative, you could do

fn parse(input: &mut &[u8]) -> PResult<(u16, u16)> {
    let endianness = dispatch!{u16(Endianness::Little);
        0x8000 => success(Endianness::Little),
        0x0080 => success(Endianness::Big),
        _ => return fail,
    }.parse_next(input);

    (u16(endianness), u16(endianness)).parse_next(input)
}

but I'm not sure that actually buys you much.

Using Endianness will make the code more maintainable though unsure if it offers the best performance. As an alternative, you could do:

fn parse(input: &mut &[u8]) -> PResult<(u16, u16)> {
    dispatch!{u16(Endianness::Little);
        0x8000 => (u16_le, u16_le),
        0x0080 => (u16_be, u16_be),
        _ => return fail,
    }.parse_next(input)
}

While performance is great, it can be more important to be able to maintain your code and so you have to weigh out the cost of duplicating logic. At least you benefit from the code lining up to make issues more obvious.