Improving error messages from OneOf

[randomeizer]

So, I've been using the throwing version of the library here. I have a OneOf which contains about 7 different possible sub-parsers. When all of them fail, you end up with a lot of error messages.

A thought I had for improving that would be for the 'many error' aggregator to try and pick a winner based on which one progressed furthest through the input before failing. In most cases, the parser which gets furthest is most likely the (eventually) correct one.

If any sub-parsers are equal, they all get aggregated, otherwise just the winner gets passed on.

The progress could come from either the thrown ParsingError's record of the Input, or if it's not a ParsingError, the unconsumed input post-OneOf. This will be more generally helpful once ParsingError is made public.

[randomeizer]

This is not for all uses of 'manyErrors', by the way: specifically OneOf, but I think that's the primary use case currently.

[randomeizer]

Any thoughts on this @stephencelis / @mbrandonw?

[stephencelis]

@randomeizer Thanks for starting the discussion! We were busy prepping the release so haven't had a chance to catch up till now 😄

We definitely think this could be improved with the internal ParsingError type, but the guts there are a little messy still. My hunch is the following changes could help make it happen:

1. Add an explicit expectedInput case to ParsingError

2. When flattening and sorting/ranking all errors of a manyFailed, we could group expectedInput cases that have the exact same originalInput and remainingInput (which affects the source range being highlighted)

3. We could then print those cases in a more compact format:

   error: multiple failures occurred
   
   error: unexpected input
    ---> input:1:8
   1 | 1,Blob,gues
     |        ^ expected "admin"
     |          expected "guest"
     |          expected "member"

   Or even:

   error: unexpected input
    ---> input:1:8
   1 | 1,Blob,gues
     |        ^ expected "admin", "guest", or "member"

We'll hopefully explore this soon! If you end up messing around with things, be warned that it's probably the messiest part of the code base 😅

[stephencelis]

Oh, and secondly, and perhaps more attainable, we really ought to have a label operator that can replace the expectation message with something customizable. I.e., then the above could be achieved today without the grouping logic, but a little extra explicit work:

OneOf {
  "admin".map { Role.admin }
  "guest".map { Role.guest }
  "member".map { Role.member }
}
.label(#""admin", "guest", or "member""#)

The big question here is API specifics: naming, etc.

[randomeizer]

Yeah, no worries, figured you were busy.

The cleaner output would definitely improve readability, and would still make sense when multiple parsers fail at the same line:character point.

But for me at least, there is still massive value in having it only report the "most successful" failure for OneOf in particular. For example, in my case, I'm parsing code documentation comments in a custom format. Here are some examples of the first two lines of documentation for different types:

--- === module.name ===
---
...

--- module.name.function(a) -> boolean
--- Function
...

--- module.name.variable <boolean>
--- Variable
...

--- module.name:method(a) -> boolean
--- Method
...

--- module.name.field <boolean>
--- Field

You can see fuller examples here if you want.

Anyway, let's say that I put this for my "module" documentation:

--- === module.name
---
...

I forgot to put the closing "===" after the name. The only relevant parser type is the "Module Parser" - function/variable/etc aren't even close with that "===" prefix there. In this case, the "module" parser would fail at character 1:19, outputting that it expected "===". So far so good.

The other parsers for feature/method/etc, will all fail at 1:5. Currently, they will also get bundled into the error message. However, I don't want them - they are obviously (to me, a human) not candidates. If OneOf only reported the error that made the most progress, it would only output the "module" error in this case.

Now, what if there was an error while defining a Variable, with a typo in "Variable". Eg:

--- module.name.value <string>
--- Variabl

The first line for both Field (occurs on and Variable have the same signature. We can only determine which it is once the word "Field" or "Variable" appears on line 2.

So, in this case, "module" fails at 1:5 (no "==="), "field" fails at 1:22 (no parentheses), and "method" fails at 1:16 (expected ":"). Both "variable" and "field" will fail to match either "Field" or "Variable" at 2:4. Because they are the furthest along, but also first equal to each other, the ParseError.multiple(...) would output both the "variable" and "field" results. Which is great — that's where the error actually was.

Hopefully that illustrates my point more clearly.

[randomeizer]

Oh, and secondly, and perhaps more attainable, we really ought to have a label operator that can replace the expectation message with something customizable. I.e., then the above could be achieved today without the grouping logic, but a little extra explicit work:

I have written a Require parser which takes a @ParserBuilder for the upstream Parser, and also a closure that receives the error + input and should output another error. This is essentially the same as the replaceError function, except that it replaces it with a different error.

In the case of your example, it would look something like:

Require {
  OneOf {
     "admin".map { Role.admin }
     "guest".map { Role.guest }
     "member".map { Role.member }
   }
 } orThrow: { error, input in
     ParsingError.inputExpected("either 'admin', 'guest' or 'member'", at: &input)
 }

This could pretty much be done with the replaceError function, except that it only returns a value, not an Error.

The big question here is API specifics: naming, etc.

Yeah, label does not do what I would guess it does with that name. It could be errorLabel or something, I guess, but I still wouldn't be sure what the outcome would be.

[randomeizer]

Now that I'm looking at it, my implementations of Require {} orThrow: {} and Try {} catch: {} very much overlap. I'll probably be retiring Require and just keeping Try, internally.