fix(registry): restrict same-module and unique-name suffix matches on receivers - #1128
fix(registry): restrict same-module and unique-name suffix matches on receivers#1128sahil-mangla wants to merge 3 commits into
Conversation
|
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:
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. |
25457d2 to
7c78413
Compare
7c78413 to
0b22112
Compare
…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>
ab4e7e5 to
e6e5377
Compare
|
@DeusData the registry fix it ready to be reviewed! |
|
Thanks for the update. I see current head |
|
Thank you for this — the false-positive class you are killing is real, and your tests demonstrate it convincingly. 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 1. A regression the tests cannot see — Java enum constants. The diff rewrites The consequence: for any Java enum that also declares methods, the constants ( 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 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 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. |
|
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
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 Two further recall paths I had not previously named:
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 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. |
Description
This PR tightens the call-resolution core in the registry to prevent false-positive
CALLSedge resolutions when dealing with qualified receivers, improving precision across all supported languages.Changes
resolve_same_moduleon receivers: Theresolve_same_modulestrategy 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.getor_get_store().get), preventing spurious edges to same-named functions within the current module.resolve_name_lookupfor qualified callees: Added aqn_ends_with_qualifiedcheck. 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
get,add,update) when invoked on unrelated imported or delegated receiver objects.resolve_same_module_only_on_self_receiverand verified the full test suite runs cleanly.