Smarter primer diff: detect changed messages via position-based pairing

Instead of reporting moved or reworded diagnostics as separate
removal + addition, pair residual messages by (symbol, path, obj) and
show a compact diff of what changed (line, message text, etc.).

Also: move test fixtures from cases/ to fixtures/, add line_moved and
message_changed_line test scenarios, and update truncation expected
files for the </details> closing-tag fix.

Author: Pierre-Sassoulas

pylint/testutils/_primer/comparator.py

@@ -5,65 +5,175 @@
 from __future__ import annotations
 
 import json
-from collections.abc import Generator
+from collections import defaultdict
+from collections.abc import Iterator
 from pathlib import Path
 
 from pylint.reporters.json_reporter import OldJsonExport
-from pylint.testutils._primer.primer_command import (
-    PackageData,
-    PackageMessages,
-)
+from pylint.testutils._primer.primer_command import PackageMessages
 
+ChangedMessage = tuple[OldJsonExport, OldJsonExport]  # (old, new)
 
-class Comparator:
-    def __init__(
-        self, base_file: str, new_file: str, batches: int | None = None
-    ) -> None:
-        self._base_file = base_file
-        self._new_file = new_file
-        self._batches = batches
-
-    def __iter__(self) -> Generator[tuple[str, PackageData, PackageData]]:
-        main_data: PackageMessages
-        if self._batches is None:
-            main_data = self._load_json(self._base_file)
-            pr_data = self._load_json(self._new_file)
+_LOCATION_KEYS = {"line", "column", "endLine", "endColumn"}
+
+
+def _position_key(msg: OldJsonExport) -> tuple[str, str, str]:
+    """Key that identifies a diagnostic independently of its text or location.
+
+    Two messages that share (symbol, path, obj) are the "same diagnostic" — if
+    they differ only in line numbers or message text, they should be reported as
+    *changed* rather than as a separate removal + addition.
+    """
+    return (msg["symbol"], msg["path"], msg["obj"])
+
+
+def _match_residuals(
+    old_residuals: list[OldJsonExport], new_residuals: list[OldJsonExport]
+) -> tuple[list[ChangedMessage], list[OldJsonExport], list[OldJsonExport]]:
+    """Pair residual messages by position key ``(symbol, path, obj)``.
+
+    Messages that share the same key are paired 1:1 in order.  Any left-over
+    messages remain as genuinely missing or genuinely new.
+
+    Returns ``(paired, unmatched_old, unmatched_new)``.
+    """
+    old_by_key: dict[tuple[str, str, str], list[OldJsonExport]] = defaultdict(list)
+    new_by_key: dict[tuple[str, str, str], list[OldJsonExport]] = defaultdict(list)
+    for m in old_residuals:
+        old_by_key[_position_key(m)].append(m)
+    for m in new_residuals:
+        new_by_key[_position_key(m)].append(m)
+
+    paired: list[ChangedMessage] = []
+    paired_old_ids: set[int] = set()
+    paired_new_ids: set[int] = set()
+    for key in old_by_key:
+        if key not in new_by_key:
+            continue
+        for old, new in zip(old_by_key[key], new_by_key[key]):
+            paired.append((old, new))
+            paired_old_ids.add(id(old))
+            paired_new_ids.add(id(new))
+
+    final_missing = [m for m in old_residuals if id(m) not in paired_old_ids]
+    final_new = [m for m in new_residuals if id(m) not in paired_new_ids]
+    return paired, final_missing, final_new
+
+
+def format_span(msg: OldJsonExport) -> str:
+    """Format a message's location as ``line:col to endLine:endCol``."""
+    start = f"{msg['line']}:{msg['column']}"
+    end_line = msg.get("endLine")
+    end_col = msg.get("endColumn")
+    if end_line is not None and end_col is not None:
+        return f"{start} to {end_line}:{end_col}"
+    return start
+
+
+def message_diff(old: OldJsonExport, new: OldJsonExport) -> str:
+    """Return a compact summary of changed fields between two messages.
+
+    Location changes are merged into a single human-readable span.
+    String fields (like ``message``) get a ``diff`` code block so GitHub
+    renders them with red/green highlighting.
+    """
+    changed_keys: set[str] = set()
+    for key in old:
+        if old[key] != new[key]:  # type: ignore[literal-required]
+            changed_keys.add(key)
+
+    parts: list[str] = []
+    # Location: combine line/column/endLine/endColumn into one sentence.
+    if changed_keys & _LOCATION_KEYS:
+        parts.append(f"Was raised on {format_span(old)}, now on {format_span(new)}.")
+
+    # Other fields (typically ``message``).
+    for key in sorted(changed_keys - _LOCATION_KEYS):
+        old_val = old[key]  # type: ignore[literal-required]
+        new_val = new[key]  # type: ignore[literal-required]
+        if isinstance(old_val, str) and isinstance(new_val, str):
+            parts.append(f"The {key} changed:\n```diff\n- {old_val}\n+ {new_val}\n```")
         else:
-            main_data = {}
-            pr_data = {}
-            for idx in range(self._batches):
-                main_data.update(
-                    self._load_json(
-                        self._base_file.replace("BATCHIDX", "batch" + str(idx))
-                    )
-                )
-                pr_data.update(
-                    self._load_json(
-                        self._new_file.replace("BATCHIDX", "batch" + str(idx))
-                    )
-                )
+            parts.append(f"{key}: {old_val!r} -> {new_val!r}")
+    return "\n".join(parts)
+
+
+class Comparator:
+    """Cross-reference two primer JSON outputs and iterate over differences.
+
+    Yields ``(package, missing, new, changed)`` for each package that has at
+    least one difference.  *changed* contains pairs of ``(old, new)`` messages
+    that are the same diagnostic but with altered details (line, message text,
+    etc.).
+    """
+
+    def __init__(self, main_data: PackageMessages, pr_data: PackageMessages) -> None:
+        self.missing_messages: dict[str, list[OldJsonExport]] = {}
+        self.new_messages: dict[str, list[OldJsonExport]] = {}
+        self.changed_messages: dict[str, list[ChangedMessage]] = {}
+        self.commits: dict[str, str] = {}
 
-        missing_messages: PackageMessages = {}
         for package, data in main_data.items():
-            package_missing_messages: list[OldJsonExport] = []
+            self.commits[package] = pr_data[package]["commit"]
+            # First pass: exact-match removal.
+            residual_old: list[OldJsonExport] = []
             for message in data["messages"]:
                 try:
                     pr_data[package]["messages"].remove(message)
                 except ValueError:
-                    package_missing_messages.append(message)
-            missing_messages[package] = PackageData(
-                commit=pr_data[package]["commit"],
-                messages=package_missing_messages,
+                    residual_old.append(message)
+
+            # Second pass: pair residuals by position to detect *changed*
+            # messages (same diagnostic, different line or text).
+            paired, final_missing, final_new = _match_residuals(
+                residual_old, pr_data[package]["messages"]
             )
 
-        for package, pkg_missing in missing_messages.items():
-            new_messages = pr_data[package]
-            if not pkg_missing["messages"] and not new_messages["messages"]:
+            self.missing_messages[package] = final_missing
+            self.new_messages[package] = final_new
+            self.changed_messages[package] = paired
+
+    def __iter__(
+        self,
+    ) -> Iterator[
+        tuple[
+            str,
+            list[OldJsonExport],
+            list[OldJsonExport],
+            list[ChangedMessage],
+        ]
+    ]:
+        for package, missing in self.missing_messages.items():
+            new = self.new_messages[package]
+            changed = self.changed_messages[package]
+            if not missing and not new and not changed:
                 continue
-            yield package, pkg_missing, new_messages
+            yield package, missing, new, changed
 
     @staticmethod
-    def _load_json(file_path: Path | str) -> PackageMessages:
-        with open(file_path, encoding="utf-8") as f:
-            result: PackageMessages = json.load(f)
-        return result
+    def from_json(
+        base_file: str, new_file: str, batches: int | None = None
+    ) -> Comparator:
+        """Build a Comparator from JSON file paths, handling batched runs."""
+        main_data: PackageMessages
+        pr_data: PackageMessages
+        if batches is None:
+            main_data = _load_json(base_file)
+            pr_data = _load_json(new_file)
+        else:
+            main_data = {}
+            pr_data = {}
+            for idx in range(batches):
+                main_data.update(
+                    _load_json(base_file.replace("BATCHIDX", "batch" + str(idx)))
+                )
+                pr_data.update(
+                    _load_json(new_file.replace("BATCHIDX", "batch" + str(idx)))
+                )
+        return Comparator(main_data, pr_data)
+
+
+def _load_json(file_path: Path | str) -> PackageMessages:
+    with open(file_path, encoding="utf-8") as f:
+        result: PackageMessages = json.load(f)
+    return result

pylint/testutils/_primer/primer_compare_command.py

@@ -5,15 +5,20 @@
 
 from pathlib import PurePosixPath
 
-from pylint.testutils._primer.comparator import Comparator
-from pylint.testutils._primer.primer_command import PackageData, PrimerCommand
+from pylint.reporters.json_reporter import OldJsonExport
+from pylint.testutils._primer.comparator import (
+    ChangedMessage,
+    Comparator,
+    message_diff,
+)
+from pylint.testutils._primer.primer_command import PrimerCommand
 
 MAX_GITHUB_COMMENT_LENGTH = 65536
 
 
 class CompareCommand(PrimerCommand):
     def run(self) -> None:
-        comparator = Comparator(
+        comparator = Comparator.from_json(
             self.config.base_file, self.config.new_file, self.config.batches
         )
         comment = self._create_comment(comparator)
@@ -22,11 +27,15 @@ def run(self) -> None:
 
     def _create_comment(self, comparator: Comparator) -> str:
         comment = ""
-        for package, missing_messages, new_messages in comparator:
+        for package, missing_messages, new_messages, changed_messages in comparator:
             if len(comment) >= MAX_GITHUB_COMMENT_LENGTH:
                 break
             comment += self._create_comment_for_package(
-                package, new_messages, missing_messages
+                package,
+                missing_messages,
+                new_messages,
+                changed_messages,
+                comparator.commits[package],
             )
         comment = (
             f"🤖 **Effect of this PR on checked open source code:** 🤖\n\n{comment}"
@@ -39,32 +48,52 @@ def _create_comment(self, comparator: Comparator) -> str:
         return self._truncate_comment(comment)
 
     def _create_comment_for_package(
-        self, package: str, new_messages: PackageData, missing_messages: PackageData
+        self,
+        package: str,
+        missing_messages: list[OldJsonExport],
+        new_messages: list[OldJsonExport],
+        changed_messages: list[ChangedMessage],
+        commit: str,
     ) -> str:
-        comment = f"\n**Effect on [{package}]({self.packages[package].url}):**\n\n"
-        # Create comment for new messages
-        count = 1
+        url = self.packages[package].url
+        clone_dir = self.packages[package].clone_directory
+
+        assert not url.endswith(
+            ".git"
+        ), "You don't need the .git at the end of the github url."
+
+        def _source_link(msg: OldJsonExport) -> str:
+            filepath = str(PurePosixPath(msg["path"]).relative_to(clone_dir))
+            return f"{url}/blob/{commit}/{filepath}#L{msg['line']}"
+
+        comment = f"\n**Effect on [{package}]({url}):**\n\n"
+
+        # -- Changed messages (same diagnostic, different details) -----------
+        if changed_messages:
+            comment += "The following messages have been changed:\n\n<details>\n"
+            for count, (old, new) in enumerate(changed_messages, 1):
+                comment += (
+                    f"{count}) {new['symbol']}:\n"
+                    f"{message_diff(old, new)}\n"
+                    f"{_source_link(new)}\n"
+                )
+            comment += "</details>\n\n"
+
+        # -- New messages ----------------------------------------------------
         astroid_errors = 0
         new_non_astroid_messages = ""
-        if new_messages["messages"]:
-            print("Now emitted:")
-        for message in new_messages["messages"]:
-            filepath = str(
-                PurePosixPath(message["path"]).relative_to(
-                    self.packages[package].clone_directory
-                )
-            )
-            # Existing astroid errors may still show up as "new" because the timestamp
-            # in the message is slightly different.
+        count = 0
+        for message in new_messages:
+            # Existing astroid errors may still show up as "new" because the
+            # timestamp in the message is slightly different.
             if message["symbol"] == "astroid-error":
                 astroid_errors += 1
             else:
+                count += 1
                 new_non_astroid_messages += (
                     f"{count}) {message['symbol']}:\n*{message['message']}*\n"
-                    f"{self.packages[package].url}/blob/{new_messages['commit']}/{filepath}#L{message['line']}\n"
+                    f"{_source_link(message)}\n"
                 )
-                print(message)
-                count += 1
 
         if astroid_errors:
             comment += (
@@ -78,29 +107,16 @@ def _create_comment_for_package(
                 + "</details>\n\n"
             )
 
-        # Create comment for missing messages
-        count = 1
-        if missing_messages["messages"]:
+        # -- Missing messages ------------------------------------------------
+        if missing_messages:
             comment += "The following messages are no longer emitted:\n\n<details>\n"
-            print("No longer emitted:")
-        for message in missing_messages["messages"]:
-            comment += f"{count}) {message['symbol']}:\n*{message['message']}*\n"
-            filepath = str(
-                PurePosixPath(message["path"]).relative_to(
-                    self.packages[package].clone_directory
+            for count, message in enumerate(missing_messages, 1):
+                comment += (
+                    f"{count}) {message['symbol']}:\n*{message['message']}*\n"
+                    f"{_source_link(message)}\n"
                 )
-            )
-            assert not self.packages[package].url.endswith(
-                ".git"
-            ), "You don't need the .git at the end of the github url."
-            comment += (
-                f"{self.packages[package].url}"
-                f"/blob/{new_messages['commit']}/{filepath}#L{message['line']}\n"
-            )
-            count += 1
-            print(message)
-        if missing_messages["messages"]:
             comment += "</details>\n\n"
+
         return comment
 
     def _truncate_comment(self, comment: str) -> str: