gh-153195: Move the syntax error generation rules into a separate grammar file#153197
gh-153195: Move the syntax error generation rules into a separate grammar file#153197serhiy-storchaka wants to merge 2 commits into
Conversation
…te grammar file Grammar/python.gram now contains only the pure Python grammar and by itself produces a complete parser which only emits generic error messages. The alternatives for generating specialized error messages are moved to a new grammar file Grammar/python_errors.gram. They are defined in rule extensions (marked with the "extend" flag) which insert them into the rules of the main grammar, at automatically deduced positions or before all existing alternatives, and are only used in the second parsing pass. Only a few error rules shared by several alternatives are still defined as named rules. The pegen parser generator can now read several grammar files and merge them. The generated parser has equivalent behavior; only the order and grouping of the error alternatives is changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documentation build overview
|
Two groups with the same syntax but different actions or variable
names were deduplicated into a single artificial rule, because the
cache key was based on str(), which ignores actions and variable
names. With the error alternatives inlined before the base
alternatives, the artificial rule for the group in an error
alternative (without an action) could shadow the artificial rule for
the same-looking group in a base alternative (with an action), e.g.
"class C(B): ..." lost its bases. Use repr() in the cache key.
The action of an alternative inserted by a rule extension can return
NULL without setting an error (a conditional action which only
sometimes reports the error, e.g. in invalid_legacy_expression). The
generated code treated this as a result of the whole rule, so the
rule failed (and the failure was memoized) instead of trying the
remaining alternatives. Generate a NULL check which continues with
the next alternative, like a reference to an invalid_* rule returning
NULL behaves.
Also do not insert new alternatives before alternatives inserted by
preceding extensions: they raise an error rather than succeed, so
they cannot shadow the new alternative, and the order of extensions
should define their relative order ("'in' expected after for-loop
variables" must be reported before descending into the for targets).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pablogsal
left a comment
There was a problem hiding this comment.
Thanks Serhiy, this looks like a very good direction.
I left some comments around the extension insertion machinery. I think we need to make that a bit more conservative before landing.
| ei += 1 | ||
| continue | ||
| inserted_heads, _ = _head_symbols(inserted_item) | ||
| if inserted_heads & self.head_set(base_item): |
There was a problem hiding this comment.
I think this is not conservative enough. A base alternative like pair 'c' with pair: 'a' 'b' still shadows an extension like 'a' 'b' 'c' 'd', because we append the extension after the base alternative.
We probably need to either prove this through rule expansions/cuts/lookaheads or insert before when unsure.
| # A proper prefix only if something is left to match. | ||
| return ei < len(inserted_items) | ||
| base_item = base_items[bi] | ||
| if isinstance(base_item, Cut): |
There was a problem hiding this comment.
Cuts make this more subtle, no? A base alternative like 'a' ~ 'b' commits before a later 'a' 'c' extension can run, so we probably need to place the extension before the base alternative here.
| f"{_alt_key(alt)!r}" | ||
| ) | ||
| # Inserted alternatives are only used in the second parsing pass. | ||
| alt.invalid = True |
There was a problem hiding this comment.
This marks the alt as invalid, but the Python generator does not look at Alt.invalid, no?
A Python parser generated from merged grammars will accept these alternatives in the normal pass. We should either teach the Python backend about this or reject extensions there.
| # str() ignores actions and variable names, so it cannot be | ||
| # used as a part of the key: two groups with the same syntax, | ||
| # but different actions must produce different artificial rules. | ||
| key = f"{prefix}_{node!r}" |
There was a problem hiding this comment.
This is better, but NamedItem.__repr__() still drops the item type. Two groups that only differ by a[void*] vs a[expr_ty] can still share the same _tmp_* rule.
| raise parser.make_syntax_error(grammar_file) | ||
| The returned parser and tokenizer are those of the last grammar file. | ||
| """ | ||
| if isinstance(grammar_files, str): |
There was a problem hiding this comment.
This now breaks Path callers. open(Path(...)) worked before, but now Path is treated as an iterable. Maybe normalize with os.fspath() before this branch?
| invalid_tstring_conversion_character: | ||
| | '!' &(':' | '}') { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("t-string: missing conversion character") } | ||
| | '!' !NAME { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("t-string: invalid conversion character") } | ||
|
|
There was a problem hiding this comment.
Nit: blank line at EOF here.
Grammar/python.gramnow contains only the pure Python grammar, without a single reference to the error rules. By itself it produces a complete parser which parses exactly the same language and only emits generic error messages.The alternatives for generating specialized syntax error messages are moved to a new grammar file
Grammar/python_errors.gram. They are written (with their actions) in rule extensions (marked with the "extend" flag) which insert them into the rules of the main grammar. The position of an inserted alternative is deduced automatically: it is inserted before the leftmost alternative of the base rule which can succeed by matching a proper prefix of the code matched by the inserted alternative (such alternative would shadow the specialized error if it was tried first), or after all alternatives if there is no such alternative. Alternatives which must preempt a base alternative which reports an error itself rather than failing are written before..., which stands for the alternatives of the base rule. Several rules can be extended at once, and a rule can be extended several times. The inserted alternatives are only used in the second parsing pass; only ten error rules which are shared, memoized or referenced from other alternatives remain defined as named rules.The pegen parser generator can now read several grammar files and merge them.
The generated parser has equivalent behavior, with only the order and grouping of the error alternatives changed. Verified by the test suite (including a debug build, which validates the produced AST) and by a differential test running both parsers on a corpus of 873 invalid inputs covering every error message in the grammar (identical exception types, messages and locations) and on 911 stdlib files (identical AST including attributes).
Inlining the error alternatives exposed two bugs in the parser generator, fixed in the second commit: two groups with the same syntax but different actions were deduplicated into a single artificial rule (so
class C(B): ...lost its bases), and an inserted alternative with a conditional action returning NULL without an error failed the whole (memoized) rule instead of falling through to the next alternative.🤖 Generated with Claude Code