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

Combinator Parsing library: parser generator rep1 doesn't propagate errors #1100

Closed
scabug opened this issue Jul 9, 2008 · 5 comments
Closed
Assignees
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jul 9, 2008

Parser generator

def rep1[T](first: => Parser[T], p: => Parser[T]): Parser[List[T]]

in scala.util.parsing.combinator.Parsers never returns error. So if first or p parser returns error this error is not propagated further.

It is almost obvious from code of this method:

  def rep1[T](first: => Parser[T], p: => Parser[T]): Parser[List[T]] = Parser{ in0 =>
    val xs = new scala.collection.mutable.ListBuffer[T]
    var in = in0

    var res = first(in)

    while(res.successful) {
      xs += res.get
      in = res.next
      res = p(in)
    }

    // assert(res.isInstanceOf[NoSuccess])

    if (!xs.isEmpty) {
      // the next parser should start parsing where p failed, 
      // since `!p(in).successful', the next input to be consumed is `in'
      Success(xs.toList, in)  // TODO: I don't think in == res.next holds
    }
    else {
      Failure(res.asInstanceOf[NoSuccess].msg, in0)
    }
  }

Suppose that after while finished res is an Error. This error is not propagated further - the result of rep1 depends only on emptiness of list xs!

res should be tested. The implementation of this method should be changed to something like this:

  def rep1[T](first: => Parser[T], p: => Parser[T]): Parser[List[T]] = Parser{ in0 =>
    val xs = new scala.collection.mutable.ListBuffer[T]
    var in = in0
    var res = first(in)
    while(res.successful) {
      xs += res.get
      in = res.next
      res = p(in)
    }
    
    res match {
      case e: Error => e
      case _  => if (!xs.isEmpty) Success(xs.toList, in) else Failure(res.asInstanceOf[NoSuccess].msg, in0) 
    }
  }

As a result of incorrect rep1, combinators * and + work also incorrectly (they finally call mentioned method).

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 9, 2008

Imported From: https://issues.scala-lang.org/browse/SI-1100?orig=1
Reporter: Ilya Klyuchnikov (klyuchnikov)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 15, 2008

@dubochet said:
Adriaan, may I reassign this issue to you? If you don't like this idea, just reassign it back to scala_reviewer.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 16, 2008

@adriaanm said:
ouch! sorry... fixed in r15555 (btw: how can I change the status of the ticket from the commit message? I thought "fixed SI-1100" would do the trick...

@scabug scabug closed this May 18, 2011
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 23, 2011

Commit Message Bot (anonymous) said:
(extempore in r25881) Fix for combinator regression.

Propagate Error in repN. I have no time for a test case, I will gladly
take a contribution. References #1100, Closes #5108, No review.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 24, 2011

Commit Message Bot (anonymous) said:
(extempore in r25889) Test case for #1100/#5108.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.