Scala Programming Language
  1. Scala Programming Language
  2. SI-1100

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Scala 2.10.0
    • Component/s: Misc Library
    • Labels:
      None
    • Environment:

      combinator parsing

      Description

      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).

        Activity

        Hide
        Gilles Dubochet added a comment -

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

        Show
        Gilles Dubochet added a comment - Adriaan, may I reassign this issue to you? If you don't like this idea, just reassign it back to scala_reviewer.
        Hide
        Adriaan Moors added a comment -

        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...

        Show
        Adriaan Moors added a comment - 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...
        Hide
        Commit Message Bot added a comment -

        (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 SI-1100, Closes SI-5108, No review.

        Show
        Commit Message Bot added a comment - (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 SI-1100 , Closes SI-5108 , No review.
        Hide
        Commit Message Bot added a comment -

        (extempore in r25889) Test case for SI-1100/SI-5108.

        Show
        Commit Message Bot added a comment - (extempore in r25889 ) Test case for SI-1100 / SI-5108 .

          People

          • Assignee:
            Adriaan Moors
            Reporter:
            Ilya Klyuchnikov
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development