diff --git a/TECHNICAL.md b/TECHNICAL.md index 13db91b..e2308ce 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -1885,6 +1885,40 @@ survive a version upgrade indefinitely. #66 is fixed; the alias blind spot registries pin `codegraph_extraction_version`, and `integrity.check` warns — naming `codegraph index`, never `sync` — when the index predates it. +**The third correction: the oracle was stricter than its own rule (#95, #96, #100).** +The module's rule is that the oracle refuses an edge only when no syntax in the +caller could denote the callee. In practice it refused the commonest real shapes, +and each refusal was a confident false accusation waiting for an untested branch. +- **Method callee:** any `x.` reference now supports it, whatever the + receiver is (a local, a parameter, `Svc.build()`, `A().run()`). This is the + coarse rule `self.`/`cls.` already had. +- **Module-level function callee:** still strict, so the #66 shape + (`rows.setdefault(k, []).append(v)` against `ledger.append`) still refuses. +- **Aliased imports** (`from .svc import append as ap; ap()`) resolve to the + original name. +- **Sibling closures** (a nested def calling another nested def in the same + enclosing function) count as support. testgraph's own run found this one: + `close_block → note`, its only remaining oracle gap. +- **Traced `co_qualname`** (`A.run`, `outer..git`) is matched against + CodeGraph's `qualified_name` (`A::run`, `outer::git`). Before, it was matched + by bare name, so `B.run` counted as run and became a proof-grade STATIC MISS. + A symbol goes to `ambiguous_symbols` when there are several same-named nodes + with no qualified match, or when one node's qualified name contradicts the + trace (`B::run` for a traced `A.run`). Its candidates still count as present for + the runtime witness, which must over-state presence: dropping them would turn + oracle gaps into confirmed phantoms. They are left out of the static-miss count, + where crediting a node that didn't run is a false proof. +- **`find()`** judges the union of every same-named def and ignores the indexed + `start_line`. On a stale index the def on that line can be a different + function, or a same-named method of another class. +- **`from . import x as y`** resolves too. The empty module means the package + itself, as it already did without the alias. + +On testgraph's own suite (535 tests traced): 55 static misses before and after. +Findings went from 2 (an oracle gap on `close_block → note` and a source-only +phantom on `harness/ast_oracle.py:_is_test`, a real +`from testgraph.select import _is_test as _sel_is_test` alias) to 0. + ### Phase 6 — `verify`, and the gate that redesigned it (PRs #88, #89) The plan was: select journeys, expand them to tests via Phase 4's stored trace diff --git a/testgraph/reconcile.py b/testgraph/reconcile.py index 11aeb5e..ca30234 100644 --- a/testgraph/reconcile.py +++ b/testgraph/reconcile.py @@ -118,7 +118,13 @@ def file_map(conn): } -def resolve_traced(conn, symbols, files): +def _qualified(qualname): + """The tracer's `co_qualname` (`A.run`, `outer..git`) in CodeGraph's + `qualified_name` form (`A::run`, `outer::git`).""" + return "::".join(p for p in qualname.split(".") if p != "") + + +def resolve_traced(conn, symbols, files, ambiguous=None): """[(relpath, qualname)] -> ({node_id}, [unresolved]). Only ANONYMOUS frames are dropped — ``, ``, `` @@ -134,7 +140,18 @@ def resolve_traced(conn, symbols, files): A name that resolves only in a DIFFERENT file is reported unresolved, not accepted — "the graph has no node for this" (an indexing gap) and "the graph has a node and no path to it" (a traversal gap) are different - defects with different fixes, and merging them reports one as the other.""" + defects with different fixes, and merging them reports one as the other. + + Several same-file nodes of that name (`A.run`, `B.run`, every `__init__`) + are told apart by `qualified_name` (#96): resolving by bare name credited + `B.run` as run when only `A.run` was, and reported it as a proof-grade + STATIC MISS. An exact qualified match wins. Otherwise a single hit is + accepted only when its qualified_name cannot contradict the trace (empty, + or the bare name an older index wrote); a hit named `B::run` for a traced + `A.run` is not credited. Everything else goes to `ambiguous` as + `(rel, qualname, ids)`: not resolved and not unresolved. `trace_nodes` + still counts the candidates as PRESENT for the runtime witness, which must + over-state presence, and leaves them out of the miss count only.""" resolved, unresolved = set(), [] for rel, qualname in symbols: if "<" in qualname.split(".")[-1]: @@ -142,6 +159,18 @@ def resolve_traced(conn, symbols, files): name = qualname.split(".")[-1] ids = dbmod.resolve_symbol(conn, name, os.path.basename(rel)) hits = [i for i in ids if path_matches(files.get(i, ""), rel)] + if hits: + want = _qualified(qualname) + marks = ",".join("?" * len(hits)) + qn = dict(conn.execute( + f"SELECT id, qualified_name FROM nodes WHERE id IN ({marks})", hits)) + exact = [i for i in hits if qn.get(i) == want] + if exact: + hits = exact + elif len(hits) > 1 or qn.get(hits[0]) not in (None, "", name): + if ambiguous is not None: + ambiguous.append((rel, qualname, frozenset(hits))) + continue if hits: resolved.update(hits) else: @@ -149,14 +178,29 @@ def resolve_traced(conn, symbols, files): return resolved, unresolved -def trace_nodes(conn, trace_tests, files=None): +def trace_nodes(conn, trace_tests, files=None, ambiguous=None, strict=None): """tgtrace's `{nodeid: [[rel, qualname], ...]}` -> ({test: {node_id}}, - [unresolved]). One resolution pass, reused by both halves of the report.""" + [unresolved]). One resolution pass, reused by both halves of the report. + + The returned sets are PRESENCE: they include every candidate of a symbol + `resolve_traced` could not tell apart, because the runtime witness must + over-state what ran (dropping them turned oracle gaps into confirmed + phantoms). `strict`, when given, gets the same map without those + candidates, for the STATIC MISS count, where crediting a node that did not + run is a false proof. Ambiguous symbols are added to `ambiguous` once each.""" files = files if files is not None else file_map(conn) - per_test, unresolved = {}, [] + per_test, unresolved, seen = {}, [], set() for test, symbols in trace_tests.items(): - ids, missing = resolve_traced(conn, [tuple(s) for s in symbols], files) - per_test[test] = ids + amb = [] + ids, missing = resolve_traced(conn, [tuple(s) for s in symbols], files, amb) + if strict is not None: + strict[test] = set(ids) + per_test[test] = set(ids).union(*(a[2] for a in amb)) + if ambiguous is not None: + for a in amb: + if a[:2] not in seen: + seen.add(a[:2]) + ambiguous.append(a) unresolved.extend(missing) return per_test, unresolved @@ -223,7 +267,6 @@ def __init__(self, tree): self.bindings = collections.defaultdict(set) self.star = False self.functions = collections.defaultdict(list) - self.by_line = {} self.module_level = set() self._index(tree) @@ -252,9 +295,6 @@ def _index(self, tree): self.bindings[local].add(("symbol", (mod, a.name))) elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): self.functions[node.name].append(node) - self.by_line[node.lineno] = node - for dec in node.decorator_list: - self.by_line.setdefault(dec.lineno, node) for top in tree.body: if isinstance(top, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): self.module_level.add(top.name) @@ -264,13 +304,15 @@ def _index(self, tree): self.module_level.add(t.id) def find(self, name, start_line=None): - """The def(s) an index node names. `start_line` first — CodeGraph may - record either the `def` line or the first decorator, so both are - indexed — then every same-named def as a fallback. Several defs share - a name legitimately (a method on two classes); support is the UNION - over them, which again over-supports rather than over-accuses.""" - if start_line is not None and start_line in self.by_line: - return [self.by_line[start_line]] + """Every def of that name in the file, judged as a UNION: several defs + share a name legitimately (a method on two classes), and the union + over-supports rather than over-accuses. + + `start_line` is accepted and deliberately ignored. It used to pick one + def, but the index can be stale against the working tree this reads: + the def now on that line can be a different function (#100), or a + same-named one in another class (`A.run` where `B.run` was indexed), + and judging that one body turned a real edge into a phantom.""" return list(self.functions.get(name, ())) @@ -347,7 +389,8 @@ def _local_names(func): return names -def _supports(func, module, caller_file, callee_name, callee_file): +def _supports(func, module, caller_file, callee_name, callee_file, + callee_kind=None): """Does anything in `func` denote `callee_name` defined in `callee_file`? Every reference is weighed, not only call positions: passing a function as @@ -369,6 +412,17 @@ def _supports(func, module, caller_file, callee_name, callee_file): and node.name == callee_name and node is not func): return True for node in ast.walk(func): + if (isinstance(node, ast.Attribute) and node.attr == callee_name + and callee_kind == "method"): + # A METHOD is reached through an instance or its class, and which + # one cannot be resolved without types: `svc.create()` on a + # parameter, `s.create()` on a local, `Svc.build()`, `A().run()`. + # Refusing these called the commonest real edges phantoms (#95). + # Same coarse rule `self.`/`cls.` already get. A module-level + # FUNCTION callee does not take this branch, so the #66 shape — + # `rows.setdefault(k, []).append(v)` against `ledger.append` — + # still refuses below. + return True if isinstance(node, ast.Attribute) and node.attr == callee_name: receiver = _dotted(node.value) if receiver is None: @@ -386,6 +440,18 @@ def _supports(func, module, caller_file, callee_name, callee_file): ) if any(_module_matches(c, callee_file) for c in candidates): return True + elif (isinstance(node, ast.Name) and node.id != callee_name + and node.id not in locals_): + # `from .svc import append as ap; ap()` — the alias denotes the + # original name (#95). + for kind, value in module.bindings.get(node.id, ()): + if kind == "symbol": + mod, orig = value + # An empty `mod` is `from . import x as y`: the package + # itself, accepted exactly as the unaliased branch below + # accepts `from . import x`. + if orig == callee_name and (not mod or _module_matches(mod, callee_file)): + return True elif isinstance(node, ast.Name) and node.id == callee_name: if callee_name in locals_: continue # shadowed by a parameter or a local @@ -401,6 +467,13 @@ def _supports(func, module, caller_file, callee_name, callee_file): # module, so the name-based check silently rejected every # same-file call. return True + if callee_name in module.functions and caller_file == callee_file: + # A def nested in an ENCLOSING function, reached as a closure: + # `close_block` calling its sibling `note` inside + # `_parse_unified_diff` (found by running this on testgraph). + # Any same-named def in the file counts — over-support, the + # safe direction. + return True return False @@ -423,7 +496,7 @@ def source_support(repo_root, caller, callee, cache): if not defs: return UNKNOWN, UNREADABLE for func in defs: - if _supports(func, module, ca, callee["name"], cb): + if _supports(func, module, ca, callee["name"], cb, callee.get("kind")): return SUPPORTED, None # A caller in the callee's OWN file, referencing it by a bare name, is # handled above via `module_level`. Reaching here means no reference of @@ -557,11 +630,13 @@ def reconcile(db_path, repo_root, registry, trace): """The whole report, as data. `trace` is a parsed tgtrace payload.""" conn = dbmod.connect(db_path) files = file_map(conn) - per_test, unresolved = trace_nodes(conn, trace.get("tests", {}), files) + ambiguous, strict = [], {} + per_test, unresolved = trace_nodes(conn, trace.get("tests", {}), files, + ambiguous, strict) node_tests = invert(per_test) reach, contained_by = dbmod.dependency_graph(conn) - misses = static_misses(conn, registry, per_test, reach, contained_by, files) + misses = static_misses(conn, registry, strict, reach, contained_by, files) findings, counts, footprints = phantom_edges( conn, repo_root, registry, node_tests, reach, contained_by ) @@ -576,6 +651,7 @@ def reconcile(db_path, repo_root, registry, trace): return { "tests_traced": len(per_test), "unresolved_symbols": sorted(set(unresolved)), + "ambiguous_symbols": sorted(a[:2] for a in ambiguous), "static_misses": misses, "edges": findings, "edge_counts": dict(counts), @@ -676,6 +752,13 @@ def render(report, max_examples=8): f"node in the index — an indexing gap, kept out of the miss count " f"because it needs a different fix than a missing edge" ) + if report.get("ambiguous_symbols"): + lines.append("") + lines.append( + f" {len(report['ambiguous_symbols'])} traced symbol(s) match several " + f"same-named nodes and no qualified name — not counted as run, so " + f"they cannot make a miss (#96)" + ) return "\n".join(lines) diff --git a/tests/test_reconcile.py b/tests/test_reconcile.py index 230089c..6265cc6 100644 --- a/tests/test_reconcile.py +++ b/tests/test_reconcile.py @@ -449,6 +449,227 @@ def test_a_missing_caller_definition_is_not_judged(self): ) +class CommonCallShapeTests(unittest.TestCase): + """#95: the strict oracle refused the commonest Python call shapes and + reported real edges as phantoms. A METHOD callee is supported by any + attribute reference of its name, like `self.`/`cls.`; a module-level + FUNCTION callee stays strict, so the #66 shape still refuses.""" + + def _support(self, source, caller, callee_name, callee_kind, callee_file="mod.py", + files=None): + d = tempfile.mkdtemp(prefix="tg-oracle-") + try: + for rel, text in (files or {}).items(): + with open(os.path.join(d, rel), "w") as fh: + fh.write(text) + with open(os.path.join(d, "mod.py"), "w") as fh: + fh.write(source.lstrip("\n")) + return rec.source_support( + d, + {"name": caller, "file_path": "mod.py", "start_line": None, + "kind": "function"}, + {"name": callee_name, "file_path": callee_file, "kind": callee_kind}, + {}, + )[0] + finally: + shutil.rmtree(d, ignore_errors=True) + + def test_a_method_on_a_parameter_or_local_is_supported(self): + for src in ("def f(svc):\n svc.create()\n", + "def f():\n s = Svc()\n s.create()\n"): + with self.subTest(src): + self.assertEqual(self._support(src, "f", "create", "method", + "svc.py"), rec.SUPPORTED) + + def test_a_method_on_a_class_name_or_call_result_is_supported(self): + for src, name in (("from .svc import Svc\n\ndef f():\n Svc.build()\n", "build"), + ("def f():\n A().run()\n", "run")): + with self.subTest(src): + self.assertEqual(self._support(src, "f", name, "method", "svc.py"), + rec.SUPPORTED) + + def test_a_sibling_closure_is_supported(self): + """Found by running reconcile on testgraph itself: `close_block` calls + `note`, both nested inside `_parse_unified_diff`. The bare name is a + closure binding, neither module-level nor nested in the caller.""" + src = ("def outer():\n def note(p):\n pass\n\n" + " def close_block():\n note(1)\n") + self.assertEqual(self._support(src, "close_block", "note", "function"), + rec.SUPPORTED) + + def test_a_same_named_def_in_another_file_is_not_a_closure(self): + src = ("def outer():\n def note(p):\n pass\n\n" + " def close_block():\n note(1)\n") + self.assertEqual(self._support(src, "close_block", "note", "function", + "other.py"), rec.UNSUPPORTED) + + def test_the_66_shape_still_refuses_a_function_callee(self): + src = "def f(rows, k, v):\n rows.setdefault(k, []).append(v)\n" + self.assertEqual(self._support(src, "f", "append", "function", "ledger.py"), + rec.UNSUPPORTED) + + def test_an_aliased_import_supports_the_original_name(self): + src = "from .svc import append as ap\n\ndef f(row):\n ap(row)\n" + self.assertEqual(self._support(src, "f", "append", "function", "svc.py"), + rec.SUPPORTED) + + def test_a_relative_package_alias_is_supported(self): + """Review F1: `from . import helper as h` has no module name, and the + alias branch required one; the unaliased spelling was supported.""" + src = "from . import helper as h\n\ndef f(x):\n return h(x)\n" + self.assertEqual(self._support(src, "f", "helper", "function", "__init__.py"), + rec.SUPPORTED) + + def test_an_alias_from_another_module_does_not(self): + src = "from .other import append as ap\n\ndef f(row):\n ap(row)\n" + self.assertEqual(self._support(src, "f", "append", "function", "svc.py"), + rec.UNSUPPORTED) + + +class StaleStartLineTests(unittest.TestCase): + """#100: on a stale index the def at the indexed start_line is a different + function; judging its body turned a real edge into a phantom.""" + + def test_a_def_with_another_name_on_that_line_is_not_used(self): + d = tempfile.mkdtemp(prefix="tg-oracle-") + self.addCleanup(shutil.rmtree, d, ignore_errors=True) + with open(os.path.join(d, "mod.py"), "w") as fh: + fh.write("def other():\n pass\n\n\ndef caller():\n helper()\n\n\n" + "def helper():\n pass\n") + support = rec.source_support( + d, {"name": "caller", "file_path": "mod.py", "start_line": 1, + "kind": "function"}, + {"name": "helper", "file_path": "mod.py", "kind": "function"}, {})[0] + self.assertEqual(support, rec.SUPPORTED) + + +class StaleSameNameLineTests(unittest.TestCase): + """Review F4: a stale start_line that lands on a SAME-named def in another + class judged only that body.""" + + def test_same_named_defs_are_judged_as_a_union(self): + d = tempfile.mkdtemp(prefix="tg-oracle-") + self.addCleanup(shutil.rmtree, d, ignore_errors=True) + with open(os.path.join(d, "mod.py"), "w") as fh: + fh.write("class A:\n def run(self):\n pass\n\n\n" + "class B:\n def run(self, x):\n x.go()\n") + support = rec.source_support( + d, {"name": "run", "file_path": "mod.py", "start_line": 2, + "kind": "method"}, + {"name": "go", "file_path": "other.py", "kind": "method"}, {})[0] + self.assertEqual(support, rec.SUPPORTED) + + +class QualifiedTraceTests(unittest.TestCase): + """#96: a traced `A.run` resolved by bare name to every `run` in the file, + so `B.run`, which never ran, became a proof-grade STATIC MISS.""" + + def _conn(self, rows): + conn = build_conn() + conn.executemany("INSERT INTO nodes VALUES (?,?,?,?,?,?,?)", rows) + conn.commit() + return conn + + def test_a_qualified_name_picks_the_method_that_ran(self): + conn = self._conn([ + ("m:A.run", "method", "run", "A::run", "app/svc.py", 10, 12), + ("m:B.run", "method", "run", "B::run", "app/svc.py", 20, 22), + ]) + resolved, unresolved = rec.resolve_traced( + conn, [("app/svc.py", "A.run")], rec.file_map(conn)) + self.assertEqual(resolved, {"m:A.run"}) + self.assertEqual(unresolved, []) + + def test_a_nested_def_matches_through_locals(self): + conn = self._conn([ + ("f:outer.git", "function", "git", "outer::git", "app/svc.py", 10, 12), + ("f:other.git", "function", "git", "other::git", "app/svc.py", 20, 22), + ]) + resolved, _ = rec.resolve_traced( + conn, [("app/svc.py", "outer..git")], rec.file_map(conn)) + self.assertEqual(resolved, {"f:outer.git"}) + + def test_a_single_hit_that_contradicts_the_trace_is_not_credited(self): + """Review F3: only `B::run` indexed, trace says `A.run` ran.""" + conn = self._conn([ + ("m:B.run", "method", "run", "B::run", "app/svc.py", 20, 22), + ]) + ambiguous = [] + resolved, _ = rec.resolve_traced( + conn, [("app/svc.py", "A.run")], rec.file_map(conn), ambiguous) + self.assertEqual(resolved, set()) + self.assertEqual([a[:2] for a in ambiguous], [("app/svc.py", "A.run")]) + + def test_ambiguous_symbols_still_count_as_present_at_runtime(self): + """Review F2: dropping them from the runtime witness turned an oracle + gap into a confirmed phantom. Presence over-states; only the miss + count leaves them out.""" + conn = self._conn([ + ("m:A.run", "method", "run", "X::run", "app/svc.py", 10, 12), + ("m:B.run", "method", "run", "Y::run", "app/svc.py", 20, 22), + ]) + strict, ambiguous = {}, [] + per_test, _ = rec.trace_nodes( + conn, {"t1": [["app/svc.py", "A.run"]]}, rec.file_map(conn), + ambiguous=ambiguous, strict=strict) + self.assertEqual(per_test["t1"], {"m:A.run", "m:B.run"}) + self.assertEqual(strict["t1"], set()) + self.assertEqual(len(ambiguous), 1) + + def test_several_hits_with_no_qualified_match_are_ambiguous(self): + conn = self._conn([ + ("m:A.run", "method", "run", "X::run", "app/svc.py", 10, 12), + ("m:B.run", "method", "run", "Y::run", "app/svc.py", 20, 22), + ]) + ambiguous = [] + resolved, unresolved = rec.resolve_traced( + conn, [("app/svc.py", "A.run")], rec.file_map(conn), ambiguous) + self.assertEqual(resolved, set()) + self.assertEqual(unresolved, []) + self.assertEqual([a[:2] for a in ambiguous], [("app/svc.py", "A.run")]) + + +class AmbiguousReportTests(unittest.TestCase): + def test_the_report_counts_ambiguous_symbols_on_their_own_line(self): + out = rec.render({ + "tests_traced": 1, "unresolved_symbols": [], + "ambiguous_symbols": [("app/svc.py", "A.run")], + "static_misses": [{"journey": "J1", "name": "one", "entries_resolved": 1, + "covering_tests": 1, "traced_nodes": 2, + "footprint": 2, "misses": []}], + "edges": [], "edge_counts": {}, "labels": {}, + }) + self.assertIn("1 traced symbol(s) match several", out) + self.assertNotIn("have no node in the index", out) + + +class AmbiguousIsNotAMissTests(unittest.TestCase): + """Review F2, end to end through `reconcile()`: an ambiguous symbol is + present for the runtime witness but must not be credited as RUN in the + static-miss count, where that would be a false proof.""" + + def test_an_ambiguous_symbol_outside_the_footprint_is_not_a_miss(self): + d = tempfile.mkdtemp(prefix="tg-reconcile-") + self.addCleanup(shutil.rmtree, d, ignore_errors=True) + conn = build_conn() + conn.executemany("INSERT INTO nodes VALUES (?,?,?,?,?,?,?)", [ + ("m:X.run", "method", "run", "X::run", "app/store.py", 20, 22), + ("m:Y.run", "method", "run", "Y::run", "app/store.py", 30, 32), + ]) + conn.execute("CREATE TABLE files(path TEXT)") + conn.commit() + db = os.path.join(d, "cg.db") + dst = sqlite3.connect(db) + conn.backup(dst) + dst.close() + trace = {"tests": {"t::a": [["app/handler.py", "handle"], + ["app/store.py", "A.run"]]}} + report = rec.reconcile(db, d, REGISTRY, trace) + missed = {m for row in report["static_misses"] for m in row["misses"]} + self.assertFalse(missed & {"m:X.run", "m:Y.run"}, missed) + self.assertEqual(report["ambiguous_symbols"], [("app/store.py", "A.run")]) + + class HelperTests(unittest.TestCase): def test_dotted_refuses_a_call_receiver(self): """The one rule the phantom half rests on."""