From e5a6b1f74664f0a34b2b4e2bf8ff5a9e2df97d93 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 4 Oct 2026 17:37:37 -0700 Subject: [PATCH] fix(code-health): pair repeated names by body; keep the cap of a function moved and edited Why: - banditburai (inline, py_structure.py:97) and unsupportedpastels M3: repeated names (Python `@method(...) def _`, `f#2`; TS `#n`) were paired by source-order ordinal, so a new CC-22 `_` inserted above a trimmed one took its cap (exit 0), and inserting a small `_` above an unchanged CC-22 one made the old one "new" (exit 1). Same-file name matching now groups units by name without the ordinal and pairs the most similar bodies first (difflib ratio >= 0.6 over code lines, comments/whitespace dropped); the last unit left with a name is that name's unit, exactly as a unique name was before. Exact-hash passes still run first. If a body span is unknown, the group falls back to the ordinal. - unsupportedpastels M2: cross-file matching was body-hash only, so a function moved to another file with one constant changed lost its cap (P1 exit 1; same edit in place exit 0). New last pass: another file, same name, similar body (>= 0.6), only when the name is gone from its origin file and exactly one unit of that name left and exactly one arrived unmatched. A copy (origin keeps it), two arrivals, or an unrelated body reusing a deleted name stay new code (andrexibiza: never arbitrary same-name credit). Its existing hits follow it through the departed-occurrence pool (one-to-one), a second swallow is new. Unit gains an optional end_line; until the measurers report it (py_structure: lane A, measure.py: lane C) the span is estimated from the unit's own FUNC_LINES. Tests pass both ways. --- scripts/code_health/compare.py | 118 +++++++++++++++++++-- scripts/code_health/model.py | 3 + tests/scripts/test_code_health_identity.py | 80 +++++++++++++- 3 files changed, 190 insertions(+), 11 deletions(-) diff --git a/scripts/code_health/compare.py b/scripts/code_health/compare.py index 2ce822128f..d29ab8b763 100644 --- a/scripts/code_health/compare.py +++ b/scripts/code_health/compare.py @@ -4,13 +4,17 @@ Every unit has its own cap: a function or file already over target may not grow it has on the base revision; anything new must meet the target. Pattern rules compare multisets of fingerprints, so fixing one violation and adding another still fails. -Code that moves keeps its cap and its existing violations. Head units are matched to base units -ONE-TO-ONE (a base unit is consumed by at most one head unit), in this order: +Code that moves keeps its cap and its existing violations. Head units are matched to base +units ONE-TO-ONE (a base unit is consumed by at most one head unit), in this order: 1. same file, same name, same body (unchanged code; reserved first, so a copy of it is new); 2. same file, same body (a rename, or an anonymous callback whose ordinal shifted); 3. any file, same body, when the origin's name is gone from its own file (a real move); -4. same file, same name (edited in place). +4. same file, same name (edited in place). Repeated names (Python `_`, `f#2`; TS `#3`) + are one group per name, paired by body similarity, never by their source-order ordinal; + the last unit left with a name is that name's unit, as for a unique name; +5. another file, same name, similar body (moved AND edited), only when the name is gone + from its origin file and exactly one unit of that name left and one arrived unmatched. Every base hit is then owned by exactly one head scope: the head unit its unit matched, else the same scope in the file's head path. Each old occurrence pays for one new occurrence, never two. @@ -30,6 +34,7 @@ origin line survives, so nothing departed. from __future__ import annotations +import re from collections import Counter, defaultdict from collections.abc import Callable from difflib import SequenceMatcher @@ -41,6 +46,38 @@ from scripts.code_health.model import MODULE_SCOPE, FileMeasure, Finding, Hit, U Key = tuple[str, str] # (path, qualname) +# Bodies at least this similar (difflib ratio over their code lines) are the same unit edited. +# An edit of a few lines in a 40-line function scores ~0.9+; an unrelated body that happens to +# reuse a name shares little beyond its `def` line and scores well under 0.3. +SIMILAR = 0.6 +_ORDINAL = re.compile(r"#\d+$") + + +def _base_name(qual: str) -> str: + """``f#2`` -> ``f``: ordinals are source order, not identity.""" + return _ORDINAL.sub("", qual) + + +def _code(fm: FileMeasure, unit: Unit) -> tuple[str, ...] | None: + """The unit's code lines (comments and whitespace dropped), for similarity only.""" + end = unit.end_line + if end is None and "FUNC_LINES" in unit.metrics: + # The measurer did not report the end: its own lines from its first line approximate it. + end = unit.line + unit.metrics["FUNC_LINES"] - 1 + if end is None: + return None + return tuple(code for n in range(unit.line, end + 1) if (code := fm.code_line(n))) + + +def _similarity(old: tuple[str, ...] | None, new: tuple[str, ...] | None) -> float | None: + if old is None or new is None: + return None + matcher = SequenceMatcher(None, old, new, autojunk=False) + if matcher.real_quick_ratio() < SIMILAR or matcher.quick_ratio() < SIMILAR: + return 0.0 + return matcher.ratio() + + class _Matcher: def __init__(self, base: dict[str, FileMeasure], head: dict[str, FileMeasure], base_of: dict[str, str | None], head_of: dict[str, str | None]) -> None: @@ -76,7 +113,8 @@ class _Matcher: for step in (self._same_unchanged, self._same_file_body, self._moved_body): pending = [(hpath, q, u) for hpath, q, u in pending if not step(hpath, q, u)] self.same_body = set(self.match) - pending = [(hpath, q, u) for hpath, q, u in pending if not self._same_name(hpath, q, u)] + self._same_name(pending) + self._moved_edited([(hpath, q, u) for hpath, q, u in pending if (hpath, q) not in self.match]) return self.match def _same_unchanged(self, hpath: str, qual: str, unit: Unit) -> bool: @@ -104,12 +142,72 @@ class _Matcher: return True return False - def _same_name(self, hpath: str, qual: str, unit: Unit) -> bool: - bpath, bf = self._base_file(hpath) - if bpath is None or bf is None or qual not in bf.units or not self._free(bpath, qual): - return False - self._claim((hpath, qual), (bpath, bf.units[qual])) - return True + def _same_name(self, pending: list[tuple[str, str, Unit]]) -> None: + """Edited in place: same file, same name. Repeated names (`_`, `f#2`, `#3`) are + one group per base name, paired by body, so inserting a unit above another never + hands it the other's cap.""" + groups: dict[tuple[str, str], list[tuple[str, Unit]]] = defaultdict(list) + for hpath, qual, unit in pending: + groups[(hpath, _base_name(qual))].append((qual, unit)) + for (hpath, name), heads in sorted(groups.items()): + bpath, bf = self._base_file(hpath) + if bpath is None or bf is None: + continue + olds = [(q, u) for q, u in bf.units.items() if _base_name(q) == name and self._free(bpath, q)] + if len(heads) > 1 or len(olds) > 1: + heads, olds = self._pair_similar(hpath, bpath, heads, olds) + # The one unit left with a name is that name's unit, edited (as for a unique name). + if len(heads) == 1 and len(olds) == 1: + self._claim((hpath, heads[0][0]), (bpath, olds[0][1])) + + def _pair_similar(self, hpath: str, bpath: str, heads: list[tuple[str, Unit]], + olds: list[tuple[str, Unit]]) -> tuple[list[tuple[str, Unit]], list[tuple[str, Unit]]]: + """Pair the most similar bodies first; returns the units left unpaired.""" + hf, bf = self.head[hpath], self.base[bpath] + new_code = [_code(hf, u) for _, u in heads] + old_code = [_code(bf, u) for _, u in olds] + if None in new_code or None in old_code: # bodies unknown: the ordinal is all there is + old_by_qual = dict(olds) + for qual, _ in heads: + if qual in old_by_qual: + self._claim((hpath, qual), (bpath, old_by_qual[qual])) + return [], [] + scored = sorted((-(_similarity(o, n) or 0.0), hi, oi) + for hi, n in enumerate(new_code) for oi, o in enumerate(old_code)) + paired_h: set[int] = set() + paired_o: set[int] = set() + for neg_score, hi, oi in scored: + if -neg_score >= SIMILAR and hi not in paired_h and oi not in paired_o: + paired_h.add(hi) + paired_o.add(oi) + self._claim((hpath, heads[hi][0]), (bpath, olds[oi][1])) + return ([h for i, h in enumerate(heads) if i not in paired_h], + [o for i, o in enumerate(olds) if i not in paired_o]) + + def _moved_edited(self, pending: list[tuple[str, str, Unit]]) -> None: + """Moved to another file AND edited: the name left its origin file, exactly one unit + of that name left anywhere and exactly one arrived unmatched, and the bodies are + similar. A copy (origin keeps the name), an ambiguous name, or an unrelated body + reusing a deleted name stays new code.""" + gone: dict[str, list[tuple[str, Unit]]] = defaultdict(list) + for bpath, bf in sorted(self.base.items()): + hpath = self.head_of.get(bpath, bpath) + hf = self.head.get(hpath) if hpath else None + left = {_base_name(q) for q in hf.units} if hf else set() + for qual, unit in bf.units.items(): + if self._free(bpath, qual) and _base_name(qual) not in left: + gone[_base_name(qual)].append((bpath, unit)) + arrived: dict[str, list[tuple[str, str, Unit]]] = defaultdict(list) + for hpath, qual, unit in pending: + arrived[_base_name(qual)].append((hpath, qual, unit)) + for name, heads in sorted(arrived.items()): + olds = gone.get(name, []) + if len(heads) != 1 or len(olds) != 1: + continue + (hpath, qual, unit), (bpath, origin) = heads[0], olds[0] + score = _similarity(_code(self.base[bpath], origin), _code(self.head[hpath], unit)) + if score is not None and score >= SIMILAR: + self._claim((hpath, qual), (bpath, origin)) def _file_findings(path: str, hf: FileMeasure, bf: FileMeasure | None) -> list[Finding]: diff --git a/scripts/code_health/model.py b/scripts/code_health/model.py index a4079e6c18..f8054418df 100644 --- a/scripts/code_health/model.py +++ b/scripts/code_health/model.py @@ -17,6 +17,9 @@ class Unit: metrics: dict[str, int] body_hash: str parent: str | None = None # nearest enclosing function (nested defs are their own units) + # Last line of the unit, for comparing bodies of units that are not byte-identical. + # None when the measurer does not report it (the comparison then estimates the span). + end_line: int | None = None @dataclass(frozen=True) diff --git a/tests/scripts/test_code_health_identity.py b/tests/scripts/test_code_health_identity.py index e4e72a8170..953c2642ef 100644 --- a/tests/scripts/test_code_health_identity.py +++ b/tests/scripts/test_code_health_identity.py @@ -16,7 +16,10 @@ import pytest from scripts.code_health.ts_measure import pinned_typescript, resolve_typescript from tests.scripts.test_code_health import _LEGACY, _SWALLOW, REPO, _commit, _git, _repo, _verdict - +_EDITED = _LEGACY.replace("return 7\n", "return 77\n") # one constant changed, CC still 22 +# Same name and CC as _LEGACY, unrelated body: a new function reusing a deleted name. +_UNRELATED = "def legacy(x):\n" + "".join( + f" while x > {i}:\n x = x // 2 - {i}\n" for i in range(21)) + " return x\n" _MODULE_SWALLOW = "try:\n import json\nexcept Exception:\n pass\n" @@ -52,6 +55,61 @@ def test_module_level_hits_follow_code_moved_to_another_file(tmp_path, capsys, f assert code == (1 if blocks else 0), out +# --- M2: a function moved to another file AND edited keeps its cap ---------------------- + +_LEGACY_SW = _LEGACY.replace("def legacy(x):\n", "def legacy(x):\n" + _SWALLOW.split("\n", 1)[1]) + + +@pytest.mark.parametrize("extra_base, files, blocks", [ + # P1: moved with one constant changed (CC still 22) + ({}, {"pkg/a.py": _SWALLOW, "pkg/a_legacy.py": _EDITED}, False), + # P2 (control): the same edit in place + ({}, {"pkg/a.py": _EDITED + "\n\n" + _SWALLOW}, False), + # moved, edited and grown: the cap is the old value, not a pass + ({}, {"pkg/a.py": _SWALLOW, "pkg/a_legacy.py": _EDITED + " if x == 99:\n return 99\n"}, True), + # a copy (the origin still has it) is new code + ({}, {"pkg/a_legacy.py": _EDITED}, True), + # an unrelated function that reuses the deleted name is new code + ({}, {"pkg/a.py": _SWALLOW, "pkg/a_legacy.py": _UNRELATED}, True), + # two arrivals with that name: neither is provably the moved one + ({}, {"pkg/a.py": _SWALLOW, "pkg/b.py": _EDITED, "pkg/c.py": _EDITED}, True), + # the moved+edited function keeps its existing swallow, but not a second one + ({"pkg/a.py": _LEGACY_SW + "\n\n" + _SWALLOW}, + {"pkg/a.py": _SWALLOW, "pkg/a_legacy.py": _LEGACY_SW.replace("return 7\n", "return 77\n")}, False), + ({"pkg/a.py": _LEGACY_SW + "\n\n" + _SWALLOW}, + {"pkg/a.py": _SWALLOW, "pkg/a_legacy.py": _LEGACY_SW.replace("return 7\n", "return 77\n") + + " try:\n pass\n except Exception:\n pass\n"}, True), +]) +def test_function_moved_and_edited_keeps_its_cap(tmp_path, capsys, extra_base, files, blocks): + repo, base = _base(tmp_path, extra_base) if extra_base else _repo(tmp_path) + code, out = _verdict(repo, base, files, capsys) + assert code == (1 if blocks else 0), out + + +# --- B3: repeated names pair by body, not by ordinal (banditburai) ------------------------ + +_DECORATOR = "def method(name):\n return lambda f: f\n" + + +def _handler(route: str, branches: int, fmt: str = " if x == {i}:\n return {i}\n") -> str: + return f'\n\n@method("{route}")\ndef _(x):\n' + "".join(fmt.format(i=i) for i in range(branches)) + " return x\n" + + +@pytest.mark.parametrize("files, blocks", [ + # a new CC-22 `_` above the old one (trimmed to 20) is new code + ({"pkg/h.py": _DECORATOR + _handler("new", 21, ' if x == "s{i}":\n return "s{i}" * 2\n') + + _handler("old", 19)}, True), + # a small `_` above the old one, whose CC-22 body only changes a literal, keeps its cap + ({"pkg/h.py": _DECORATOR + _handler("new", 0) + _handler("old", 21).replace("return 7\n", "return 77\n")}, False), + # control: the old handler grows; its cap is 22 wherever it sits + ({"pkg/h.py": _DECORATOR + _handler("new", 0) + _handler("old", 22)}, True), +]) +def test_repeated_python_names_pair_by_body(tmp_path, capsys, files, blocks): + repo, base = _base(tmp_path, {"pkg/h.py": _DECORATOR + _handler("old", 21)}) + code, out = _verdict(repo, base, files, capsys) + assert code == (1 if blocks else 0), out + + # --- TypeScript identity ---------------------------------------------------------------- def _ts_base(tmp_path: Path, files: dict[str, str | None]) -> tuple[Path, str]: @@ -127,6 +185,26 @@ def test_ts_identity_ignores_comments(tmp_path, capsys, files, blocks): assert code == (1 if blocks else 0), out +_RUN_DECL = "declare function run(cb: (x: number) => number): void\n" +_SMALL_CB = "run((x: number) => {\n return x + 1\n})\n" +_BIG_CB = "run((x: number) => {\n" + _ifs(21, indent=" ") + " return -1\n})\n" + + +@pytest.mark.parametrize("files, blocks", [ + # a new callback above shifts every `#n`; the edited CC-22 callback keeps its cap + ({"web/c.ts": _RUN_DECL + "run((x: number) => {\n return x * 2\n})\n" + _SMALL_CB + + _BIG_CB.replace("return 7\n", "return 77\n")}, False), + # a new CC-22 callback above, the old one trimmed to 20: the new one is new code + ({"web/c.ts": _RUN_DECL + "run((x: number) => {\n" + "".join( + f" while (x > {i}) x = x / 2 - {i}\n" for i in range(21)) + " return x\n})\n" + + _SMALL_CB + _BIG_CB.replace(_ifs(2, 19, " "), "")}, True), +]) +def test_ts_anonymous_callbacks_pair_by_body(tmp_path, capsys, files, blocks): + repo, base = _ts_base(tmp_path, {"web/c.ts": _RUN_DECL + _SMALL_CB + _BIG_CB}) + code, out = _verdict(repo, base, files, capsys) + assert code == (1 if blocks else 0), out + + # --- m5: the TypeScript install honours the repo's .npmrc and fails as a RuntimeError ----- def test_typescript_install_uses_repo_npmrc_and_fails_cleanly(tmp_path, monkeypatch):