Skip to content

Commit

Permalink
Fix issue 263 (#296)
Browse files Browse the repository at this point in the history 
* Fix issue 263

* make a small fix

* move filterFails to the end of oneOf

* format tests
  • Loading branch information
@johnynek
johnynek committed Nov 11, 2021
1 parent a0ec067 commit 135a9a3
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 36 deletions.
70 changes: 54 additions & 16 deletions core/shared/src/main/scala/cats/parse/Parser.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1282,12 +1282,19 @@ object Parser {
/** product with the first argument being a Parser
*/
def product10[A, B](first: Parser[A], second: Parser0[B]): Parser[(A, B)] =
Impl.Prod(first, second)
first match {
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case _ => Impl.Prod(first, second)
}

/** product with the second argument being a Parser
*/
def product01[A, B](first: Parser0[A], second: Parser[B]): Parser[(A, B)] =
Impl.Prod(first, second)
first match {
case p1: Parser[A] => product10(p1, second)
case _ => Impl.Prod(first, second)
}

/** softProduct, a variant of product A soft product backtracks if the first succeeds and the
* second is an epsilon-failure. By contrast product will be a failure in that case
Expand All @@ -1312,7 +1319,11 @@ object Parser {
* see @Parser.soft
*/
def softProduct10[A, B](first: Parser[A], second: Parser0[B]): Parser[(A, B)] =
Impl.SoftProd(first, second)
first match {
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case _ => Impl.SoftProd(first, second)
}

/** softProduct with the second argument being a Parser A soft product backtracks if the first
* succeeds and the second is an epsilon-failure. By contrast product will be a failure in that
Expand All @@ -1321,7 +1332,11 @@ object Parser {
* see @Parser.soft
*/
def softProduct01[A, B](first: Parser0[A], second: Parser[B]): Parser[(A, B)] =
Impl.SoftProd(first, second)
first match {
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case _ => Impl.SoftProd(first, second)
}

/** transform a Parser0 result
*/
Expand All @@ -1348,10 +1363,8 @@ object Parser {
case _ => AndThen(f0).andThen(fn)
}
Impl.Map(p0, f1)
case Impl.Fail() | Impl.FailWith(_) =>
// these are really Parser[Nothing{
// but scala can't see that, so we cast
p.asInstanceOf[Parser[B]]
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case _ => Impl.Map(p, fn)
}

Expand Down Expand Up @@ -1382,21 +1395,31 @@ object Parser {
* to optimization
*/
def flatMap0[A, B](pa: Parser0[A])(fn: A => Parser0[B]): Parser0[B] =
Impl.FlatMap0(pa, fn)
pa match {
case p: Parser[A] => flatMap10(p)(fn)
case _ => Impl.FlatMap0(pa, fn)
}

/** Standard monadic flatMap where you start with a Parser Avoid this function if possible. If you
* can instead use product, ~, *>, or <* use that. flatMap always has to allocate a parser, and
* the parser is less amenable to optimization
*/
def flatMap10[A, B](pa: Parser[A])(fn: A => Parser0[B]): Parser[B] =
Impl.FlatMap(pa, fn)
pa match {
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case _ => Impl.FlatMap(pa, fn)
}

/** Standard monadic flatMap where you end with a Parser Avoid this function if possible. If you
* can instead use product, ~, *>, or <* use that. flatMap always has to allocate a parser, and
* the parser is less amenable to optimization
*/
def flatMap01[A, B](pa: Parser0[A])(fn: A => Parser[B]): Parser[B] =
Impl.FlatMap(pa, fn)
pa match {
case p1: Parser[A] => flatMap10(p1)(fn)
case _ => Impl.FlatMap(pa, fn)
}

/** tail recursive monadic flatMaps This is a rarely used function, but needed to implement
* cats.FlatMap Avoid this function if possible. If you can instead use product, ~, *>, or <* use
Expand Down Expand Up @@ -1588,10 +1611,8 @@ object Parser {
case v @ Impl.Void(_) => v
case _ =>
Impl.unmap(pa) match {
case f @ (Impl.Fail() | Impl.FailWith(_)) =>
// these are really Parser[Nothing]
// but scala can't see that, so we cast
f.asInstanceOf[Parser[Unit]]
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case p: Impl.Str => p
case p: Impl.StringIn => p
case p: Impl.IgnoreCase => p
Expand Down Expand Up @@ -2208,6 +2229,8 @@ object Parser {
state.error = Eval.later(Chain.one(Expectation.Fail(offset)))
null.asInstanceOf[A]
}

def widen[B]: Parser[B] = this.asInstanceOf[Parser[B]]
}

case class FailWith[A](message: String) extends Parser[A] {
Expand All @@ -2216,6 +2239,21 @@ object Parser {
state.error = Eval.later(Chain.one(Expectation.FailWith(offset, message)))
null.asInstanceOf[A]
}

def widen[B]: Parser[B] = this.asInstanceOf[Parser[B]]
}

/*
* We don't want to include empty Fail messages inside of a oneOf
* since they are the zeros of the orElse operation
*/
final def filterFails(offset: Int, fs: Chain[Expectation]): Chain[Expectation] = {
val fs1 = fs.filter {
case Expectation.Fail(o) if o == offset => false
case _ => true
}
if (fs1.isEmpty) Chain.one(Expectation.Fail(offset))
else fs1
}

final def oneOf[A](all: Array[Parser0[A]], state: State): A = {
Expand All @@ -2241,7 +2279,7 @@ object Parser {
}
// if we got here, all of them failed, but we
// never advanced the offset
state.error = errs
state.error = errs.map(filterFails(offset, _))
null.asInstanceOf[A]
}

Expand Down
59 changes: 39 additions & 20 deletions core/shared/src/test/scala/cats/parse/ParserTest.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,12 @@ class ParserTest extends munit.ScalaCheckSuite {
.withMinSuccessfulTests(tests)
.withMaxDiscardRatio(10)

// override val scalaCheckInitialSeed =
// past regressions that we can check periodically
// "bUQvEfxFZ73bxtVjauK8tJDrEKOFbxUfk6WrGiy3bkH="
// "PPsKExr4HRlyCXkMrC6Rki5u59V88vwSeVTiGWJFS3G="
// "Ova1uT18mkE4uTX4RdgQza6z70fxyv6micl4hIZvywP="

def parseTest[A: Eq](p: Parser0[A], str: String, a: A) =
p.parse(str) match {
case Right((_, res)) =>
Expand Down Expand Up @@ -811,24 +817,28 @@ class ParserTest extends munit.ScalaCheckSuite {
}
}

def orElse[A](p1: Parser0[A], p2: Parser0[A], str: String): Either[Parser.Error, (String, A)] = {
if (p1 == Parser.Fail) p2.parse(str)
else if (p2 == Parser.Fail) p1.parse(str)
else
p1.parse(str) match {
case left @ Left(err) =>
if (err.failedAtOffset == 0) {
p2.parse(str)
.leftMap { err1 =>
if (err1.failedAtOffset == 0) {
val errs = err.expected ::: err1.expected
Parser.Error(err1.failedAtOffset, Parser.Expectation.unify(errs))
} else err1
}
} else left
case right => right
}
}
def orElse[A](p1: Parser0[A], p2: Parser0[A], str: String): Either[Parser.Error, (String, A)] =
p1.parse(str) match {
case left @ Left(err) =>
if (err.failedAtOffset == 0) {
p2.parse(str)
.leftMap { err1 =>
if (err1.failedAtOffset == 0) {
val errs = err.expected ::: err1.expected
NonEmptyList.fromList(errs.filter {
case Parser.Expectation.Fail(0) => false
case _ => true
}) match {
case None =>
Parser.Error(0, NonEmptyList(Parser.Expectation.Fail(0), Nil))
case Some(nel) =>
Parser.Error(err1.failedAtOffset, Parser.Expectation.unify(nel))
}
} else err1
}
} else left
case right => right
}

property("oneOf0 composes as expected") {
forAll(ParserGen.gen0, ParserGen.gen0, Arbitrary.arbitrary[String]) { (genP1, genP2, str) =>
Expand Down Expand Up @@ -1429,11 +1439,11 @@ class ParserTest extends munit.ScalaCheckSuite {
property("with1 *> and with1 <* work as expected") {
forAll(ParserGen.gen0, ParserGen.gen, Arbitrary.arbitrary[String]) { (p1, p2, str) =>
val rp1 = p1.fa.with1 *> p2.fa
val rp2 = (p1.fa.with1 ~ p2.fa).map(_._2)
val rp2 = (p1.fa.void.with1 ~ p2.fa).map(_._2)
assertEquals(rp1.parse(str), rp2.parse(str))

val rp3 = p1.fa.with1 <* p2.fa
val rp4 = (p1.fa.with1 ~ p2.fa).map(_._1)
val rp4 = (p1.fa.with1 ~ p2.fa.void).map(_._1)
assertEquals(rp3.parse(str), rp4.parse(str))
}
}
Expand Down Expand Up @@ -2102,6 +2112,15 @@ class ParserTest extends munit.ScalaCheckSuite {
}
}

property("a Parser never succeeds and does not advance") {
forAll(ParserGen.gen, Arbitrary.arbitrary[String]) { (genP, str) =>
genP.fa.parse(str) match {
case Right((rest, _)) => assertNotEquals(rest, str)
case Left(_) => ()
}
}
}

property("select on pure values works as expected") {
forAll { (left: Option[Either[Int, String]], right: Option[Int => String], str: String) =>
val pleft = left match {
Expand Down

0 comments on commit 135a9a3

Please sign in to comment.