fix(extract): scope Pascal/Delphi call resolution + resolve inherited calls across files#1739
Closed
richtext wants to merge 2 commits into
Closed
fix(extract): scope Pascal/Delphi call resolution + resolve inherited calls across files#1739richtext wants to merge 2 commits into
richtext wants to merge 2 commits into
Conversation
… chain
Both extract_pascal (tree-sitter) and _extract_pascal_regex (fallback)
resolved every call via 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
COM/TLB wrapper classes) silently collapsed onto whichever declaration
was inserted last, producing wrong cross-class `calls` edges.
Add _resolve_pascal_callee_factory, shared by both extractors, which
resolves a call in this order: (1) a method on the caller's own class,
(2) a method on an ancestor class via the already-resolved `inherits`
edges, (3) a file-level free function, (4) an unambiguous global match
(exactly one procedure with that name in the file). Ambiguous at every
level -> no edge, rather than guessing wrong (mirrors the god-node guard
already used by resolve_ruby_member_calls for the analogous Ruby
problem).
Also fixes a related bug found while testing the ancestor-chain case:
_extract_pascal_regex's base-class resolution always went through the
cross-file, one-class-per-file convention lookup, creating a duplicate
stub node for a base class that is declared in the same file as its
subclass. It now reuses the real same-file node when present, matching
what extract_pascal already did correctly.
Verified against a real MTM/Delphi codebase (COM/TLB import unit with 3
near-identical wrapper classes sharing method names like Create/
Set_RaiseExceptions): 26 of 86 `calls` edges were cross-class false
positives before this fix; all are now correctly suppressed while
same-class and inherited-method calls (e.g. TKernel.ConnectTo ->
TKernel.DisConnect) resolve precisely per class.
Adds tests/fixtures/sample_scoped_calls.pas and
tests/test_pascal_call_scoping.py: 8 tests (4 scenarios x 2 extractors)
covering own-class resolution, the cross-class collision regression (both
directions), and ancestor-chain resolution. Confirmed these fail on the
pre-fix code (5/8, including an order-dependent case that happened to
pass by luck) and pass after the fix. No regressions in the existing
Pascal/Delphi/Lazarus suite (38 tests) or the full test suite (2845
passed; the 25 unrelated failures pre-exist in Terraform/Ollama/
image-vision/install areas untouched by this change).
… files The scoped call resolution added in the previous commit only sees a single file at a time (each Pascal/Delphi file is extracted independently), so a call from a manual descendant class to a method it inherits from an ancestor declared in a DIFFERENT file -- the common code-generator-base + manual-descendant split (e.g. Sistec's Th0Xxx/Th5Xxx) -- fell outside any one extraction's own scope and was silently dropped. Add graphify/pascal_resolution.py: a corpus-wide, post-extraction resolver (registered via resolver_registry, same mechanism as resolve_ruby_member_ calls) that walks the `inherits` chain across file boundaries using the full merged node/edge graph. It intentionally does NOT fall back to a global by-name match the way the per-file pass's last tier does -- walking `inherits` mirrors Delphi's actual method-lookup semantics, so it is a structurally justified resolution; guessing by name across an entire multi-thousand-file corpus is a different bet and stays out of scope here. extract_pascal and _extract_pascal_regex now report locally-unresolved calls via a `raw_calls` list (source_file, source_location, caller_nid, callee) instead of dropping them, following the same convention other languages already use for their own cross-file resolvers. Ownership (which class a raw call's caller belongs to) is looked up from the `method` edges in the final merged graph rather than carried as a separate field on the raw call -- a field the generic id-remap machinery that runs before resolvers would not know to keep in sync with the corpus's finalized ids. cache.py: raw_calls now goes through the same source_file relativize/ absolutize treatment as nodes/edges/hyperedges, so cached entries carrying unresolved calls stay portable across machines/checkout roots. Also fixes a related bug surfaced while testing the cross-file case: both Pascal extractors, on resolving a base class to another file via _pascal_resolve_class, still added a duplicate stub node for it (comment: "Stub for RTL/external/cross-file base classes") carrying the REFERENCING file's source_file instead of the base class's own. That duplicate collided with the base class's real node under cross-file id disambiguation, producing two different salted ids for what should be one class -- the `inherits` edge pointed at one id, the class's real `method` edges lived under the other, so no ancestor-chain walk could ever connect them. Stubs are now only created for the true external/unresolvable case (RTL classes etc.); a successfully resolved cross-file base reuses its real node's id. Tests: tests/test_pascal_resolution.py (4 tests) exercise the full extract() pipeline over static fixtures in tests/fixtures/pascal_cross_file/ (own-class call still resolves, cross-file inherited call resolves, an unrelated same-named method in a third file is not crossed into, and the resolver is registered). Verified these fail without this commit's changes (including a recurrence check confirming the duplicate-stub bug independently breaks the cross-file case even with the resolver present) and pass with them. Static fixtures are used instead of pytest's tmp_path because _pascal_project_root walks up looking for the highest ancestor with 2+ .pas files to find a "project root" for cross-file lookups; tmp_path lives under the shared system temp directory, which can already contain unrelated .pas files at some ancestor level on a real dev machine, escalating the search past the test's own directory. Re-validated against a real MTM/Delphi client-customization module (____M3_Customizacoes/M3FmPneus/H, 191 files): the generated/manual split (uh5*.pas extends uh0*.pas in _Gerados/) resolves across files as expected; 7 previously-invisible cross-file calls now resolve, all correctly (spot- checked). Lower than the file count might suggest -- this MTM-style framework leans more on override/dispatch semantics (already captured by the existing inherits+method edges) than on direct unqualified calls to inherited methods, so the biggest win here is precision and correctness on the calls that DO exist this way, not raw edge volume. No regressions: existing Pascal/Delphi/Lazarus suite (46 tests) and the full test suite (2849 passed, same 25 pre-existing unrelated failures in Terraform/Ollama/image-vision/install areas as before this change) both pass unchanged.
safishamsi
pushed a commit
that referenced
this pull request
Jul 8, 2026
… 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>
Collaborator
|
Merged on v8 in d89efbf, authored to you. This is a well-engineered pair of fixes and the writeup made the design intent clear. Because the #1737 module refactor landed on v8 after you opened this, the PR conflicted (the Pascal extractors moved from One adaptation your PR predated: on current v8 the tree-sitter Pascal extractor emits a Verified: |
safishamsi
added a commit
that referenced
this pull request
Jul 9, 2026
Two correctness fixes found while analysing the reported 'graphify update occasionally writes a partial graph.json' bug. Enumeration (P0): detect()'s os.walk had no onerror handler, so any os.scandir failure -- a transient PermissionError, or a directory created/deleted mid-walk by concurrent writes (e.g. benchmarking racing the scan) -- was silently swallowed and that entire subtree dropped out of the file list with no log, no error. Downstream that becomes a silently partial graph.json. The walk now records each skipped directory (surfaced as walk_errors in detect()'s result) and warns to stderr, while still enumerating the rest of the tree. This stays visible even when a --force/GRAPHIFY_FORCE rebuild bypasses the shrink guards. Relatedly, to_json's #479 anti-shrink guard was fail-OPEN: a non-empty but unreadable existing graph.json (corrupt or mid-write) proceeded with the overwrite. It now fails SAFE -- refuse and point at force=True -- while an empty/whitespace existing file (no nodes to lose) still proceeds. The size-cap check keeps running before any read, so an oversized existing file is not loaded into memory. Pascal edges (P1): a class method declared in the interface section and defined in the implementation section each emitted a edge to the same node id, and the edge helpers (unlike the node helpers) did not dedup, so ~half of a Pascal/Delphi graph's method edges were doubled -- inflating degree/centrality and tripping the #1739 cross-file resolver's single-owner god-node guard. Both extractors now dedup edges on (source, target, relation). Adds regression tests for all three behaviours. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
safishamsi
added a commit
that referenced
this pull request
Jul 9, 2026
Two correctness fixes found while analysing the reported 'graphify update occasionally writes a partial graph.json' bug. Enumeration (P0): detect()'s os.walk had no onerror handler, so any os.scandir failure -- a transient PermissionError, or a directory created/deleted mid-walk by concurrent writes (e.g. benchmarking racing the scan) -- was silently swallowed and that entire subtree dropped out of the file list with no log, no error. Downstream that becomes a silently partial graph.json. The walk now records each skipped directory (surfaced as walk_errors in detect()'s result) and warns to stderr, while still enumerating the rest of the tree. This stays visible even when a --force/GRAPHIFY_FORCE rebuild bypasses the shrink guards. Relatedly, to_json's #479 anti-shrink guard was fail-OPEN: a non-empty but unreadable existing graph.json (corrupt or mid-write) proceeded with the overwrite. It now fails SAFE -- refuse and point at force=True -- while an empty/whitespace existing file (no nodes to lose) still proceeds. The size-cap check keeps running before any read, so an oversized existing file is not loaded into memory. Pascal edges (P1): a class method declared in the interface section and defined in the implementation section each emitted a "method" edge to the same node id, and the edge helpers (unlike the node helpers) did not dedup, so ~half of a Pascal/Delphi graph's method edges were doubled -- inflating degree/centrality and tripping the #1739 cross-file resolver's single-owner god-node guard. Both extractors now dedup edges on (source, target, relation). Adds regression tests for all three behaviours. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related fixes to Pascal/Delphi
callsedge resolution, found while evaluating graphify against a large real-world Delphi/MTM-framework codebase:Scope call resolution to class + inherits chain (same file). Both
extract_pascal(tree-sitter) and_extract_pascal_regex(fallback) resolved every call via a single file-wide{method_name_lower: node_id}dict with no class scoping. Two unrelated classes declaring a same-named method (a common pattern — property accessors, generated COM/TLB wrapper classes) silently collapsed onto whichever declaration was inserted last, producing wrong cross-classcallsedges. Now resolves via: own class → ancestor chain (inherits) → file-level free function → unambiguous file-wide match: ambiguous at every level → no edge, rather than a guess (same "god-node guard" principle asresolve_ruby_member_calls).Resolve calls to methods inherited across file boundaries. 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 — e.g. Sistec's
Th0Xxx/Th5Xxxpattern). A call from the descendant to a method it inherits from the base fell outside any one file's own scope. Addsgraphify/pascal_resolution.py, a corpus-wide post-extraction resolver (registered viaresolver_registry, same mechanism as the Ruby resolver) that walks theinheritschain across files using the full merged graph. Also fixes a related bug surfaced while testing this: both extractors, on resolving a base class to another file, still added a duplicate stub node carrying the referencing file'ssource_fileinstead of the base class's own — colliding with the real node under cross-file id disambiguation and producing two different ids for what should be one class.Verification
tests/test_pascal_call_scoping.py,tests/test_pascal_resolution.py), covering both extractors, own-class resolution, the cross-class collision regression (both directions), ancestor-chain resolution, and cross-file resolution (own-class call still resolves, cross-file inherited call resolves, an unrelated same-named method in a third file is not crossed into, resolver registration).callsedges on a COM/TLB wrapper module with 3 near-identical classes sharing method names, and correctly resolved cross-file inherited calls on a generated-base/manual-descendant module pair.Test plan
pytest tests/test_pascal.py tests/test_pascal_call_scoping.py tests/test_pascal_resolution.py— all passpytest -q) — no new failures