mirror of
https://github.com/safishamsi/graphify.git
synced 2026-09-23 14:05:43 +00:00
fix(extract): scope Pascal/Delphi call resolution + resolve inherited calls across files (#1739)
Both Pascal extractors resolved every call via a single file-wide
{method_name: node_id} dict, so two unrelated classes declaring a same-named
method (property accessors, generated COM/TLB wrapper classes) collapsed onto
whichever declaration was inserted last, producing wrong cross-class `calls`
edges. Resolution is now scoped: own class -> ancestor chain (inherits) ->
file-level free function -> unambiguous file-wide match; ambiguous at every
level emits no edge rather than guessing (same god-node guard as the Ruby
resolver).
Adds graphify/pascal_resolution.py, a corpus-wide post-extraction resolver
(registered via resolver_registry) that walks the inherits chain across file
boundaries, so a call from a manual descendant to a method it inherits from a
base class in a separate unit (the generated-base/manual-descendant split)
resolves. Also stops both extractors from emitting a duplicate base-class stub
carrying the referencing file's source_file, which collided with the real node
under cross-file id disambiguation. cache.py gives the new raw_calls bucket the
same portable-path treatment as nodes/edges so it round-trips.
Re-applied to the post-#1737 module layout (extractor hunks land in
graphify/extractors/pascal.py; registration stays in extract.py). Added one
adaptation the original PR predated: the cross-file resolver's god-node guard
now counts DISTINCT method nids, because the tree-sitter extractor emits a
method edge for both the interface declaration and the implementation, so the
same method_nid arrives twice -- without deduping, every inherited call looked
ambiguous and resolved to nothing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
safishamsi
co-authored by
Claude Opus 4.8
parent
2f9deae8d5
commit
d89efbf9ef
+18
@@ -0,0 +1,18 @@
|
||||
unit BaseGadget;
|
||||
|
||||
interface
|
||||
|
||||
type
|
||||
TBaseGadget = class(TObject)
|
||||
public
|
||||
procedure Prepare;
|
||||
end;
|
||||
|
||||
implementation
|
||||
|
||||
procedure TBaseGadget.Prepare;
|
||||
begin
|
||||
{ base prepare }
|
||||
end;
|
||||
|
||||
end.
|
||||
@@ -0,0 +1,21 @@
|
||||
unit DerivedGadget;
|
||||
|
||||
interface
|
||||
|
||||
uses
|
||||
BaseGadget;
|
||||
|
||||
type
|
||||
TDerivedGadget = class(TBaseGadget)
|
||||
public
|
||||
procedure Run;
|
||||
end;
|
||||
|
||||
implementation
|
||||
|
||||
procedure TDerivedGadget.Run;
|
||||
begin
|
||||
Prepare;
|
||||
end;
|
||||
|
||||
end.
|
||||
@@ -0,0 +1,18 @@
|
||||
unit OtherGadget;
|
||||
|
||||
interface
|
||||
|
||||
type
|
||||
TOtherGadget = class(TObject)
|
||||
public
|
||||
procedure Prepare;
|
||||
end;
|
||||
|
||||
implementation
|
||||
|
||||
procedure TOtherGadget.Prepare;
|
||||
begin
|
||||
{ unrelated prepare }
|
||||
end;
|
||||
|
||||
end.
|
||||
+60
@@ -0,0 +1,60 @@
|
||||
unit ScopedCallsUnit;
|
||||
|
||||
interface
|
||||
|
||||
type
|
||||
TFirstWidget = class(TObject)
|
||||
public
|
||||
procedure Configure;
|
||||
procedure Reset;
|
||||
end;
|
||||
|
||||
TSecondWidget = class(TObject)
|
||||
public
|
||||
procedure Configure;
|
||||
procedure Reset;
|
||||
end;
|
||||
|
||||
TBaseWidget = class(TObject)
|
||||
public
|
||||
procedure Prepare;
|
||||
end;
|
||||
|
||||
TDerivedWidget = class(TBaseWidget)
|
||||
public
|
||||
procedure Run;
|
||||
end;
|
||||
|
||||
implementation
|
||||
|
||||
procedure TFirstWidget.Configure;
|
||||
begin
|
||||
Reset;
|
||||
end;
|
||||
|
||||
procedure TFirstWidget.Reset;
|
||||
begin
|
||||
{ first reset }
|
||||
end;
|
||||
|
||||
procedure TSecondWidget.Configure;
|
||||
begin
|
||||
Reset;
|
||||
end;
|
||||
|
||||
procedure TSecondWidget.Reset;
|
||||
begin
|
||||
{ second reset }
|
||||
end;
|
||||
|
||||
procedure TBaseWidget.Prepare;
|
||||
begin
|
||||
{ base prepare }
|
||||
end;
|
||||
|
||||
procedure TDerivedWidget.Run;
|
||||
begin
|
||||
Prepare;
|
||||
end;
|
||||
|
||||
end.
|
||||
@@ -0,0 +1,102 @@
|
||||
"""Regression tests for scoped call resolution in the Pascal/Delphi extractor.
|
||||
|
||||
Before this fix, both `extract_pascal` (tree-sitter path) and
|
||||
`_extract_pascal_regex` (fallback path) resolved every call by a single
|
||||
file-wide ``{method_name_lower: node_id}`` dict with no class scoping. Two
|
||||
unrelated classes declaring a same-named method (a common Pascal/Delphi
|
||||
pattern -- property accessors, generated wrapper classes such as TLB import
|
||||
units) silently collapsed onto whichever declaration was inserted last,
|
||||
producing wrong cross-class `calls` edges. See `sample_scoped_calls.pas`.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
from pathlib import Path
|
||||
|
||||
FIXTURES = Path(__file__).parent / "fixtures"
|
||||
FIXTURE_PATH = FIXTURES / "sample_scoped_calls.pas"
|
||||
|
||||
|
||||
def _extractors():
|
||||
from graphify.extract import extract_pascal, _extract_pascal_regex
|
||||
return [extract_pascal, _extract_pascal_regex]
|
||||
|
||||
|
||||
def _class_node_id(r, class_label):
|
||||
matches = [n["id"] for n in r["nodes"] if n["label"] == class_label]
|
||||
assert len(matches) == 1, f"expected exactly one node labeled {class_label!r}, got {matches}"
|
||||
return matches[0]
|
||||
|
||||
|
||||
def _method_node_id(r, class_label, method_label):
|
||||
class_id = _class_node_id(r, class_label)
|
||||
node_by_id = {n["id"]: n for n in r["nodes"]}
|
||||
for e in r["edges"]:
|
||||
if e["relation"] == "method" and e["source"] == class_id:
|
||||
node = node_by_id.get(e["target"])
|
||||
if node and node["label"] == method_label:
|
||||
return node["id"]
|
||||
raise AssertionError(f"no method edge {class_label}.{method_label} found")
|
||||
|
||||
|
||||
def _has_call(r, src_id, tgt_id):
|
||||
return any(
|
||||
e["relation"] == "calls" and e["source"] == src_id and e["target"] == tgt_id
|
||||
for e in r["edges"]
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("extract", [
|
||||
pytest.param(0, id="tree-sitter"),
|
||||
pytest.param(1, id="regex-fallback"),
|
||||
])
|
||||
def test_calls_scoped_to_own_class(extract):
|
||||
r = _extractors()[extract](FIXTURE_PATH)
|
||||
first_configure = _method_node_id(r, "TFirstWidget", "Configure()")
|
||||
first_reset = _method_node_id(r, "TFirstWidget", "Reset()")
|
||||
assert _has_call(r, first_configure, first_reset)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("extract", [
|
||||
pytest.param(0, id="tree-sitter"),
|
||||
pytest.param(1, id="regex-fallback"),
|
||||
])
|
||||
def test_calls_do_not_cross_unrelated_classes(extract):
|
||||
r = _extractors()[extract](FIXTURE_PATH)
|
||||
first_configure = _method_node_id(r, "TFirstWidget", "Configure()")
|
||||
second_reset = _method_node_id(r, "TSecondWidget", "Reset()")
|
||||
assert not _has_call(r, first_configure, second_reset), (
|
||||
"TFirstWidget.Configure must not resolve Reset() to the unrelated "
|
||||
"TSecondWidget.Reset -- same-named methods on unrelated classes must "
|
||||
"not collapse into a cross-class edge"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("extract", [
|
||||
pytest.param(0, id="tree-sitter"),
|
||||
pytest.param(1, id="regex-fallback"),
|
||||
])
|
||||
def test_calls_scoped_other_direction(extract):
|
||||
r = _extractors()[extract](FIXTURE_PATH)
|
||||
second_configure = _method_node_id(r, "TSecondWidget", "Configure()")
|
||||
second_reset = _method_node_id(r, "TSecondWidget", "Reset()")
|
||||
first_reset = _method_node_id(r, "TFirstWidget", "Reset()")
|
||||
assert _has_call(r, second_configure, second_reset)
|
||||
assert not _has_call(r, second_configure, first_reset), (
|
||||
"TSecondWidget.Configure must not resolve Reset() to the unrelated "
|
||||
"TFirstWidget.Reset"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("extract", [
|
||||
pytest.param(0, id="tree-sitter"),
|
||||
pytest.param(1, id="regex-fallback"),
|
||||
])
|
||||
def test_calls_resolve_via_ancestor_chain(extract):
|
||||
r = _extractors()[extract](FIXTURE_PATH)
|
||||
derived_run = _method_node_id(r, "TDerivedWidget", "Run()")
|
||||
base_prepare = _method_node_id(r, "TBaseWidget", "Prepare()")
|
||||
assert _has_call(r, derived_run, base_prepare), (
|
||||
"TDerivedWidget.Run should resolve the inherited Prepare() to "
|
||||
"TBaseWidget.Prepare via the inherits chain"
|
||||
)
|
||||
@@ -0,0 +1,95 @@
|
||||
"""Tests for cross-file Pascal/Delphi inherited-method-call resolution.
|
||||
|
||||
The per-file Pascal/Delphi extractors resolve a call to the caller's own
|
||||
class, its ancestor chain, or a file-level free function -- but only within
|
||||
the single file being extracted. Real Delphi/MTM-style code very commonly
|
||||
splits a class across two files (a generated base class + a manual
|
||||
descendant that extends it in a separate unit), so a call from the
|
||||
descendant to a method it inherits from the base falls outside any one
|
||||
file's own scope. graphify.pascal_resolution closes that gap as a
|
||||
corpus-wide, post-extraction pass. See its module docstring for the full
|
||||
rationale.
|
||||
|
||||
Uses static fixtures under tests/fixtures/pascal_cross_file/ rather than
|
||||
pytest's tmp_path: the Pascal extractor's cross-file class lookup
|
||||
(_pascal_project_root) walks UP the directory tree looking for the highest
|
||||
ancestor with 2+ .pas files, to find the project root. tmp_path lives under
|
||||
the shared system temp directory, which on a dev machine can easily already
|
||||
contain 2+ stray .pas files at some ancestor level (other tools' scratch
|
||||
files, other tests' leftover fixtures) -- the walk-up then escalates past the
|
||||
test's own directory and picks up unrelated files. tests/fixtures/ has no
|
||||
such siblings above it, so it is a stable project root for these tests.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
from graphify.extract import extract, extract_pascal
|
||||
|
||||
FIXTURES = Path(__file__).parent / "fixtures" / "pascal_cross_file"
|
||||
BASE = FIXTURES / "BaseGadget.pas"
|
||||
OTHER = FIXTURES / "OtherGadget.pas"
|
||||
DERIVED = FIXTURES / "DerivedGadget.pas"
|
||||
|
||||
|
||||
def _find_raw_call(result: dict, callee: str) -> dict | None:
|
||||
for rc in result.get("raw_calls", []):
|
||||
if rc.get("callee") == callee:
|
||||
return rc
|
||||
return None
|
||||
|
||||
|
||||
def _labels(nodes: list[dict]) -> dict[str, str]:
|
||||
return {n["id"]: str(n.get("label", "")) for n in nodes}
|
||||
|
||||
|
||||
def _call_edge(graph: dict, src_label: str, tgt_label: str):
|
||||
labels = _labels(graph["nodes"])
|
||||
for e in graph["edges"]:
|
||||
if e.get("relation") != "calls":
|
||||
continue
|
||||
if labels.get(e.get("source")) == src_label and labels.get(e.get("target")) == tgt_label:
|
||||
return e
|
||||
return None
|
||||
|
||||
|
||||
def test_single_file_extraction_reports_unresolved_inherited_call():
|
||||
"""Sanity check for the gap this resolver closes: the per-file extractor
|
||||
alone cannot see BaseGadget.pas while extracting DerivedGadget.pas, so it
|
||||
must NOT emit a `calls` edge for Run -> Prepare, and must report it via
|
||||
raw_calls instead of silently dropping it."""
|
||||
r = extract_pascal(DERIVED)
|
||||
assert _call_edge(r, "Run()", "Prepare()") is None
|
||||
rc = _find_raw_call(r, "prepare")
|
||||
assert rc is not None
|
||||
assert rc["caller_nid"]
|
||||
|
||||
|
||||
def test_calls_resolve_across_files_via_inherits_chain(tmp_path):
|
||||
# cache_root only controls where graphify-out/cache/ is written -- it has
|
||||
# no bearing on the Pascal cross-file class lookup, which is keyed off
|
||||
# each source path's own project root (see module docstring). Using
|
||||
# tmp_path here just keeps cache artifacts out of the repo.
|
||||
graph = extract([BASE, DERIVED], cache_root=tmp_path, parallel=False)
|
||||
edge = _call_edge(graph, "Run()", "Prepare()")
|
||||
assert edge is not None
|
||||
assert edge.get("confidence") == "EXTRACTED"
|
||||
|
||||
|
||||
def test_cross_file_calls_do_not_cross_unrelated_classes(tmp_path):
|
||||
"""TDerivedGadget inherits only from TBaseGadget. TOtherGadget declares an
|
||||
unrelated same-named Prepare in a third file -- Run() must resolve to
|
||||
TBaseGadget.Prepare, never to TOtherGadget.Prepare."""
|
||||
graph = extract([BASE, OTHER, DERIVED], cache_root=tmp_path, parallel=False)
|
||||
edge = _call_edge(graph, "Run()", "Prepare()")
|
||||
assert edge is not None
|
||||
node_by_id = {n["id"]: n for n in graph["nodes"]}
|
||||
target = node_by_id[edge["target"]]
|
||||
assert "BaseGadget.pas" in target.get("source_file", "")
|
||||
assert "OtherGadget.pas" not in target.get("source_file", "")
|
||||
|
||||
|
||||
def test_pascal_resolver_registered():
|
||||
from graphify.resolver_registry import registered_resolvers
|
||||
names = {r.name for r in registered_resolvers()}
|
||||
assert "pascal_inherited_calls" in names
|
||||
Reference in New Issue
Block a user