fix(extract): merge-dedup duplicate Function nodes for Java interface/enum methods (#1234)

[harshitaajoshi]

What does this PR do?

Fixes #1234.

Java interface and enum methods were emitted as both a Method node (correct, via extract_class_methods) and a duplicate Function node (incorrect, via the walk_defs fallback).

Two changes fix this:

1. extract_class_methods now descends into enum_body_declarations so Java enum methods get proper class-qualified Method definitions. Previously only interface methods received Method nodes; enum methods only existed as Function nodes from the walk_defs fallthrough.

2. cbm_dedup_class_method_functions runs after cbm_extract_unified completes and merges duplicate Function defs into their corresponding Method defs. For each Function that shares (name, file, start line) with a Method, it rewrites any call records whose enclosing_func_qn referenced the Function QN to use the surviving Method QN, then removes the Function def. Dedup is a merge, not a delete.

The Method emission also fixes a side effect on main: eight other record kinds that carry enclosing_func_qn (usages, throws, read/write, type refs, env accesses, type assigns, string refs, channels) were dangling for enum method bodies because no definition with the class-qualified QN existed.

Verification

• build/c/test-runner extraction -- 266 passed
• build/c/test-runner convergence_probe -- 48 passed
• New extraction tests: java_interface_no_duplicate_function_issue1234 and java_enum_dedup_preserves_calls_issue1234 assert Method nodes survive, Function count is zero
• New convergence probe: cp_interface_method_calls_after_dedup asserts surviving Method node carries CALLS edges after dedup

Checklist

• Every commit is signed off (git commit -s)
• Tests pass locally (make -f Makefile.cbm test)
  ASan hangs on macOS Tahoe 26.5 (Apple Clang 17). All focused suite tests pass when built without sanitizers (SANITIZE="").
• Lint passes (make -f Makefile.cbm lint-ci)
• New behavior is covered by a test (reproduce-first for bug fixes)

[DeusData]

Thank you for this — and thank you for the sibling PR #1329, which I merged earlier tonight (the GoogleTest unique-name fix, 4573a51d8). This one needs a change before it can follow.

The failing checks here are genuinely yours, which is worth saying plainly because it is not true of most of the red PRs I have been through tonight — several were cancellation noise or main-side breakage. This one is not.

The signature is unambiguous: the same assertion fails deterministically on all five platforms —

FAIL tests/test_convergence_probe.c:1143: ASSERT(calls >= 1)

macOS 14, macOS 15 Intel, Ubuntu latest, Ubuntu arm64 and Windows CLANG64 all fail identically, while other PRs' runs pass that suite on comparable merge refs. One cause, five legs, no flake pattern.

What is happening. The dedup in internal/cbm/extract_defs.c removes the duplicate Java interface-method Function node, which is the right goal — but the node it drops is the one the convergence probe expects to carry at least one CALLS edge. So the duplicate goes away and the edge goes away with it.

The fix shape: re-attach the edges to the surviving node before deduplicating, rather than dropping a node that still owns relationships. Deduplication in this graph has to be a merge, not a delete — otherwise it trades one accuracy bug (a spurious duplicate node) for a worse one (a missing CALLS edge), and we care more about losing true edges than about carrying a redundant node.

To reproduce locally in seconds, rather than waiting on CI:

build/c/test-runner convergence_probe

That is the smallest arena that shows the failure, so you can iterate on it directly and only widen once it is green.

Worth adding a test alongside the fix that asserts the surviving node still carries its CALLS edge after dedup — that is exactly the property that broke, and nothing currently guards it.

The underlying change is a real improvement and closes a genuine issue (#1234); it is only the edge-preservation half that is missing. Ping me when it is pushed and I will pick it straight back up.

[harshitaajoshi]

@DeusData Pushed the fix. You were right, the dedup needed to be a merge, not a delete. Rewrote it as a post-extraction pass that transfers call records to the surviving Method node before removing the duplicate Function. Also added cp_interface_method_calls_after_dedup to guard that property. Both convergence_probe and extraction suites fully green locally. Thanks for the detailed breakdown, it made the fix path clear.

[DeusData]

Thanks for the fast turnaround — this round holds up, and I traced the whole path rather than taking the description at its word.

What actually fixes the edge is not the part you'd expect. The enum_body_declarations reach-through in extract_class_methods gives enum methods a proper class-qualified Method definition, and that is what re-anchors the records already sitting in those bodies. The call-record rewrite is, for Java specifically, a no-op: the unified walk pushes SCOPE_CLASS for interface_declaration/enum_declaration (lang_specs.c:320–323) and class-qualifies the method scope QN, while the class-qualify branch in extract_func_def is C++/CUDA-only — so no call record ever carried the spurious Function's QN in the first place. The rewrite is harmless defensive code that would cover a hypothetical language whose walk emits the Function-flavoured QN. Worth knowing so you don't rely on it as the mechanism.

There's a genuinely nice side effect you didn't claim. Eight other record kinds carry enclosing_func_qn — usages, throws, read/write, type refs, env accesses, type assigns, string refs, channels. For enum method bodies those were already dangling on main, because no definition with the class-qualified QN existed at all. Your Method emission makes them resolve. So this improves THROWS, WRITES and usage edges out of enum methods too, not just CALLS.

The new probe binds properly. fns == 0 fails without the pass, and calls >= 1 pins survival of the App.run → greet edge at store level — the exact class the earlier CI failure exposed. Survivor is the precise Method label with parent_class set and full metrics, which is the right side of our label-precision rule.

Two asks before merge:

1. The PR description is now stale. It still documents the round-1 push_class_body_children approach, which commit 11ee4905 reverted — cumulative deletions are zero. Please rewrite it to describe the dedup-pass design you actually shipped, so the merge commit doesn't reference code that isn't there.
2. One assertion proves less than its name suggests. In java_enum_dedup_preserves_calls_issue1234, ASSERT_TRUE(r->calls.count >= 1) passes whether or not the dedup runs — raw call records survive regardless. The binding assertions in that test are the Method-existence ones and Function == 0. Don't lean on the calls.count line as the merge-vs-delete guard; the D6d probe is the real one.

And one thing that is on me, not you. My round-1 wording — "the dedup must merge, not delete" — plausibly steered you away from your own commit 1, which was the root-cause fix: it stopped the spurious Function being minted at all. What ships now mints it for every interface and enum method in every Java file and then deletes it again. Your two commits actually compose: prevention plus the Method emission, with the dedup pass optionally kept as a safety net. There's a real counter-argument — interface_body/enum_body are shared node-type names across grammars (TypeScript, Apex, Dart), so prevention carries its own cross-language blast radius — which is exactly why I've put the choice to the maintainer rather than asking you to redo it again on my say-so.

So: please don't rework anything yet. Do the two small asks above, and I'll come back with the architecture answer. Nothing you shipped is defective — this is a question about which of two correct shapes we want to live with.

[harshitaajoshi]

@DeusData Done! Updated the PR description to document the dedup-pass design and dropped the non-binding calls.count assertion from the extraction test. The Method-existence and Function == 0 checks remain as the binding assertions, with the D6d probe covering the CALLS edge property end-to-end. Also changed the PR title to reflect the merge-dedup approach, happy to revert that if you prefer the original. Appreciate the thorough review and the callout on the enum method side effect, that was a good catch.

[DeusData]

Both asks done, thank you — and yes, keep the new title. It describes what the diff actually does, which is the only thing a title owes anyone.

Dropping the calls.count assertion was the right call rather than the tempting one. An assertion that passes either way is worse than no assertion at all, because it reads like coverage in a review and provides none.

Nothing further is needed from you right now. The remaining question is the architecture one I flagged — prevention at the source versus the post-hoc dedup pass — and that is genuinely the maintainer's to answer, not a defect in what you shipped. I have put it to him with both sides written out, including the argument for your current shape: interface_body and enum_body are shared node-type names across TypeScript, Apex and Dart, so restoring the prevention line carries a cross-language blast radius that the dedup pass avoids. That is a real point in its favour, not a consolation.

I will come back to you with the answer. If it goes the prevention way, the work is mostly un-reverting your own commit 1, so nothing you have done is wasted either way.