Error handling questions

[BLucky-gh]

I'm writing a brainfuck parser, and I'm finding that it's very easy to write a working parser with winnow, but pretty annoying to make it report good errors, at least with a grammar as simple as brainfuck (which I do admit might be simpler than what winnow was designed for). I keep either getting errors in places where I can't attach context (for example the eof that my parser gets chained with in Parser::parse), or the error's context holds the wrong offset because it backtracked too far, or not enough.

Here's the parser code (it's a bit of a mess because I've been trying to find a way to make it both work correctly and report the correct offsets and locations in the errors, and as it is right now I actually broke bracket parsing in the process so it thinks any opening bracket is mismatched)

pub enum Token {
    Next,
    Previous,
    Incr,
    Decr,
    Input,
    Output,
    Loop(Vec<Token>),
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Ast {
    root: Vec<Token>,
}

impl FromStr for Ast {
    type Err = String;

    fn from_str(input: &str) -> Result<Self, Self::Err> {
        Ok(Ast {
            root: (
                parse_bf,
                // have to check for EOF in case of unmatched `]`
                // which would stop the parser before it finishes
                // even though `Parser::parse` chains it with `eof` already
                // but doesn't let me attach context
                eof.context(StrContext::Label("unmatched closing bracket")),
            )
                .parse(input)
                .map(|(v, _)| v)
                .map_err(|e| e.to_string())?,
        })
    }
}

fn junk_parser<'a>(input: &mut &'a str) -> PResult<&'a str> {
    take_till(0.., VALID_TOKENS).parse_next(input)
}

fn parse_bf(input: &mut &str) -> PResult<Vec<Token>> {
    trace(
        "parse_bf",
        terminated(
            repeat_till(0.., preceded(junk_parser, parse_token), peek(']').void()).map(|(v, _)| v),
            junk_parser,
        ),
    )
    .parse_next(input)
}

fn parse_token(input: &mut &str) -> PResult<Token> {
    alt((trace("loop_tokens", loop_tokens), simple_tokens)).parse_next(input)
}

fn loop_tokens(input: &mut &str) -> PResult<Token> {
    let checkpt = input.checkpoint();
    preceded(
        '[',
        (
            peek(cut_err(take_until(1.., ']'))),
            terminated(parse_bf, ']'),
        ),
    )
    .map(|(_, v)| Token::Loop(v))
    .parse_next(input)
    .map_err(|e| {
        //I have to do this song and dance to force it to report the error
        //at the correct character position instead of one after it
        //which does unfortunately mean I have to go over parts of the input twice
        if let ErrMode::Cut(_) = e {
            input.reset(&checkpt);
            cut_err(fail::<_, &str, _>.context(StrContext::Label("unmatched opening bracket")))
                .parse_next(input)
                .expect_err("this branch must always return an error")
        } else {
            e
        }
    })
}

fn simple_tokens(input: &mut &str) -> PResult<Token> {
    trace(
        "simple_token",
        dispatch! {any;
            '+' => empty.value(Token::Incr),
            '-' => empty.value(Token::Decr),
            '>' => empty.value(Token::Next),
            '<' => empty.value(Token::Previous),
            ',' => empty.value(Token::Input),
            '.' => empty.value(Token::Output),
            _   => empty.map(|_| unreachable!()),
        },
    )
    .parse_next(input)
}

it would be really nice if I could just manually edit the offset or provide and offset to the reported offset as usize so that I don't have to do the song and dance I had to do when catching the error just to reset a checkpoint and throw another error with fail (which also changes the error cause, which thankfully don't need in this case)

another thing that would be nice is if take_till accepted FnMut as the parser, and not just Fn, since I tried to implement the recursion in my parser by doing a take_till with a closure that just counts the number of opening and closing brackets to take until the matching bracket, but since take_till requires FnMut, I have to use Cell, and interior mutability just to satisfy a trait boundary that I'm not sure needs a reason to be there (tbf, I know there might be one, I don't don't know it) rubs me the wrong way.

also general tips on how I could improve the parser are welcome. Chances are whatever you suggested I have already tried but had to change for one reason or another, usually for error reporting reasons.

[epage]

(for example the eof that my parser gets chained with in Parser::parse),

That is more of a safety thing and it is reasonable to have your own eof if you need to attach state.

the error's context holds the wrong offset because it backtracked too far, or not enough.

As I'm unfamiliar with this language, it would help to step through some of the error cases you are trying to create but have difficulty with.

I'm guessing one is that for un-terminated brackets, you want to point at the open bracket but have the close bracket context. Is that right?

another thing that would be nice is if take_till accepted FnMut as the parser, and not just Fn, since I tried to implement the recursion in my parser by doing a take_till with a closure that just counts the number of opening and closing brackets to take until the matching bracket, but since take_till requires FnMut

I'd recommend creating an issue for discussing this.

[BLucky-gh]

As I'm unfamiliar with this language, it would help to step through some of the error cases you are trying to create but have difficulty with.
I'm guessing one is that for un-terminated brackets, you want to point at the open bracket but have the close bracket context. Is that right?

yeah pretty much. The only invalid syntax in bf is unmatched brackets, and I can't seem to be able to make it so that the correct offset is shown on errors, aka the opening bracket when there's no matching closing bracket, or the closing bracket when there's no opening bracket.

in the former case the parser needs to somehow backtrack all the way back to the opening bracket after reaching eof without balancing out the brackets, also being careful about the recursive nature of the parser

and in the latter case, at least with how the parser is written right now, it terminates the parser instead of erroring out, needing an eof, but then the error points to the next byte, because the closing bracket was already consumed by the parser.

The available options I have tried either involve ugly hacks like restoring a checkpoint on error and calling fail just to get another error from it because the error types already in the library are either not documented in the tutorials, or not public (don't remember which one is the case cause it's been a bit), or break something else in the parser (balancing brackets from the outside in, causing [ [ [ ] ] to error out on the middle bracket instead of the first one, which I guess is still valid, or only handling one of the bracket cases, or causing a loop of backtrack_err and cut_err just backtracking the parser all the way to the start due to the recursive nature of the parser, among other ones)

I'd recommend creating an issue for discussing this.

will do
EDIT: just opened #558