Repository navigation
Conversation
Member
|
Low effort. At least read the code you're submitting. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reconstructoremitsr:a?bforr: a? b. That output is not the grammar it came from:GrammarError: Unexpected colon, at line 1 column 5;lark/grammars/lark.larkit re-parses into a different tree:?bis a singleRULEtoken there, so the optional operator is gone and the reference tobhas become a reference to?b.Why only
?OPis[+*]|[?](?![a-z_])(load_grammar.py:83): a?immediately followed by a name character is not the operator.*,+and~ncarry no such lookahead, sor: a* breconstructs tor:a*band round-trips today. I checked each one; this is specific to?.Root cause
reconstruct.py:105decides separation by character class — a space goes in only when the characters on both sides areis_id_continue— and?is not one. The change adds the single case where a trailing operator can still merge with the name after it.Prior art
#1172 reported this in 2022, found the same way (reconstructing
lark.lark), and was closed with the advice to pass apostprocthat inserts the space. That works and it is what the reporter did. This PR proposes the library do it instead, because the grammars lark itself ships are affected and every caller reconstructing a Lark grammar has to rediscover the same three lines.a?bstays illegal input — the only thing that changes is that lark stops emitting it.Blast radius, measured on this repo's own grammars
Reconstruct each of the 524 top-level statements in the 37
.larkfiles here, re-parse, compare trees, then repair only the?spacing to attribute the cause: 6 statements in 5 files are this defect, and all 6 round-trip after the fix. Two are in grammars lark ships:lark/grammars/lark.lark!import_path: "."? name ("." name)*!import_path:"."?name("."name)*lark/grammars/python.lark?await_expr: AWAIT? atom_expr?await_expr:AWAIT?atom_exprexamples/advanced/python2.larksuite: … _NEWLINE? stmt+ ……_NEWLINE?stmt+…examples/advanced/template_lark.lark!import_path{name}: "."? name ("." name)*…"."?name…examples/composition/json.larkarrayandobject(_WS? value)…_WS?value…The other statements that do not round-trip have unrelated causes (alternation handling and terminal patterns, cf. #927, #1300) and are untouched here.
Tests
python -m tests→ 1330 tests, OK (181 skipped); 1329 before, plus the one added. The new test is red before the change and green after. The existing reconstructor tests compare with whitespace stripped, which is why none of them could catch this.Scope
This is the narrow fix. The general one is lexical: ask the target grammar's lexer whether two adjacent items would re-lex as one token, rather than deciding by character class. That would also cover the opposite case — a grammar where
?is a prefix sigil and the name is a separate token, where this patch inserts a space that is not wanted. I did not want to change the spacing policy of every reconstructed output on my own judgement, but I am happy to write the lexical version instead if you prefer it.