From 30b104297092f824165c04c7c1748db2bf8c248e Mon Sep 17 00:00:00 2001 From: Evan Sultanik Date: Wed, 9 Sep 2026 10:23:33 -0400 Subject: [PATCH] Stop printing a replacement twice inside a list GraphtageFormatter.print takes a with_edits flag, but it drops the flag when it dispatches to the resolved formatter, because with_edits is not part of the print_(printer, node) calling convention. Formatters rely on being able to hand a node back to the protocol: a list formatter that meets a nested dict has no print_MappingNode of its own, so it calls self.parent.print(...) to reach the format's dict formatter. That call restarted the protocol with with_edits defaulting to True, found the node's Replace still attached, and printed the whole edit a second time, so [1, {"a": 1}] against [1, [2, 3]] rendered as [1,{"a": 1} -> [2,3] -> [2,3]]. Threading the flag through the dispatch would mean adding a with_edits parameter to every print_* method in the package, since the delegating methods forward *args and **kwargs into implementations that do not accept it. Instead, GraphtageFormatter.print now records a node while that node's own formatter is running and treats a re-entrant call for the same node as with_edits=False. The decision about a node's edit is made once, by the outermost call, which is the invariant the delegating formatters already assumed. The suppression covers the node being printed and not its children, so edits nested inside a delegated subtree still print. JSON, JSON5, YAML, TOML, and INI all shared the defect; plist and XML never delegate across container types and were unaffected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GypKU5KdLfs2Cf8kS2TzJa --- docs/printing.rst | 19 ++++++++++-- graphtage/tree.py | 53 +++++++++++++++++++++++++++----- test/test_issue_152.py | 69 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 10 deletions(-) create mode 100644 test/test_issue_152.py diff --git a/docs/printing.rst b/docs/printing.rst index 795e5ce..cb5da40 100644 --- a/docs/printing.rst +++ b/docs/printing.rst @@ -11,8 +11,8 @@ The protocol for delegating how a :class:`graphtage.TreeNode` or :class:`graphta * If ``with_edits``, then choose the edit * Otherwise, choose :attr:`node_or_edit.from_node ` * If ``node_or_edit`` is a :class:`graphtage.TreeNode`: - * If ``with_edits`` *and* the node is edited and has a non-zero cost, - then choose :attr:`node_or_edit.edit `:: + * If ``with_edits``, the node's own formatter is not already on the stack, *and* the node is edited and has a + non-zero cost, then choose :attr:`node_or_edit.edit `:: isinstance(node_or_edit, EditedTreeNode) and \ node_or_edit.edit is not None and node_or_edit.edit.has_non_zero_cost() @@ -29,6 +29,7 @@ The protocol for delegating how a :class:`graphtage.TreeNode` or :class:`graphta * If not, try calling the edit's :func:`graphtage.Edit.print` method. If :exc:`NotImplementedError` is *not* raised, return. #. If the chosen object is a node, or if we failed to find a printer for the edit: + * Record that this node's formatter is on the stack, for the duration of the next two steps. * See if there is a specialized formatter for this node by calling :meth:`graphtage.formatter.Formatter.get_formatter` * If so, delegate to that formatter and return. @@ -38,6 +39,20 @@ The protocol for delegating how a :class:`graphtage.TreeNode` or :class:`graphta This is implemented in :meth:`graphtage.GraphtageFormatter.print`. See the :ref:`Formatting Protocol` for how formatters are chosen. +Re-entering the protocol with the same node +------------------------------------------- + +``with_edits`` is an argument to :meth:`graphtage.GraphtageFormatter.print`, but it is not part of the +``print_(printer, node)`` calling convention that formatters implement, so it stops at the formatter +boundary. That matters because formatters hand nodes back to the protocol: a list formatter that meets a nested dict +has no ``print_MappingNode`` of its own, so it calls ``self.parent.print(printer, node)`` to reach the format's dict +formatter, as :meth:`graphtage.json.JSONListFormatter.print_SequenceNode` does. Such a call starts the protocol over +for a node whose edit the outer call has already printed, or has deliberately declined to print. + +Recording the node for the duration of its own formatter is what keeps that second pass from printing the edit again. +It applies to the node being printed only, not to its children, so an edit nested inside the delegated subtree still +prints normally. + Status Output ------------- diff --git a/graphtage/tree.py b/graphtage/tree.py index 4aba506..f6e6e99 100644 --- a/graphtage/tree.py +++ b/graphtage/tree.py @@ -22,6 +22,20 @@ FormatterType = Formatter[Union['TreeNode', 'Edit']] +_NODES_BEING_PRINTED: set[int] = set() +"""The :func:`id` of every node whose formatter is currently on the stack. + +:meth:`GraphtageFormatter.print` decides whether to print a node's edit *before* it hands the node to that node's +formatter, so by the time the formatter runs, the decision has already been made and acted upon. Formatters routinely +re-enter :meth:`GraphtageFormatter.print` with the very same node to pass it to a sibling formatter, for example +:meth:`graphtage.json.JSONListFormatter.print_SequenceNode`, which forwards a dict nested in a list to +:class:`graphtage.json.JSONDictFormatter` through ``self.parent.print(...)``. That re-entry cannot repeat the +decision: the ``with_edits`` argument is not part of the ``print_*`` calling convention, so it never reaches the +sub-formatter and the re-entry would default to printing the edit a second time. + +""" + + class GraphtageFormatter(FormatterType): """A base class for defining formatters that operate on :class:`TreeNode` and :class:`Edit`.""" @@ -31,7 +45,9 @@ def print(self, printer: Printer, node_or_edit: Union['TreeNode', 'Edit'], with_ Args: printer: The printer to which to write. node_or_edit: The node or edit to print. - with_edits: If :keyword:True, print any edits associated with the node. + with_edits: If :keyword:True, print any edits associated with the node. This is also implied to be + :keyword:False while the node's own formatter is on the stack, so that a formatter delegating the + same node to a sibling formatter does not print its edit twice. Note: The protocol for determining how a node or edit should be printed is very complex due to its extensibility. @@ -44,7 +60,7 @@ def print(self, printer: Printer, node_or_edit: Union['TreeNode', 'Edit'], with_ else: edit: Edit | None = None node: TreeNode = node_or_edit.from_node - elif with_edits: + elif with_edits and id(node_or_edit) not in _NODES_BEING_PRINTED: if isinstance(node_or_edit, EditedTreeNode) and \ node_or_edit.edit is not None and node_or_edit.edit.has_non_zero_cost(): edit: Edit | None = node_or_edit.edit @@ -66,14 +82,35 @@ def print(self, printer: Printer, node_or_edit: Union['TreeNode', 'Edit'], with_ return except NotImplementedError: pass - formatter = self.get_formatter(node) - if formatter is not None: - formatter(printer, node) - else: - log.debug(f"""There is no formatter that can handle nodes of type {node.__class__.__name__} + self._print_node(printer, node) + + def _print_node(self, printer: Printer, node: 'TreeNode'): + """Hands a node to its formatter, suppressing that node's edit for the duration of the call. + + Whether to print ``node``'s edit was already settled by :meth:`GraphtageFormatter.print`, so any re-entrant + call for the same node must print the bare node. See :data:`_NODES_BEING_PRINTED`. + + Args: + printer: The printer to which to write. + node: The node to print. + + """ + node_id = id(node) + is_outermost = node_id not in _NODES_BEING_PRINTED + if is_outermost: + _NODES_BEING_PRINTED.add(node_id) + try: + formatter = self.get_formatter(node) + if formatter is not None: + formatter(printer, node) + else: + log.debug(f"""There is no formatter that can handle nodes of type {node.__class__.__name__} Falling back to the node's internal printer Registered formatters: {''.join([f.__class__.__name__ for f in FORMATTERS])}""") - node.print(printer) + node.print(printer) + finally: + if is_outermost: + _NODES_BEING_PRINTED.discard(node_id) @runtime_checkable diff --git a/test/test_issue_152.py b/test/test_issue_152.py new file mode 100644 index 0000000..2838587 --- /dev/null +++ b/test/test_issue_152.py @@ -0,0 +1,69 @@ +from io import StringIO +from typing import Any +from unittest import TestCase + +import graphtage +from graphtage import json as graphtage_json +from graphtage.printer import Printer +from graphtage.pydiff import print_diff + +REPLACEMENT_IN_A_LIST: tuple[Any, Any] = ([1, {"a": 1}], [1, [2, 3]]) +"""A dict nested in a list, replaced by a list. + +Rendering the replaced dict needs a formatter that the enclosing list formatter does not provide, so the list +formatter hands the node back to the format's root formatter. That hand-off is what printed the replacement twice. + +""" + +REPLACEMENT_IN_A_DICT: tuple[Any, Any] = ({"k": {"a": 1}}, {"k": [2, 3]}) +"""The same replacement as a dict value, which never took the delegating path and always rendered correctly.""" + +DELEGATING_TYPENAMES = ("json", "json5", "yaml", "toml", "ini") +"""The filetypes whose list and dict formatters delegate to each other through ``self.parent.print(...)``.""" + +TYPENAMES_THAT_RENDER_A_REPLACED_DICT_VALUE = ("json", "json5", "yaml", "ini") +""":data:`DELEGATING_TYPENAMES` minus TOML, which writes a replaced table value as the original table and no edit.""" + + +class TestIssue152(TestCase): + """Reproduces https://github.com/trailofbits/graphtage/issues/152""" + + @staticmethod + def render(from_obj: Any, to_obj: Any, typename: str) -> str: + """Returns the diff of two Python objects as rendered by the default formatter for ``typename``.""" + from_tree = graphtage_json.build_tree(from_obj) + to_tree = graphtage_json.build_tree(to_obj) + formatter = graphtage.FILETYPES_BY_TYPENAME[typename].get_default_formatter() + stream = StringIO() + printer = Printer(out_stream=stream, ansi_color=False, quiet=True) + with printer: + formatter.print(printer, from_tree.diff(to_tree)) + return stream.getvalue() + + def test_json_prints_a_replacement_in_a_list_once(self): + self.assertEqual( + '[\n 1,\n {\n "a": 1\n } -> [\n 2,\n 3\n ]\n]', + self.render(*REPLACEMENT_IN_A_LIST, "json"), + ) + + def test_pydiff_prints_a_replacement_in_a_list_once(self): + """Renders the exact reproducer from the issue.""" + stream = StringIO() + printer = Printer(out_stream=stream, ansi_color=False, quiet=True) + with printer: + print_diff(*REPLACEMENT_IN_A_LIST, printer=printer) + self.assertEqual('[1,{"a": 1} -> [2,3]]', stream.getvalue()) + + def test_delegating_formats_print_a_replacement_in_a_list_once(self): + """Every format whose list formatter delegates to a sibling formatter shared the same defect.""" + for typename in DELEGATING_TYPENAMES: + with self.subTest(typename=typename): + rendered = self.render(*REPLACEMENT_IN_A_LIST, typename) + self.assertEqual(1, rendered.count(" -> "), f"printed more than one replacement: {rendered!r}") + + def test_delegating_formats_print_a_replacement_in_a_dict_once(self): + """The control case from the issue: a replaced dict value was always rendered correctly.""" + for typename in TYPENAMES_THAT_RENDER_A_REPLACED_DICT_VALUE: + with self.subTest(typename=typename): + rendered = self.render(*REPLACEMENT_IN_A_DICT, typename) + self.assertEqual(1, rendered.count(" -> "), f"printed more than one replacement: {rendered!r}")