fix(extract): resolve Python cross-file type references instead of ghosting them (#2363) - #2369
fix(extract): resolve Python cross-file type references instead of ghosting them (#2363)#2369Rishet11 wants to merge 4 commits into
Conversation
…osting them (Graphify-Labs#2363) Two defects behind the duplicate-node reports, both Python-only gaps against behavior other languages already have. Class-body field annotations were never collected. `_python_collect_type_refs` was reached only from function parameters and return types, so the dataclass shape @DataClass class Quote: point: PricePoint emitted no edge at all. Java (record components / field declarations) and TS (`public_field_definition`) both emit `references` with context="field" for the identical shape; Python now does too, via the same `_semantic_reference_edge` emitter the parameter/return path uses. An assignment with no `type` child is a plain value binding, not a type reference, so it is skipped. When a class simple-name is ambiguous, `_rewire_unique_stub_nodes` bails, so the sourceless shadow stub survives as a bare `pricepoint` node and the reference edge stays stuck on it, leaving both real definitions looking unreferenced. A `from pkg.a.base import PricePoint` names the defining module exactly, which is how Java (`_resolve_java_type_references`), PHP and C# disambiguate; Python had no equivalent. `_resolve_python_type_references` adds it, keyed on the local binding so `import X as Y` resolves too, and runs after the unique-stub rewire so it only handles the ambiguous remainder. Its repoint set is deliberately narrower than the Java/C# ones, which also carry `imports`: a Python file-level import edge is resolved upstream and never lands on a bare-name stub, verified against ambiguous, aliased and unresolvable-module corpora, so including it would be an untested no-op. The resolver is also gated on shadow stubs existing, so its parse pass is paid only on a corpus that has unresolved references. A reference with no import naming its definition is still left alone: guessing one of two equally-plausible classes would invent an edge.
…#2363) Nine cases, each pinned to a distinct path: - dataclass field annotation emits a `references`/field edge, and no second node - a container-wrapped field type is a generic_arg, matching the parameter path - a plain `x = 1` class attribute emits nothing - an ambiguous class name resolves through the file's import, for both a parameter annotation and a field annotation - an aliased `import X as Y` resolves on the local binding - an ambiguous name with no import is left alone, so no edge is invented - four referencing files against one definition stay a single node, pinning the behavior that already worked so it cannot silently regress - the real CLI writes a graph.json with no ghost and the field edge present The last one runs `python -m graphify extract . --code-only`, which needs no API key, because a unit test on `extract()` can pass while the shipped pipeline drops the result.
There was a problem hiding this comment.
Pull request overview
This PR improves Graphify’s Python AST extraction so cross-file type annotations produce correct references edges and don’t leave behind “shadow” stub nodes when class names are ambiguous across modules (issue #2363). It brings Python closer to the existing Java/C#/PHP type-resolution behavior by collecting missing class-body field annotations and adding an import-aware post-pass resolver for remaining ambiguous stubs.
Changes:
- Emit
referencesedges for Python class-body field annotations (including generic-arg contexts) during extraction. - Add a Python type-reference resolver that repoints dangling edges from sourceless stubs to the imported definition when disambiguated by
from ... import ...bindings. - Add regression tests (including an end-to-end CLI run) to prevent both “dropped edge” and “ghost stub” regressions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_python_type_references.py | Adds regression coverage for class-body field annotations and ambiguous imported type resolution, including a CLI-level assertion on graph.json. |
| graphify/extractors/resolution.py | Introduces _resolve_python_type_references to repoint edges off sourceless stubs using import facts, then prune unreferenced stubs. |
| graphify/extractors/engine.py | Walks Python class_definition bodies to collect annotated assignments and emit references edges with field / generic_arg context. |
| graphify/extract.py | Integrates the new Python type-reference resolver into the extraction pipeline after _rewire_unique_stub_nodes and cross-file import resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """`import X as Y` binds Y — the resolver keys on the local name, not the | ||
| exported one, so the annotation `p: PP` still finds `pkg.a.base.PricePoint`.""" |
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR adds Python cross-file type-reference resolution to the graphify extractor. It introduces a new _resolve_python_type_references pass (in resolution.py) that re-points dangling type-annotation edges from ambiguous sourceless "shadow" stub nodes onto the actual imported class definitions using each file's own imports, and drops any stubs left unreferenced; this pass is wired into the main extract flow for Python files. It also extends _extract_generic in engine.py to collect class-body field annotations (e.g. dataclass fields like point: PricePoint) and emit references/field edges for them. A new test file (tests/test_python_type_references.py) covers these behaviors, including field-annotation edges, ambiguous same-named classes across packages, aliased imports, and a CLI-based single-node-per-class check.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1598 functions depend on the 496 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 360 callers, 29 callees - worse:
_collect_python_symbol_resolution_facts()— 3 callers, 10 callees
Verification — 1598 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1471 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
…ver's cost note (Graphify-Labs#2363) Two self-review gaps. `inherits` is in the resolver's repoint set but had no test. Superclasses reach `ensure_named_node` from a different call site than type annotations, so the annotation cases did not cover it. Added the ambiguous-superclass case; it passes, so this pins behavior rather than fixing it. The docstring claimed the second parse is "paid only on a corpus that actually has unresolved stubs, which is the abnormal case". That is wrong: every stdlib or third-party annotation (`sqlite3.Connection`, `pathlib.Path`) leaves a sourceless stub, so ordinary code trips the gate and most real corpora do pay it, at roughly 15ms per file. Narrowing the gate to "a stub label names a known definition" was tried and reverted, because the stub for `from X import Y as Z` carries the alias `Z` and no definition is labeled `Z` — it silently disabled alias resolution. The note now states the real cost and records the rejected alternative.
|
Self-review follow-up in c8b3780, correcting two things in what I first pushed. The performance note in the PR body was wrong. I wrote that the resolver's parse pass is "only paid on a corpus that actually has unresolved references" and that "a clean corpus pays nothing". That is not true. Every stdlib or third-party annotation leaves a sourceless stub — this one-file corpus produces two: import sqlite3
from pathlib import Path
def f(c: sqlite3.Connection, p: Path) -> None: ...So ordinary code trips the gate and most real corpora do pay the pass, measured at ~15ms/file (1.18s across this repo's own 80 files under I tried tightening the gate to "a stub label names a real definition in the corpus", which does shut it on the case above. I reverted it: the stub for
Now 10 tests. Suite 4 failed / 3905 passed, same four env-dependent failures that reproduce with the change reverted; ruff and One thing still open from the issue, which I do not want to paper over: I could not reproduce the path-qualified ghost variant ( |
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR adds Python cross-file type-reference resolution to the graphify extractor. In the engine, it extends _extract_generic to collect type references from class-body field annotations (e.g. dataclass fields like point: PricePoint), which previously were not walked. In resolution, it introduces a new _resolve_python_type_references function—wired into the extract pipeline after cross-file import resolution—that re-points dangling type-reference edges stuck on ambiguous sourceless "shadow" stubs to the correct imported definition using each file's own imports. The change also adds a new test file (tests/test_python_type_references.py) covering the field-annotation and ambiguous-name scenarios. The large set of listed changed symbols appears mostly incidental to the diff shown, which centers on extract.py, engine.py, and resolution.py.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1600 functions depend on the 498 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 361 callers, 29 callees - worse:
_collect_python_symbol_resolution_facts()— 3 callers, 10 callees
Verification — 1600 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1473 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
…fy-Labs#2363) The second ghost shape in the report, `signal_intelligence_<path>_py_pricepoint`, now has a reproduction and turns out to share the bare stub's root cause. The missing ingredient was a second same-named definition: with one referring file the ambiguous reference leaves the bare `pricepoint` stub, and with several, the 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 `_rewire_unique_stub_nodes` can collapse them. Against base, the fixture emits exactly the four ids the issue lists: signal_intelligence_connections_prices_py_pricepoint signal_intelligence_fronts_agri_deviation_py_pricepoint signal_intelligence_fronts_upstream_watch_signals_py_pricepoint signal_intelligence_signal_resolution_py_pricepoint The import-aware resolver already handles them, so this adds coverage rather than changing behavior: both reported id shapes are one defect. Also corrects a test docstring that said `import X as Y` while exercising `from X import Y as Z`.
|
Closing the loop on the one thing I said I could not explain. The path-qualified ghosts reproduce, and they share the bare stub's root cause — 271b130 adds the fixture, PR body updated. The ingredient I was missing is a second same-named definition elsewhere in the corpus. With one, everything resolves cleanly, which is why my earlier repros came out empty. Add a Those are @SatishRockzz's four ids character for character. Each referring file mints its own sourceless stub with the bare id That also predicts something checkable in the reporter's corpus: each of those 11 classes should have a second definition of the same name somewhere — a vendored copy, a No behavior change from this commit — the resolver already handled the case, so it is coverage for a shape I previously could not assert. 11 tests now; 8 fail at base, the 3 that pass are the pins and negative controls. Suite 4 failed / 3906 passed, empty failure-set diff; ruff and skillgen clean. Also fixed the docstring wording Copilot flagged. |
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This pull request adds a new Python cross-file type-reference resolution pass to the extractor pipeline, targeting issue #2363 around duplicate/ghost nodes. It does two things: collects type references from class-body field annotations (the dataclass shape) in the generic extractor, and adds a new _resolve_python_type_references function in resolution.py that re-points dangling type-reference edges from sourceless "shadow" stubs onto the imported definition using each file's own imports. The changes wire this new resolver into extract(), add the function's import, and modify the class-body handling in engine.py to emit references edges for annotated fields. A new test file (tests/test_python_type_references.py) is added covering the field-annotation and ambiguous-name resolution scenarios. The large list of "changed symbols" appears to reflect renamed/reindexed rationale-comment identifiers across these files rather than substantive logic changes.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1602 functions depend on the 500 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 362 callers, 29 callees - worse:
_collect_python_symbol_resolution_facts()— 3 callers, 10 callees
Verification — 1602 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1475 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
Closes #2363
What I found
The reported duplication is one defect with two id shapes, plus a second, separate defect that the issue title also names. Both are Python-only gaps against behavior other languages already have.
I first measured the reporter's layout with a single definition of
PricePointand four referring files: that comes out clean onv8head, one node and four correct edges. The missing ingredient for the ghosts is a second same-named definition elsewhere in the corpus. With that, both reported shapes reproduce, and which one you get depends only on how many files refer to it:Those are the four ids from the issue, character for character. The mechanism: each referring file mints its own sourceless stub with the bare id
pricepoint; when there are several, they collide on that id but carry differentorigin_files, so_disambiguate_colliding_node_idssalts each into a path-qualified id before_rewire_unique_stub_nodesruns. So the "two different ID-generation schemes colliding" the issue describes is really one scheme observed at two corpus sizes.1. Class-body field annotations were never collected.
_python_collect_type_refshad exactly two call sites, function parameters and return types. Nothing walked aclass_definitionbody, so the dataclass shape named in the issue title emitted no edge at all — a dropped edge, not a ghost:Java and TS both handle their equivalent today. Measured on identical corpora:
2. An ambiguous class name leaves the reference stuck on a shadow stub.
_rewire_unique_stub_nodesbails when the label is not globally unique, so with two modules definingPricePointthe sourceless stub survives and the edge stays on it — which is exactly the bare-lowercase ghost variant the reporter hit 7 times:from pkg.a.base import PricePointnames the defining module exactly. Java resolves this with_resolve_java_type_references, PHP and C# with their own resolvers. Python had none —_collect_python_symbol_resolution_factscollectsimportsandcalls, never type references. Java on the identical corpus gets it right:The fix
Both changes follow existing precedent rather than introducing anything new — no new edge type, no schema, exporter or
affectedchanges.engine.py: a Python class-body walk emittingreferenceswith contextfield/generic_arg, mirroring the Java record-component branch, through the same_semantic_reference_edgeemitter the parameter/return path already uses (it validates the context againstREFERENCE_CONTEXTS).resolution.py+extract.py:_resolve_python_type_references, mirroring_resolve_java_type_references, keyed on the local binding soimport X as Yresolves. Runs after_rewire_unique_stub_nodes, so it only handles the ambiguous remainder.Two deliberate narrowings, both measured rather than assumed:
imports, which Java and C# both carry. A Python file-level import edge is resolved upstream and never lands on a bare-name stub — verified against ambiguous, aliased and unresolvable-module corpora — so including it would have been an untested no-op.Before / after, real CLI
graphify extract . --code-onlyon one definition ofPricePointinpkg/a/base.py, a second inpkg/z/other.py, and aQuotedataclass importing the first:Before — the field reference is absent entirely, and
Quoteis isolated:After — the edge exists and binds to the imported definition, not the other same-named one:
Verification
tests/test_python_type_references.py. Checked out against base, 8 fail and 3 pass — the 3 are the pins and negative controls (the no-import bail-out, the plainx = 1attribute, the already-working unambiguous case), which are supposed to be green both sides.graph.json, since a unit test onextract()can pass while the shipped pipeline drops the result.test_ollama.pyx3 andtest_llm_backends.py, all reading real env vars), so the failure-set diff is empty.ruffclean.python -m tools.skillgen --check→check OK: 134 artifact(s).Known limitations
from pkg.a.base import *) are not used for disambiguation.Relationship to #1231
#1231 targets this same area and has been conflicting since June. I did not build on it — it routes annotations through
_SymbolUseFact, which is a larger redesign of which layer owns Python type references, and it emitscontext="annotation", which is not a member ofREFERENCE_CONTEXTS. Happy to close this if you would rather revive that one.