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
Store terminal failures as TerminalParseFailures #26
Conversation
When attempting to print out a failure message using `failure_reason` for my project `propose`[1], I was encountering the following error: NoMethodError: undefined method `unexpected' for [0, "[a-z]", false]:Array> with backtrace It looks like the problem here is that terminal failures are stored in `@terminal_failures` as tuples, and when you call `terminal_failures` it does a check to see if the first item is a `TerminalParseFailures`, and converts the list if it isn't. However, if you call `terminal_failures` multiple times, it's possible other errors were added, and so you'll have a mixture of `Array` tuples and `TerminalParseFailure`s. The fix is to store them as `TerminalParseFailure`s when the failure is first encountered in `terminal_parse_failure`. It's not overly expensive and allows us to avoid this unnecessary complexity. [1] https://github.com/sds/propose
Looks like
I'm honestly not sure what I'm doing to cause this behavior. FWIW, I think this change makes sense even if this weren't an issue, since it seems a bit odd to store parse errors this way, and this pull request actually reduces complexity/LOC IMHO. If you're still opposed to merging, I'll try to find some time later next week to dig deeper. |
Not opposed, just want to understand so as to avoid fixing what's not broken. I hope that explains the (historical) rationale anyhow. Whatever makes sense now will |
Sorry for the delay in following up on this issue. I've dug around and it's clear that there are multiple invocations of If you look at Does that make sense? It seems like we can avoid the confusion by always storing an actual Interested in hearing your thoughts. |
Thanks for investigating. There is no need to call a method in order to pop terminal_parse_failures (which is needed in semantic predicates and in repetition node types). I changed the code generation to just pop @terminal_parse_failures directly to restore the original side-effect-free intention. |
Great, that solved the problem. Thanks! |
When attempting to print out a failure message using
failure_reason
for my tiny personal project
propose
[1], I was encountering the following error:This started happening after upgrading to
treetop
1.6.0+.It looks like the problem here is that terminal failures are stored in
@terminal_failures
as tuples, and when you callterminal_failures
itdoes a check to see if the first item is a
TerminalParseFailure
, andconverts the list if it isn't. However, if you call
terminal_failures
multiple times, it's possible other errors were added, and so you'll
have a mixture of
Array
tuples andTerminalParseFailure
s.The fix is to store them as
TerminalParseFailure
s when the failure isfirst encountered in
terminal_parse_failure
. It's not overly expensiveand allows us to avoid this unnecessary complexity.
[1] https://github.com/sds/propose