A parser to recover from exceptions

[eriksunsol]

I created CatchParser originally because I wanted to bail out from a delegate/lambda used with .Select() and be able to report sensible errors. I then found it to be a convenient means to bail out from deeply nested parsing, i.e. when using ExpressionParser and do common error handling and reporting on a higher level.

Before preparing this pull request I did however take a step back and achieved the same goal instead by letting errors detected deep down bubble up to higher level context by letting parsers return an error result instead of just failing. It turned out to work equally well or even better and make my code easier to follow as it does not skip out of deeply nested calls using try-catch.

With that said, I would generally recommend to design parsers so that they report errors or other events to outer contexts by returning results to represent such situations before resorting to using CatchParser. I can't think of any situation that could not be dealt with using what is already available in Pidgin using the strategy to basically never let a composition of nested parsers just fail but return a result representing the failure instead.

Nevertheless since I already wrote it and there may be use cases that I didn't think of, here it is. Feel free to accept or reject it.

[benjamin-hodgson]

Thank you very much for the contribution and for your interest!

1. Throwing an exception from inside a parser seems problematic. You'll lose any rewinding that was meant to happen between the exception and the Catch, meaning that by the time you're inside the Catch the parser is in an unpredictable (possibly even corrupted) state.
   • I guess this is why you had the Catch parser do its own rewinding? I'm not sure that that's enough, though.
2. Additionally, throwing and catching exceptions commits me to using the runtime stack to represent chained parsers. Ideally I'd like the freedom to use my own parsing stack in the future.
3. Perhaps we should instead be thinking about a more natural way to do "tagged failure"? It might even be as simple as an object-typed field on the ParseError which can be examined in RecoverWith, though I think we'd need to think about that a bit.
4. As a name, Catch falsely mirrors Try and is perhaps a little too short for a discouraged API. I'd prefer a more explicit name like CatchException. Or perhaps we should put it in a separate namespace for people who know what they're doing.
5. I do have some general misgivings about the exceptions-as-control-flow style this API enables.

Point 1 seems especially tricky.

I left some notes for code style in the diff.

Thanks!

[benjamin-hodgson]

No need for xmldocs on internal classes

[benjamin-hodgson]

I'm not sure that we should rewind here. I imagine that clients might want to (eg) get the CurrentPos at the point of failure. Should be feasible to code up the rewind logic yourself, ie

Parser CatchAndRewind<TException>()
    => Try(this.Catch<TException>(e => Fail()));

[benjamin-hodgson]

I don't think there's much need for this overload - seems overly specific (there are other components of the state which get rewound too) and you should be able to code it up yourself using CurrentOffset.

[benjamin-hodgson]

I'd suggest modelling this as a parser which returns an exception, so

Parser<TToken, TException> Catch<TException>()
    where TException : Exception

[benjamin-hodgson]

Test seems a little too complex in my opinion. I don't think we need to test this with all the different token types, nor do we need to run the parser on lots of different test inputs.

I'd suggest just adding a method in StringParserTests.cs and test the three main cases:

1. Throwing the expected exception
2. Throwing a different exception
3. Throwing no exception

[benjamin-hodgson]

You can make this a private class rather than a separate one, or (my preference) just use some off-the-shelf exception types (eg InvalidOperationException and the like)