Address feedback

Author: lysnikolaou

InternalDocs/parser.md

@@ -528,9 +528,11 @@ separately. The keyword tables and the `@trailer` code are only in the main C
 file, while the `@header` and `@subheader` code is copied into every C file,
 so it must not define non-`static` functions or variables (they would be defined
 more than once). Rules that are called from another C file, including rules of
-included files that the trailer calls, can't be `static`; the generated code
-gives them a `_PyPegen_` prefix with a `#define`, since CPython requires that
-prefix on its non-static symbols. Helper rules (for groups, repetitions and so on)
+included files that the trailer or a C action calls, can't be `static`; the
+generated code gives them a `_PyPegen_` prefix with a `#define`, since CPython
+requires that prefix on its non-static symbols, and marks them
+`Py_LOCAL_SYMBOL` so that they aren't exported from the shared library. Helper
+rules (for groups, repetitions and so on)
 are never shared between files: each C file gets its own `static` copy.
 
 Tokenization

Lib/test/test_peg_generator/test_c_generator.py

@@ -342,8 +342,8 @@ def test_included_rules_go_to_their_own_file(self):
             self.assertNotIn("_PyPegen_start_rule", output)
         self.assertIn("static asdl_seq *_loop1_", root)
         self.assertIn("static void *\nstart_rule(Parser *p)", root)
-        self.assertIn("\nvoid *\nitem_rule(Parser *p)", root)
-        self.assertIn("\nvoid *\ninvalid_item_rule(Parser *p)", extra)
+        self.assertIn("\nPy_LOCAL_SYMBOL void *\nitem_rule(Parser *p)", root)
+        self.assertIn("\nPy_LOCAL_SYMBOL void *\ninvalid_item_rule(Parser *p)", extra)
         # A helper for an identical subexpression is not shared between
         # files: each one has its own static copy.
         self.assertIn("static void *_tmp_", root)
@@ -361,8 +361,21 @@ def test_trailer_can_call_rules_of_included_files(self):
         self.assertIn("return start_rule(p);", root)
         for output in root, rules:
             self.assertIn("#define start_rule _PyPegen_start_rule\n", output)
-            self.assertIn("\nvoid *start_rule(Parser *p);\n", output)
-        self.assertIn("\nvoid *\nstart_rule(Parser *p)\n{", rules)
+            self.assertIn("\nPy_LOCAL_SYMBOL void *start_rule(Parser *p);\n", output)
+        self.assertIn("\nPy_LOCAL_SYMBOL void *\nstart_rule(Parser *p)\n{", rules)
+
+    def test_actions_can_call_rules_of_included_files(self):
+        main = self.write("main.gram", """\
+            @include "extra.gram"
+            start: NAME NEWLINE { extra_rule(p) }
+            """)
+        self.write("extra.gram", "extra: NAME\n")
+        root, included = self.generate(main)
+        extra = included["extra.gram"]
+        for output in root, extra:
+            self.assertIn("#define extra_rule _PyPegen_extra_rule\n", output)
+            self.assertIn("\nPy_LOCAL_SYMBOL void *extra_rule(Parser *p);\n", output)
+        self.assertIn("\nPy_LOCAL_SYMBOL void *\nextra_rule(Parser *p)\n{", extra)
 
     def test_without_includes_all_rules_are_static(self):
         main = self.write("main.gram", """\

Parser/parser.c

@@ -123,7 +123,8 @@ def collect_rules(self) -> None:
             self._collected = True
 
     def _shared_rules(self) -> set[str]:
-        # Rules called from a rule whose code goes in another C file.
+        # Rules called from a rule (or its actions) whose code goes in
+        # another C file.
         shared = set()
         for rule in self.all_rules.values():
             collector = _NameCollector()
@@ -138,7 +139,7 @@ def _shared_rules(self) -> set[str]:
         # The trailer is C code in the root file that calls rules by their
         # function name (the default one calls start_rule()).
         trailer = self.grammar.metas.get("trailer", EXTENSION_SUFFIX) or ""
-        for name in re.findall(r"\b(\w+)_rule\b", trailer):
+        for name in _RULE_CALL.findall(trailer):
             callee = self.rules.get(name)
             if callee is not None and callee.source is not None:
                 shared.add(name)
@@ -170,9 +171,18 @@ def _prepare_trailer(self, rules: tuple[CRule, ...]) -> str | None:
         return None
 
 
+# A call to a rule's C function, as written in actions and the trailer.
+_RULE_CALL = re.compile(r"\b(\w+)_rule\b")
+
+
 class _NameCollector(grammar.GrammarVisitor):
     def __init__(self) -> None:
         self.names: set[str] = set()
 
     def visit_NameLeaf(self, node: grammar.NameLeaf) -> None:
         self.names.add(node.value)
+
+    def visit_Alt(self, node: grammar.Alt) -> None:
+        if node.action:
+            self.names.update(_RULE_CALL.findall(node.action))
+        self.generic_visit(node)

Parser/parser_invalid.c

@@ -77,7 +77,7 @@ def _emit_declarations(self) -> None:
             for rule in shared_rules:
                 name = f"{rule.signature.name}_rule"
                 self.print(f"#define {name} {SHARED_RULE_PREFIX}{name}")
-                self.print(rule.signature.declaration(static=False))
+                self.print(f"Py_LOCAL_SYMBOL {rule.signature.declaration(static=False)}")
             self.print()
         for _, rule in self._own_rules():
             if not rule.shared:

Tools/peg_generator/pegen/c_generator.py

@@ -97,7 +97,7 @@ def emit(self) -> None:
             self._print(f"// {line}")
         if rule.left_recursive and rule.leader:
             self._print(f"static {result_type} {signature.name}_raw(Parser *);")
-        self._print(result_type if rule.shared else f"static {result_type}")
+        self._print(f"Py_LOCAL_SYMBOL {result_type}" if rule.shared else f"static {result_type}")
         self._print(f"{signature.name}_rule(Parser *p)")
         if rule.left_recursive and rule.leader:
             self._emit_left_recursive_wrapper()