Left recursion fix #75
Conversation
It doesn't seem to be used anywhere, and it's broken (walk_node vs walk_Node). PreOrderWalker does the same thing
Didn't test it properly but it seems to work. I'm not exactly happy with some of the code (mainly how it deals with RuleRef).
The real solution would be to resolve RuleRef beforehand and replace them with the correct Rule instance, but that would mess with code generation
|
Wow! I'll take a look at this ASAP! |
|
Travis is still complaining but I don't know how to get it to shut up about those warnings, so I'll leave it like this for now |
|
Hi @Victorious3 This branch has the mods required to let type checking and unit tests pass: https://github.com/neogeny/TatSu/tree/Victorious3-left_recursion_fix The two skipped left-recursion tests are for valid grammars, and they point to defects in either the implementation of left-recursion, or the algorithm. There are other left recursive grammars in the TatSu issues that also fail, and should probably be part of the unit tests. I'll take another look at all this in the next few days. |
The last implementation was advancing the input way too much, this resets it after every rule invokation I can get the tests to pass by stripping all whitespace, something's off with that.
One of the grammars was actually broken (not PEG)
| @@ -310,8 +307,8 @@ def test_left_recursion_bug(self, trace=False): | |||
| start = expression $ ; | |||
|
|
|||
| expression = | |||
| | paren_expression | |||
There was a problem hiding this comment.
It doesn't consider minus_expression a valid alternative after it successfully parsed a paren_expression,
therefore (3 - 2) - 1 was correctly failing.
|
Those tests literally took forever without memoization, I better get to fixing that ^^ |
This still discards many useful memos but for now this is the best I can do
…owever, the examples are non trivial and hard to understand so I don't think these cases are common occurance.
|
@apalala This is pretty much done apart from cosmetic changes, and hopefully good enough to let me continue my project. |
|
I couldn't come up with a test case for While doing this I also discovered that
So yea, that's it from my side unless you have anything I should change to get it merged. |
I don't think @tatsumasu can accept any other parameters. It's still a bit ugly but better than before. @leftrec now needs to be applied before @tatsumasu
|
The bootstrap test doesn't include anything related to left recursion, it would be a good idea to make sure that the tests are run twice, once directly and once using the generated parser |
|
It's still pretty conservative at doing memos during left recursion but it should be solid at least. I ran my project using it and went down from 37s on parsing 1000 lines of |
|
I didn't have any problems with this over the course of several months, from my perspective its ready to merge? |
|
I'm sorry I haven't had the time to review this in detail, @Victorious3 . You're in charge now. If you think it's ready, then it is. I'd just check that all the non-leftrec unit tests pass. I see some merge conflicts, but I'm sure you're aware of them. Do the docs need any updates? |
|
There's one issue with how the unittests run currently. There are significant differences between how the parser behaves when running from the generated code and when running in immediate. Tatsu's self test doesn't cover all the features that have been added, so for completeness all the other unit tests should run twice. The docs don't need changes, it's running "as advertised" now ^^ |
Fixes #57, fixes #69 see discussion on there for context.
Now fixes #27 as well
This resolves left recursion when the grammar is created instead of figuring it out on the fly. For that it runs some code in
leftrec.pyto detect left recursive cycles and marks one rule as left recursive to break the cycle. For the code generation this information is saved with a new annotation@leftrecthat can be used together with@tatsumasu.Furthermore, the annotation
@nomemois used to block memoization on rules that are part of a left recursive cycle.TODO:
->) and Cut (~) interacts with left recursionThis still doesn't cover all cases, but we are getting closer. The PR adds new failing test cases. (skipped for now)