Skip to content

fix(registry): restrict same-module and unique-name suffix matches on receivers - #1128

Open
sahil-mangla wants to merge 3 commits into
DeusData:mainfrom
sahil-mangla:fix/registry-resolution
Open

fix(registry): restrict same-module and unique-name suffix matches on receivers#1128
sahil-mangla wants to merge 3 commits into
DeusData:mainfrom
sahil-mangla:fix/registry-resolution

Conversation

@sahil-mangla

Copy link
Copy Markdown
Contributor

Description

This PR tightens the call-resolution core in the registry to prevent false-positive CALLS edge resolutions when dealing with qualified receivers, improving precision across all supported languages.

Changes

  • Restricted resolve_same_module on receivers: The resolve_same_module strategy now validates the receiver prefix. It will correctly resolve self-receivers (self.get), exact module prefixes (proj.pkg.service.get), and last-segment namespace matches (service.get). It actively rejects resolving to local functions when the receiver is clearly unrelated (e.g., axios.get or _get_store().get), preventing spurious edges to same-named functions within the current module.
  • Restricted resolve_name_lookup for qualified callees: Added a qn_ends_with_qualified check. If a callee name is explicitly qualified (e.g., contains . or ::), the resolution candidate's qualified name must properly terminate with that exact qualification (unless the candidate is explicitly labeled as a Method), preventing cross-namespace leakage on generic method names.

Impact

  • Precision vs Recall: This change heavily favors precision, eliminating wide blast-radius false positives on common method names (like get, add, update) when invoked on unrelated imported or delegated receiver objects.
  • Testing: Added regression tests for resolve_same_module_only_on_self_receiver and verified the full test suite runs cleanly.

@sahil-mangla
sahil-mangla requested a review from DeusData as a code owner July 16, 2026 10:33
@DeusData DeusData added the bug Something isn't working label Jul 16, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 16, 2026
@DeusData DeusData added parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 16, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for taking on the receiver false-positive problem. The exact head is security-clean and DCO passes, but it is not correct enough to merge:

  1. Every full Unix/macOS/Windows test job fails deterministically at cp_enum_method_java: valid Java enum calls such as d.label() and d.isWeekend() become unresolved. This existing convergence guard must remain green.
  2. The blanket Method/type-like-parent exemption preserves the main false-positive class: an unrelated axios.get can still resolve through unique_name when the sole candidate is labeled Method. The new negative test uses a Function, so it does not exercise that common path.
  3. Add direct coverage for the unique and multi-candidate paths, imported receivers, . and :: qualification, and candidates labeled Method.
  4. Use the canonical cbm_label_is_type_like() policy rather than duplicating the label set in the registry.

The precision goal is valid, but the registry currently lacks enough receiver/language/type evidence for this global heuristic: it is simultaneously too strict for valid Java dispatch and too permissive for unrelated Method candidates. Please redesign the suppression around receiver-aware evidence, preserve the Java enum convergence test, and update the PR's “full suite clean” statement after the complete matrix is actually green.

@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch 2 times, most recently from 25457d2 to 7c78413 Compare July 16, 2026 16:59
@sahil-mangla
sahil-mangla marked this pull request as draft July 16, 2026 17:01
@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch from 7c78413 to 0b22112 Compare July 16, 2026 19:00
@sahil-mangla
sahil-mangla marked this pull request as ready for review July 16, 2026 19:03
@sahil-mangla
sahil-mangla marked this pull request as draft July 17, 2026 10:27
…ce receivers

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

Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch from ab4e7e5 to e6e5377 Compare July 19, 2026 06:08
@sahil-mangla
sahil-mangla marked this pull request as ready for review July 19, 2026 07:21
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData the registry fix it ready to be reviewed!

@DeusData

Copy link
Copy Markdown
Owner

Thanks for the update. I see current head e6e537759e176e8a23d06828ea2808b7cff16fc8 and a green matrix. The exact-head re-review will focus on the prior Java enum-dispatch regression and Method-labelled receiver false positives before we give further technical feedback. No additional contributor action is requested in the meantime.

@DeusData

Copy link
Copy Markdown
Owner

Thank you for this — the false-positive class you are killing is real, and your tests demonstrate it convincingly. axios.get resolving to an unrelated same-named method on main today is exactly the kind of wrong edge that erodes trust in the graph. I want to be specific about what I verified, because two things need your attention before a merge decision.

What holds up. The registry logic is sound and the new tests bind. Restricting suffix matching on receivers is a defensible precision improvement, and the thread-local _import_map_cache use is fine.

1. A regression the tests cannot see — Java enum constants.

The diff rewrites find_class_body to descend enum_body → enum_body_declarations. But extract_enum_members walks the direct children of whatever find_class_body() returns, looking for enum_constant nodes. In tree-sitter-java, enum constants are children of enum_body — they are siblings of enum_body_declarations, not inside it.

The consequence: for any Java enum that also declares methods, the constants (MON, TUE, …) stop being extracted as Variable nodes. They just disappear. Your java_enum_method test asserts only the Enum and its Methods, so CI stays green while the constants are lost.

Worth adding a test that asserts the constants and the methods on the same enum — that would have caught this and will stop it recurring.

2. Scope — there is a second feature in here.

The description covers the registry precision change. The diff also rewrites find_class_body, adds a Java identifier-fallback for class names, and extends push_class_body_children for enum_body/enum_body_declarations. That is Java enum extraction work, and I suspect it was added to make the receiver-awareness test's Java case pass.

We hold to atomic PRs, so I would like to see this split: the Java-enum extraction improvement is independently valuable and should land on its own merits (with the constants bug fixed), and the registry change should stand or fall on its own.

3. One question that is above my pay grade, so I have referred it to the maintainer.

This changes the precision/recall balance of the resolution core for every language at once, and I could not find evidence of what it costs. The self-receiver list is hardcoded {self, this, cls, @self} — that misses $this in PHP and Me in VB, and C++ ns::helper() calls will stop resolving where namespaces are not part of the qualified name. Killing false positives is good; losing true CALLS edges is a findability regression, and we care about that more.

We have a per-language benchmark and evaluation harness precisely for this kind of trade-off. The maintainer will decide whether this needs to go through it first. That is not a criticism of your work — it is a question about how much evidence a change to the resolution core needs, and it is his call rather than mine.

Points 1 and 2 are worth acting on regardless of how point 3 lands. Thanks again for digging into a genuinely hard part of the system.

@DeusData

Copy link
Copy Markdown
Owner

Follow-up with more detail, now that I have also reviewed your #893 and closed it in favour of this PR (nothing unique was lost — the comparison is in that thread).

Reviewing the two together produced a sharper picture of the recall risk I flagged earlier. This is not a request to change anything yet, since the direction call is the maintainer's — but the specifics are worth having on the record, because they are the evidence that decision will rest on.

Where the receiver heuristic loses true edges. The self-receiver list is {self, this, cls, @self}, matched against raw callee node text. Concretely:

  • PHP emits $this.helper with the sigil preserved, so "$this" is not in the list.
  • Perl convention is $self.
  • VB.NET is Me.
  • Rust associated calls are Self::new.

PHP and Rust have LSP cover, but the registry is their fallback path; VB-style languages have neither. This matters more than it might look, because cbm_registry_resolve splits at the first separator, so for roughly 150 non-LSP languages this heuristic is the primary resolver.

Two further recall paths I had not previously named:

  1. Multi-segment module-qualified callees. The prefix check uses only the first segment, so pkg.service.get inside module proj.pkg.service fails Strategy 2 ("pkg" ≠ module tail "service"), while the less qualified service.get passes — the more precise reference resolves worse than the vaguer one. Strategy 3 can rescue it via the qualified tail, but only below REG_MAX_CANDIDATES; at our scale the hot names (get, add, update) blow straight past that cap and come back empty. Strategy 2's module fallback was the scale-proof path for exactly those names.
  2. Alias receivers. The resolve_name_lookup enforcement drops true edges for Function candidates behind an alias: Elixir alias Foo.Bar, as: B; B.helper() yields a winner proj.foo.bar.helper that does not end in B.helper, so it resolves to nothing where it previously resolved correctly. The same shape hits Ruby constant aliases and CommonJS const u = require('./utils'); u.helper() whenever import extraction misses the alias. Elixir aliasing in particular is idiomatic essentially everywhere.

Your PR description is candid that the change "heavily favours precision", and that honesty is appreciated — it is also precisely why this needs a maintainer decision rather than a reviewer's. This project's stated priority runs the other way: a false edge is noise, but a lost true edge is a thing an agent can no longer find. Killing axios.get false positives is a real win, and I do not want to lose it; the question is what it costs across the other languages, and that is measurable with the per-language bench and eval harness rather than arguable.

Also worth noting independently of all of the above: there is no test anywhere in the suite that asserts Java enum constants. That is why the enum defect I raised earlier stays green in CI on this PR, on #893, and on #984. Worth adding regardless of which way the direction question lands.

Nothing for you to do on the direction question — that is ours to answer, and I will come back to you. The enum constants fix and the Java gating from my earlier comment are still the concrete asks.

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/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants