Repository: drhagen/parsita

Issue:
Predicate parser drops failures of inner parser when predicate is false
In Parsita 1, the `pred` parser discards all errors from a successful run of the inner parser if the predicate turns out to be false. (The predicate failure is the only failure.) This is not how Parsita promises to work. It promises to always report the furthest error, which is usually what the user wants. This matters when one parser parses a prefix of another parser (basically all parsers of mathematical expressions). The short parser succeeds, the long parser fails. This is normally fine because, when the overall parser fails to consume the full input, the long parser's message is displayed because it made more progress before the ultimate failure.

Below is a parser (clearly a subset of a full expression parser) that demostrates this unexpected behavior.

```python
from parsita import *


def is_static(expression):
    if isinstance(expression, str):
        return False
    if isinstance(expression, int):
        return True
    elif isinstance(expression, list):
        return all(is_static(e) for e in expression)


class PredParsers(ParserContext):
    name = reg(r"[a-zA-Z]+")
    integer = reg(r"[0-9]+") > int
    function = name >> "(" >> repsep(expression, ",") << ")"
    expression = function | name | integer
    static_expression = pred(expression, is_static, "static expression")

print(PredParsers.static_expression.parse("1"))
# Success(1)
print(PredParsers.static_expression.parse("sin(0)"))
# Success([0])
print(PredParsers.static_expression.parse("a"))
# Expected static expression but found 'a'
# Line 1, character 1
# a
# ^
print(PredParsers.static_expression.parse("f(a,1)"))
# Expected static expression but found 'f'
# Line 1, character 1
# f(a,1)
# ^
print(PredParsers.static_expression.parse("f(2,1)"))
# Success([2, 1])
print(PredParsers.static_expression.parse("f(2,1"))
# Expected static expression but found 'f'
# Line 1, character 1
# f(2,1
# ^
```

This last error message shows the problem. The farther error is clearly the issue, but the early success takes precedence. This is easy to fix in Parsita 1; it's just a missing `.merge(status)`:

```python
class PredicateParser(Generic[Input, Output], Parser[Input, Input]):
    def __init__(self, parser: Parser[Input, Output], predicate: Callable[[Output], bool], description: str):
        super().__init__()
        self.parser = parser
        self.predicate = predicate
        self.description = description

    def consume(self, reader: Reader[Input]):
        remainder = reader
        status = self.parser.consume(remainder)
        if isinstance(status, Continue):
            if self.predicate(status.value):
                return status
            else:
                return Backtrack(remainder, lambda: self.description)#.merge(status)
        else:
            return status
```

Sure enough, this makes `pred` report the farthest error.

```python
print(PredParsers.static_expression.parse("f(2,1"))
# Expected ',' or ')' but found end of source
```

However, this now breaks the `pred` parser error reporting. Because the pred parser fails at the start of the input, the inner parser *always* makes more progress than `pred` so any way that the parser could be extended becomes the farther error. The pred parser error message is actually hard to trigger and, in fact, cannot be triggered by `static_expressions` in the example at all.

```python
print(PredParsers.static_expression.parse("a"))
# Expected '(' but found end of source
```

This was discovered because Parsita 2 fixes the original bug (because `merge`s no longer need to be manually applied) and I was relying on `pred` reporting an error at least sometimes. The expected behavior of `pred` could be this:

* If the inner parser fails, it backtracks like normal.
* If the predicate passes, it continues like normal.
* If the predicate fails, the failure is reported at the end of the consumed input, not the beginning. Note that this completely messes up the concept of "actual" for the predicate parser unless we add the ability to customize the actual message on a per-parser basis.

There is a question here on whether or not failures that tie the predicate parser should be superseded by the predicate parser. For example,

```python
print(PredParsers.static_expression.parse("a"))
# Expected '(' or static expression but found end of source  # allowing ties
# Expected static expression but found end of source  # superseding ties
```

For this particular parser, superseding ties is clearly better, but it is difficult to convince myself that superseding ties is always the correct behavior for `pred`. This would be the only parser that supersedes other failures. While unusual, it kind of makes sense given that `pred` applies a post-processing step and therefore acts as a kind of checkpoint on the parser.

In the same vein, there is also a question of whether or not the predicate parser should clear any errors generated by the inner parser at the end of the input it consumed.

Ultimately, the `pred` parser may not be completely sane because it is not really a parser, but a post-processor of parsed input. I have only used the `pred` parser where I should probably be using a type checker on top of the AST, but that was too much work.

Discussion:
codecov-commenter: ## [Codecov](https://app.codecov.io/gh/drhagen/parsita/pull/87?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen) Report
> Merging [#87](https://app.codecov.io/gh/drhagen/parsita/pull/87?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen) (8e619ef) into [master](https://app.codecov.io/gh/drhagen/parsita/commit/2821c901a0e181bcb88992b83eaf2431aec22d24?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen) (2821c90) will **not change** coverage.
> The diff coverage is `100.00%`.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please [install the Github App Integration](https://github.com/apps/codecov) for your organization. [Read more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen).

```diff
@@            Coverage Diff            @@
##            master       #87   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          810       809    -1     
  Branches       105       105           
=========================================
- Hits           810       809    -1     
```


| [Impacted Files](https://app.codecov.io/gh/drhagen/parsita/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen) | Coverage Δ | |
|---|---|---|
| [src/parsita/parsers/\_predicate.py](https://app.codecov.io/gh/drhagen/parsita/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen#diff-c3JjL3BhcnNpdGEvcGFyc2Vycy9fcHJlZGljYXRlLnB5) | `100.00% <100.00%> (ø)` | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hagen)


Patch:
diff --git a/src/parsita/parsers/_predicate.py b/src/parsita/parsers/_predicate.py
index 281b326..af31f6a 100644
--- a/src/parsita/parsers/_predicate.py
+++ b/src/parsita/parsers/_predicate.py
@@ -14,13 +14,12 @@ class PredicateParser(Generic[Input, Output], Parser[Input, Input]):
         self.description = description
 
     def consume(self, state: State[Input], reader: Reader[Input]):
-        remainder = reader
-        status = self.parser.cached_consume(state, remainder)
+        status = self.parser.cached_consume(state, reader)
         if isinstance(status, Continue):
             if self.predicate(status.value):
                 return status
             else:
-                state.register_failure(self.description, reader)
+                state.register_failure(self.description, status.remainder)
                 return None
         else:
             return status


Test patch:
diff --git a/tests/test_basic.py b/tests/test_basic.py
index c82ce46..ceef14a 100644
--- a/tests/test_basic.py
+++ b/tests/test_basic.py
@@ -65,7 +65,7 @@ def test_predicate():
     assert TestParsers.a.parse("A") == Success("A")
     assert TestParsers.d.parse("2") == Success("2")
     assert TestParsers.d.parse("23") == Failure(ParseError(StringReader("23", 1), ["end of source"]))
-    assert TestParsers.d.parse("a") == Failure(ParseError(StringReader("a", 0), ["digit"]))
+    assert TestParsers.d.parse("a") == Failure(ParseError(StringReader("a", 1), ["digit"]))
     assert str(TestParsers.a) == "a = pred(any1, letter A)"
 
 
diff --git a/tests/test_regex.py b/tests/test_regex.py
index b45b303..84bffd9 100644
--- a/tests/test_regex.py
+++ b/tests/test_regex.py
@@ -54,17 +54,44 @@ def test_literal_multiple():
     assert TestParsers.keyword.parse("int") == Success("int")
 
 
-def test_interval():
+def test_pred_interval():
     class TestParsers(ParserContext, whitespace="[ ]*"):
         number = reg(r"\d+") > int
         pair = "[" >> number << "," & number << "]"
         interval = pred(pair, lambda x: x[0] <= x[1], "ordered pair")
 
     assert TestParsers.interval.parse("[1, 2]") == Success([1, 2])
-    assert isinstance(TestParsers.interval.parse("[2, 1]"), Failure)
+    assert TestParsers.interval.parse("[2, 1]") == Failure(ParseError(StringReader("[2, 1]", 6), ["ordered pair"]))
     assert TestParsers.pair.parse("[1,a]") == TestParsers.interval.parse("[1,a]")
 
 
+def test_pred_static_expression():
+    def is_static(expression):
+        if isinstance(expression, str):
+            return False
+        if isinstance(expression, int):
+            return True
+        elif isinstance(expression, list):
+            return all(is_static(e) for e in expression)
+
+    class PredParsers(ParserContext):
+        name = reg(r"[a-zA-Z]+")
+        integer = reg(r"[0-9]+") > int
+        function = name >> "(" >> repsep(expression, ",") << ")"
+        expression = function | name | integer
+        static_expression = pred(expression, is_static, "static expression")
+
+    assert PredParsers.static_expression.parse("1") == Success(1)
+    assert PredParsers.static_expression.parse("f(2,1)") == Success([2, 1])
+    assert PredParsers.static_expression.parse("a") == Failure(
+        ParseError(StringReader("a", 1), ["'('", "static expression"])
+    )
+    assert PredParsers.static_expression.parse("f(a,1)") == Failure(
+        ParseError(StringReader("f(a,1)", 6), ["static expression"])
+    )
+    assert PredParsers.static_expression.parse("f(2,1") == Failure(ParseError(StringReader("f(2,1", 5), ["','", "')'"]))
+
+
 def test_regex():
     class TestParsers(ParserContext, whitespace="[ ]*"):
         digits = reg(r"\d+")