Fail ignores its argument and uses "fail"

[reid-spencer]

The Fail operator appears to ignore its documentation and always produces a failure message of "fail". The documentation shows this example:

def failWithLabel[$: P] = P( Fail("custom fail msg") )
val Parsed.Failure(_, 0, extra) = parse("asdad", failWithLabel(_))
assert(extra.trace().longMsg == """Expected failWithLabel:1:1 / fail:1:1, found "asdad"""")

I believe the intent of using Fail within failWithLabel, based on its name, is to provide a new failure label. However, the assertion on the last line shows the issue. I believe it should say:

assert(extra.trace().longMsg == """Expected failWithLabel:1:1 / custom fail message:1:1, found "asdad"""")

If you look at this line of code you can see the mistake:

/**
   * No-op parser with a custom error message that always fails, consuming zero characters
   */
  def Fail(msg: String)(implicit ctx: P[_]): P[Nothing] = {
    val res = ctx.freshFailure()
    if (ctx.verboseFailures) ctx.reportTerminalMsg(ctx.index, () => "fail")   <===== "fail" should be msg
    res
  }

Changing "fail" (static message) to msg will make use of the message and provide much more significant value in reporting custom error messages in fastparse.

[reid-spencer]

@lihaoyi - Locally, running the tests succeed. In the CI, they fail because of the return code (1 instead of 0). The only difference I can see is hardware and the JVM is 17 not 8. Do you have a "must pass all tests in CI" policy before merging?

[lihaoyi]

Yes, we want CI to be green before merging. The last CI run on master was green, so I would expect any PRs to be green as well

[reid-spencer]

Yes, we want CI to be green before merging.

Good, I concur.

The last CI run on master was green, so I would expect any PRs to be green as well

I've merged nothing to main, yet. See PR #297 and its various failures, if you're interested.

I'll continue to look for solutions.