Demand Ply 3.11 -- SIMICS-23703 - #454
Conversation
| @wraps(rule) | ||
| def wrapped(t): | ||
| fixup_emptyprod_lexpos(t) | ||
| return rule(t) |
There was a problem hiding this comment.
You were performance stingy, so I kept the way fixup_emptyprod_lexpos(t) gets called identical to before this change. That said, back when I actually measured the impact of the len(t) == 1 branch being taken a gazillion times, it was minimal, so I'd like to just unify these two into a simple if len(t) <= 1: check. If you're fine with it.
... Honestly I'd prefer to skip out on the emptyprod_re check entirely and unconditionally augment if len(t) <= 1: but I'd get if that'd be a step too far for you.
There was a problem hiding this comment.
What you have definitely looks good enough to accept without benchmarking.
If you want to compress stuff further, then I suppose squashing if/else to allow inlining fixup_emptyprod_lexpos into its only remaining use would also be uncontroversial.
| def wrapper(rule): | ||
| rule.__doc__ = 'ident : ' + "\n| ".join(idents) | ||
| return rule | ||
| return wrapper |
There was a problem hiding this comment.
lex.TOKEN has changed operation since 3.4, and sets .regex instead of __doc__. As you could've guessed, .regex is parsed as, well, a regex. Hence this change.
| assert lines[0:3] == ['', 'Conflicts:', ''], lines | ||
| conflicts = lines[3:] | ||
| assert len(conflicts) == 10, conflicts | ||
| assert len(conflicts) == (11 if version == '12' else 13), conflicts |
There was a problem hiding this comment.
I haven't looked into these yet, but I'm willing to bet this is just a case of Ply having gotten better at detecting conflicts. At the very least, t126 passes.
There was a problem hiding this comment.
I think these are scary and need to be looked into -- in particular because it shows a diff between DML 1.2 and 1.4 that wasn't seen before.
There was a problem hiding this comment.
My hope was to first smoke-test it against verification in the hopes of catching whatever problems this were to introduce, but considering we won't be able to run verification against this for at least a week I suppose I can study the Ply output grammar instead.
There was a problem hiding this comment.
Looked into. The new conflicts do change semantics, but range from harmless to good.
First: an existing shift/reduce conflict on : in an after-on-hook context now gets explicitly reported. It is handled the exact same (desirable) way as previously. Now ply just warns about it for whatever reason instead of keeping mum about it.
Second:
saved (
^look-ahead
In the context of a method body used to reduce saved to ident (interpreting this as a function call). Ply 3.11 instead shifts the ( to enable the grammar rules for saved (bool b, int i) syntax like what is possible in object block context.
Third (common to 1.2 and 1.4):
new int(
^ look-ahead
ctypedecl has some weird rules to handle parenthesis, which causes a shift/reduce conflict. These rules have until now been entirely non-functional, as ply 3.4 handles the shift/reduce by reduce int as an identifier (again to allow new int( to be interpreted as a function call, even though that decision affects all places ctypedecl is used). Under ply 3.11, the ( is instead shifted to enable the aforementioned weird rules.
The weird rules are incredibly questionable and weird, and from looking at history were introduced without justification. The easiest way to handle this case is just to rip them out. That way they're as inaccessible under 3.11 as they have always been.
db4d2c6 to
fb92698
Compare
These are unreachable with how ply 3.4 handles shift/reduce conflicts
Ply 3.11 actually offers an escape hatch to let decorated production rules work
fb92698 to
849cbb8
Compare
2 participants
t126 passes locally with a custom venv featuring Ply 3.11.
This will have to be merged after a base-deps PR with both the bump to the Ply version and the bump to the DMLC submodule.