group alternations and escape range endpoints in terminal patterns - #1633
group alternations and escape range endpoints in terminal patterns#1633netliomax25-code wants to merge 2 commits into
Conversation
MegaIng
left a comment
MegaIng
left a comment
There was a problem hiding this comment.
While I agree that this PR implements the intended behavior, this is a breaking change.
People may have (accidentally) relied on this behavior to construct more complex terminals. But I suspect this is rare enough that we can put this off as a bug fix.
| # A bare '|' binds looser than concatenation, so an item carrying one has to be grouped | ||
| # before it's joined, or it swallows its neighbors: /a|b/ "c" must not become 'a|bc'. | ||
| regexps = [i.to_regexp() for i in items] | ||
| pattern = ''.join('(?:%s)' % r if _has_bare_alternation(r) else r for r in regexps) |
There was a problem hiding this comment.
We can just unconditionally use an anonymous group, trying to manually check if there is an alternation is a recipe for infinite edge cases.
There was a problem hiding this comment.
Done, dropped the helper and the join now wraps every item in (?:...) unconditionally. One consequence worth flagging: the compiled patterns for the bundled grammars are no longer byte-identical to master (they pick up redundant groups), just equivalent. Full suite and mypy still green.
| codepoint = ord(char) | ||
| if char.isascii(): | ||
| return re.escape(char) | ||
| return '\\x%02x' % codepoint if codepoint <= 0xff else '\\U%08x' % codepoint |
There was a problem hiding this comment.
Done, switched to f-strings.
|
Agreed on the framing. Anyone relying on the old splicing was depending on the terminal matching outside what its own definition says, so treating it as a bug fix seems right to me. Both review points are addressed in the latest push: the join groups every item unconditionally (the alternation scanner is gone), and the escape helper uses f-strings. |
|
Overall looks good, the bugs are real and the fix is in the right direction. A few things to address:
|
3 participants
TerminalTreeToPattern.expansionjoins sub-patterns by plain concatenation, so a regexp literal holding a top-level|binds looser than the join:T: /admin|user/ "_id"compiles toadmin|user_id, which rejectsadmin_idand accepts a bareadmin.PrepareLiterals.rangesplices the endpoints' raw source text into the character class, so an endpoint that is itself a metacharacter escapes it:T: "^".."z"compiles to[^-z], a negated class matching tab and digits while rejectingz.Either way a terminal written as a restriction matches outside the language it declares, which is the part that worries me when the grammar is what validates untrusted input.
Grouping is applied only when the item carries a
|outside any group, so an escaped\|, one inside a character class, and an already-parenthesized one are left alone. Range endpoints are unescaped and then re-escaped for a class, kept ascii because the pattern may still be encoded to bytes underuse_bytes(the dynamic lexer encodes utf-8, the basic one latin-1). The other two spellings of that alternation,/a|b/iand("a"|"b"), already went through(?:...), so only the third one changes.Compiled patterns for
common.lark,python.larkandlark.larkcome out identical to master. Full suite green, mypy clean, and the two tests added intests/test_grammar.pyfail on master and pass here.