From 95184762a9435a595b7f20c0a78bc9b1ae583beb Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Fri, 5 Nov 2021 18:54:34 -1000 Subject: [PATCH 1/4] Fix issue 263 --- .../src/main/scala/cats/parse/Parser.scala | 70 ++++++++++++++----- .../test/scala/cats/parse/ParserTest.scala | 50 +++++++------ 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/core/shared/src/main/scala/cats/parse/Parser.scala b/core/shared/src/main/scala/cats/parse/Parser.scala index 6b5fe3a3..51e9ffbd 100644 --- a/core/shared/src/main/scala/cats/parse/Parser.scala +++ b/core/shared/src/main/scala/cats/parse/Parser.scala @@ -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 @@ -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 @@ -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 */ @@ -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) } @@ -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 @@ -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 @@ -2202,6 +2223,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] { @@ -2210,6 +2233,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(_) => false + case _ => true + } + if (fs1.isEmpty) Chain.one(Expectation.Fail(offset)) + else fs1 } final def oneOf[A](all: Array[Parser0[A]], state: State): A = { @@ -2228,7 +2266,7 @@ object Parser { // we failed to parse, but didn't consume input // is unchanged we continue // else we stop - errs = for { e1 <- errs; e2 <- err } yield e1 ++ e2 + errs = for { e1 <- errs; e2 <- err } yield filterFails(offset, e1 ++ e2) state.error = null idx = idx + 1 } diff --git a/core/shared/src/test/scala/cats/parse/ParserTest.scala b/core/shared/src/test/scala/cats/parse/ParserTest.scala index 779b67bf..16ba3b3a 100644 --- a/core/shared/src/test/scala/cats/parse/ParserTest.scala +++ b/core/shared/src/test/scala/cats/parse/ParserTest.scala @@ -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)) => @@ -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(_) => 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) => @@ -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)) } } From a7fce89fd771c18c873c456a7b43e09556c87b13 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Fri, 5 Nov 2021 19:03:35 -1000 Subject: [PATCH 2/4] make a small fix --- core/shared/src/main/scala/cats/parse/Parser.scala | 2 +- core/shared/src/test/scala/cats/parse/ParserTest.scala | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/shared/src/main/scala/cats/parse/Parser.scala b/core/shared/src/main/scala/cats/parse/Parser.scala index 51e9ffbd..c9c203c8 100644 --- a/core/shared/src/main/scala/cats/parse/Parser.scala +++ b/core/shared/src/main/scala/cats/parse/Parser.scala @@ -2243,7 +2243,7 @@ object Parser { */ final def filterFails(offset: Int, fs: Chain[Expectation]): Chain[Expectation] = { val fs1 = fs.filter { - case Expectation.Fail(_) => false + case Expectation.Fail(o) if o == offset => false case _ => true } if (fs1.isEmpty) Chain.one(Expectation.Fail(offset)) diff --git a/core/shared/src/test/scala/cats/parse/ParserTest.scala b/core/shared/src/test/scala/cats/parse/ParserTest.scala index 16ba3b3a..871ca87e 100644 --- a/core/shared/src/test/scala/cats/parse/ParserTest.scala +++ b/core/shared/src/test/scala/cats/parse/ParserTest.scala @@ -826,7 +826,7 @@ class ParserTest extends munit.ScalaCheckSuite { if (err1.failedAtOffset == 0) { val errs = err.expected ::: err1.expected NonEmptyList.fromList(errs.filter { - case Parser.Expectation.Fail(_) => false + case Parser.Expectation.Fail(0) => false case _ => true }) match { case None => From ef2608cb01203caacf414f426abefe0934967d21 Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Fri, 5 Nov 2021 19:13:46 -1000 Subject: [PATCH 3/4] move filterFails to the end of oneOf --- core/shared/src/main/scala/cats/parse/Parser.scala | 4 ++-- core/shared/src/test/scala/cats/parse/ParserTest.scala | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/core/shared/src/main/scala/cats/parse/Parser.scala b/core/shared/src/main/scala/cats/parse/Parser.scala index c9c203c8..876f9a40 100644 --- a/core/shared/src/main/scala/cats/parse/Parser.scala +++ b/core/shared/src/main/scala/cats/parse/Parser.scala @@ -2266,14 +2266,14 @@ object Parser { // we failed to parse, but didn't consume input // is unchanged we continue // else we stop - errs = for { e1 <- errs; e2 <- err } yield filterFails(offset, e1 ++ e2) + errs = for { e1 <- errs; e2 <- err } yield e1 ++ e2 state.error = null idx = idx + 1 } } // 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] } diff --git a/core/shared/src/test/scala/cats/parse/ParserTest.scala b/core/shared/src/test/scala/cats/parse/ParserTest.scala index 871ca87e..5825c9ce 100644 --- a/core/shared/src/test/scala/cats/parse/ParserTest.scala +++ b/core/shared/src/test/scala/cats/parse/ParserTest.scala @@ -2112,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 { From a9a1eeded7ccccdba0e5dbaad57a71948e2e03fa Mon Sep 17 00:00:00 2001 From: Patrick Oscar Boykin Date: Fri, 5 Nov 2021 19:16:43 -1000 Subject: [PATCH 4/4] format tests --- .../test/scala/cats/parse/ParserTest.scala | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/core/shared/src/test/scala/cats/parse/ParserTest.scala b/core/shared/src/test/scala/cats/parse/ParserTest.scala index 5825c9ce..aa964b7f 100644 --- a/core/shared/src/test/scala/cats/parse/ParserTest.scala +++ b/core/shared/src/test/scala/cats/parse/ParserTest.scala @@ -602,11 +602,11 @@ class ParserTest extends munit.ScalaCheckSuite { .withMinSuccessfulTests(tests) .withMaxDiscardRatio(10) - //override val scalaCheckInitialSeed = - // past regressions that we can check periodically - //"bUQvEfxFZ73bxtVjauK8tJDrEKOFbxUfk6WrGiy3bkH=" - //"PPsKExr4HRlyCXkMrC6Rki5u59V88vwSeVTiGWJFS3G=" - //"Ova1uT18mkE4uTX4RdgQza6z70fxyv6micl4hIZvywP=" + // 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 { @@ -829,11 +829,11 @@ class ParserTest extends munit.ScalaCheckSuite { 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)) - } + 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