Behavior of Labelled if the wrapped parser consumed input

[DanielSchuessler]

Hi! Very nice library, thanks for open-sourcing it.

I noticed some odd behavior of Parser.Labelled(), for example, given this parser:

var tupleParser = LetterOrDigit.AtLeastOnceString()
                    .Separated(Char(','))
                    .Between(Char('('), Char(')'))
                    .Labelled("Tuple");

tupleParser.ParseOrThrow("(1,2,!!,3)");

The error message is:

Pidgin.ParseException : Parse error.
    unexpected !
    expected Tuple
    at line 1, col 6

Which is misleading IMHO, since a tuple isn't actually expected at col 6. A possible fix would be to change WithExpectedParser.Parse() as follows:

            internal override InternalResult<T> Parse(ref ParseState<TToken> state)
            {
                state.BeginExpectedTran();
                var result = _parser.Parse(ref state);
                state.EndExpectedTran(commit: result.ConsumedInput);
                if (!result.Success && !result.ConsumedInput)
                {
                    state.AddExpected(_expected);
                }
                return result;
            }

I only just started looking at the Codebase, so I hope I understood the Expected transaction mechanism correctly :) With this change, the message is as if the Labelled wasn't there:

Pidgin.ParseException : Parse error.
    unexpected !
    expected letter or digit
    at line 1, col 6

Maybe a better fix would be to include a sort of stacktrace of Labelleds that enclosed the error, e.g. "Error while parsing Tuple: ...", but this would be a larger change. Not sure if it should go into the error message, the Expecteds, or some new construct.

[benjamin-hodgson]

I think I can see reasons to do it either way. If your Labelled is at the level of a lexeme, you probably want your error message to say "expected identifier" even if it failed half way through the identifier. But I agree that at the syntactic level, "expected statement" is not a good error message when the parse error occurred inside a token.

Would you mind sending a pull request with your change, so that we have something concrete to discuss?