fix(registry): drop unique_name CALLS across language boundaries - #1702
rudi193-cmd wants to merge 4 commits into
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
CI red on this PR is not the #1572 change. Every failing shard dies on the same two tests, both predating this branch:
Windows shard 2/2 also has an unrelated UTF-8 compare (
|
|
Thank you for keeping this to the remaining |
|
Picking this up, and doing the review that was promised on 18 August rather than leaving you waiting further. Sorry it took two weeks. Your CI diagnosis was right, and there is now an extra fact you could not have had. Both of those tests — One caveat before you rebase: The reviewThe two things the 18 August note said would be verified before any merge decision — same-language-family behaviour, and the negative cross-language cases — both hold. The production change is two lines:
That third bullet is the one I care most about, and you tested it. Every existing guard in this file deliberately preserves What actually holds it upNot this PR's quality. It belongs to the call-resolution cluster — #1128, #1324, #1386, this one, #1766 — and there is a standing decision that they are judged against one shared census rather than one at a time. #1128 is the most global member, since it changes strategies inside So this will not merge in isolation, and that is a sequencing constraint rather than a verdict on the change. I would rather tell you that plainly than let it sit silently for another fortnight. Thank you for holding the scope where you did — excluding the import-edge cleanup, the receiver disambiguation and the external-specifier question was the right call, and it is why this one was quick to read. |
|
Census done — this PR and #1766 were measured together on real graphs (ten languages, ~3.1M CALLS edges, every removed edge classified, three runs each; the two guards compose exactly: removed(both) = removed(#1766) ∪ removed(#1702) on nine corpora with zero additions, so the CALLS sets are deterministic). Widening the guard to
Fix: reuse |
492e59a to
9e92e67
Compare
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @rudi193-cmd, for a careful follow-through. All three family rules from the 5 September census are in, exactly as asked:
c_cpp_familynow protects C → header calls;jvm_familycovers java / kt / kts / groovy / gradle (.gradleand.ktsland there through the extension table);- vue / svelte / astro joined the JS/TS family.
Each comes with a test, and the branch merges cleanly with current main. Thank you for sticking with this through a long wait.
One thing has to change before we can merge, and I'd rather explain why than just ask. Commits 5770ed83, 8165c340 and f5ec03f0 go past what the census measured. cbm_suppress_cross_language_calls_edge drops every cross-family resolution except same_module and lsp_builtin*, including import_map and in-repo lsp_*. Those are the import- and type-aware strategies that the guard's own contract (registry.c:567-574) promises to keep. We only land suppressors that shed high-confidence edges across all languages per language and with a measured census, and we have no numbers for this one. Several true cross-language cases also fall outside the families:
- CUDA calling functions declared in
.h(it goes through the same C/C++ LSP but isn't inc_cpp_family); - Objective-C
.mcalling C functions in headers; - Scala calling Java;
- inline HTML
<script>calling.js.
As far as we can tell, the edges those commits were chasing in the #1572 fixture come from the Python LSP and import_map resolving the external unittest.mock.patch to the in-repo patch. That is the external-specifier problem you rightly scoped out in the PR description, and it deserves its own targeted fix rather than a cross-language veto.
Could you split the branch?
- This PR keeps
508e8bdc+0b2bddb8: the measuredunique_namewidening plus the three families. - A follow-up PR takes the LSP /
import_mapcommits, together with thepipeline_..._issue1572fixture test that needs them. It would target the external-specifier root cause, and we'll measure it on the same corpora.
Two small things while you're in there:
tests/test_registry.c:1011and:1013are past 100 columns, somake -f Makefile.cbm lint-ciwill flag them;- the merge commit carries no
Signed-off-by, so a plain rebase ontomainavoids any DCO question.
After the split we'll re-run the census on the narrowed PR, and it merges on green. Thanks again. The family work is exactly what makes the unique_name fix safe to ship.
|
One addition to the review above, @rudi193-cmd, now that we've decided it: please also widen the families in the narrowed PR, so the
A test per family, in the same style as your C, JVM and Vue ones, would pin them. We'll include these in the census re-run after the split. Thank you! |
5557f0e to
8ffd03e
Compare
|
Per review feedback, this branch is now rebased onto
Removed from this PR: LSP / Follow-up work is on |
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @rudi193-cmd. This is a really clean split. The branch now carries exactly the measured unique_name widening plus the C/C++, JVM and Vue/Svelte/Astro families. It's freshly rebased with every commit signed off, and the over-long test lines are fixed. Stacking the LSP / import_map work on fix/1572-external-specifier-lsp rather than folding it in is exactly the right shape. Please keep that as a separate draft for now; we'll measure it on its own.
We'd like to land this one first, with your name on it. Three things before the census re-run, each with the why:
-
The three families from the 26 Sep addendum aren't in the code yet.
c_cpp_familyis still C/C++ only, andjs_ts_familyhas no HTML. Withunique_namein the guard, each of these would be dropped as cross-language:- a
.cufile calling a function declared in a.h; - an Objective-C
.mcalling a C header function; - an inline HTML
<script>calling a.jsfunction.
They're all true calls. CUDA and ObjC go through the same C/C++ front end, and inline scripts are JS; the weak-member gate in
pass_calls.calready treats HTML that way. Could you addCBM_LANG_CUDAandCBM_LANG_OBJCtoc_cpp_family, andCBM_LANG_HTMLtojs_ts_family, each with an assert in the style of your C, Groovy and Vue lines? - a
-
.mtargets are classified as MATLAB. The guard maps the target language from the filename (cbm_language_for_filename), and in the extension table.mis MATLAB. Objective-C is only detected from file content, during discovery (discover.c). So even with OBJC in the C family, an ObjC→ObjC call between two.mfiles reads as OBJC→MATLAB and would be dropped. The same applies to the other extensions whose language is decided by content:.cls,.inc,.cfcand.frm. The smallest safe fix is for the guard to return false (keep the edge) when the target's extension is one of those. A test with an ObjC caller and a.mtarget would pin it. -
pipeline_cross_language_unique_name_does_not_share_calls_issue1572is still intests/test_pipeline.c(and in the run list). Your note says it moved to the follow-up, and we think it belongs there. Thepatchedges in that fixture appear to come from the Python LSP /import_mapresolvingunittest.mock.patch, which this PR deliberately doesn't touch, so it would likely go red here. Could you move it to the stacked branch?
Once those are in, we'll re-run the census on the narrowed PR: the original corpora plus CUDA, Objective-C and HTML-inline-script repos. If it stays a pure win, this merges on green.
A heads-up so nothing surprises you later: we have a broader change of our own in the same area, not yet opened. It applies the same veto to the other name-only strategies, and it uses each file's detected language rather than its extension. We'll rebase it onto yours after this lands, so your work goes in first and ours builds on it.
Thanks again for your patience and care on this one!
Python `from unittest.mock import patch` to a unique TSX `patch`. Same language-family guard; JS/TS/TSX stay one family. Fixes DeusData#1572 Signed-off-by: rudi193-cmd <rudi193@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: rudi193-cmd <rudi193@gmail.com>
Widening unique_name to all callers made the helper dead code; -Werror unused-function broke CI on the production build. Signed-off-by: rudi193-cmd <rudi193@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: rudi193-cmd <rudi193@gmail.com>
Signed-off-by: rudi193-cmd <rudi193@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add CUDA and Objective-C to c_cpp_family, HTML to js_ts_family, and skip suppression when the target extension is content-disambiguated (.m, .cls, .inc, .cfc, .frm). Move pipeline DeusData#1572 integration test to the external-specifier follow-up branch. Signed-off-by: rudi193-cmd <rudi193@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the 27 Sep review on
Ready for your census re-run when CI settles. |
8ffd03e to
b46c6d4
Compare
|
Thank you, @rudi193-cmd, for the September 27 follow-up and for working through the split and family cases we requested. We need more time to review the updated branch and complete the promised census. That next validation step is on the maintainer side; please do not spend time on routine rebases solely to keep this waiting PR fresh. We will follow up here when we have a concrete result or need a specific change. Thanks again for your patience. |
What does this PR do?
Follow-on to #1647 / #725. That PR dropped
suffix_matchCALLS when caller language ≠ target file language and leftunique_name(candidates == 1) for this issue.Python
from unittest.mock import patchwas binding to a unique TSXfunction patchviaunique_name. The same helper now dropsunique_nameunder the same rules: JS/TS/TSX stay one family;same_module/import_map/lsp_*stay.Not in this PR: treating
unittest.mockas an external specifier, IMPORTS-edge cleanup, #1555 receiver disambiguation, or #1128.Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
scripts/test.sh --suites registry,pipeline)make -f Makefile.cbm lint-ci)Fixes #1572
Made with Cursor