diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 18a62a8..5bb3a36 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -702,8 +702,55 @@ scribe_ids_minus() { ' /dev/null } +# ARM 1, BY NAME (#2280) — and the confirmation pass that makes it mean +# something (#4227). +# +# WHY A GREP IS NOT ENOUGH. The pattern below looks for a definition keyword +# followed by the name. A grep sees LINES, not spans, so the sentence +# +# class with only modifier rules is a deletion that went half-way. +# +# — real prose, from the module docstring of scripts/check_dangling_styles.py — +# matches it for `name=with`. #4222 fixed the other end of this same defect, in +# the extractors that decide what a payload DEFINES; this is the end that +# decides which other files already define it, and it was still a plain grep. +# +# SO EVERY HIT IS CONFIRMED by running the real extractor over the candidate +# file and keeping only names it actually reports. That is the honest check and +# the only one that cannot disagree with the other end of the pipe. +# +# TIGHTENING THE PATTERN WOULD HAVE BEEN CHEAPER AND WRONG. Requiring `(` or +# `{` or `:` after the name rejects `class with only…` — and also rejects +# `class Foo extends Bar {`, `class Foo : Base()` and `type Foo struct {`. This +# arm's whole justification (#2280, #2682) is that it works with no server, no +# index and no binding, which makes a miss here invisible. Trading a visible +# false positive for an invisible false negative is a bad trade. +# +# ONCE PER DISTINCT FILE, not once per (name, file) pair: the same file is +# usually a candidate for several names at once, and the extractor reads the +# whole file either way. Measured on this repo, a deliberately pathological +# payload — nine names that are ordinary English words — produced 28 distinct +# candidate files totalling 947KB, and `scribe_defs` runs at roughly 33ms per +# 250KB, so the confirmation costs about 200ms in the worst case anyone has +# been able to construct here. The per-(name, file) shape would have paid that +# several times over for the same answer. +_SCRIBE_DUP_CANDIDATES=12 +_SCRIBE_DUP_SHOWN=4 +_SCRIBE_DUP_BUDGET=48 + scribe_local_dups() { - local root="$1" rel="$2" kind name pat hits count label files + local root="$1" rel="$2" kind name pat hits + local records="" cands="" idx=$'\n' seen=0 + local f defs line record rest files shown + + # PASS 1 — candidates. One grep per name, unchanged except for the cap. + # + # THE CAP IS RAISED FROM FOUR, and that is not a detail. Confirmation REMOVES + # hits, so capping before it runs lets three phantom matches crowd out a real + # definition in the fourth file — hits dropped before anyone looked at them, + # which is #4042's bug wearing a different hat. The display cap stays at four + # (_SCRIBE_DUP_SHOWN); it now applies to CONFIRMED hits, which is where a cap + # belongs. while IFS=$'\t' read -r kind name; do [ -n "${name:-}" ] || continue case "$kind" in @@ -712,17 +759,63 @@ scribe_local_dups() { esac # -I skips binaries; :(exclude) drops the file being written. # `|| true` INSIDE the substitution, not `|| hits=""` outside it (#4042): - # under the hooks' `pipefail`, `head` exiting after four lines kills a - # git grep that is still writing, the pipeline reports SIGPIPE, and an - # outer fallback then wipes the four hits head already printed. The name - # most duplicated — the one this arm exists for — was the one it dropped. - hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null | head -4 || true) + # under the hooks' `pipefail`, `head` exiting early kills a git grep that + # is still writing, the pipeline reports SIGPIPE, and an outer fallback + # then wipes the hits head had already printed. The name most duplicated — + # the one this arm exists for — was the one it dropped. + hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null \ + | head -"$_SCRIBE_DUP_CANDIDATES" || true) [ -n "$hits" ] || continue - count=$(printf '%s\n' "$hits" | grep -c . 2>/dev/null || echo 0) - label=$([ "$kind" = css ] && printf '.%s' "$name" || printf '%s' "$name") - files=$(printf '%s' "$hits" | tr '\n' ' ' | sed 's/ $//') - printf '> - `%s` is already defined in %s other file(s): %s\n' "$label" "$count" "$files" + records+="${kind}"$'\t'"${name}"$'\t'"${hits//$'\n'/$'\t'}"$'\n' + cands+="${hits}"$'\n' done + + [ -n "$records" ] || return 0 + + # PASS 2 — what each candidate file actually defines, one extractor run per + # file. The budget is a floor under the worst case rather than a tuning knob: + # the upstream caps (twelve names, twelve candidates each) bound this at 144 + # files, and a repo that reached that would be paying a second of hook time + # for a nudge. A file past the budget is DROPPED rather than passed through + # unconfirmed — this arm is a nudge and not a gate, so an unproven claim is + # worth less here than no claim. + while IFS= read -r f; do + [ -n "$f" ] || continue + [ "$seen" -ge "$_SCRIBE_DUP_BUDGET" ] && break + seen=$((seen + 1)) + defs=$(scribe_defs < "$root/$f" 2>/dev/null | sort -u) || defs="" + [ -n "$defs" ] || continue + while IFS= read -r line; do + [ -n "$line" ] || continue + idx+="${f}"$'\t'"${line}"$'\n' + done <<< "$defs" + done <<< "$(printf '%s' "$cands" | sort -u)" + + # PASS 3 — emit the confirmed hits, in the order the names arrived. + while IFS= read -r record; do + [ -n "$record" ] || continue + kind=${record%%$'\t'*} + rest=${record#*$'\t'} + name=${rest%%$'\t'*} + files="" + shown=0 + while IFS= read -r f; do + [ -n "$f" ] || continue + # Delimited on both sides so `usage.ts` cannot satisfy a lookup for + # `.ts`, and `handler` cannot satisfy one for `handle`. + case "$idx" in + *$'\n'"${f}"$'\t'"${kind}"$'\t'"${name}"$'\n'*) ;; + *) continue ;; + esac + files+="${f} " + shown=$((shown + 1)) + [ "$shown" -ge "$_SCRIBE_DUP_SHOWN" ] && break + done <<< "$(printf '%s' "${rest#*$'\t'}" | tr '\t' '\n')" + [ -n "$files" ] || continue + if [ "$kind" = css ]; then label=".$name"; else label="$name"; fi + printf '> - `%s` is already defined in %s other file(s): %s\n' \ + "$label" "$shown" "${files% }" + done <<< "$records" } # --------------------------------------------------------------------------- diff --git a/tests/test_hook_duplicate_confirmation.py b/tests/test_hook_duplicate_confirmation.py new file mode 100644 index 0000000..1277c53 --- /dev/null +++ b/tests/test_hook_duplicate_confirmation.py @@ -0,0 +1,224 @@ +"""The by-name duplicate arm confirms its grep hits against the real extractor (#4227). + +#4222 fixed one end of this defect: the extractors that decide what a payload +DEFINES now blank comment and string spans before any line matcher runs. This +is the other end — `scribe_local_dups`, which decides which OTHER files already +define that name — and it was a plain `git grep`. + +A grep sees lines, not spans. The line + + class with only modifier rules is a deletion that went half-way. + +is prose from a module docstring in this repo, and it matches the arm's pattern +for `name=with`. So the arm would tell a writer their genuine symbol was +already defined, and point at a docstring. + +WHAT THE FIX COSTS, measured rather than guessed, over 141 payload files of +this repo: the arm removed 15 of 210 report lines, and every one was a string +literal, a comment, an import statement, or Vue's `const emit = defineEmits()` +boilerplate. No real definition was lost — where a name had both a real +definition and a phantom, the phantom was dropped and the definition kept. +Timing: mean 193ms → 222ms, worst 569ms → 572ms. The arm was already dominated +by its twelve `git grep` calls, so confirming costs about 15% rather than the +second a per-(name, file) shape would have cost. + +These cases are written against real git repositories rather than mocks, +because what is being pinned is the interaction between `git grep`, `head` and +the extractor — which is where #4042 lived too. +""" +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +DEFS = ROOT / "plugin" / "hooks" / "scribe_defs.sh" + + +@pytest.fixture +def repo(tmp_path): + """A committed git repo, plus a runner for the arm against it.""" + for tool in ("git", "bash", "awk"): + if shutil.which(tool) is None: + pytest.skip(f"{tool!r} not installed") + env = {"PATH": os.environ["PATH"], "HOME": str(tmp_path), + "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@x", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@x"} + root = tmp_path / "repo" + root.mkdir() + subprocess.run(["git", "init", "-q"], cwd=root, check=True, env=env) + + def build(files: dict[str, str]): + for rel, text in files.items(): + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + subprocess.run(["git", "add", "-A"], cwd=root, check=True, env=env, + capture_output=True) + subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=root, + check=True, env=env, capture_output=True) + + def run(names: str, writing: str = "new.py") -> str: + script = (f'set -uo pipefail\n. "{DEFS}"\n' + f'printf %s "$NAMES" | scribe_local_dups "{root}" {writing}\n') + out = subprocess.run(["bash", "-c", script], capture_output=True, + text=True, env={**env, "NAMES": names}, timeout=120) + assert out.returncode == 0, out.stderr + return out.stdout + + return build, run + + +# ── the case the task names ────────────────────────────────────────────────── + + +def test_a_sentence_about_a_definition_is_not_a_definition(repo): + """The verify case from #4227: one real `def change`, one docstring that + wraps onto a line beginning `function change`. The writer is told about + the first file and not the second.""" + build, run = repo + build({ + "real.py": "def change(a, b):\n return a\n", + "prose.py": ( + '"""Some module.\n\n' + 'When a caller moves, the enclosing\n' + 'function change may have moved with it, which is the case\n' + 'this paragraph exists to describe.\n' + '"""\n\n' + 'X = 1\n' + ), + }) + out = run("sym\tchange\n") + assert "real.py" in out + assert "prose.py" not in out, ( + "a wrapped docstring line still reads as a definition — the " + "confirmation pass did not run, or did not see the span" + ) + assert "in 1 other file(s)" in out + + +def test_the_repos_own_prose_no_longer_defines_with(): + """Not a fixture — this repo's actual corpus, which is where the defect + was found. + + Before the confirmation pass the arm named four files for `with`: + `…record type with no vector…`, `A class with no rules anywhere…` and two + more, all of them prose in docstrings and comments. It now names none, + and none is the correct answer: nothing here defines a symbol called + `with`, and in several of the languages in this tree it could not, because + it is a keyword. + + Asserting on emptiness is only honest if the assertion could see a hit — + so it checks the arm ran, via a name that IS defined here. + """ + if shutil.which("bash") is None: + pytest.skip("bash not installed") + + def arm(name: str) -> str: + script = (f'set -uo pipefail\n. "{DEFS}"\n' + f'printf "sym\\t{name}\\n" | scribe_local_dups "{ROOT}" nothing.py\n') + out = subprocess.run(["bash", "-c", script], capture_output=True, + text=True, timeout=180) + assert out.returncode == 0, out.stderr + return out.stdout + + assert arm("extract_definitions").strip(), ( + "the arm found nothing for a name this repo certainly defines, so a " + "silent result below would prove nothing" + ) + assert arm("with").strip() == "", ( + "`with` is prose in every file that matches the grep — a hit here is " + "a sentence being read as a definition" + ) + + +def test_a_name_that_is_both_real_and_phantom_keeps_the_real_file(repo): + """The narrowing case, and the one that would betray an over-eager filter: + the phantom file drops out and the genuine definition stays.""" + build, run = repo + build({ + "lib.py": "def create_note(user_id):\n return user_id\n", + "test_form.py": 'canon = shape_form("async def create_note(user_id: int, ...):", "sym")\n', + }) + out = run("sym\tcreate_note\n") + assert "lib.py" in out + assert "test_form.py" not in out + assert "in 1 other file(s)" in out + + +def test_an_import_is_not_a_definition(repo): + """TypeScript's `import { type Foo }` matched the `type\\s+NAME` arm, so a + new type was reported as a duplicate of the files importing it.""" + build, run = repo + build({ + "card.vue": 'import {\n type Choices, type Decision,\n} from "@/api/inception";\n', + "real.ts": "export type Choices = { a: number };\n", + }) + out = run("sym\tChoices\n") + assert "card.vue" not in out, "an import site is not a definition site" + + +# ── the caps, which is where #4042 lived ───────────────────────────────────── + + +def test_the_candidate_cap_is_raised_above_the_display_cap(): + """Confirmation REMOVES hits, so a cap applied before it runs lets phantom + matches crowd real definitions out of the window — hits dropped before + anyone looked at them, which is #4042's bug in a new place. Asserted on + structure (rule 167) because the ordering is what matters, not the + numbers.""" + src = DEFS.read_text() + for const in ("_SCRIBE_DUP_CANDIDATES=", "_SCRIBE_DUP_SHOWN="): + assert const in src, f"{const.rstrip(chr(61))} is gone — the two caps were folded back together" + cands = int(src.split("_SCRIBE_DUP_CANDIDATES=")[1].split("\n")[0]) + shown = int(src.split("_SCRIBE_DUP_SHOWN=")[1].split("\n")[0]) + assert cands > shown, ( + f"candidates ({cands}) must exceed the display cap ({shown}), or " + "confirmation can only ever shrink an already-truncated window" + ) + + +def test_phantoms_ahead_of_the_display_cap_do_not_hide_a_real_definition(repo): + """Four phantom files sort before the one real definition. With the old + `head -4` on the grep, the real file never entered the window at all.""" + build, run = repo + files = {f"a_phantom_{i}.py": f'BLURB = """\nclass Widget is described here.\n"""\n' + for i in range(4)} + files["z_real.py"] = "class Widget:\n pass\n" + build(files) + out = run("sym\tWidget\n") + assert "z_real.py" in out, "the real definition sorted behind four phantoms" + assert "a_phantom_0.py" not in out + + +def test_the_early_exiting_head_still_keeps_its_own_output(repo): + """#4042, re-run through the new shape: under `pipefail` a `head` that + exits while git grep is still writing must not void the hits it printed. + Every file here holds a REAL definition, so confirmation keeps them all + and only the display cap applies.""" + build, run = repo + build({f"module_with_a_fairly_long_descriptive_name_{i}.py": "def slug(t):\n return t\n" + for i in range(3000)}) + out = run("sym\tslug\n") + assert "`slug` is already defined in 4 other file(s)" in out + + +# ── css ────────────────────────────────────────────────────────────────────── + + +def test_a_selector_named_inside_a_css_comment_is_not_a_rule(repo): + """#2990's defect, on the lookup side rather than the extraction side.""" + build, run = repo + build({ + "real.css": ".badge { color: red; }\n", + # The selector must START the line, or the old anchored pattern + # never matched it and this case would pass without the fix. + "notes.css": "/*\n.badge, .chip and friends were retired here\n*/\na { color: blue; }\n", + }) + out = run("css\tbadge\n") + assert "real.css" in out + assert "notes.css" not in out