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 `<anon>#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.
This commit is contained in:
Teknium
@@ -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 `<anon>#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`, `<anon>#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]:
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 `<anon>#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):
|
||||
|
||||
Reference in New Issue
Block a user