Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions .github/scripts/constant_rename_equivalence.py
Original file line number Diff line number Diff line change
Expand Up @@ -793,10 +793,19 @@ def scope_modules(base: RevisionIndex, head: RevisionIndex, everything: bool) ->
"""Modules to compare, plus the renamed/removed constants that put them
there. Scope is deliberately narrow: the modules that touch a constant
whose DECLARED NAME changed between the two revisions. `--all` widens it
to every module mentioning the pattern."""
to every module mentioning the pattern.

A pure addition (a name present only at head) is NOT in scope: the
module declaring a brand-new constant necessarily differs from base,
so whole-module AST equality could never hold for it, and additions
were never what this gate proves. A removal or a rename both still
land here, because the old name is base-only either way — for a
rename, pulling the old name into scope compares the SAME module
that now declares the new name, so the equivalence proof still
covers it."""
base_names = base.declared_matching()
head_names = head.declared_matching()
moved = {n: base_names.get(n) or head_names.get(n) or "?" for n in set(base_names) ^ set(head_names)}
moved = {n: base_names[n] for n in set(base_names) - set(head_names)}

shared = sorted(set(base.sources) & set(head.sources))
if everything:
Expand Down Expand Up @@ -988,10 +997,18 @@ def main(argv: list[str] | None = None) -> int:
return len(findings)

if moved:
print(f"Constants whose declared name changed between {base_rev} and {head_rev}:")
print(f"Constants removed or renamed away between {base_rev} and {head_rev}:")
for name in sorted(moved):
side = "base only" if name in base.declared_matching() else "head only"
print(f" - {name} ({side}, {moved[name]})")
print(f" - {name} (was declared in {moved[name]})")
# Informational only — NOT part of scope. A rename's new name lands
# here too (it is head-only), so an operator reading a scope failure
# can see both halves; a pure addition unrelated to any removal above
# also lands here and is indistinguishable from one by name alone.
added = sorted(set(head.declared_matching()) - set(base.declared_matching()))
if added:
print(f"\nConstants declared only at {head_rev} (informational, not compared):")
for name in added:
print(f" - {name} ({head.declared_matching()[name]})")
print()

findings += check_equivalence(base, head, targets)
Expand Down
69 changes: 66 additions & 3 deletions .github/scripts/tests/test_constant_rename_equivalence.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,16 +153,48 @@ def test_fstring_prefix_extraction_is_clean(self):
code, out = compare(root)
self.assertEqual(code, 0, out)

def test_fstring_prefix_change_is_caught(self):
base = {"m.py": 'def f(reason):\n return f"phash-{reason}"\n'}
def test_fstring_prefix_change_is_caught_when_bundled_with_a_rename(self):
# An isolated extraction — nothing else matching the pattern renamed
# or removed anywhere in the module — cannot be told apart, by
# declared-name diffing alone, from a harmless brand-new constant for
# brand-new logic (see test_an_isolated_extraction_with_a_wrong_value_
# is_a_known_scope_blind_spot below), so it needs a companion rename
# in the SAME module to land in scope — exactly how it happens in
# practice: PR #567 introduced LANDS_PHASH_SKIP_REASON_PREFIX in the
# same file as dozens of other real renames.
base = {
"m.py": 'A_SKIP_REASON = "a"\n\n\ndef f(x, reason):\n return A_SKIP_REASON if x else f"phash-{reason}"\n'
}
head = {
"m.py": 'PHASH_SKIP_REASON_PREFIX = "phash2-"\n\n\ndef f(reason):\n return f"{PHASH_SKIP_REASON_PREFIX}{reason}"\n'
"m.py": 'B_SKIP_REASON = "a"\nPHASH_SKIP_REASON_PREFIX = "phash2-"\n\n\n'
'def f(x, reason):\n return B_SKIP_REASON if x else f"{PHASH_SKIP_REASON_PREFIX}{reason}"\n'
}
with _repo(base, head) as root:
code, out = compare(root)
self.assertEqual(code, 1, out)
self.assertIn("m.py", out)

def test_an_isolated_extraction_with_a_wrong_value_is_a_known_scope_blind_spot(self):
# KNOWN LIMITATION, pinned deliberately, not a bug: base declares no
# matching constant at all in this module, so nothing "disappeared"
# from base and scope_modules correctly treats this exactly like a
# harmless brand-new constant for brand-new logic (see
# test_pure_addition_of_a_new_matching_constant_is_out_of_scope in
# TestScopeAndNotes) — the two shapes are provably identical under
# name-diffing; there is no way to tell them apart without also
# accepting whole-module comparisons for every unrelated new-feature
# PR that happens to add a matching-pattern constant, which is the
# false-positive class this gate exists to avoid. Pinned so a future
# scope_modules change doesn't silently start "catching" this again
# without someone noticing the tradeoff moved back the other way.
base = {"m.py": 'def f(reason):\n return f"phash-{reason}"\n'}
head = {
"m.py": 'PHASH_SKIP_REASON_PREFIX = "phash2-"\n\n\ndef f(reason):\n return f"{PHASH_SKIP_REASON_PREFIX}{reason}"\n'
}
with _repo(base, head) as root:
code, out = compare(root)
self.assertEqual(code, 0, out)

def test_string_concatenation_is_folded(self):
base = {"m.py": 'def f():\n return "phash-" + "miss"\n'}
head = {"m.py": 'PHASH_MISS_SKIP_REASON = "phash-miss"\n\n\ndef f():\n return PHASH_MISS_SKIP_REASON\n'}
Expand Down Expand Up @@ -521,6 +553,37 @@ def test_default_base_falls_back_when_there_is_no_origin(self):
code, out = run_tool(root, "--head", "HEAD")
self.assertEqual(code, 0, out)

def test_pure_addition_of_a_new_matching_constant_is_out_of_scope(self):
# scope_modules used to take a SYMMETRIC difference of declared
# names, so a constant added at head with nothing removed anywhere
# put its own module in scope — and that module's AST can never
# equal base's, since base never declared the new name. Any PR
# merely adding a matching constant would fail this gate permanently.
base = {"m.py": "def f():\n return 1\n"}
head = {"m.py": 'NEW_SKIP_REASON = "x"\n\n\ndef f():\n return NEW_SKIP_REASON\n'}
with _repo(base, head) as root:
code, out = compare(root)
self.assertEqual(code, 0, out)
self.assertIn("nothing to prove", out)

def test_rename_still_puts_the_module_in_scope_and_catches_a_real_diff(self):
base = {"m.py": 'A_SKIP_REASON = "x"\n\n\ndef f(v):\n return A_SKIP_REASON if v else ""\n'}
head = {"m.py": 'B_SKIP_REASON = "x"\n\n\ndef f(v):\n return B_SKIP_REASON if not v else ""\n'}
with _repo(base, head) as root:
code, out = compare(root)
self.assertEqual(code, 1, out)
self.assertIn("A_SKIP_REASON", out)
self.assertIn("m.py", out)

def test_pure_removal_still_puts_the_module_in_scope(self):
base = {"m.py": 'A_SKIP_REASON = "x"\n\n\ndef f():\n return A_SKIP_REASON\n'}
head = {"m.py": 'def f():\n return "x"\n'}
with _repo(base, head) as root:
code, out = compare(root)
self.assertEqual(code, 0, out)
self.assertIn("A_SKIP_REASON", out)
self.assertNotIn("nothing to prove", out)


class TestNormaliserUnits(unittest.TestCase):
def test_first_difference_locates_a_node_path(self):
Expand Down
Loading
Loading