remove lastNoSuccessVar

[svalaskevicius]

lastNoSuccessVar is introducing a non referentially transparent, hard to debug source
of errors where parsing Failures are remembered and, even if they are covered by subsequent
combinators, such as |, still replaces correct Failures at the end of phrase parsing with them.

This is confusing from two perspectives:

• how is that error that should have been covered not fixed by the |?
• what is the real parsing error that I should focus to fix in my input?

a small example:

let's assume we are creating a parser to parse (IF condition THEN 1 END)*.
let condition be TRUE|FALSE.

when given input IF FALSE THEN 1 IF TRUE THEN 1 END (note the missing END), the parser would have returned:
TRUE expected, FALSE given.

Which is obviously not correct, as FALSE is a valid option described in the parser, and the correct error is that the END token is missing.

This PR replaces issue #107 .

[SethTisue]

perhaps of interest to @mrerrormessage, since this might change some NetLogo error messages

[mrerrormessage]

Thanks for the heads-up @SethTisue. It looks like we only use phrase in one place, which contains a note saying that we use it to find the longest possible parse to report errors on. This change looks like it would probably break that capability, so we'll hold off on upgrading until we have a solution in place.

[svalaskevicius]

@mrerrormessage unless I misunderstood your comment - the parsed length should not be affected by this change, only the error it gives on failure to parse (instead of remembering the non-error errors, like in the TRUE|FALSE scenario above, it would fail with either the last parser failure or the "not all input consumed" if the given parser reports success but there is some input left)

[mrerrormessage]

@svalaskevicius , thanks for the clarification - I misunderstood the change. I wouldn't expect this change to affect us in that case.

[svalaskevicius]

@gourlaysama why?

[gourlaysama]

@svalaskevicius sorry, GitHub automatically closed the PR when the base branch was renamed (master to 1.1.x).

I've reopened it (and this should make it into 1.1.1)

[svalaskevicius]

ah, thanks :) was just surprised a bit as there was no explanation

would be good though if anyone could review it, and decide whether it's worth to merge it or better to close - as it has been hanging here for a while now :)

I do remember having some frustrations when debugging the issues caused by this :D

[SethTisue]

needs a rebase now

[SethTisue]

@svalaskevicius you still around...?

[svalaskevicius]

Hi. I am, will this get merged once rebased? :)

[SethTisue]

@svalaskevicius I'm in favor of merging it, and from @gourlaysama's last comment, it sounds like he is too

[svalaskevicius]

Awesome, I'll rebase it when I have a minute (hopefully not too many conflicts) and will ping here again :)

[svalaskevicius]

@SethTisue it's rebased now :)

[liskin]

I think you can remove the entire test file now. Its only purpose was testing that lastNoSuccessVar doesn't leak a circular reference. See #69

[SethTisue]

sure, want to PR it?

[liskin]

Unfortunately not, it's been a few years since I last used this. Life has taken me away from Scala... I hoped the PR's author (@svalaskevicius) might wish to drop it.