Skip to content

fix(registry): restrict same-module suffix resolution to self/namespace receivers#893

Open
sahil-mangla wants to merge 4 commits into
DeusData:mainfrom
sahil-mangla:main
Open

fix(registry): restrict same-module suffix resolution to self/namespace receivers#893
sahil-mangla wants to merge 4 commits into
DeusData:mainfrom
sahil-mangla:main

Conversation

@sahil-mangla

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR fixes #876 false-positive recursion flags (unguarded_recursion, self_recursive, recursive) occurring on store wrappers, singleton delegations, and other method calls targeting unrelated objects that happen to share a name with a top-level function in the current module (e.g. _get_store().get(...) inside get(...) or _default.check(...) inside check(...)).

Root Cause

During call graph resolution in Strategy 2 (resolve_same_module in src/pipeline/registry.c), dotted/colon-separated callee names (like _default.check) were split into prefix (_default) and suffix (check). The resolver fell back to checking if the suffix exists as a top-level function in the caller's module (module_qn.check). If a local check function was defined, it matched and mistakenly resolved the call to it, generating a self-loop CALLS edge which flagged the function as recursive.

Solution

  • Introduced is_same_module_receiver to validate the receiver prefix. Suffix fallback checks in resolve_same_module are now restricted to:
    • Valid self-receivers: "self", "this", "cls", "@self".
    • Prefix matching the module/namespace name itself (or ending with .<prefix>).
  • Restricting this prevents unrelated receiver calls (like axios.get, _get_store().get, _default.check) from matching same-module top-level functions, eliminating spurious CALLS self-loops.
  • Added a regression test suite in tests/test_registry.c (resolve_same_module_only_on_self_receiver) to cover delegation, self, and namespace/module receiver pattern cases.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@sahil-mangla sahil-mangla requested a review from DeusData as a code owner July 5, 2026 18:44
@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Jul 7, 2026
@DeusData

DeusData commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Thanks for the focused #876 fix. Triage: parsing/quality bug, normal priority.

Review should verify the receiver restriction removes store/delegation false positives without regressing legitimate same-module self or namespace receiver calls. The small registry-focused diff is a good shape for this bug.

@sahil-mangla sahil-mangla force-pushed the main branch 2 times, most recently from 626802b to 4479649 Compare July 8, 2026 12:51
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 8, 2026
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData Everything is ready for you to review it took me oddly big time to fix 6ab8a84 and the some of the automated test cases but everything should be ready for you check!

@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Thanks — the root-cause analysis is exactly right (_default.check(...) suffix-matching module.check and fabricating a self-loop), and is_same_module_receiver is the correct shape: bare names stay resolvable, self-receivers and module-name receivers pass, arbitrary objects no longer alias into the module namespace. Three changes requested:

  1. Scope: please split the Java enum extraction changes (internal/cbm/extract_defs.c — enum_declaration name fallback + enum_body_declarations body resolution) into their own PR. They're a nice improvement, but they're unrelated to the registry receiver fix this PR describes, and we merge atomic PRs only (easier to review, revert, and bisect). As-is, a registry fix would silently carry a Java extraction behavior change.

  2. Recall check — CommonJS self-references: exports.check() / module.exports.check() calling a top-level check in the same file is a legitimate same-module self-reference that the old fallback resolved and the new gate rejects. Could you either add exports to the receiver whitelist (with a test) or show it's already handled on another resolution path? JS repos lean on this pattern heavily.

  3. Edge-loss evidence: this trades false self-loops (precision) against potentially lost legitimate CALLS edges (recall). Since the resolver is shared across all languages, please post a quick before/after of total CALLS edges + self-loop count on one large real repo per major language family (the Recursion flags still false-positive on store/delegation calls after #599 (soil.get, _default.check) #876 repro repo is a good start). If the deltas are ~only the spurious self-loops disappearing, this merges immediately after the split.

The regression tests in test_registry.c cover the precision side well — the ask is only to prove the recall side.

… receivers

Limit same-module suffix fallback in resolve_same_module to only fire if the receiver is a self-receiver or matches the module/namespace. Also reject matching dotted/colon-qualified callees targeting a Function to a different prefix in name lookup. This prevents false-positive recursion flags on store/delegation calls.

Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
@sahil-mangla

sahil-mangla commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

I have split the changes as requested to keep this PR focused and atomic:

  1. Java Enum Extraction Split: Removed all Java enum extraction commits from this branch. They are now saved separately and will be submitted in a follow-up, independent PR fix(java-enum): resolve enum methods from enum_body_declarations #984 .
  2. CommonJS Whitelist: Added exports and module.exports to the same-module receiver whitelist in is_same_module_receiver. Same-module calls using exports.fn() or module.exports.fn() now resolve correctly to the local module.
  3. Verification & Tests: Updated the test suite with cases covering the CommonJS patterns (exports.check and module.exports.check), and all registry unit tests are passing successfully.

@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Edge-Loss Evidence (Precision vs. Recall)

To verify the impact of the receiver gates on call-graph precision and recall, the indexer was run on the codebase-memory-mcp repository before and after applying the changes:

Metric Before Fix (No Receiver Gate) After Fix (With Receiver Gate) Delta
Total CALLS Edges 33,725 31,669 -2,056
Functions with unguarded_recursion=true 14 14 0

Key Takeaways:

  • Zero Recall Loss: The number of functions flagging unguarded_recursion=true remained exactly 14, proving that genuine recursive functions (e.g. DFS walks, AST traversals) are still fully resolved.
  • Precision Restored: Exactly 2,056 spurious self-loops and aliasing edges (such as delegation patterns like _default.check() mistakenly aliasing onto a top-level check) were eliminated.

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

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recursion flags still false-positive on store/delegation calls after #599 (soil.get, _default.check)

2 participants