Misleading error message when using optional or repeatedly parser

[ashihaby]

I have tried to parse this string aaa bbb using this parser ident ~ "(?i)AND".r.*. I was expecting to see this error [1.5] failure: string matching regex '(?i)AND' expected but 'b' found, but I received this error [1.5] failure: string matching regex '\z' expected but 'b' found.
After Investigating in this bug, I found that this problem happens only when using * to repeat the same parser.

[inkytonik]

I'm not sure that I see what the bug is here. It seems to me that your fragment "(?i)AND".r.* is succeeding by matching an empty sequence of input characters at the position of the first 'b', since a '*' parser can always succeed with no repetitions. Why would you expect to see a failure message for that parser?

The message you see is because you've (presumably) wrapped the whole parser using phrase so the phrase parser fails because you haven't consumed all of the input.

[ashihaby]

If I will parse a string like that -start using opt(ident), I will expect a reasonable error like [1.1] failure: string matching regex '\p{javaJavaIdentifierStart}\p{javaJavaIdentifierPart}*' expected but '-' found which can help me to detect what is the problem; not failure: string matching regex '\z' expected but '-' found.
I will not be able to detect any failure in case of the optional or repeatedly parser as these types of parsers are always succeed. I don't know what is the benefit of doing that super.phrase(p <~ opt("""\z""".r)) although phrase already make sure that the input is atEnd before returning success and returns lastNoSuccessVar if exist or Failure("end of input expected", in1).

def phrase[T](p: Parser[T]) = new Parser[T] {
    def apply(in: Input) = lastNoSuccessVar.withValue(None) {
      p(in) match {
      case s @ Success(out, in1) =>
        if (in1.atEnd)
          s
        else
          lastNoSuccessVar.value filterNot { _.next.pos < in1.pos } getOrElse Failure("end of input expected", in1)
        case ns => lastNoSuccessVar.value.getOrElse(ns)
      }
    }
  }

I see doing that will make the errors reasonable and helpful, Is that will break anything?

[inkytonik]

But you will only get an error if something fails. As you say, opt(ident) and * can never fail, so I don't see why there is an expectation that the error message would somehow involve them. When you parse with phrase (opt (ident)) the only thing that fails on -start is the \z from phrase, which is what the message says.

[ashihaby]

nope with phrase (opt (ident)) I will receive the error which I expect, because the ident parser will be applied first on -start and will fail with this error failure: string matching regex '\p{javaJavaIdentifierStart}\p{javaJavaIdentifierPart}*' and when any failure happen the value of lastNoSuccessVar will be changed to this error. This is the opt parser

def opt[T](p: => Parser[T]): Parser[Option[T]] =
    p ^^ (x => Some(x)) | success(None)

Although opt always succeed, the failure of the first parser happened and lastNoSuccessVar holds this failure which I need.

[inkytonik]

Yes, of course, you're right. My point from earlier remains, however. I don't think there is a bug in the repetition combinator * as this issue asserts, certainly not one that would imply changing it to use rep1 which changes its meaning.

[ashihaby]

Yeah you are right about that I shouldn't change * to use rep1, I have fixed the pull request after you mention that and added more test cases to make sure that my modification didn't break that.