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

Fix issue 263 #296

Merged
merged 4 commits into from
Nov 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 54 additions & 16 deletions core/shared/src/main/scala/cats/parse/Parser.scala
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
Comment on lines +1286 to +1287
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit 🎨

Suggested change
case f @ Impl.Fail() => f.widen
case f @ Impl.FailWith(_) => f.widen
case f: Impl.Fail[A] => f.widen
case f: Impl.FailWith[A] => f.widen

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few more cases below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you like this change? I prefer the opposite. I think the compile to the same code don't they?

I don't like general reflective matching and to a reviewer the : style might be that, so they have to read a bit carefully.

But I'm interested to know your view on it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got me curious as well because my main motivation for this was based on the fact that this generated different bytecode before. However, after a quick check with this use case, I remembered things badly and they do in fact generate the same bytecode. That said, I'm totally ok with it 😄

You might be right on it needing careful reading but the current one imo has a slight "disadvantage" of keeping track of the number of args. It's not worth changing because of this.

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 @@ -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] {
Expand All @@ -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(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 @@ -2235,7 +2273,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
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