diff --git a/graphify/extract.py b/graphify/extract.py index 300a59618..f1ebffa8a 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -118,6 +118,7 @@ _resolve_export_target, _resolve_java_type_references, _resolve_php_type_references, + _resolve_python_type_references, _resolve_js_import_path, _resolve_js_import_target, _resolve_js_module_path, @@ -5118,6 +5119,13 @@ def _learn(e: dict) -> None: except Exception as exc: import logging logging.getLogger(__name__).warning("Cross-file import resolution failed, skipping: %s", exc) + # Re-point type-reference edges the bare-name rewire left on shadow stubs + # because the label was ambiguous, using the file's own imports (#2363). + try: + _resolve_python_type_references(paths, root, all_nodes, all_edges) + except Exception as exc: + import logging + logging.getLogger(__name__).warning("Python type-reference resolution failed, skipping: %s", exc) # Cross-file Java import resolution java_paths = [p for p in paths if p.suffix == ".java"] diff --git a/graphify/extractors/engine.py b/graphify/extractors/engine.py index 7f25d02a9..40f53378b 100644 --- a/graphify/extractors/engine.py +++ b/graphify/extractors/engine.py @@ -2510,6 +2510,39 @@ def walk(node, parent_class_nid: str | None = None) -> None: base_nid = ensure_named_node(base, line) add_edge(class_nid, base_nid, "inherits", line) + # Class-body field annotations (`point: PricePoint`), which is the + # dataclass shape (#2363). _python_collect_type_refs was only ever + # reached from function parameters and return types, so a field type + # produced no edge at all — while Java (record components / field + # declarations) and TS (`public_field_definition`) both emit + # `references`/context="field" for the identical shape. An assignment + # without a `type` child is a plain value binding (`plain = 3`), not a + # type reference, so it is skipped. + body_node = node.child_by_field_name("body") + if body_node is not None: + for stmt in body_node.children: + if stmt.type != "expression_statement": + continue + for assign in stmt.children: + if assign.type != "assignment": + continue + type_node = assign.child_by_field_name("type") + if type_node is None: + continue + field_refs: list[tuple[str, str]] = [] + _python_collect_type_refs(type_node, source, False, field_refs) + field_line = assign.start_point[0] + 1 + for ref_name, role in field_refs: + ctx = "generic_arg" if role == "generic_arg" else "field" + target_nid = ensure_named_node(ref_name, field_line) + if target_nid != class_nid: + # Same emitter as the parameter/return-type path + # below, which validates ctx against + # REFERENCE_CONTEXTS rather than trusting it. + edges.append(_semantic_reference_edge( + class_nid, target_nid, ctx, str_path, field_line + )) + # Swift-specific: conformance / inheritance if config.ts_module == "tree_sitter_swift": swift_kind = _swift_declaration_keyword(node) if t == "class_declaration" else "protocol" diff --git a/graphify/extractors/resolution.py b/graphify/extractors/resolution.py index 097c32b6a..680b3dc8f 100644 --- a/graphify/extractors/resolution.py +++ b/graphify/extractors/resolution.py @@ -2252,6 +2252,118 @@ def walk(n) -> None: return new_edges +def _resolve_python_type_references( + paths: list[Path], + root: Path, + all_nodes: list[dict], + all_edges: list[dict], +) -> None: + """Re-point dangling Python type-reference edges at the imported definition. + + Python type annotations resolve by bare name and fall back to a sourceless + "shadow" stub (``ensure_named_node``). ``_rewire_unique_stub_nodes`` repairs + that only when the label is globally unique; when two modules define the same + class name it bails, so the reference stays stuck on the shadow node and both + real definitions look unreferenced (#2363). A ``from pkg.a.base import + PricePoint`` names the defining module exactly, so it disambiguates where + bare-name matching cannot. + + Mirrors ``_resolve_java_type_references`` and the C# resolver, including + their cost: ``_collect_python_symbol_resolution_facts`` re-parses every + Python file, and it already ran once earlier in the pipeline. The shadow-stub + check below skips that when there is nothing to repoint, but a sourceless + stub is NOT rare — every stdlib or third-party annotation + (``sqlite3.Connection``, ``pathlib.Path``) leaves one — so on most real + corpora this pass does run, at roughly 15ms per file. Narrowing the gate to + "a stub label names a known definition" was tried and rejected: it breaks + ``from X import Y as Z``, where the stub carries the local alias ``Z`` and no + definition is labeled ``Z``. + + Mutates ``all_nodes``/``all_edges`` in place. Runs after id-disambiguation so + target ids are final, and after ``_rewire_unique_stub_nodes`` so it only has + to handle the ambiguous remainder. + """ + py_paths = [path for path in paths if path.suffix == ".py"] + if not py_paths: + return + + # Sourceless shadow stubs with a type-like label — the only repoint targets. + stub_label: dict[str, str] = { + node["id"]: node.get("label", "") + for node in all_nodes + if node.get("id") + and not node.get("source_file") + and node.get("label", "")[:1].isupper() + } + if not stub_label: + return + + # (defining file, symbol label) -> definition node id. + def_by_file_label: dict[tuple[str, str], str] = {} + for node in all_nodes: + source_file = str(node.get("source_file", "")) + label = str(node.get("label", "")) + nid = node.get("id") + if not (source_file and label and isinstance(nid, str) and nid): + continue + if not _is_type_like_definition(node): + continue + def_by_file_label.setdefault((_source_key(source_file, root), label), nid) + + facts = _SymbolResolutionFacts() + _collect_python_symbol_resolution_facts(py_paths, root, facts) + if not facts.imports: + return + + # importing file -> local binding -> definition node id. Keyed on the LOCAL + # name so `from pkg.a.base import PricePoint as PP` binds `PP`. + alias_by_file: dict[str, dict[str, str]] = {} + for imp in facts.imports: + if imp.target_path is None or not imp.local_name: + continue + resolved = def_by_file_label.get( + (_source_key(str(imp.target_path), root), imp.imported_name) + ) + if resolved: + alias_by_file.setdefault( + _source_key(str(imp.file_path), root), {} + )[imp.local_name] = resolved + if not alias_by_file: + return + + # Deliberately NARROWER than the Java/C# repoint sets, which also carry + # `imports`: a Python file-level `imports` edge is resolved to the real + # definition upstream and never lands on a bare-name stub, so including it + # here would be an untested no-op. Verified against ambiguous, aliased, and + # unresolvable-module corpora. + REPOINT_RELATIONS = {"references", "inherits", "implements"} + repointed_from: set[str] = set() + for edge in all_edges: + if edge.get("relation") not in REPOINT_RELATIONS: + continue + target = edge.get("target") + label = stub_label.get(target) + if not label: + continue + ref_file = _source_key(str(edge.get("source_file", "")), root) + resolved = alias_by_file.get(ref_file, {}).get(label) + if resolved and resolved != target: + edge["target"] = resolved + repointed_from.add(str(target)) + + if not repointed_from: + return + + # Drop shadow stubs no edge references anymore. + still_referenced: set[str] = set() + for edge in all_edges: + still_referenced.add(edge.get("source")) + still_referenced.add(edge.get("target")) + all_nodes[:] = [ + node for node in all_nodes + if node.get("id") not in repointed_from or node.get("id") in still_referenced + ] + def _resolve_java_type_references( per_file: list[dict], paths: list[Path], diff --git a/tests/test_python_type_references.py b/tests/test_python_type_references.py new file mode 100644 index 000000000..4195b8c2e --- /dev/null +++ b/tests/test_python_type_references.py @@ -0,0 +1,367 @@ +"""Regression tests: Python cross-file type references (#2363). + +Two defects, both reported as "ghost duplicate nodes": + +1. Class-body field annotations (`point: PricePoint`, the dataclass shape) were + never collected. `_python_collect_type_refs` had exactly two call sites, + function parameters and return types, so a dataclass field emitted no edge at + all. Java (`_java_collect_type_refs` on record components / field + declarations) and TS (`_ts_walk_class_members`) already emit + `references`/context="field" for the same shape. + +2. When a simple name is ambiguous (two same-named classes in different + packages), `_rewire_unique_stub_nodes` bails, so the sourceless stub survives + as a bare `pricepoint` node and the reference edge stays stuck on it — even + though `from pkg.a.base import PricePoint` names the defining module exactly. + Java resolves this with `_resolve_java_type_references`, PHP and C# with their + own resolvers; Python had none. + +The unambiguous single-definition case already worked at 0.9.31 (the sourceless +stub is collapsed by the corpus rewire); it is pinned here so it cannot silently +regress. +""" +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +from graphify.extract import extract + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +def _nodes_labeled(result: dict, label: str) -> list[dict]: + return [node for node in result["nodes"] if node.get("label") == label] + + +def _sole_node_id(result: dict, label: str, source_file: str) -> str: + matches = [ + node["id"] + for node in result["nodes"] + if node.get("label") == label and node.get("source_file") == source_file + ] + assert len(matches) == 1, matches + return matches[0] + + +def _refs(result: dict, source: str) -> set[tuple[str, str | None]]: + """(target, context) for every references edge out of `source`.""" + return { + (edge["target"], edge.get("context")) + for edge in result["edges"] + if edge["source"] == source and edge["relation"] == "references" + } + + +_DEF = ( + "from dataclasses import dataclass\n" + "\n" + "@dataclass\n" + "class PricePoint:\n" + " value: float\n" +) + + +def test_dataclass_field_annotation_emits_reference_edge(tmp_path: Path): + """Defect 1: a dataclass field type produced no edge and no node.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + quote = _write( + tmp_path / "pkg/b/quote.py", + "from dataclasses import dataclass\n" + "from pkg.a.base import PricePoint\n" + "\n" + "@dataclass\n" + "class Quote:\n" + " point: PricePoint\n", + ) + + result = extract([base, quote], cache_root=tmp_path) + + price_nid = _sole_node_id(result, "PricePoint", "pkg/a/base.py") + quote_nid = _sole_node_id(result, "Quote", "pkg/b/quote.py") + assert (price_nid, "field") in _refs(result, quote_nid) + # The reference must not fabricate a second PricePoint. + assert len(_nodes_labeled(result, "PricePoint")) == 1 + + +def test_class_body_field_generic_arg(tmp_path: Path): + """A container-wrapped field type is a generic_arg, matching the param path.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + book = _write( + tmp_path / "pkg/b/book.py", + "from pkg.a.base import PricePoint\n" + "\n" + "class Book:\n" + " rows: list[PricePoint]\n", + ) + + result = extract([base, book], cache_root=tmp_path) + + price_nid = _sole_node_id(result, "PricePoint", "pkg/a/base.py") + book_nid = _sole_node_id(result, "Book", "pkg/b/book.py") + assert (price_nid, "generic_arg") in _refs(result, book_nid) + + +def test_plain_class_attribute_without_annotation_emits_nothing(tmp_path: Path): + """`x = 1` binds a value, not a type — it must not become a reference.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + cfg = _write( + tmp_path / "pkg/b/cfg.py", + "class Config:\n" + " retries = 3\n" + " name = 'x'\n", + ) + + result = extract([base, cfg], cache_root=tmp_path) + + cfg_nid = _sole_node_id(result, "Config", "pkg/b/cfg.py") + assert _refs(result, cfg_nid) == set() + + +def test_ambiguous_same_named_class_resolves_through_the_import(tmp_path: Path): + """Defect 2: the bare sourceless ghost, and an edge pointing at it.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + other = _write(tmp_path / "pkg/z/other.py", "class PricePoint:\n pass\n") + consumer = _write( + tmp_path / "pkg/b/consumer.py", + "from pkg.a.base import PricePoint\n" + "\n" + "def handle(p: PricePoint) -> None:\n" + " return None\n", + ) + + result = extract([base, other, consumer], cache_root=tmp_path) + + # Both real definitions survive; the sourceless ghost must not. + labeled = _nodes_labeled(result, "PricePoint") + assert len(labeled) == 2, labeled + assert all(node.get("source_file") for node in labeled), labeled + + wanted = _sole_node_id(result, "PricePoint", "pkg/a/base.py") + handle_nid = _sole_node_id(result, "handle()", "pkg/b/consumer.py") + assert (wanted, "parameter_type") in _refs(result, handle_nid) + + +def test_ambiguous_field_annotation_resolves_through_the_import(tmp_path: Path): + """Both defects at once: an ambiguous name reached via a dataclass field.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + other = _write(tmp_path / "pkg/z/other.py", "class PricePoint:\n pass\n") + quote = _write( + tmp_path / "pkg/b/quote.py", + "from pkg.z.other import PricePoint\n" + "\n" + "class Quote:\n" + " point: PricePoint\n", + ) + + result = extract([base, other, quote], cache_root=tmp_path) + + # Imported from pkg.z.other, so it must bind there — not to pkg.a.base. + wanted = _sole_node_id(result, "PricePoint", "pkg/z/other.py") + quote_nid = _sole_node_id(result, "Quote", "pkg/b/quote.py") + assert (wanted, "field") in _refs(result, quote_nid) + + +def test_ambiguous_superclass_resolves_through_the_import(tmp_path: Path): + """`inherits` is in the resolver's repoint set, so it needs its own case. + + Superclasses reach `ensure_named_node` from a different call site than type + annotations do, so the annotation tests do not cover this path. + """ + base = _write(tmp_path / "pkg/a/base.py", _DEF) + other = _write(tmp_path / "pkg/z/other.py", "class PricePoint:\n pass\n") + sub = _write( + tmp_path / "pkg/b/sub.py", + "from pkg.a.base import PricePoint\n" + "\n" + "class Sub(PricePoint):\n" + " pass\n", + ) + + result = extract([base, other, sub], cache_root=tmp_path) + + assert not [n for n in result["nodes"] if not n.get("source_file")] + wanted = _sole_node_id(result, "PricePoint", "pkg/a/base.py") + sub_nid = _sole_node_id(result, "Sub", "pkg/b/sub.py") + assert any( + edge["source"] == sub_nid + and edge["target"] == wanted + and edge["relation"] == "inherits" + for edge in result["edges"] + ) + + +def test_ambiguous_aliased_import_resolves_through_the_local_name(tmp_path: Path): + """`from X import Y as Z` binds Z — the resolver keys on the local name, not + the exported one, so `p: PP` still finds `pkg.a.base.PricePoint`.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + other = _write(tmp_path / "pkg/z/other.py", "class PricePoint:\n pass\n") + consumer = _write( + tmp_path / "pkg/b/consumer.py", + "from pkg.a.base import PricePoint as PP\n" + "\n" + "def handle(p: PP) -> None:\n" + " return None\n", + ) + + result = extract([base, other, consumer], cache_root=tmp_path) + + wanted = _sole_node_id(result, "PricePoint", "pkg/a/base.py") + handle_nid = _sole_node_id(result, "handle()", "pkg/b/consumer.py") + assert (wanted, "parameter_type") in _refs(result, handle_nid) + + +def test_ambiguous_name_across_many_referrers_leaves_no_path_qualified_ghosts( + tmp_path: Path, +): + """The issue's second ghost shape: `_py_pricepoint`. + + One referring file leaves the bare `pricepoint` stub. With several, those + per-file stubs share that id but carry different `origin_file`s, so + `_disambiguate_colliding_node_ids` salts each into a path-qualified id before + the rewire runs — which is why one defect reports as two id shapes. At base + this corpus produced exactly the four ids the issue lists. + """ + base = _write(tmp_path / "agri/baseline.py", _DEF) + legacy = _write(tmp_path / "vendor/legacy.py", "class PricePoint:\n pass\n") + rels = ( + "connections/prices", + "fronts/agri_deviation", + "fronts/upstream_watch/signals", + "signal_resolution", + ) + referrers = [ + _write( + tmp_path / f"signal_intelligence/{rel}.py", + "from agri.baseline import PricePoint\n" + "\n" + "def latest(p: PricePoint) -> PricePoint:\n" + " return p\n", + ) + for rel in rels + ] + + result = extract([base, legacy, *referrers], cache_root=tmp_path) + + labeled = _nodes_labeled(result, "PricePoint") + assert len(labeled) == 2, labeled + assert all(node.get("source_file") for node in labeled), labeled + + wanted = _sole_node_id(result, "PricePoint", "agri/baseline.py") + for rel in rels: + latest_nid = _sole_node_id( + result, "latest()", f"signal_intelligence/{rel}.py" + ) + assert (wanted, "parameter_type") in _refs(result, latest_nid) + + +def test_ambiguous_name_with_no_import_is_left_alone(tmp_path: Path): + """No import names the intended definition, so no edge may be invented.""" + base = _write(tmp_path / "pkg/a/base.py", _DEF) + other = _write(tmp_path / "pkg/z/other.py", "class PricePoint:\n pass\n") + consumer = _write( + tmp_path / "pkg/b/consumer.py", + "def handle(p: PricePoint) -> None:\n" + " return None\n", + ) + + result = extract([base, other, consumer], cache_root=tmp_path) + + handle_nid = _sole_node_id(result, "handle()", "pkg/b/consumer.py") + real_ids = { + _sole_node_id(result, "PricePoint", "pkg/a/base.py"), + _sole_node_id(result, "PricePoint", "pkg/z/other.py"), + } + # Guessing one of two equally-plausible definitions would be a false edge. + assert not (real_ids & {target for target, _ in _refs(result, handle_nid)}) + + +def test_unambiguous_cross_file_annotation_stays_a_single_node(tmp_path: Path): + """Pins the behavior that already worked at 0.9.31 (the issue's main claim). + + Four referencing files, one definition: the per-file sourceless stubs must + keep collapsing onto the real node instead of being salted apart by + ``_disambiguate_colliding_node_ids``. + """ + base = _write(tmp_path / "agri/baseline.py", _DEF) + consumers = [ + _write( + tmp_path / f"signal_intelligence/{name}.py", + "from agri.baseline import PricePoint\n" + "\n" + "def latest(rows: list[PricePoint]) -> PricePoint:\n" + " return rows[0]\n", + ) + for name in ("prices", "deviation", "watch_signals", "resolution") + ] + + result = extract([base, *consumers], cache_root=tmp_path) + + assert len(_nodes_labeled(result, "PricePoint")) == 1 + price_nid = _sole_node_id(result, "PricePoint", "agri/baseline.py") + for name in ("prices", "deviation", "watch_signals", "resolution"): + latest_nid = _sole_node_id( + result, "latest()", f"signal_intelligence/{name}.py" + ) + assert (price_nid, "return_type") in _refs(result, latest_nid) + + +_KEY_VARS = ("GEMINI_API_KEY", "GOOGLE_API_KEY", "OPENAI_API_KEY", "OPENAI_BASE_URL", + "ANTHROPIC_API_KEY", "MOONSHOT_API_KEY", "DEEPSEEK_API_KEY") + + +def test_written_graph_has_one_node_per_class_via_the_cli(tmp_path: Path): + """End-to-end through the real CLI, asserting on the written graph.json. + + A unit test on ``extract()`` can pass while the shipped pipeline drops the + result, so the user-visible artifact gets its own check (`--code-only` needs + no API key, so this runs in CI). + """ + repo = tmp_path / "repo" + _write(repo / "pkg/a/base.py", _DEF) + _write(repo / "pkg/z/other.py", "class PricePoint:\n pass\n") + _write( + repo / "pkg/b/quote.py", + "from dataclasses import dataclass\n" + "from pkg.a.base import PricePoint\n" + "\n" + "@dataclass\n" + "class Quote:\n" + " point: PricePoint\n", + ) + + env = {k: v for k, v in os.environ.items() if k not in _KEY_VARS} + env["GRAPHIFY_OUT"] = str(repo / "graphify-out") + result = subprocess.run( + [sys.executable, "-m", "graphify", "extract", ".", "--code-only"], + cwd=repo, capture_output=True, text=True, env=env, + ) + assert result.returncode == 0, result.stderr + + graph = json.loads((repo / "graphify-out" / "graph.json").read_text()) + # NetworkX <= 3.1 serialises edges as "links" (build.py:1154 reads both). + graph_edges = graph.get("edges", graph.get("links", [])) + price_nodes = [n for n in graph["nodes"] if n.get("label") == "PricePoint"] + # Exactly the two real definitions — no sourceless ghost. + assert len(price_nodes) == 2, price_nodes + assert all(n.get("source_file") for n in price_nodes), price_nodes + + quote = next(n for n in graph["nodes"] if n.get("label") == "Quote") + wanted = next( + n["id"] for n in price_nodes if n.get("source_file") == "pkg/a/base.py" + ) + # The build layer renames the `references` relation to `uses`; context is + # what pins this to the field-annotation path either way. + assert any( + e.get("relation") in ("references", "uses") + and e.get("context") == "field" + and {e.get("source"), e.get("target")} == {quote["id"], wanted} + for e in graph_edges + ), "dataclass field reference missing from the written graph"