remove lastNoSuccessVar#108
Conversation
svalaskevicius
force-pushed
the
remove-lastNoSuccessVar
branch
from
c8f9f64 to
f5757ff
Compare
|
perhaps of interest to @mrerrormessage, since this might change some NetLogo error messages |
|
Thanks for the heads-up @SethTisue. It looks like we only use |
|
@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) |
|
@svalaskevicius , thanks for the clarification - I misunderstood the change. I wouldn't expect this change to affect us in that case. |
|
@gourlaysama why? |
|
@svalaskevicius sorry, GitHub automatically closed the PR when the base branch was renamed ( I've reopened it (and this should make it into 1.1.1) |
|
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 |
|
needs a rebase now |
|
@svalaskevicius you still around...? |
|
Hi. I am, will this get merged once rebased? :) |
|
@svalaskevicius I'm in favor of merging it, and from @gourlaysama's last comment, it sounds like he is too |
|
Awesome, I'll rebase it when I have a minute (hopefully not too many conflicts) and will ping here again :) |
`lastNoSuccessVar` is introducing a non referrentially 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.
svalaskevicius
force-pushed
the
remove-lastNoSuccessVar
branch
from
f5757ff to
df4b17b
Compare
|
@SethTisue it's rebased now :) |
| ru.typeOf[Parsers].decl(ru.TermName("lastNoSuccessVar")).asTerm.accessed.asTerm | ||
| mirror.reflect(p).reflectField(lastNoSuccessVarField).get. | ||
| asInstanceOf[DynamicVariable[Option[_]]] | ||
| */ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This change means revival of lastNoSuccessVar(deleted by scala#108). However, in this time, a new variable(`lastFailure` in `Success` class) is immutable(i.e. this variable does not means revival of side effects). That is why, probably, this change does not break referentially transparent.
This change means revival of lastNoSuccessVar(deleted by scala#108). However, in this time, a new variable(`lastFailure` in `Success` class) is immutable(i.e. this variable does not means revival of side effects). That is why, probably, this change does not break referentially transparent.
This change means revival of lastNoSuccessVar(deleted by scala#108). However, in this time, a new variable(`lastFailure` in `Success` class) is immutable(i.e. this variable does not means revival of side effects). That is why, probably, this change does not break referentially transparent.
This change means revival of lastNoSuccessVar(deleted by scala#108). However, in this time, a new variable(`lastFailure` in `Success` class) is immutable(i.e. this variable does not means revival of side effects). That is why, probably, this change does not break referentially transparent.
5 participants
lastNoSuccessVaris introducing a non referentially transparent, hard to debug sourceof 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:
|?a small example:
let's assume we are creating a parser to parse
(IF condition THEN 1 END)*.let
conditionbeTRUE|FALSE.when given input
IF FALSE THEN 1 IF TRUE THEN 1 END(note the missingEND), 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
ENDtoken is missing.This PR replaces issue #107 .