Skip to content

Make sepEndBy stack safe #95

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

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

Conversation

hjmtql
Copy link
Contributor

@hjmtql hjmtql commented Aug 28, 2022

I encountered stack overflow when I used sepEndBy for large data.
It seemed to be caused by sepEndBy and sepEndBy1 are calling each other.
So I added a test and fixed sepEndBy1 using stack safe many.

It seems that chainl and chainr should also be fixed.
However I fixed only sepEndBy with this PR because these functions are a little complicated to me.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

as <- sepEndBy p sep
pure (cons' a as)
) <|> pure (NEL.singleton a)
as <- many $ try (sep *> p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, this changes the behavior of the function in case of a parser error in p due to the usage of try. PS I'm currently on vacation and may not be very swift to respond.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you pointed out is correct.
Thanks for the quick review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE

It gets

> runParser (sepEndBy (char 'a') (char ';')) "a;b"
(Right ('a' : Nil))

It seems that b is trashed. I think this is a little problem.

I found this result occurs also in original sebEndBy.
I wonder if it is the intended behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah, I can't think of obvious situations where this would be what you want. In test/BasicSpecs.purs we have some basic specs that say what should be parsable and what shouldn't. Perhaps we should extend these specs to also cover partial successes and assert on the expected remaining unparsed string.

We should also look at what purescript-parsing does in these cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would naievely expect sepEndBy to successfully parse a separated sequence and whenever either the sep or the p fails to just stop parsing and yield what has been parsed so far.

@hjmtql hjmtql marked this pull request as draft August 29, 2022 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants