diff --git a/tests/guards/test_debt_ratchet.py b/tests/guards/test_debt_ratchet.py index b23a660f2f..7635f5863d 100644 --- a/tests/guards/test_debt_ratchet.py +++ b/tests/guards/test_debt_ratchet.py @@ -921,6 +921,21 @@ def test_an_unchanged_absence_on_a_rewritten_line_is_not_written_but_a_new_one_i assert report["introduced_lines"][identity] == [2] +def test_an_absence_moved_with_its_class_to_another_module_is_not_written(repo): + holder = "class Holder:\n def check(self, v):\n return v is None\n" + other = "class Other:\n def check(self, v):\n return v is None\n" + base = repo.commit({"attempts.py": holder, "executions.py": "VALUE = 1\n"}) + moved = repo.commit({"attempts.py": "VALUE = 2\n", "executions.py": holder}) + status, report = repo.compare(base, moved) + identity = f"{NoneAbsence.__name__}:{repo.root}" + assert report["introduced"][f"{identity}/executions.py"] == 0 + deleted = repo.commit({"attempts.py": holder, "executions.py": None}) + moved_out = repo.commit({"attempts.py": None, "owners.py": holder + "\n\n" + other}) + status, report = repo.compare(deleted, moved_out) + assert status == 1 + assert report["introduced_lines"][f"{identity}/owners.py"] == [8] + + def test_git_context_configuration_cannot_mark_unchanged_lines_as_new(repo): repo.git("config", "diff.interHunkContext", "3") base = repo.commit({"names.py": "first = 1\nold_receipt = 1\nlast = 1\n"}) diff --git a/tools/debt_ratchet.py b/tools/debt_ratchet.py index e7bd8e1e65..0ae53e8ce8 100644 --- a/tools/debt_ratchet.py +++ b/tools/debt_ratchet.py @@ -495,6 +495,14 @@ def read(cls, repo: Path, base: str, head: str, path: str) -> ChangedSource: base_source = git(repo, "show", f"{base}:{path}") if listed else b"" return cls(path, git(repo, "show", f"{head}:{path}"), frozenset(lines), base_source) + @classmethod + def departed(cls, repo: Path, base: str, head: str, path: str) -> ChangedSource: + """A changed module present at the base, read for what left it; a module the head + deleted is empty there.""" + listed = git(repo, "ls-tree", "--name-only", head, "--", path).strip() + source = git(repo, "show", f"{head}:{path}") if listed else b"" + return cls(path, source, frozenset(), git(repo, "show", f"{base}:{path}")) + class ScopeNames(ast.NodeVisitor): """Each node's enclosing class and function names, dotted, through the AST's own dispatch.""" @@ -528,7 +536,9 @@ class CleanChangedLines(OccurrenceMeasure): An occurrence is identified by its enclosing scope and its source text, so rewriting the rest of a line around an unchanged occurrence writes nothing new, while a different - occurrence elsewhere never pays for a new one. + occurrence elsewhere never pays for a new one. Scopes are class and function names, not + modules, so an occurrence that leaves one changed module and arrives in another, as a + class moved between files, is the same occurrence moved, not a new one written. """ @staticmethod @@ -536,11 +546,24 @@ def site_lines(node: ast.AST) -> range: return range(node.lineno, node.end_lineno + 1) @staticmethod - def introduced(repo: Path, base: str, head: str, path: str) -> dict[str, list[int]]: - """Each strict measure's sites on the path's changed lines.""" - source = ChangedSource.read(repo, base, head, path) - return {f"{measure.__name__}:{path}": measure.changed_sites(source) - for measure in Measure.members_with(CleanChangedLines)} + def introduced( + repo: Path, base: str, head: str, paths: tuple[str, ...], departed_from: tuple[str, ...], + ) -> dict[str, list[int]]: + """Each strict measure's sites on each path's changed lines. Occurrences that left the + changed modules in ``departed_from`` (those present at the base) pay for the same + occurrences arriving elsewhere.""" + sources = [ChangedSource.read(repo, base, head, path) for path in paths] + left = [ChangedSource.departed(repo, base, head, path) for path in departed_from] + found: dict[str, list[int]] = {} + for measure in Measure.members_with(CleanChangedLines): + departed: Counter[tuple[str, str]] = Counter() + for source in left: + gone = measure.identities(source.base_tree, source.path) + gone.subtract(measure.identities(source.tree, source.path)) + departed.update(+gone) + for source in sources: + found[f"{measure.__name__}:{source.path}"] = measure.changed_sites(source, departed) + return found @staticmethod def scopes(tree: ast.AST) -> dict[ast.AST, str]: @@ -560,8 +583,9 @@ def identities(cls, tree: ast.AST, filename: str) -> Counter[tuple[str, str]]: return found @classmethod - def changed_sites(cls, source: ChangedSource) -> list[int]: - """The first changed line of each occurrence written there that the base lacks.""" + def changed_sites(cls, source: ChangedSource, departed: Counter[tuple[str, str]]) -> list[int]: + """The first changed line of each occurrence written there that the base lacks and no + changed module gave up; a moved occurrence spends the departure it came from.""" scopes = cls.scopes(source.tree) unwritten = cls.identities(source.tree, source.path) unwritten.subtract(cls.identities(source.base_tree, source.path)) @@ -570,9 +594,11 @@ def changed_sites(cls, source: ChangedSource) -> list[int]: if ((count := cls.occurrences(node)) and (touched := source.lines.intersection(cls.site_lines(node)))): identity = scopes[node], ast.unparse(node) - written = max(0, min(count, unwritten[identity])) - unwritten[identity] -= written - sites.extend([min(touched)] * written) + arrived = max(0, min(count, unwritten[identity])) + unwritten[identity] -= arrived + moved = min(arrived, departed[identity]) + departed[identity] -= moved + sites.extend([min(touched)] * (arrived - moved)) return sites @@ -627,8 +653,9 @@ def count(cls, source: bytes, filename: str) -> int: return sum(cls.occurrences(node) for node in ast.walk(module.tree)) + len(cls.prose_lines(module)) @classmethod - def changed_sites(cls, source: ChangedSource) -> list[int]: - return [*super().changed_sites(source), *(cls.prose_lines(source) & source.lines)] + def changed_sites(cls, source: ChangedSource, departed: Counter[tuple[str, str]]) -> list[int]: + prose = cls.prose_lines(source) & source.lines + return [*super().changed_sites(source, departed), *prose] @dataclass(frozen=True) @@ -1936,8 +1963,11 @@ def compare(repo: Path, base: str, head: str, root: str, workers: int) -> Compar parts = {measure: [pool.submit(part) for part in measure.parts(repo, base, head, paths, root)] for measure in measures} - sites = [pool.submit(CleanChangedLines.introduced, repo, base, head, path) - for path in sorted(paths & python_paths(repo, head, root))] + sites = [pool.submit( + CleanChangedLines.introduced, repo, base, head, + tuple(sorted(paths & python_paths(repo, head, root))), + tuple(sorted(paths & python_paths(repo, base, root))), + )] # Each measure combines as soon as its parts finish, while slower checkers still run. measured = {} owners = {future: measure for measure, futures in parts.items() for future in futures}