Address review comments

Make the position deducer more careful:
* A rule reference in a base alternative can match several items of
  the inserted alternative ("pair 'c'" with "pair: 'a' 'b'" shadows
  "'a' 'b' 'c' 'd'").
* A failure after a cut fails the whole rule, so "'a' 'c'" must be
  inserted before "'a' ~ 'b'".
* An inserted alternative which can match the same code as the base
  alternative or its proper prefix would report a false error on
  valid code re-parsed in the second pass; never insert it before
  the base alternative.
The only change in the generated parser: the error alternative in
with_item is now inserted before the base alternative (this is
equivalent, as its action only reports an error for an invalid
target).

An alternative inserted by a rule extension now always fails in a
generated Python parser (which has no second parsing pass), like an
alternative which references an invalid_* rule.

NamedItem.__repr__() now includes the item type, so two groups which
only differ by the item type do not share an artificial rule.

build_parser() accepts path-like objects again.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: serhiy-storchaka

Grammar/python_errors.gram

@@ -534,4 +534,3 @@ invalid_fstring_conversion_character:
 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") }
-

Lib/test/test_peg_generator/test_pegen.py

@@ -2,6 +2,7 @@
 import difflib
 import io
 import os
+import pathlib
 import tempfile
 import textwrap
 import unittest
@@ -256,6 +257,84 @@ def test_extend_rule_duplicates_alternative(self) -> None:
         with self.assertRaisesRegex(GrammarError, "duplicates the alternative"):
             merge_grammars([base_grammar, extension_grammar])
 
+    def test_extend_rule_deduction_rule_reference_in_base(self) -> None:
+        # The base alternative can shadow the inserted alternative by
+        # matching several of its items with a single rule reference.
+        base_grammar_source = """
+        start: rule_a NEWLINE
+        rule_a: pair 'c' { PC } | 'x'
+        pair: 'a' 'b'
+        """
+        extension_grammar_source = """
+        rule_a (extend):
+            | 'a' 'b' 'c' 'd'
+        """
+        base_grammar = parse_string(base_grammar_source, GrammarParser)
+        extension_grammar = parse_string(extension_grammar_source, GrammarParser)
+        merged = merge_grammars([base_grammar, extension_grammar])
+        self.assertEqual(
+            str(merged.rules["rule_a"]), "rule_a: 'a' 'b' 'c' 'd' | pair 'c' | 'x'"
+        )
+
+    def test_extend_rule_deduction_cut_in_base(self) -> None:
+        # The cut commits to the base alternative, so a failure after
+        # it would never reach an alternative inserted after it.
+        base_grammar_source = """
+        start: rule_a NEWLINE
+        rule_a: 'a' ~ 'b' { AB } | 'x'
+        """
+        extension_grammar_source = """
+        rule_a (extend):
+            | 'a' 'c'
+        """
+        base_grammar = parse_string(base_grammar_source, GrammarParser)
+        extension_grammar = parse_string(extension_grammar_source, GrammarParser)
+        merged = merge_grammars([base_grammar, extension_grammar])
+        self.assertEqual(
+            str(merged.rules["rule_a"]), "rule_a: 'a' 'c' | 'a' ~ 'b' | 'x'"
+        )
+
+    def test_extend_rule_deduction_more_general_alternative(self) -> None:
+        # An inserted alternative which can match all the code matched
+        # by the base alternative would succeed (and report its error)
+        # on valid code re-parsed in the second pass, so it is not
+        # inserted before the base alternative.
+        base_grammar_source = """
+        start: rule_a NEWLINE
+        rule_a: 'a' NAME { N } | 'x'
+        """
+        extension_grammar_source = """
+        rule_a (extend):
+            | 'a' expr
+        expr: NAME | NUMBER
+        """
+        base_grammar = parse_string(base_grammar_source, GrammarParser)
+        extension_grammar = parse_string(extension_grammar_source, GrammarParser)
+        merged = merge_grammars([base_grammar, extension_grammar])
+        self.assertEqual(
+            str(merged.rules["rule_a"]), "rule_a: 'a' NAME | 'x' | 'a' expr"
+        )
+
+    def test_extended_grammar_python_parser_skips_inserted_alternatives(self) -> None:
+        # The Python parser generator has no second pass; an inserted
+        # alternative fails like an alternative referencing an
+        # invalid_* rule.
+        base_grammar_source = """
+        start: rule_a NEWLINE { rule_a }
+        rule_a: 'a' { 1 }
+        """
+        extension_grammar_source = """
+        rule_a (extend):
+            | 'a' 'b' { RAISE_SYNTAX_ERROR("nope") }
+        """
+        base_grammar = parse_string(base_grammar_source, GrammarParser)
+        extension_grammar = parse_string(extension_grammar_source, GrammarParser)
+        merged = merge_grammars([base_grammar, extension_grammar])
+        out = io.StringIO()
+        genr = PythonParserGenerator(merged, out)
+        genr.generate("<string>")
+        self.assertNotIn("RAISE_SYNTAX_ERROR", out.getvalue())
+
     def test_extend_rule_marks_inserted_alternatives_invalid(self) -> None:
         base_grammar_source = """
         start: rule_a NEWLINE
@@ -375,13 +454,23 @@ def test_extended_grammar_parses(self) -> None:
         extension_grammar = parse_string(extension_grammar_source, GrammarParser)
         merged = merge_grammars([base_grammar, extension_grammar])
         parser_class = generate_parser(merged)
-        node = parse_string("a c\n", parser_class)
-        self.assertEqual(node, "ac")
+        # The Python parser generator has no second pass, so the
+        # inserted alternative never matches.
+        with self.assertRaises(SyntaxError):
+            parse_string("a c\n", parser_class)
         node = parse_string("a b\n", parser_class)
         self.assertEqual(node, "ab")
         node = parse_string("a\n", parser_class)
         self.assertEqual(node, "a")
 
+    def test_build_parser_path_like(self) -> None:
+        grammar_source = "start: NUMBER NEWLINE\n"
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            grammar_file = pathlib.Path(tmp_dir) / "grammar.gram"
+            grammar_file.write_text(grammar_source)
+            grammar, parser, tokenizer = build_parser(grammar_file)
+        self.assertEqual(list(grammar.rules), ["start"])
+
     def test_build_parser_multiple_files(self) -> None:
         first_grammar_source = """
         start: the_rule NEWLINE | extra_rule

Parser/parser.c

@@ -401,9 +401,14 @@ def may_swallow(
             )
         base_items = [named_item.item for named_item in base_alt.items]
         inserted_items = [named_item.item for named_item in inserted_alt.items]
-        return self._match_prefix(base_items, inserted_items)
+        return self._match_prefix(base_items, inserted_items, seen)
 
-    def _match_prefix(self, base_items: list[Any], inserted_items: list[Any]) -> bool:
+    def _match_prefix(
+        self,
+        base_items: list[Any],
+        inserted_items: list[Any],
+        seen: frozenset[str] = frozenset(),
+    ) -> bool:
         bi = ei = 0
         while True:
             # Skip non-consuming inserted items (over-approximation).
@@ -416,8 +421,9 @@ def _match_prefix(self, base_items: list[Any], inserted_items: list[Any]) -> boo
                 return ei < len(inserted_items)
             base_item = base_items[bi]
             if isinstance(base_item, Cut):
-                bi += 1
-                continue
+                # A failure after the cut fails the whole rule, so the
+                # alternative must be inserted before this one.
+                return True
             if isinstance(base_item, Lookahead):
                 if ei < len(inserted_items):
                     matches = self.overlap(base_item.node, inserted_items[ei])
@@ -431,7 +437,7 @@ def _match_prefix(self, base_items: list[Any], inserted_items: list[Any]) -> boo
                 bi += 1
                 continue
             if isinstance(base_item, (Opt, Repeat0)):
-                if self._match_prefix(base_items[bi + 1:], inserted_items[ei:]):
+                if self._match_prefix(base_items[bi + 1:], inserted_items[ei:], seen):
                     return True
                 if ei == len(inserted_items):
                     return False
@@ -450,6 +456,20 @@ def _match_prefix(self, base_items: list[Any], inserted_items: list[Any]) -> boo
                 bi += 1
                 ei += 1
                 continue
+            if isinstance(base_item, NameLeaf):
+                # A rule reference can match several inserted items;
+                # check the rule's alternatives in its place.
+                rule = self.rules.get(base_item.value)
+                if rule is not None and rule.name not in seen:
+                    rest = base_items[bi + 1:]
+                    return any(
+                        self._match_prefix(
+                            [item.item for item in alt.items] + rest,
+                            inserted_items[ei:],
+                            seen | {rule.name},
+                        )
+                        for alt in rule.rhs.alts
+                    )
             inserted_heads, _ = _head_symbols(inserted_item)
             if inserted_heads & self.head_set(base_item):
                 # The base item is more general: it can match the same
@@ -468,6 +488,40 @@ def _match_prefix(self, base_items: list[Any], inserted_items: list[Any]) -> boo
                 )
             return False
 
+    def terminal_head_set(self, node: Any) -> set[str]:
+        """The token and string symbols which can start a match of the node."""
+        return {
+            symbol for symbol in self.head_set(node) if symbol not in self.rules
+        }
+
+    def may_cover(self, inserted_alt: Alt, base_alt: Alt) -> bool:
+        """Whether inserted_alt may match the same code as base_alt or
+        its proper prefix (every item is at least as general as the
+        corresponding base item).
+
+        Inserting it before base_alt would report a false error on
+        valid code re-parsed in the second pass.
+        """
+        base_items = [
+            named_item.item for named_item in base_alt.items
+            if not isinstance(named_item.item, (Lookahead, Cut))
+        ]
+        inserted_items = [
+            named_item.item for named_item in inserted_alt.items
+            if not isinstance(named_item.item, (Lookahead, Cut))
+        ]
+        if len(inserted_items) > len(base_items):
+            return False
+        for base_item, inserted_item in zip(base_items, inserted_items):
+            if str(base_item) == str(inserted_item):
+                continue
+            if not (
+                self.terminal_head_set(base_item)
+                <= self.terminal_head_set(inserted_item)
+            ):
+                return False
+        return True
+
     def deduce_index(self, base_alts: list[Alt], inserted_alt: Alt) -> int | None:
         rule = self._referenced_rule(inserted_alt)
         if rule is not None:
@@ -481,8 +535,13 @@ def deduce_index(self, base_alts: list[Alt], inserted_alt: Alt) -> int | None:
                 # shadow the new alternative; their relative order is
                 # defined by the order of the extensions.
                 continue
-            if any(self.may_swallow(base_alt, alt) for alt in inserted_alts):
-                return index
+            if not any(
+                self.may_swallow(base_alt, alt) for alt in inserted_alts
+            ):
+                continue
+            if any(self.may_cover(alt, base_alt) for alt in inserted_alts):
+                continue
+            return index
         return None
 
 
@@ -593,7 +652,7 @@ def build_parser(
 
     The returned parser and tokenizer are those of the last grammar file.
     """
-    if isinstance(grammar_files, str):
+    if isinstance(grammar_files, (str, os.PathLike)):
         grammar_files = [grammar_files]
     grammars: list[Grammar] = []
     for grammar_file in grammar_files:
@@ -686,8 +745,8 @@ def build_python_generator(
 
 
 def grammars_display_name(grammar_files: str | Sequence[str]) -> str:
-    if isinstance(grammar_files, str):
-        return grammar_files
+    if isinstance(grammar_files, (str, os.PathLike)):
+        return os.fspath(grammar_files)
     return ", ".join(os.path.basename(grammar_file) for grammar_file in grammar_files)
 
 

Tools/peg_generator/pegen/build.py

@@ -220,7 +220,9 @@ def __str__(self) -> str:
             return str(self.item)
 
     def __repr__(self) -> str:
-        return f"NamedItem({self.name!r}, {self.item!r})"
+        if self.type is None:
+            return f"NamedItem({self.name!r}, {self.item!r})"
+        return f"NamedItem({self.name!r}, {self.item!r}, {self.type!r})"
 
     def __iter__(self) -> Iterator[Item]:
         yield self.item

Tools/peg_generator/pegen/grammar.py

@@ -338,7 +338,12 @@ def visit_Alt(self, node: Alt, is_loop: bool, is_gather: bool) -> None:
             self.print("):")
             with self.indent():
                 action = node.action
-                if not action:
+                if node.invalid:
+                    # An inserted alternative has a C action and is only
+                    # used in the second parsing pass, which the Python
+                    # parser does not have; make it always fail.
+                    action = "UNREACHABLE"
+                elif not action:
                     if is_gather:
                         assert len(self.local_variable_names) == 2
                         action = (