Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A parser to recover from exceptions #144

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eriksunsol
Copy link

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.

Copy link
Owner

@benjamin-hodgson benjamin-hodgson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
}

/// <summary>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for xmldocs on internal classes

catch (TException e)
{
var count = state.Location - bookmark;
state.Rewind(bookmark);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

/// <typeparam name="TException">The exception to catch.</typeparam>
/// <param name="errorHandler">A function which returns a parser to apply when the current parser throws <typeparamref name="TException"/>.</param>
/// <returns>A parser twhich runs the current parser, running <paramref name="errorHandler"/> if it throws <typeparamref name="TException"/>.</returns>
public Parser<TToken, T> Catch<TException>(Func<TException, int, Parser<TToken, T>> errorHandler)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// <typeparam name="TException">The exception to catch.</typeparam>
/// <param name="errorHandler">A function which returns a parser to apply when the current parser throws <typeparamref name="TException"/>.</param>
/// <returns>A parser twhich runs the current parser, running <paramref name="errorHandler"/> if it throws <typeparamref name="TException"/>.</returns>
public Parser<TToken, T> Catch<TException>(Func<TException, Parser<TToken, T>> errorHandler)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


namespace Pidgin.Tests;

public partial class CatchTests : ParserTestBase
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


namespace Pidgin.Tests;

public class CatchTest1Exception : Exception
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants