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

fix: support empty sep for separated_list0 & separated_list1 #1491

Closed
wants to merge 1 commit into from

Conversation

suikammd
Copy link

When separator eats nothing(e.g. opt(tag(","))), separated_list0 & separated_list1 should parse without error. But current implementation returns error when separator eats nothing to prevent infinite loop.
I think the infinite loop check should between loops. If no bytes eaten during the whole loop body, then this will result in infinite loop and should return error.

@Geal Geal added this to the 8.0 milestone Mar 14, 2022
@SaltyKitkat
Copy link

Seems this will fix #1573 and #1691

@epage
Copy link
Contributor

epage commented Nov 14, 2023

If I understand this fix, it changes it from require progress from the sep to requiring progress from either sep or f. Personally, I don't think this is a good idea. While this would allow the caller to choose whether sep or f can be empty, it can also allow both to be empty sometimes and requires either careful reading of the documentation (rarely done) and auditing of the code or exhaustive testing to catch that an infinite loop exists.

A lot of these higher level parsers are relatively trivial (well, barring the 8.0 GAT changes) and people can easily write a parser tailored to their needs. For myself, I only use parsers like this for prototyping but when I'm lining up with a grammar, I use lower level parsers so my code lines up with my BNF grammar.

@gdennie
Copy link
Contributor

gdennie commented Jan 15, 2024

Just spent a while "discovering" this issue.

The documentation does not indicate there is a "must consume" requirement for the separator parser, causing the resulting error to be surprising. As a result, the type signature is misleading. Perhaps we establish a type to indicate the "must consume" constraint.

Now, in considering the expression, separated_list1(space0, element_parser), it is clear that the list is not technically separated by space0 since it may not exist between elements. As such perhaps a more appropriate solution is a another combinator: maybe_separated_by(optional_separator, element_parser).

@Geal
Copy link
Collaborator

Geal commented May 5, 2024

sorry, I did not see this PR when writing c655955, which was roughly equivalent. I will close this PR now

@Geal Geal closed this May 5, 2024
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.

None yet

5 participants