Stop printing a replacement twice inside a list

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 <graphtage.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 <graphtage.EditedTreeNode.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 <graphtage.EditedTreeNode.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_<NodeType>(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
 -------------
 

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

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}")