diff: walk LCS as ordered list, not set, to fix block ordering

doc_build/ast_diff.py

@@ -382,36 +382,44 @@ def _merge_with_lcs(
     `_pair_adjacent_changes` needs to fold them into substitution-style
     annotations.
 
+    The LCS is consumed as an *ordered* sequence: at each LCS element we drain
+    every preceding non-LCS element from `before` as a deletion, then every
+    preceding non-LCS element from `after` as an insertion, then take the LCS
+    element itself.  A set-membership check on the LCS would be incorrect:
+    with duplicate elements (or after earlier drift between the two pointers)
+    two different elements can each be 'in the LCS' while not being the same
+    LCS element, and pairing them would advance both pointers across
+    mismatched positions.
+
     Elements may be Pandoc blocks (``PandocNode``) or list items
     (``List[PandocNode]``); the only requirement is that ``json.dumps(element,
     sort_keys=True)`` gives a stable equality key.
     """
-    lcs_set = {json.dumps(n, sort_keys=True) for n in lcs_elements}
+    before_strs = [json.dumps(n, sort_keys=True) for n in before]
+    after_strs = [json.dumps(n, sort_keys=True) for n in after]
+    lcs_strs = [json.dumps(n, sort_keys=True) for n in lcs_elements]
 
     raw: List[Tuple[str, Any]] = []
     ptr_a = ptr_b = 0
-    while ptr_a < len(before) or ptr_b < len(after):
-        a = before[ptr_a] if ptr_a < len(before) else None
-        b = after[ptr_b] if ptr_b < len(after) else None
-        a_str = json.dumps(a, sort_keys=True) if a is not None else None
-        b_str = json.dumps(b, sort_keys=True) if b is not None else None
-
-        if a is not None and a_str not in lcs_set:
-            raw.append(("deletion", a))
-            ptr_a += 1
-        elif b is not None and b_str not in lcs_set:
-            raw.append(("insertion", b))
-            ptr_b += 1
-        elif a is not None and b is not None:
-            raw.append(("equal", a))
-            ptr_a += 1
-            ptr_b += 1
-        elif ptr_a < len(before):
+    for target in lcs_strs:
+        while ptr_a < len(before) and before_strs[ptr_a] != target:
             raw.append(("deletion", before[ptr_a]))
             ptr_a += 1
-        else:
+        while ptr_b < len(after) and after_strs[ptr_b] != target:
             raw.append(("insertion", after[ptr_b]))
             ptr_b += 1
+        # Both pointers are now at an instance of the current LCS element.
+        raw.append(("equal", before[ptr_a]))
+        ptr_a += 1
+        ptr_b += 1
+
+    while ptr_a < len(before):
+        raw.append(("deletion", before[ptr_a]))
+        ptr_a += 1
+    while ptr_b < len(after):
+        raw.append(("insertion", after[ptr_b]))
+        ptr_b += 1
+
     return raw
 
 

pyproject.toml

@@ -22,13 +22,14 @@ doc_build = { path = ".", editable = true }
 build = "python tests/build_scripts/build_docs.py build"
 build-diff = "python tests/build_scripts/build_diff.py"
 feature-test = "python -m unittest discover -s tests/feature -t ."
+regression-test = "python -m unittest discover -s tests/regression -t ."
 
 [tool.pixi.tasks.test]
-    # Convenience command to run feature-test, build, and build-diff
-    #   ie, `pixi run test` = `pixi run feature-test ; pixi run build ; pixi run build-diff`
+    # Convenience command to run feature-test, regression-test, build, and build-diff
+    #   ie, `pixi run test` = `pixi run feature-test ; pixi run regression-test ; pixi run build ; pixi run build-diff`
     # Note that `test` / depends-on tasks cannot be passed extra arguments...
     # (see note below)
-    depends-on = ["feature-test", "build", "build-diff"]
+    depends-on = ["feature-test", "regression-test", "build", "build-diff"]
 
     # NOTE: this cmd is just to cause an error message to print if you try to pass args
     # to `pixi run test`
@@ -37,7 +38,7 @@ feature-test = "python -m unittest discover -s tests/feature -t ."
     #     pixi run test --keep-pdf-latexn
     # ...will seem to work, but actually the flag is just silently discarded.
     # This command causes the test task to fail in this scenario
-    cmd = "python -c 'import sys; sys.argv[1:] and sys.exit(\"test does not accept extra args; pass them directly to build, build-diff, or feature-test\")'"
+    cmd = "python -c 'import sys; sys.argv[1:] and sys.exit(\"test does not accept extra args; pass them directly to build, build-diff, feature-test, or regression-test\")'"
 
 [tool.pixi.workspace]
 channels = [

tests/_ast_diff_helpers.py

@@ -1,6 +1,6 @@
 """Shared Pandoc AST helpers for ast_diff tests.
 
-Factored out for tests/feature/test_ast_diff.py and a soon-to-come
+Used by tests/feature/test_ast_diff.py and
 tests/regression/test_diff_ordering_with_duplicate_nodes.py.  The functions
 construct just enough of the real Pandoc AST shape for the ast_diff module
 to walk over without going through pandoc itself.

tests/regression/README.md

@@ -0,0 +1,19 @@
+# Regression tests for `doc_build`
+
+Stdlib-`unittest` regression tests for the Python modules in `doc_build/`.
+No third-party test dependency.
+
+Each test here corresponds to a specific bug that was fixed; the docstring
+should describe the bug and the failure mode it would reproduce against the
+unfixed code. Add a new test file (or test case) per fixed bug, rather than
+rewriting existing ones.
+
+## Running
+
+From the repo root:
+
+    pixi run python -m unittest discover -s tests/regression -t .
+
+Or run a single file directly:
+
+    pixi run python tests/regression/test_diff_ordering_with_duplicate_nodes.py

tests/regression/test_diff_ordering_with_duplicate_nodes.py

@@ -0,0 +1,82 @@
+#!/usr/bin/env python
+"""Regression tests for doc_build.ast_diff.
+
+Each test here corresponds to a specific bug that was fixed and should be
+expected to FAIL when run against the unfixed code.  Tests that document
+normal behavior or structural shape (and pass against both fixed and unfixed
+code) belong under tests/unit/ instead.
+"""
+
+import sys
+import unittest
+from pathlib import Path
+
+# Make `doc_build` and the shared test helpers importable when run as a
+# script (e.g. `python test_diff_ordering_with_duplicate_nodes.py`) as well
+# as via `python -m unittest discover -s tests/regression`.
+_REPO_ROOT = Path(__file__).resolve().parents[2]
+_TESTS_DIR = _REPO_ROOT / "tests"
+for _p in (_REPO_ROOT, _TESTS_DIR):
+    if str(_p) not in sys.path:
+        sys.path.insert(0, str(_p))
+
+from doc_build.ast_diff import diff_block_lists  # noqa: E402
+
+from _ast_diff_helpers import block_kind, header  # noqa: E402
+
+
+class TestDiffBlockListsLcsOrderingRegression(unittest.TestCase):
+    """Regression tests for the bug where lcs_set membership lost positional
+    info, causing two pointers that were each 'in the LCS' but at different
+    positions to be treated as a matched pair.
+
+    The most visible symptom in the spec build was a section like:
+
+        #### Deprecated Fields
+        These fields are reserved...
+        - <bullet items, with one added>
+
+    rendering as:
+
+        #### Deprecated Fields
+        [INSERTED bullet list]
+        These fields are reserved...
+        [REMOVED bullet list]
+
+    instead of the expected substitution between the two lists.
+    """
+
+    def test_extra_duplicate_does_not_eat_following_lcs_block(self):
+        # Minimal direct repro of the lcs_set-vs-lcs_list bug.  'X' appears
+        # 3 times in 'before' and 2 times in 'after'.  The LCS uses two of
+        # the X instances (the 1st and 3rd in 'before'), leaving the middle
+        # one as a deletion.  But the buggy algorithm checked "X in lcs_set"
+        # rather than "X is the next LCS element here", so the surplus X
+        # was treated as preserved, forcing the next LCS element ('C' in
+        # 'after') to be falsely reported as a deletion.
+        x = header(2, "x", "X")
+        a = header(2, "a", "A")
+        b = header(2, "b", "B")
+        c = header(2, "c", "C")
+
+        before = [x, a, x, b, x, c]
+        after = [x, a, x, c]
+
+        merged = diff_block_lists(before, after)
+        kinds = [block_kind(blk) for blk in merged]
+
+        # Expected: [X, A, X, del(B), del(X), C] -- the surplus X is a
+        # deletion, B is a deletion, and C is preserved.
+        # Buggy output was [X, A, X, del(B), X, del(C)] (C falsely deleted).
+        self.assertEqual(
+            kinds,
+            ["Header", "Header", "Header", "Div(deletion)", "Div(deletion)", "Header"],
+            "the surplus duplicate X should be reported as a deletion, "
+            "not allowed to consume the following preserved element",
+        )
+        # And specifically, C must remain preserved, not deleted.
+        self.assertEqual(merged[-1], c)
+
+
+if __name__ == "__main__":
+    unittest.main()