Enhance rule success strategy

[0dminnimda]

Closes #81

~~Also now it does not matter, if a user decides to return None as a node for some rule, now it will not affect anything Also now instead of calling bool on the returning objects (which may be expensive) only a boolean flag will be set~~ This will also help a little with #70

Use func() is not None where suitable and not rely on the boolean value of the returned object this seems to be more reasonable for more broader applications

Now PythonCallMakerVisitor will determine if the value should be checked to be not None or should be checked as a boolean value (previous behavior)

Also read the conversation

[MatthieuDartiailh]

I am not familiar enough with the project history to know if something similar was considered before. @pablogsal would know more but he has been quite busy recently.

[lysnikolaou]

I don't think something like this was considered while initially working on the project. However, we were more focussed on the C generator, making such edge cases in the python generator easier to miss.

To be honest, I'm a bit torn on this. While this seems like something that would work, I'm inclined to say that just checking is not None everywhere seems like an easier way that would also make the generated parsers easier to understand. Returning None from actions to have the rule fail is also more a feature than a bug and consistency with the CPython parser (where we're returning NULL to make rules fail all over the place) is something I'd love to keep going forward.

Not a strong opinion, so I'll let @pablogsal make the final call.

[0dminnimda]

just checking is not None everywhere

Almost, the original bug is that loop0 can return an empty list, wich is evaluates to be False, but it shoud be considered a successful match. This logic is fine for loop1, but not for loop0

So the smallest fix is to handle

• loop1 as func()
• loop0 as func() is not None, or func(), (it doesn't really matter as those funcitons always returns a list)

Returning None from actions to have the rule fail is also more a feature than a bug and consistency with the CPython parser (where we're returning NULL to make rules fail all over the place) is something I'd love to keep going forward.

If I think about it as a feature it does indeed looks to be a neat thing to have, but it's not necessarily lost. This pr introduces more control as to what will be considered a match and having None to be a failed match is not hard Just change 3 lines of code and it's there.

What I like about this implementation is that only the function itself (and not the caller) is responsible for saying is this a match or not

On a side note I haven't had time to investigate what was the problem of a failed CI jobs, when I have the time I will look into that

[0dminnimda]

Ok so the problem is that memorization does not restore value of the _matched

1) One way is to return the value alongside the actual return 2) The other way is to add the code to handle it in the memorizer, but I'd rather not change the code as it is made to handle general memorization and I don't want to ruin it 3) And the third way to go is to handle the responcibility of handling the output value to the caller (as it was done before)

I don't want to touch the 2, although the logic keeping _matched is sync would not really risk breaking the code, but as mentioned it would become less general

1 seems to be a little messy it adds more code to the call (ex. ((l := self._result(self.term()),) and self._matched) and self.term would return tuple[actual_result, matched]), although this is a code generator and the generated code is not meant to be read it's just not nice

3 would grant the least amoung of changing but None would remain to be a invalid match, although given the argument of @lysnikolaou I would consider this to be fine

---

In the end I think that I tried to squish two prs/features into one, and if we really want to get read of the None as a stop value we it should be discussed and then changed in a different pr. In this pr I'd rather concentrate on fixing the actual problem #81

And I think that for now if users want to return None they should either use their own empty value like EmptyValue = object() or wrap it in another class like Wrapper(None) and to get it they use wrapper.value

Also if we want users be able to make empty values that are bool(value) == False, then we should use not bool(self.x()) as match check and use is None, I am gonna ensure it in this pr as well

[0dminnimda]

Ok, tox straight up just does not work, I ran all of the pytests, linting tests, regen-metaparser but tox refures to succeed either here or locally