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

Predicates shouldn't be wrapped in SILENT_FAILS #455

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

markw65
Copy link

@markw65 markw65 commented Dec 12, 2023

If a predicate fails, its failure needs to be recorded:

Before:

% echo 'start = &[b] "bb"' | node bin/peggy.js -t "a" 
Error running test
Error: Expected , or undefined but "a" found.
 --> command line:1:1
  |
1 | a
  | ^

% echo 'start = &[b] "bb" / "c"' | node bin/peggy.js -t "a"
Error running test
Error: Expected "c" but "a" found.
 --> command line:1:1
  |
1 | a
  | ^

Note the first one doesn't have a failure explanation at all, and the second says that 'c' was expected, when 'b' or 'c' would have been acceptable.

After:

% echo 'start = &[b] "bb"' | node bin/peggy.js -t "a"      
Error running test
Error: Expected [b] but "a" found.
 --> command line:1:1
  |
1 | a
  | ^
mwilliams@MacBook-Pro peggy % echo 'start = &[b] "bb" / "c"' | node bin/peggy.js -t "a"
Error running test
Error: Expected "c" or [b] but "a" found.
 --> command line:1:1
  |
1 | a
  | ^

There were tests for the second case, but they were testing the actual behavior, rather than what (I think) it should be doing.

@Mingun
Copy link
Member

Mingun commented Dec 12, 2023

The problem is not so simple and reporting expected from predicates can create many false-positives. This need to be carefully checked. In your examples the symbol is checked on the first position, but them actually can be deeper. Even in the test that you changed the new behaviour do report actually impossible input:

start = 'a' / &'b' / 'c'

That grammar does not accept b. The second alternative actually cannot match anything because it is contradictory.

Expectations from predicates are difficult and from negative predicates are especially difficult. Probably the current behavior the best balance between accurate and complete reporting.

@markw65
Copy link
Author

markw65 commented Dec 12, 2023

That grammar does not accept b. The second alternative actually cannot match anything because it is contradictory

That's true, but you don't take account of what might happen later in the grammar anywhere else. eg

start = ("a" / "b" / "c") []

Peggy reports '[a-c]' expected, even though the grammar doesn't match anything, in much the same way.

So I think it makes sense to report that "a" or "b" or "c" was expected in the 'a' / &'b' / 'c' - because in any real grammar there's going to be a follow up. eg

start = ('a' / &'b' / 'c') .?

Now saying that it expects 'a' or 'c' is definitely wrong - it really does accept 'a', 'b', or 'c'.

Its probably just a failure of imagination on my part, but I can't think of a case where its wrong to include the predicate failure... can you provide examples?

[edit: I guess in the negative cases, its going to be wrong because it will report that it was expecting something, when in fact it was expecting anything but something - so I can see that that part of the pull request is wrong. Not sure yet if its fixable]

@Mingun
Copy link
Member

Mingun commented Dec 12, 2023

That's true, but you don't take account of what might happen later in the grammar anywhere else. eg

It still makes sense to report expectations in this case, since the first character really has to be a, b or c. The error will be reported only at the second character, and actually such grammar even shouldn't be compiled. It is better to move in this direction -- detect contradictory grammars at compile time rather that fixing error reporting for them. This situation is the same as here:

start = 'a' / 'b' 'c'

This grammar accepts only a and bc, but if you enter, say, f, the error would be Expected "a" or "b" but "f" found. even when just b is not accepted input for the whole grammar.

Now saying that it expects 'a' or 'c' is definitely wrong - it really does accept 'a', 'b', or 'c'.

At least it does not say anything that wouldn't be true. Some valid inputs are missed in the message, but the message never contains invalid inputs. I think, that this property is more important. Believe, I tried to implement better error handling taking expectations from predicates and even created negative expectations, but I remember that in real cases that does not look well. Although I never tried to work only with positive predicates.

@markw65
Copy link
Author

markw65 commented Dec 12, 2023

I'm still having a hard time understanding your argument here.

It still makes sense to report expectations in this case, since the first character really has to be a, b or c.

Which is also the case for 'a' / &'b' / 'c'. The first character has to be a, b or c, and whether the rule matches depends on how the rule is embedded in the grammar (which isn't checked in either case).

The error will be reported only at the second character

No, it will be reported whether or not there's a second character.

You seem to be arguing that because the grammar might be self contradictory, we shouldn't report the &'b' because it might be impossible to match when providing a 'b'. But you're also arguing that we should reject self contradictory grammars - and if you were able to do that, then there would be even more reason to report the &'b', since now we know that there's a valid match somewhere.

At least it does not say anything that wouldn't be true

You still haven't provided an example where reporting (positive) matches would do that though...

@markw65 markw65 marked this pull request as draft December 13, 2023 22:05
@markw65
Copy link
Author

markw65 commented Dec 13, 2023

As noted above, its definitely not right for negative predicates.

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