fix: Don't ignore IncompleteMatch in OneOrMoreHelper#593
fix: Don't ignore IncompleteMatch in OneOrMoreHelper#593Xanewok wants to merge 3 commits intoNomicFoundation:mainfrom
Conversation
Parsing *some* input in when the first element is optional shouldn't be accepted as empty.
|
This changed because of the NomicFoundation#586 that was merged in the meantime.
| - NumericExpression (Rule): # 0..1 "2" | ||
| - DecimalLiteral (Token): "2" # 0..1 | ||
| - SKIPPED (Token): " * new\n" # 1..8 | ||
| - BinaryExpression (Rule): # 1..3 " *" |
There was a problem hiding this comment.
not sure why BinaryExpression is produced with one child (the binary operator) without any operands. Is this expected?
| │ ╰──── Error occurred here. | ||
| ───╯ | ||
| - > | ||
| Error: Expected OpenBrace or OpenParen. |
There was a problem hiding this comment.
not sure why are we expecting OpenBrace or OpenParen here. Since function call expressions are postfix operators, shouldn't we be skipping it alltogether? am I missing something?
OmarTawfik
left a comment
There was a problem hiding this comment.
Left a couple of questions
|
I investigated #593 (comment) and it seems that the removed branch is specifically used in the precedence parser: if the input is complete, the nodes are reduced, however if the sequence is incomplete, it is not, hence why we see the flat [2, *, incomplete_new] node sequence. I believe that the change itself is correct (partially parsing first term in ZeroOrMore should return an partial parse, not a valid "empty" one) but this doesn't surface in our regular parsing code, as the only place it's used is in the SeparatedBy, which is transformed as We should probably separate this branch to the precedence parser code itself or adapt the Separately from that, we still need to improve error recovery during precedence parsing. Maybe changing the notion of IncompleteMatch to Match([nodes, SKIPPED("")]) would be enough to leverage existing recovery infra, I can investigate that in a follow-up. |
|
Changing to draft as we might need to address the interaction with precedence parsing before landing this. |
Based on #593. Implements a dedicated `SeparatedHelper` (no state, can change to a free function) that implements recovering from partial matches in separated lists. There are 3 ways we can recover: 1. a separatee matches but unexpected tokens follow before a separator 2. a separatee is partially matched 3. a separatee can't be parsed Ideally we should recover in all three cases, however because the the trailing separator is not mandatory (nor legal), cases 1. and 3. are tricky. In case 2 The separated-by lists are not always in a delimited group (e.g. `IdentifierPath`) and so there's no sure way to bound the recovery, as the unexpected tokens (i.e. not a separator) can be already part of the outer parse. For instance, `0.0.0 || 0.0.0` (case 1) or `0.0. || 0.0.0` (case 3) might attempt to recover by skipping the ` || 0` part. Until we constrain the error recovery with a la FOLLOW for the recovering parser, we can't reliably recover errors for now in these cases.
|
I'll close this to not pollute the PRs; I'll come back to this once we revisit ER at some point. |
Parsing some input in when the first element is optional shouldn't be accepted as empty.