Skip to content

fix(extract): keep recursive calls when merging C# partial classes - #4017

Open
rohit-jsfreaky wants to merge 1 commit into
Graphify-Labs:v8from
rohit-jsfreaky:fix/csharp-partial-merge-keeps-self-loops
Open

rohit-jsfreaky wants to merge 1 commit into
Graphify-Labs:v8from
rohit-jsfreaky:fix/csharp-partial-merge-keeps-self-loops

Conversation

@rohit-jsfreaky

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #4016.

_merge_csharp_partial_class_nodes collapses the halves of a C# partial class onto one node, then rewrites every edge in the corpus. It skipped every edge whose endpoints were equal after the remap, so one partial class split across two files erased every recursive calls self-loop in the repo — in every language — and it deduped edges it never rewrote on a key that ignores confidence/context.

This applies the two rules graphify already uses for the same situation elsewhere:

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

Verification & Invariants

Invariant: a merge pass may only drop the self-loops it creates itself; every edge it does not remap comes out exactly as it went in.

  • test_partial_class_keeps_recursive_calls_in_other_languages — Python + TypeScript recursion survives a partial merge (real extract()).
  • test_partial_class_keeps_recursive_call_inside_a_half — a recursive C# method inside a half keeps its self-call (real extract()).
  • test_merge_drops_only_the_self_loops_it_creates — untouched self-loop kept; self-loop on the merged-away half rewired to the canonical node; the collapsed half→half edge still dropped.
  • test_merge_does_not_dedup_edges_it_never_rewrote — two untouched parallel edges that differ only in confidence both survive.

All four fail with graphify/extract.py from v8 and pass with this change. The existing #2332 / #2411 partial-class tests are unchanged and pass.

Persisted state: after the next rebuild, graph.json for a corpus with a split partial class gains back its recursive calls self-loops (and any parallel edges the old dedup pruned). No node ids change; no edge the merge does not touch is altered.

  • Read the CONTRIBUTING.md guide.
  • Reproduced the issue and identified the invariant.
  • Made the smallest fix necessary.
  • Added a regression test (if bug fix) or isolated boundary test.
  • Kept the PR description synchronized with the final implementation.
  • Documented any limitations / unsupported cases explicitly.

How was this tested?

Windows 11, Python 3.13, uv sync --all-extras --frozen.

uv run pytest tests/test_csharp_partial_classes.py -q
  -> 13 passed
  (same tests with graphify/extract.py from v8: 4 failed - the 4 new tests - 9 passed)

uv run pytest tests/test_csharp_partial_classes.py tests/test_dotnet.py tests/test_cross_repo_shared_types.py \
  tests/test_dedup.py tests/test_extract.py tests/test_csharp_*.py tests/test_swift_*.py \
  tests/test_cross_extension_reexport_self_cycle.py -q
  -> 600 passed, 5 failed; the same 5 fail with v8's extract.py (parallel-extraction
     fallback tests in test_extract.py, Windows process pool), unrelated to this change

uv run pytest tests -q
  -> 6262 passed, 23 failed, 27 skipped
     22 of the 23 fail identically with graphify/extract.py from v8 (Windows-only install /
     hooks / watch / atomic-write / non-regular-file / parallel-extraction tests).
     The 23rd, test_incremental_mtime_collision.py::test_same_size_rewrite_in_one_tick_is_requeued,
     failed once inside the full run and passes 5/5 in isolation both with and without this
     change (an mtime-tick timing test; it does not touch the partial-class merge).

uv run ruff check graphify/extract.py tests/test_csharp_partial_classes.py
  -> All checks passed!

uv run bandit -q -ll graphify/extract.py
  -> 5 findings (B314 x4, B324 x1), identical to v8, none in the changed lines

uv run pyright graphify/extract.py tests/test_csharp_partial_classes.py
  -> 119 errors, the same count as v8 (no new errors); the 4 inside the changed block are the
     existing remap.get(e.get("source"), ...) line, unchanged and carried over

graphify update <google/gson gson/src/main + Form1.cs + Form1.Designer.cs> --force --no-cluster
  -> recursive calls self-loops: 0 before, 22 after (22 = gson alone)
graphify update <JamesNK/Newtonsoft.Json Src/Newtonsoft.Json> --force --no-cluster
  -> edges 11,415 -> 11,625 (+210, 0 removed): 206 calls self-loops, 3 references, 1 inherits

Limitations: the Swift extension merge and this merge now run the same edge-rewrite loop. A shared helper would stop a third copy drifting, but I left it out to keep this PR to the bug. (build.deduplicate_by_label has the old pattern too, but it is not wired into build().)

Graphify-specific checklist

  • I updated generated skill artifacts (uv run python -m tools.skillgen --bless) when changing their source fragments. — n/a, no skill fragments touched.
  • I confirmed that AST/structural extraction remains deterministic (no ambient state dependencies like ENV variables).
  • I reviewed changes for security implications (no unsafe interpolation into shell/Python).
  • I confirmed no API keys or local-only graph data are included.
  • (If applicable) I disclosed AI authorship in my commit messages.

_merge_csharp_partial_class_nodes rewrote every edge in the corpus and
dropped every self-loop, so one partial class split across two files
erased recursive calls in every language of the repo. It also deduped
edges it never rewrote, on a key that ignores confidence and context.

Keep edges the merge does not remap verbatim (the Swift extension merge
rule, Graphify-Labs#2538) and drop a self-loop only when two distinct endpoints
collapse into one node (the deduplicate_entities rule, Graphify-Labs#3809), so a
pre-existing self-loop on a merged-away half is rewired, not lost.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Thanks for the pull request, @rohit-jsfreaky. A maintainer will review it soon.

Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions.

A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic.

@graphify-labs graphify-labs Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).

Not verified on this run: \_merge\_csharp\_partial\_class\_nodes (vacuous: never exercised).


Graphify review — findings

Fixes _merge_csharp_partial_class_nodes so it only drops self-loops it creates by collapsing two distinct partial halves into one node. Pre-existing self-loops are kept and rewired onto the canonical node, so recursive calls in Python, TypeScript and the rest no longer vanish whenever a repo contains a single C# partial class. Edges the merge doesn't remap pass through verbatim and are excluded from the dedup, which keys only on source/target/relation/file/location, so parallel edges that differ in confidence both survive.

No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 2274 functions depend on the 292 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract() — 747 callers, 50 callees
  • new: _rebuild_code() — 147 callers, 56 callees
  • new: extract_js() — 87 callers, 4 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: main() — 98 callers, 3 callees
  • new: dispatch_command() — 2 callers, 126 callees
  • new: _get_extractor() — 27 callers, 6 callees
  • new: collect_files() — 19 callers, 6 callees
  • …and 35 more — each is listed as a finding

Verification — 2274 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: 2094 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

137 of 318 test file(s) selected (43%) via static blast radius.

  • tests/test_astro_extraction.py — impact
  • tests/test_astro_import_ids.py — impact
  • tests/test_blade_extractor.py — impact
  • tests/test_build.py — impact
  • tests/test_builtin_global_type_refs.py — impact
  • tests/test_case_sensitive_resolution.py — impact
  • tests/test_cjs_module_extension.py — impact
  • tests/test_cobol_extractor.py — impact
  • tests/test_cpp_method_declarations.py — impact
  • tests/test_cpp_nested_and_cli.py — impact
  • tests/test_cpp_objc_cross_file_calls.py — impact
  • tests/test_cross_extension_reexport_self_cycle.py — impact
  • tests/test_cross_language_call_resolution.py — impact
  • tests/test_cross_repo_external_call_guards.py — impact
  • tests/test_cross_repo_member_calls.py — impact
  • tests/test_csharp_call_site_generic_args.py — impact
  • tests/test_csharp_enum_members.py — impact
  • tests/test_csharp_field_generic_args.py — impact
  • tests/test_csharp_generic_callsites.py — impact
  • tests/test_csharp_interface_dispatch.py — impact
  • tests/test_csharp_member_calls.py — impact
  • tests/test_csharp_member_nodes.py — impact
  • tests/test_csharp_object_creation.py — impact
  • tests/test_csharp_partial_classes.py — impact, changed-test
  • tests/test_csharp_tuple_type_refs.py — impact
  • tests/test_csharp_type_resolution.py — impact
  • tests/test_definition_file_portability.py — impact
  • tests/test_detect.py — impact
  • tests/test_dotnet.py — impact
  • tests/test_duplicate_annotation_edges.py — impact
  • tests/test_elixir_import_resolution.py — impact
  • tests/test_erlang_extractor.py — impact
  • tests/test_extract.py — impact
  • tests/test_extract_cache_location.py — impact
  • tests/test_extract_php_closures.py — impact
  • tests/test_file_label_disambiguation.py — impact
  • tests/test_file_node_id_spec.py — impact
  • tests/test_forwarding_review_findings.py — impact
  • tests/test_go_builtin_call_targets.py — impact
  • tests/test_go_import_repoint.py — impact
  • tests/test_go_interface_methods.py — impact
  • tests/test_go_qualified_resolution.py — impact
  • tests/test_import_extension_resolution.py — impact
  • tests/test_import_self_loops.py — impact
  • tests/test_imported_export_forwarding.py — impact
  • tests/test_incremental.py — impact
  • tests/test_indirect_call_arrow_single_param_shadow.py — impact
  • tests/test_indirect_call_block_scoped_shadow.py — impact
  • tests/test_indirect_call_catch_binding_shadow.py — impact
  • tests/test_indirect_call_external_import_shadow.py — impact
  • … and 87 more

Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.

Formal verification

Could not verify: Could not verify \_merge\_csharp\_partial\_class\_nodes.

The verifier did not have enough to check \_merge\_csharp\_partial\_class\_nodes, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: no capturable inputs from the test suite; property tier: non-vacuity: domain too small (only 1 distinct inputs exercised, need 3) — 'no divergence' would be near-vacuous

· 43 more finding(s) on lines outside this diff (see the check run).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: one C# partial class drops every recursive call in the graph, in every language

1 participant