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.

Sign up for GitHub

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

Jump to bottom

Add withContext to improve error reporting #194

Merged
merged 2 commits into from
Apr 3, 2021
Merged

Add withContext to improve error reporting #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15cccdf
Add withContext to improve error reporting
johnynek Apr 2, 2021
3b906eb
remove unneeded match
johnynek Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 94 additions & 7 deletions core/shared/src/main/scala/cats/parse/Parser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,12 @@ sealed abstract class Parser0[+A] {
def surroundedBy(b: Parser0[Any]): Parser0[A] =
between(b, b)

/** Add a string context to any Errors on parsing
* this is useful for debugging failing parsers.
*/
def withContext(str: String): Parser0[A] =
Parser.withContext0(this, str)

/** Internal (mutable) parsing method.
*
* This method should only be called internally by parser instances.
Expand Down Expand Up @@ -663,6 +669,13 @@ sealed abstract class Parser[+A] extends Parser0[A] {
*/
override def soft: Parser.Soft[A] =
new Parser.Soft(this)

/** This method overrides `Parser0#withContext` to refine the return type.
* add a string context to any Errors on parsing
* this is useful for debugging failing parsers.
*/
override def withContext(str: String): Parser[A] =
Parser.withContext(this, str)
}

object Parser {
Expand All @@ -672,6 +685,16 @@ object Parser {
*/
sealed abstract class Expectation {
def offset: Int

/** This is a reverse order stack (most recent context first)
* of this parsing error
*/
def context: List[String] =
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this tailrec? It'd probably require a reverse at the end but maybe it's worth pursuing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to me if your errors are so nested you blow the stack here you will have other issues. I don't think it is worth a cost to retain the ability of having 50k deep nested contexts.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Fair. Let's leave it ;)

this match {
case Expectation.WithContext(ctx, inner) =>
ctx :: inner.context
case _ => Nil
}
}

object Expectation {
Expand All @@ -685,6 +708,9 @@ object Parser {
// this is the result of oneOf0(Nil) at a given location
case class Fail(offset: Int) extends Expectation
case class FailWith(offset: Int, message: String) extends Expectation
case class WithContext(contextStr: String, expect: Expectation) extends Expectation {
def offset: Int = expect.offset
}

implicit val catsOrderExpectation: Order[Expectation] =
new Order[Expectation] {
Expand Down Expand Up @@ -724,16 +750,21 @@ object Parser {
if (c1 == 0) Integer.compare(a1, a2)
else c1
case (Length(_, _, _), _) => -1
case (ExpectedFailureAt(_, _), Fail(_)) => -1
case (ExpectedFailureAt(_, _), FailWith(_, _)) => -1
case (ExpectedFailureAt(_, _), Fail(_) | FailWith(_, _) | WithContext(_, _)) => -1
case (ExpectedFailureAt(_, m1), ExpectedFailureAt(_, m2)) =>
m1.compare(m2)
case (ExpectedFailureAt(_, _), _) => 1
case (Fail(_), FailWith(_, _)) => -1
case (Fail(_), FailWith(_, _) | WithContext(_, _)) => -1
case (Fail(_), _) => 1
case (FailWith(_, _), WithContext(_, _)) => -1
case (FailWith(_, s1), FailWith(_, s2)) =>
s1.compare(s2)
case (FailWith(_, _), _) => 1
case (WithContext(lctx, lexp), WithContext(rctx, rexp)) =>
val c = compare(lexp, rexp)
if (c != 0) c
else lctx.compareTo(rctx)
case (WithContext(_, _), _) => 1
}
}
}
Expand All @@ -760,22 +791,36 @@ object Parser {
Some(OneOfStr(ooss.head.offset, ssb.result().toList))
}

@annotation.tailrec
private def stripContext(ex: Expectation): Expectation =
ex match {
case WithContext(_, inner) => stripContext(inner)
case _ => ex
}

@annotation.tailrec
private def addContext(revCtx: List[String], ex: Expectation): Expectation =
revCtx match {
case Nil => ex
case h :: tail => addContext(tail, WithContext(h, ex))
}

/** Sort, dedup and unify ranges for the errors accumulated
* This is called just before finally returning an error in Parser.parse
*/
def unify(errors: NonEmptyList[Expectation]): NonEmptyList[Expectation] = {
val result = errors
.groupBy(_.offset)
.groupBy { ex => (ex.offset, ex.context) }
.iterator
.flatMap { case (_, list) =>
.flatMap { case ((_, ctx), list) =>
val rm = ListBuffer.empty[InRange]
val om = ListBuffer.empty[OneOfStr]
val fails = ListBuffer.empty[Fail]
val others = ListBuffer.empty[Expectation]

var items = list.toList
while (items.nonEmpty) {
items.head match {
stripContext(items.head) match {
case ir: InRange => rm += ir
case os: OneOfStr => om += os
case fail: Fail => fails += fail
Expand All @@ -790,7 +835,11 @@ object Parser {
val oossMerge = mergeOneOfStr(om.toList)

val errors = others.toList reverse_::: (oossMerge ++: rangeMerge)
if (errors.isEmpty) fails.toList else errors
val finals = if (errors.isEmpty) fails.toList else errors
if (ctx.nonEmpty) {
val revCtx = ctx.reverse
finals.map(addContext(revCtx, _))
} else finals
}
.toList

Expand Down Expand Up @@ -1732,6 +1781,16 @@ object Parser {
}
}

/** Add a context string to Errors to aid debugging
*/
def withContext0[A](p0: Parser0[A], ctx: String): Parser0[A] =
Impl.WithContextP0(ctx, p0)

/** Add a context string to Errors to aid debugging
*/
def withContext[A](p: Parser[A], ctx: String): Parser[A] =
Impl.WithContextP(ctx, p)

implicit val catsInstancesParser
: FlatMap[Parser] with Defer[Parser] with MonoidK[Parser] with FunctorFilter[Parser] =
new FlatMap[Parser] with Defer[Parser] with MonoidK[Parser] with FunctorFilter[Parser] {
Expand Down Expand Up @@ -1877,6 +1936,8 @@ object Parser {
case Map(p, _) => doesBacktrack(p)
case SoftProd0(a, b) => doesBacktrackCheat(a) && doesBacktrack(b)
case SoftProd(a, b) => doesBacktrackCheat(a) && doesBacktrack(b)
case WithContextP(_, p) => doesBacktrack(p)
case WithContextP0(_, p) => doesBacktrack(p)
case _ => false
}

Expand All @@ -1889,6 +1950,8 @@ object Parser {
e == min.toChar.toString
case OneOf(ss) => ss.forall(matchesString)
case OneOf0(ss) => ss.forall(matchesString)
case WithContextP(_, p) => matchesString(p)
case WithContextP0(_, p) => matchesString(p)
case _ => false
}

Expand All @@ -1901,6 +1964,7 @@ object Parser {
case Map0(p, _) => alwaysSucceeds(p)
case SoftProd0(a, b) => alwaysSucceeds(a) && alwaysSucceeds(b)
case Prod0(a, b) => alwaysSucceeds(a) && alwaysSucceeds(b)
case WithContextP0(_, p) => alwaysSucceeds(p)
// by construction we never build a Not(Fail()) since
// it would just be the same as unit
//case Not(Fail() | FailWith(_)) => true
Expand Down Expand Up @@ -1973,6 +2037,7 @@ object Parser {
case Defer0(fn) =>
Defer0(UnmapDefer0(fn))
case Rep0(p, max, _) => Rep0(unmap(p), max, Accumulator0.unitAccumulator0)
case WithContextP0(ctx, p0) => WithContextP0(ctx, unmap0(p0))
case StartParser | EndParser | TailRecM0(_, _) | FlatMap0(_, _) =>
// we can't transform this significantly
pa
Expand Down Expand Up @@ -2055,6 +2120,8 @@ object Parser {
case Defer(fn) =>
Defer(UnmapDefer(fn))
case Rep(p, min, max, _) => Rep(unmap(p), min, max, Accumulator0.unitAccumulator0)
case WithContextP(ctx, p) =>
WithContextP(ctx, unmap(p))
case AnyChar | CharIn(_, _, _) | Str(_) | StringIn(_) | IgnoreCase(_) | Fail() | FailWith(
_
) | Length(_) | TailRecM(_, _) | FlatMap(_, _) =>
Expand Down Expand Up @@ -2772,6 +2839,26 @@ object Parser {
()
}
}

case class WithContextP0[A](context: String, under: Parser0[A]) extends Parser0[A] {
override def parseMut(state: State): A = {
val a = under.parseMut(state)
if (state.error ne null) {
state.error = state.error.map(Expectation.WithContext(context, _))
}
a
}
}

case class WithContextP[A](context: String, under: Parser[A]) extends Parser[A] {
override def parseMut(state: State): A = {
val a = under.parseMut(state)
if (state.error ne null) {
state.error = state.error.map(Expectation.WithContext(context, _))
}
a
}
}
}
}

Expand Down
47 changes: 46 additions & 1 deletion core/shared/src/test/scala/cats/parse/ParserTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,16 @@ object ParserGen {
GenT[Parser0, List[g.A]](g.fa.rep0(min = min, max = max))
}

def withContext0(g: GenT[Parser0]): Gen[GenT[Parser0]] =
Arbitrary.arbitrary[String].map { ctx =>
GenT(Parser.withContext0(g.fa, ctx))(g.cogen)
}

def withContext(g: GenT[Parser]): Gen[GenT[Parser]] =
Arbitrary.arbitrary[String].map { ctx =>
GenT(Parser.withContext(g.fa, ctx))(g.cogen)
}

// this makes an integer >= min, but with power law bias to smaller values
def biasSmall(min: Int): Gen[Int] = {
require(min >= 0, s"biasSmall($min) is invalid")
Expand Down Expand Up @@ -523,7 +533,8 @@ object ParserGen {
(1, flatMapped(rec)),
(1, Gen.zip(rec, rec).flatMap { case (g1, g2) => product0(g1, g2) }),
(1, Gen.zip(rec, rec).flatMap { case (g1, g2) => softProduct0(g1, g2) }),
(1, Gen.zip(rec, rec, pures).flatMap { case (g1, g2, p) => orElse0(g1, g2, p) })
(1, Gen.zip(rec, rec, pures).flatMap { case (g1, g2, p) => orElse0(g1, g2, p) }),
(1, rec.flatMap(withContext0(_)))
)
}

Expand All @@ -548,6 +559,7 @@ object ParserGen {
(1, flatMapped1(gen0, rec)),
(1, tailRecM1(rec)),
(1, Gen.choose(1, 10).map { l => GenT(Parser.length(l)) }),
(1, rec.flatMap(withContext(_))),
(
2,
Gen.frequency(
Expand Down Expand Up @@ -2334,4 +2346,37 @@ class ParserTest extends munit.ScalaCheckSuite {
}
}

property("a context0 added is always at the top") {
forAll(ParserGen.gen0, Arbitrary.arbitrary[List[String]], Arbitrary.arbitrary[String]) {
(genP, ctx, str) =>
ctx.foldLeft(genP.fa)(_.withContext(_)).parse(str) match {
case Left(err) =>
err.expected.toList.foreach { exp =>
val ectx = exp.context
assert(ectx.length >= ctx.length)
exp.context.zip(ctx.reverse).foreach { case (l, r) =>
assertEquals(l, r)
}
}
case _ => ()
}
}
}

property("a context added is always at the top") {
forAll(ParserGen.gen, Arbitrary.arbitrary[List[String]], Arbitrary.arbitrary[String]) {
(genP, ctx, str) =>
ctx.foldLeft(genP.fa)(_.withContext(_)).parse(str) match {
case Left(err) =>
err.expected.toList.foreach { exp =>
val ectx = exp.context
assert(ectx.length >= ctx.length)
exp.context.zip(ctx.reverse).foreach { case (l, r) =>
assertEquals(l, r)
}
}
case _ => ()
}
}
}
}