Failing non-chomping andThenOneOf parser fails the already parsed oneOf value

[Janiczek]

SSCCE: https://ellie-app.com/5dYn9vN2ggra1

If we have already parsed an oneOf value, and for some reason the andThenOneOf parsers fail, we should succeed on the parsed value, not fail the whole thing.

This only shows up when an andThenOneOf parser is non-chomping (ie. Parser.succeed () or Parser.spaces or something similar).

I have found that adding backtrackable to infixHelp helps:

succeed (apply left)
  |. operator
  |= backtrackable (subExpression rightPrecedence config)
  -- ^^^^^^^^^^^^^

backtrackable <|
-- ^^^^^^^^^^^^^
  succeed (apply left)
    |. operator
    |= subExpression rightPrecedence config

I'm not sure which one of these is correct though.

EDIT: It might be worth looking at postfix too?

[rlefevre]

Thank you for your bug report.

1.

First, this is not what happens here. If a failing andThenOneOf parser was failing an already parsed oneOf one, you would not be able to even parse a single int, as the andThenOneOf parsers are also tried in this case (and fail).

If you print the exact character of you error, you can see that it fails on lse:

> parse "if 1 then 2 else 3"
Err [{ col = 14, problem = ExpectingInt, row = 1 }]
    : Result (List Parser.DeadEnd) Main.Expr
> String.dropLeft 13 "if 1 then 2 else 3"
"lse 3" : String

You are confronted to the unfortunate following elm/parser int bug: elm/parser#25

The int parser will consume a starting e because it uses Parser.number under the hood, and uses unintentionally some leaky implementation of float used to parse numbers like 1e3 (not sure why only float supports those though, number is a little messy at the moment). Note that it would also consume an uppercase E, and decimal points .

To confirm, use otherwise instead of else, it will work: https://ellie-app.com/5f9BLsBdFn3a1.

In your case, after the else, subExpression parses 2 then the Call infix operation tries to parse another sub-expression, trying again oneOf parsers. This is when the bug occurs, int consumes the e of else, fails, so the andThenOneOf parsers fail, therefore 2 succeeds, then else is not found, making the whole if_ parser fail.:sweat_smile:

You have several solutions here:

• you can make the int parser backtrackable, so it won't eat the e when failing,: https://ellie-app.com/5f2FPHbTm8fa1

            , PP.literal (P.backtrackable int)

or https://ellie-app.com/5f34QnwQtSKa1

int =
    P.map Int (P.backtrackable P.int)

• or you could make your own int parser that only accepts digits, for example: https://ellie-app.com/5f2ZdcvCJrGa1

int =
    P.getChompedString (P.chompWhile Char.isDigit)
        |> P.andThen
            (\str ->
                case String.toInt str of
                    Just i ->
                        P.succeed (Int i)

                    Nothing ->
                        P.problem "expecting integer"
            )

I'm not sure which one has the best performance, you will have to benchmark.

Also I would advice to take the time to read and become familiar with all the bug reports of elm/parser (there are not that much), because some of them can lead to very tricky errors, like this one.

2.

Secondly, I don't want any part of the library to use backtrackable by default, as stated in expression documentation, because I want to encourage avoiding it and want users to know that it is not used unless they use it themselves.

3.

Lastly, every oneOf or andThenOneOf helper can be redefined in the user code, therefore I believe that forking the library should never be needed to change the behavior of helpers.

For example if you really wanted to add backtrackable to infixLeft you could simply redefine it, modifying the original implementation:

https://ellie-app.com/5f8RtZHvr2ma1

backtrackableInfixLeft : Int -> Parser  () -> (e -> e -> e) -> PP.Config e -> ( Int, e -> Parser e )
backtrackableInfixLeft precedence operator apply config =
    ( precedence
    , \left ->
        P.backtrackable <|
            P.succeed (apply left)
                |. operator
                |= PP.subExpression precedence config
    )

But as explained in 1., this is not needed here anyway.

Another example, you could redefine your left-associative operator-less call operation without an operator like this:

https://ellie-app.com/5f925BH6LVva1

        , andThenOneOf = [ leftAssoc 99 Call ]

and

leftAssoc : Int -> (expr -> expr -> expr) -> PP.Config expr -> ( Int, expr -> Parser expr )
leftAssoc precedence apply config =
    ( precedence
    , \left ->
            P.succeed (apply left)
                |= PP.subExpression precedence config
    )

or

leftAssoc : Int -> (e -> e -> e) -> PP.Config e -> ( Int, e -> Parser e )
leftAssoc precedence apply config =
    ( precedence
    , \left -> P.map (apply left) (PP.subExpression precedence config)
    )

But again, this is not really needed here, because your succeed() parser does not consume any character, so it won't actually fail the whole andThenOneOf parser.

I will try to improve the documentation in the next release to better explain some of these points.

[Janiczek]

Thanks! Yes, I was seeing it fails on a weird spot (inside the "else" and not near the number) but didn't connect the dots wrt. 'e'. Will look over the elm/parser bugs.

Also, I agree with your assessment of the situation wrt. backtrackable. 👍