Skip to content

Reconstructor: separate ? from the item that follows it - #1652

Closed
Endika wants to merge 1 commit into
lark-parser:masterfrom
Endika:reconstructor-optional-separator
Closed

Endika wants to merge 1 commit into
lark-parser:masterfrom
Endika:reconstructor-optional-separator

Conversation

@Endika

@Endika Endika commented Sep 13, 2026

Copy link
Copy Markdown

Reconstructor emits r:a?b for r: a? b. That output is not the grammar it came from:

  • fed back to lark it does not load at all — GrammarError: Unexpected colon, at line 1 column 5;
  • parsed with lark's own lark/grammars/lark.lark it re-parses into a different tree: ?b is a single RULE token there, so the optional operator is gone and the reference to b has become a reference to ?b.
from lark import Lark
from lark.reconstruct import Reconstructor

parser = Lark.open_from_package('lark', 'lark.lark', ['grammars'],
                                parser='lalr', maybe_placeholders=False)
tree = parser.parse('r: a? b\n')
out = Reconstructor(parser).reconstruct(tree)   # 'r:a?b'
parser.parse(out + '\n') == tree                # False

Why only ?

OP is [+*]|[?](?![a-z_]) (load_grammar.py:83): a ? immediately followed by a name character is not the operator. *, + and ~n carry no such lookahead, so r: a* b reconstructs to r:a*b and round-trips today. I checked each one; this is specific to ?.

Root cause

reconstruct.py:105 decides separation by character class — a space goes in only when the characters on both sides are is_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 a postproc that 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?b stays 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 .lark files 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:

file statement reconstructed
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_expr
examples/advanced/python2.lark suite: … _NEWLINE? stmt+ … …_NEWLINE?stmt+…
examples/advanced/template_lark.lark !import_path{name}: "."? name ("." name)* …"."?name…
examples/composition/json.lark array and object (_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.

@erezsh

erezsh commented Sep 13, 2026

Copy link
Copy Markdown
Member

Low effort. At least read the code you're submitting.

@erezsh erezsh closed this Sep 13, 2026
@Endika
Endika deleted the reconstructor-optional-separator branch September 13, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants