skipSpaces always commits to branch even if no spaces are consumed

[JordanMartinez]

Describe the bug

skipSpaces always commits to its path, even if it doesn't consume any spaces. This occurs because it uses the consumeWith function.

To Reproduce

See https://try.purescript.org/?gist=a37247868f0f7aaf3244f8c07fc304f6

In the line 4, no spaces are consumed. So, the (singleLineComment <|> newline) parse shouldn't occur and we should instead move to the next part of the parser string "@" *> string "value".

Expected behavior

skipSpaces should only commit to its path if it actually consumes content.

Additional context
Add any other context about the problem here.

[natefaubion]

I'm not sure I agree with this. There are cases where it makes sense to commit to a branch even when the input stream slice is unchanged, such as with eof or string "". Is this an unintended regression, or a question of semantics?

[natefaubion]

However, if many (string " ") behaves differently, then I think one could argue that skipSpaces is just an optimization of that parser, and should not count as consumed.

[JordanMartinez]

Probably one of semantics. I'm not sure if the current behavior is different than the previous one.

[jamesdbrock]

Yeah, both of these parsers succeed for both of these inputs, but have different consumed states afterwards if the input has no spaces.

Input	skipSpaces	many (string " ")
" "	consumed	consumed
"x"	consumed	not consumed

I think the prior version of skipSpaces had the same semantics as many (string " ").

purescript-parsing/src/Text/Parsing/Parser/String.purs

Lines 149 to 151 in 24d6693

	-- | Skip whitespace characters and throw them away. Always succeeds.
	skipSpaces :: forall m. Monad m => ParserT String m Unit
	skipSpaces = skipMany (satisfyCodePoint isSpace)

[JordanMartinez]

So, which should be the correction version? If the current one (where "x" results in a consumed state), then I think its docs should be updated to state that.

[natefaubion]

I think that it should have the semantics similar to many (string " "). The issue is that consumeWith's type is too restrictive. Currently the callback returns an Either, but really what you would want is something like:

data Commit a
  = Fail String
  | Skip a
  | Commit a String String

Which is Either String (Tuple a (Maybe { consumed :: String, remainder String })). This reflects the possibility that you may want to continue with the current parser but not consume anything.

If we don't want to change the type, which I think makes sense for now, then I would make consumeWith only set the consume flag when the consumed string is non empty as suggested. eof does not use consumeWith, so I think that's ok.