diff --git a/graphify/extract.py b/graphify/extract.py index 7accab31..9761a1d2 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -33,6 +33,7 @@ from graphify.extractors.csharp import _resolve_csharp_type_references from graphify.extractors.elixir import extract_elixir # noqa: F401 from graphify.extractors.razor import extract_razor # noqa: F401 from graphify.extractors.zig import extract_zig # noqa: F401 +from graphify.paths import disambiguate_ambiguous_candidates _RECURSION_LIMIT = 10_000 @@ -13976,10 +13977,15 @@ def extract( # "did the caller's file import the callee's file?" # Use relativized paths to match how file node IDs were remapped above (#502). nid_to_file_nid: dict[str, str] = {} + # nid -> raw source_file string, for the ambiguous-name tie-breakers below + # (test/non-test classification + path proximity). Kept separate from the + # file-node-id map because tie-breaking compares the actual file paths. + nid_to_source_file: dict[str, str] = {} for n in all_nodes: sf = n.get("source_file") if not sf: continue + nid_to_source_file[n["id"]] = str(sf) sf_path = Path(sf) try: sf_rel = sf_path.relative_to(root) if sf_path.is_absolute() else sf_path @@ -14031,6 +14037,7 @@ def extract( symbol_matches = [c for c in candidates if c in imported_symbols] if len(symbol_matches) == 1: tgt = symbol_matches[0] + has_import_evidence = True else: module_matches = [ c for c in candidates @@ -14038,9 +14045,22 @@ def extract( ] if len(module_matches) == 1: tgt = module_matches[0] + has_import_evidence = True else: - continue - has_import_evidence = True + # No unique import evidence. Instead of dropping the edge + # outright (which let a single same-named test mock erase the + # real call graph, #1553), apply the shared god-node + # tie-breakers (non-test preference, then path proximity). + # Resolve only if exactly one candidate survives; otherwise + # the #543/#1219 guard still holds and we skip. + tgt = disambiguate_ambiguous_candidates( + candidates, + {c: nid_to_source_file.get(c, "") for c in candidates}, + rc.get("source_file", ""), + ) + if tgt is None: + continue + has_import_evidence = False if tgt != caller and (caller, tgt) not in existing_pairs: existing_pairs.add((caller, tgt)) # Promote to EXTRACTED when there's a direct import edge from the diff --git a/graphify/paths.py b/graphify/paths.py index 3700f295..d2bfdd9f 100644 --- a/graphify/paths.py +++ b/graphify/paths.py @@ -17,10 +17,198 @@ flow) and every reader honours it. from __future__ import annotations import os -from pathlib import Path +import re +from pathlib import Path, PurePosixPath GRAPHIFY_OUT = os.environ.get("GRAPHIFY_OUT", "graphify-out") +# Directory segments that, when they appear as a whole path component, mark the +# whole path as a test location. Matched against path *segments* (not raw +# substrings) so "src/contest.py" / "latest/x.py" / "src/greatest/x.py" do NOT +# match — only a segment that *equals* one of these names (case-insensitively). +_TEST_DIR_SEGMENTS = frozenset({"tests", "test", "spec", "specs", "__tests__"}) + +# Filename patterns marking a file as a test, matched against the *filename* +# only (case-insensitive). These are conventions across ecosystems: +# test_*.py pytest / unittest +# *_test.* Go / Python / Rust +# *.test.* JS/TS (jest, vitest) +# *.spec.* / *_spec.* Jasmine / RSpec / Karma +# *.Tests.ps1 PowerShell Pester +# *Test.java / *Tests.cs (case-sensitive convention, handled below) +_TEST_FILENAME_PATTERNS = ( + re.compile(r"^test_.*", re.IGNORECASE), + re.compile(r".*_test\..+$", re.IGNORECASE), + re.compile(r".*\.test\..+$", re.IGNORECASE), + re.compile(r".*\.spec\..+$", re.IGNORECASE), + re.compile(r".*_spec\..+$", re.IGNORECASE), + re.compile(r".*\.tests\.ps1$", re.IGNORECASE), + # Java `FooTest.java` / `FooTests.java`, C# `FooTests.cs` style. Require an + # uppercase-led `Test`/`Tests` immediately before the extension so plain + # words like "greatest"/"contest.cs" do not match. + re.compile(r".*Test\.java$"), + re.compile(r".*Tests\.java$"), + re.compile(r".*Tests\.cs$"), +) + + +def _is_test_path(path: str) -> bool: + """Classify a source path as a test path (case-insensitive, segment-aware). + + Shared by extract.py and symbol_resolution.py so cross-file call resolution + treats test mocks/stubs identically. A path is a test path when: + * any whole path segment equals a known test dir name + (``tests``/``test``/``spec``/``specs``/``__tests__``), or + * the filename matches a known test-file naming convention. + + Conservative on purpose: matches segments/filenames, never raw substrings, + so ``latest.py``, ``src/contest.py`` and ``src/greatest/x.py`` are NON-test. + """ + if not path: + return False + # Accept both POSIX and Windows separators regardless of host OS so the + # classifier is stable across the mixed paths that flow through extraction. + norm = str(path).replace("\\", "/") + pure = PurePosixPath(norm) + segments = list(pure.parts) + # Strip a leading drive/anchor segment (e.g. "C:/") that PureWindowsPath + # would surface; with the manual "\\"->"/" swap above PurePosixPath keeps + # the path body intact, but guard against a Windows drive embedded as a + # segment just in case. + for segment in segments: + if segment.lower() in _TEST_DIR_SEGMENTS: + return True + # A drive-letter colon segment like "c:" is never a test dir. + filename = pure.name + if not filename: + return False + for pattern in _TEST_FILENAME_PATTERNS: + if pattern.match(filename): + return True + return False + + +def _path_proximity_winner(call_site_file: str, candidate_files: dict[str, str]) -> str | None: + """Pick the candidate whose source file is closest to the call site. + + ``candidate_files`` maps candidate id -> its source_file. Returns a single + winning candidate id, or ``None`` when no proximity tier yields a unique + winner. Tiers, in order: + + 1. same file as the call site, + 2. same directory, + 3. longest common path-prefix (must be a strict, unique maximum). + + Used only as a secondary tie-break after the test/non-test filter, so the + god-node guard still holds when proximity is genuinely ambiguous. + """ + if not call_site_file: + return None + call_norm = str(call_site_file).replace("\\", "/") + call_dir = PurePosixPath(call_norm).parent + + # Tier 1: exact same file. + same_file = [cid for cid, f in candidate_files.items() + if str(f).replace("\\", "/") == call_norm] + if len(same_file) == 1: + return same_file[0] + if len(same_file) > 1: + return None # genuinely ambiguous within one file; bail + + # Tier 2: same directory. + same_dir = [cid for cid, f in candidate_files.items() + if PurePosixPath(str(f).replace("\\", "/")).parent == call_dir] + if len(same_dir) == 1: + return same_dir[0] + if len(same_dir) > 1: + return None + + # Tier 3: longest common path-prefix, computed over path segments. The + # winner must be a strict unique maximum, else we bail (guard holds). + call_parts = call_dir.parts + + def _common_prefix_len(f: str) -> int: + parts = PurePosixPath(str(f).replace("\\", "/")).parent.parts + n = 0 + for a, b in zip(call_parts, parts): + if a != b: + break + n += 1 + return n + + scored = sorted( + ((cid, _common_prefix_len(f)) for cid, f in candidate_files.items()), + key=lambda kv: kv[1], + reverse=True, + ) + if not scored: + return None + best = scored[0][1] + winners = [cid for cid, score in scored if score == best] + if len(winners) == 1 and best > 0: + return winners[0] + return None + + +def disambiguate_ambiguous_candidates( + candidates: list[str], + candidate_files: dict[str, str], + call_site_file: str, +) -> str | None: + """Resolve an ambiguous bare-name call to one candidate, or ``None``. + + Shared god-node tie-breaker (#1553) used by both the inline cross-file call + pass in ``extract.py`` and ``symbol_resolution.resolve_cross_file_raw_calls`` + so the heuristics stay aligned across languages. ``candidates`` is the list + of node ids sharing the callee's name; ``candidate_files`` maps each id -> + its source_file. Returns the surviving candidate id only when exactly one + survives; otherwise ``None`` (caller keeps the god-node guard / ``continue``). + + Tie-breakers, in order: + 1. NON-TEST preference. Classify the call site and each candidate as + test/non-test. When the call site is NON-test, drop test candidates. + When the call site IS a test file, prefer test-local candidates + (same file first, then any test candidate); fall back to the full set + only if no test candidate exists. + 2. PATH PROXIMITY over whatever survived step 1. + """ + if not candidates: + return None + if len(candidates) == 1: + return candidates[0] + + call_is_test = _is_test_path(call_site_file) + test_cands = [c for c in candidates if _is_test_path(candidate_files.get(c, ""))] + nontest_cands = [c for c in candidates if c not in set(test_cands)] + + if call_is_test: + # Prefer a test-local definition (same file) first. + call_norm = str(call_site_file).replace("\\", "/") + same_file_test = [ + c for c in test_cands + if str(candidate_files.get(c, "")).replace("\\", "/") == call_norm + ] + if len(same_file_test) == 1: + return same_file_test[0] + if test_cands: + survivors = test_cands + else: + survivors = nontest_cands or candidates + else: + # Non-test call site: drop test mocks/stubs entirely. + survivors = nontest_cands + + if len(survivors) == 1: + return survivors[0] + if not survivors: + return None + + # Step 2: path proximity over the survivors. + return _path_proximity_winner( + call_site_file, + {c: candidate_files.get(c, "") for c in survivors}, + ) + # Bare directory name even when GRAPHIFY_OUT is an absolute path. Used by the # path guards that walk parents looking for the output dir by name, and by the # detect scan-exclude so a custom output dir is never re-ingested as source. diff --git a/graphify/symbol_resolution.py b/graphify/symbol_resolution.py index 14c02243..892f3106 100644 --- a/graphify/symbol_resolution.py +++ b/graphify/symbol_resolution.py @@ -11,6 +11,7 @@ from collections.abc import Sequence from typing import Any from graphify.ids import make_id as _shared_make_id +from graphify.paths import disambiguate_ambiguous_candidates from graphify.security import sanitize_metadata @@ -319,6 +320,13 @@ def resolve_cross_file_raw_calls( label_index = build_label_index(all_nodes) known_pairs = existing_edge_pairs(all_edges) + # nid -> source_file, for the shared god-node tie-breakers (#1553) so a + # same-named test mock no longer erases a real cross-file call. + nid_to_source_file = { + str(n.get("id")): str(n.get("source_file", "")) + for n in all_nodes + if n.get("id") + } resolved: list[dict[str, Any]] = [] for raw_call in iter_raw_calls(per_file): @@ -328,9 +336,21 @@ def resolve_cross_file_raw_calls( if raw_call.get("is_member_call"): continue candidates = label_index.get(callee.lower(), []) - if len(candidates) != 1: + if not candidates: continue - target = candidates[0] + if len(candidates) == 1: + target: str | None = candidates[0] + else: + # Ambiguous bare name. Apply the shared tie-breakers (non-test + # preference, then path proximity); resolve only if exactly one + # candidate survives, else preserve the god-node guard and skip. + target = disambiguate_ambiguous_candidates( + candidates, + {c: nid_to_source_file.get(c, "") for c in candidates}, + str(raw_call.get("source_file", "")), + ) + if target is None: + continue caller = str(raw_call.get("caller_nid", "")) if not caller: continue diff --git a/tests/test_extract.py b/tests/test_extract.py index 2f01bc0f..1d70cd9c 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -558,6 +558,91 @@ def test_cross_file_calls_skip_ambiguous_duplicate_labels(tmp_path): ) +def test_cross_file_call_survives_same_named_test_mock(tmp_path): + """A real cross-file call must NOT be erased by a same-named test mock. + + src/caller.py calls save(); src/service.py defines the real save(); a test + mock save() lives in tests/test_service.py. Before #1553 the ambiguous-name + god-node guard dropped the edge entirely. Now the non-test tie-breaker keeps + exactly one caller->save edge pointing at the SRC definition. + """ + src = tmp_path / "src" + tests = tmp_path / "tests" + src.mkdir() + tests.mkdir() + (src / "service.py").write_text("def save():\n return 'real'\n") + (src / "caller.py").write_text("def run():\n save()\n") + (tests / "test_service.py").write_text("def save():\n return 'mock'\n") + + result = extract( + [src / "caller.py", src / "service.py", tests / "test_service.py"], + cache_root=tmp_path, + ) + nodes = {n["id"]: n for n in result["nodes"]} + save_calls = [ + e for e in result["edges"] + if e["relation"] == "calls" + and nodes[e["source"]]["label"] == "run()" + and nodes[e["target"]]["label"] == "save()" + ] + assert len(save_calls) == 1, f"expected exactly one run->save edge, got {save_calls}" + target_sf = (nodes[save_calls[0]["target"]].get("source_file") or "") + assert "service.py" in target_sf and "test_service.py" not in target_sf, target_sf + + +def test_cross_file_call_god_node_guard_two_real_defs(tmp_path): + """Two genuine NON-test defs of the same name + one caller => ZERO edges. + + Proves #543/#1219 is not reopened by the #1553 tie-breakers: with no test + candidate to drop and no proximity winner, the guard still bails. + """ + pkg_a = tmp_path / "a" + pkg_b = tmp_path / "b" + pkg_c = tmp_path / "c" + for d in (pkg_a, pkg_b, pkg_c): + d.mkdir() + (pkg_a / "svc.py").write_text("def save():\n return 'a'\n") + (pkg_b / "svc.py").write_text("def save():\n return 'b'\n") + (pkg_c / "caller.py").write_text("def run():\n save()\n") + + result = extract( + [pkg_c / "caller.py", pkg_a / "svc.py", pkg_b / "svc.py"], + cache_root=tmp_path, + ) + nodes = {n["id"]: n for n in result["nodes"]} + save_calls = [ + e for e in result["edges"] + if e["relation"] == "calls" + and nodes[e["source"]]["label"] == "run()" + and nodes[e["target"]]["label"] == "save()" + ] + assert save_calls == [], f"god-node guard must bail, got {save_calls}" + + +def test_cross_file_call_survives_many_test_mocks(tmp_path): + """One src def + many same-named test stubs + caller => exactly one src edge.""" + src = tmp_path / "src" + tests = tmp_path / "tests" + src.mkdir() + tests.mkdir() + (src / "service.py").write_text("def save():\n return 'real'\n") + (src / "caller.py").write_text("def run():\n save()\n") + for i in range(5): + (tests / f"thing{i}_test.py").write_text("def save():\n return 'mock'\n") + + paths = [src / "caller.py", src / "service.py"] + sorted(tests.glob("*_test.py")) + result = extract(paths, cache_root=tmp_path) + nodes = {n["id"]: n for n in result["nodes"]} + save_calls = [ + e for e in result["edges"] + if e["relation"] == "calls" + and nodes[e["source"]]["label"] == "run()" + and nodes[e["target"]]["label"] == "save()" + ] + assert len(save_calls) == 1, f"expected one run->save edge, got {save_calls}" + assert "service.py" in (nodes[save_calls[0]["target"]].get("source_file") or "") + + def test_extract_generic_surfaces_tree_sitter_version_mismatch_hint(monkeypatch): """When Language() raises TypeError (e.g. old tree-sitter binding meets a new tree-sitter API), the error message should point users at the upgrade diff --git a/tests/test_paths.py b/tests/test_paths.py new file mode 100644 index 00000000..e0e1a2f0 --- /dev/null +++ b/tests/test_paths.py @@ -0,0 +1,99 @@ +"""Tests for graphify.paths — the shared test-path classifier (#1553).""" + +from __future__ import annotations + +import pytest + +from graphify.paths import ( + _is_test_path, + disambiguate_ambiguous_candidates, +) + + +@pytest.mark.parametrize( + "path", + [ + # test dir segments + "tests/foo.py", + "src/tests/foo.py", + "test/foo.go", + "spec/foo.rb", + "specs/foo.rb", + "app/__tests__/foo.js", + "a/b/TESTS/foo.py", # case-insensitive segment + # test filename conventions + "src/test_service.py", + "pkg/service_test.go", + "src/service.test.ts", + "src/service.spec.ts", + "src/service_spec.rb", + "ps/Module.Tests.ps1", + "java/FooTest.java", + "java/FooTests.java", + "cs/FooTests.cs", + # windows separators + "src\\tests\\foo.py", + "src\\service_test.py", + ], +) +def test_is_test_path_positive(path: str) -> None: + assert _is_test_path(path) is True, path + + +@pytest.mark.parametrize( + "path", + [ + "", + "latest.py", + "contest.py", + "src/contest.py", + "src/greatest/x.py", + "src/service.py", + "lib/helper.go", + "src/attestation.py", # "test" only as substring, not a segment + "src/testimony.py", # filename starts with "test" but no underscore + "src/contest/x.py", # "contest" is not "test" + "src/greatest.cs", # ends with "test" but not "Tests.cs" + "src/protest.java", # not "*Test.java" + "config/manifest.json", + ], +) +def test_is_test_path_negative(path: str) -> None: + assert _is_test_path(path) is False, path + + +def test_disambiguate_drops_test_candidate_for_nontest_call_site() -> None: + winner = disambiguate_ambiguous_candidates( + ["src", "mock"], + {"src": "src/service.py", "mock": "tests/test_service.py"}, + "src/caller.py", + ) + assert winner == "src" + + +def test_disambiguate_bails_on_two_nontest_candidates() -> None: + winner = disambiguate_ambiguous_candidates( + ["a", "b"], + {"a": "alpha/a.py", "b": "beta/b.py"}, + "pkg/caller.py", + ) + assert winner is None + + +def test_disambiguate_test_call_site_prefers_test_local() -> None: + winner = disambiguate_ambiguous_candidates( + ["src", "local"], + {"src": "src/service.py", "local": "tests/test_service.py"}, + "tests/test_service.py", + ) + assert winner == "local" + + +def test_disambiguate_path_proximity_same_dir() -> None: + # Two non-test candidates; the one in the call site's directory wins. + winner = disambiguate_ambiguous_candidates( + ["near", "far"], + {"near": "pkg/a/service.py", "far": "pkg/b/service.py"}, + "pkg/a/caller.py", + ) + assert winner == "near" diff --git a/tests/test_symbol_resolution.py b/tests/test_symbol_resolution.py index 44f62690..faa3e52a 100644 --- a/tests/test_symbol_resolution.py +++ b/tests/test_symbol_resolution.py @@ -100,6 +100,9 @@ def test_resolve_cross_file_raw_calls_skips_member_calls() -> None: def test_resolve_cross_file_raw_calls_skips_ambiguous_duplicate_labels() -> None: + """Two genuine NON-test defs of the same name: the god-node guard must still + hold even with the #1553 tie-breakers, because neither the non-test filter + nor path proximity yields a unique winner (#543/#1219 stays closed).""" per_file = [ { "raw_calls": [ @@ -107,20 +110,103 @@ def test_resolve_cross_file_raw_calls_skips_ambiguous_duplicate_labels() -> None "caller_nid": "caller_run", "callee": "log", "is_member_call": False, - "source_file": "caller.py", + "source_file": "pkg/caller.py", "source_location": "L2", } ] } ] nodes = [ - {"id": "caller_run", "label": "run()", "file_type": "code"}, - {"id": "a_log", "label": "log()", "file_type": "code"}, - {"id": "b_log", "label": "log()", "file_type": "code"}, + {"id": "caller_run", "label": "run()", "file_type": "code", "source_file": "pkg/caller.py"}, + {"id": "a_log", "label": "log()", "file_type": "code", "source_file": "alpha/a.py"}, + {"id": "b_log", "label": "log()", "file_type": "code", "source_file": "beta/b.py"}, ] assert resolve_cross_file_raw_calls(per_file, nodes, []) == [] +def test_resolve_cross_file_raw_calls_real_edge_survives_test_mock() -> None: + """A real cross-file call must resolve to the SRC definition even when a + same-named TEST mock exists in the corpus (#1553).""" + per_file = [ + { + "raw_calls": [ + { + "caller_nid": "caller_run", + "callee": "save", + "is_member_call": False, + "source_file": "src/caller.py", + "source_location": "L2", + } + ] + } + ] + nodes = [ + {"id": "caller_run", "label": "run()", "file_type": "code", "source_file": "src/caller.py"}, + {"id": "src_save", "label": "save()", "file_type": "code", "source_file": "src/service.py"}, + {"id": "mock_save", "label": "save()", "file_type": "code", + "source_file": "tests/test_service.py"}, + ] + resolved = resolve_cross_file_raw_calls(per_file, nodes, []) + assert [(e["source"], e["target"]) for e in resolved] == [("caller_run", "src_save")] + assert all(e["target"] != "mock_save" for e in resolved) + + +def test_resolve_cross_file_raw_calls_n_mock_scale() -> None: + """One src def plus many same-named test stubs: exactly one edge to src.""" + per_file = [ + { + "raw_calls": [ + { + "caller_nid": "caller_run", + "callee": "save", + "is_member_call": False, + "source_file": "src/caller.py", + "source_location": "L2", + } + ] + } + ] + nodes = [ + {"id": "caller_run", "label": "run()", "file_type": "code", "source_file": "src/caller.py"}, + {"id": "src_save", "label": "save()", "file_type": "code", "source_file": "src/service.py"}, + {"id": "m1", "label": "save()", "file_type": "code", "source_file": "tests/foo_test.py"}, + {"id": "m2", "label": "save()", "file_type": "code", "source_file": "spec/bar.Tests.ps1"}, + {"id": "m3", "label": "save()", "file_type": "code", "source_file": "test/baz_test.go"}, + {"id": "m4", "label": "save()", "file_type": "code", "source_file": "__tests__/q.test.js"}, + ] + resolved = resolve_cross_file_raw_calls(per_file, nodes, []) + assert [(e["source"], e["target"]) for e in resolved] == [("caller_run", "src_save")] + + +def test_resolve_cross_file_raw_calls_call_site_is_test_prefers_test_local() -> None: + """A test file calling save() with both a src def and a test-local def present + resolves to the test-local def (call-site-is-test symmetry, #1553).""" + per_file = [ + { + "raw_calls": [ + { + "caller_nid": "test_caller", + "callee": "save", + "is_member_call": False, + "source_file": "tests/test_service.py", + "source_location": "L5", + } + ] + } + ] + nodes = [ + {"id": "test_caller", "label": "test_it()", "file_type": "code", + "source_file": "tests/test_service.py"}, + {"id": "src_save", "label": "save()", "file_type": "code", "source_file": "src/service.py"}, + {"id": "test_save", "label": "save()", "file_type": "code", + "source_file": "tests/test_service.py"}, + ] + resolved = resolve_cross_file_raw_calls(per_file, nodes, []) + targets = [e["target"] for e in resolved] + assert targets == ["test_save"] + assert "src_save" not in targets + + def test_resolve_cross_file_raw_calls_skips_existing_pair() -> None: per_file = [ {