Description

abel1502

opened on Mar 4, 2023

Issue body actions

Bug report

In the following part of pegen, the memoized result is only used if self._should_memoize(node) is true. Since loop rules are autogenerated, they are not marked with (memo), so this is always false. Despite that, in the end the result is memoized based on the different condition if node.name. Because of that, in the generated parser loop rules' results are always stored in the cache, but never accessed later.

cpython/Tools/peg_generator/pegen/c_generator.py

Lines 608 to 647 in 8de59c1

	def _handle_loop_rule_body(self, node: Rule, rhs: Rhs) -> None:
	memoize = self._should_memoize(node)
	is_repeat1 = node.name.startswith("_loop1")
	with self.indent():
	self.add_level()
	self._check_for_errors()
	self.print("void *_res = NULL;")
	if memoize:
	self.print(f"if (_PyPegen_is_memoized(p, {node.name}_type, &_res)) {{")
	with self.indent():
	self.add_return("_res")
	self.print("}")
	self.print("int _mark = p->mark;")
	self.print("int _start_mark = p->mark;")
	self.print("void **_children = PyMem_Malloc(sizeof(void *));")
	self.out_of_memory_return(f"!_children")
	self.print("Py_ssize_t _children_capacity = 1;")
	self.print("Py_ssize_t _n = 0;")
	if any(alt.action and "EXTRA" in alt.action for alt in rhs.alts):
	self._set_up_token_start_metadata_extraction()
	self.visit(
	rhs,
	is_loop=True,
	is_gather=node.is_gather(),
	rulename=node.name,
	)
	if is_repeat1:
	self.print("if (_n == 0 || p->error_indicator) {")
	with self.indent():
	self.print("PyMem_Free(_children);")
	self.add_return("NULL")
	self.print("}")
	self.print("asdl_seq *_seq = (asdl_seq*)_Py_asdl_generic_seq_new(_n, p->arena);")
	self.out_of_memory_return(f"!_seq", cleanup_code="PyMem_Free(_children);")
	self.print("for (int i = 0; i < _n; i++) asdl_seq_SET_UNTYPED(_seq, i, _children[i]);")
	self.print("PyMem_Free(_children);")
	if node.name:
	self.print(f"_PyPegen_insert_memo(p, _start_mark, {node.name}_type, _seq);")
	self.add_return("_seq")

Your environment

Does not matter - discovered through manual code analysis

Linked PRs

• gh-102416: Do not memoize incorrectly loop rules in the parser #102467
• [3.11] gh-102416: Do not memoize incorrectly loop rules in the parser (GH-102467). #102473
• [3.10] gh-102416: Do not memoize incorrectly loop rules in the parser (GH-102467). #102474

Activity

1. abel1502

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   on Mar 4, 2023
2. sobolevn

   added

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   on Mar 4, 2023
3. sobolevn commented on Mar 4, 2023

   

   sobolevn

   on Mar 4, 2023

   Member

   More actions

   cc @pablogsal
4. pablogsal commented on Mar 6, 2023

   

   pablogsal

   on Mar 6, 2023

   Member

   More actions

   Thanks for the report @abel1502 and @sobolevn for the ping!

   Reacted by abel1502 and sobolevn
5. pablogsal commented on Mar 6, 2023

   

   pablogsal

   on Mar 6, 2023

   Member

   More actions

   Opened #102467
6. pablogsal

   added 4 commits that reference this issue on Mar 6, 2023

   gh-102416: Do not memoize incorrectly loop rules in the parser (#102467)

   f533f21

   [3.11] pythongh-102416: Do not memoize incorrectly loop rules in the …

   976b508

   [3.10] pythongh-102416: Do not memoize incorrectly loop rules in the …

   58249ba

   [3.10] gh-102416: Do not memoize incorrectly loop rules in the parser (…

   6b49cc6
7. pablogsal

   closed this as completedon Mar 6, 2023
8. pablogsal

   added a commit that references this issue on Mar 6, 2023

   [3.11] gh-102416: Do not memoize incorrectly loop rules in the parser (…

   58de2eb
9. carljm

   added 2 commits that reference this issue on Mar 6, 2023

   Merge branch 'main' into nodeaddict

   33dda2a

   Merge branch 'main' into inlinecomp2

   ecb313c

Issue actions