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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ New features:

Bugfixes:
- fix spago package sets' upstream url
- make `sepEndBy` stack safe

Other improvements:

Expand Down
8 changes: 3 additions & 5 deletions src/StringParser/Combinators.purs
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,9 @@ sepEndBy p sep = (sepEndBy1 p sep <#> NEL.toList) <|> (sep $> Nil) <|> pure Nil
sepEndBy1 :: forall a sep. Parser a -> Parser sep -> Parser (NonEmptyList a)
sepEndBy1 p sep = do
a <- p
( do
_ <- sep
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.

_ <- optional sep
pure (cons' a as)

-- | Parse zero or more separated values, ending with a separator.
endBy :: forall a sep. Parser a -> Parser sep -> Parser (List a)
Expand Down
4 changes: 4 additions & 0 deletions test/Main.purs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Effect.Console (log)
import Test.CodePoints (testCodePoints)
import Test.CodeUnits (testCodeUnits)
import Test.BasicSpecs (runTestCases)
import Test.StackSafeCombinators (testStackSafeCombinators)

main :: Effect Unit
main = do
Expand All @@ -18,3 +19,6 @@ main = do

log "\n\nTesting CodePoint parsing\n"
testCodePoints

log "\n\nTesting stack safety of combinators\n"
testStackSafeCombinators
19 changes: 19 additions & 0 deletions test/StackSafeCombinators.purs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module Test.StackSafeCombinators where

import Prelude

import Data.Array (replicate)
import Data.Either (isRight)
import Data.String.Common as SC
import Effect (Effect)
import Effect.Class.Console (log)
import StringParser (Parser, char, runParser, sepEndBy)
import Test.Assert (assert)

canParse :: forall a. Parser a -> String -> Boolean
canParse p input = isRight $ runParser p input

testStackSafeCombinators :: Effect Unit
testStackSafeCombinators = do
log "Running overflow tests"
assert $ canParse (sepEndBy (char 'a') (char ';')) (SC.joinWith ";" $ replicate 100000 "a")