Repository navigation
fix(pipeline): suppress weak short-name matches for Go selector calls - #1907
ilyabrykau-orca wants to merge 9 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. |
3f1a325 to
7f4f68c
Compare
|
Rebased onto current main (post-#1903). #1903 generalized the TS/JS guard into |
|
Retriggered CI with a no-op amend (identical tree, new SHA — I lack rerun rights on this repo). The two failures on the previous run look environmental, not change-related: |
7f4f68c to
f76971f
Compare
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
The READS/WRITES resolvers and the USAGE registry fallback hand bare reference text to the short-name registry, which contains Field nodes - so once Go struct fields exist (DeusData#1935), every Go local err := ... binds whichever struct field is named err, project-wide: 21308 USAGE and 5191 WRITES onto Go fields on the measured repo, top target a test struct's field T collecting 3013 edges. In Go that binding is impossible by construction: a field is only reachable through a selector expression (x.f), and selector references resolve on the LSP path - every Field-targeted reference edge in the census carried dot-less text. Add cbm_go_suppress_bare_field_ref() next to the DeusData#1928 predicate and consult it at the same four sites (both READS/WRITES resolvers, both USAGE registry fallbacks): drop the bind when the file is Go, the target label is Field, and the reference text has no '.'. Go-gated because C#/Java/C++/Python method bodies legitimately reference their own members bare (cp_reads_writes_cs_static_field pins that shape). Field-validated on the DeusData#1940 stack: USAGE onto Go fields 21308 -> 0, WRITES 5191 -> 0; the only remaining field-targeted edges are 2466 CALLS, which are DeusData#1906/DeusData#1907's selector-guard territory. Reproduce- first pipeline probes (sequential + parallel twins) were RED on the stack without this commit. Fixes DeusData#1942 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
|
Reviewed. This is the right shape, and the care in it is above the bar for this class of change. One interaction with your own #1913 needs measuring before either lands. The guard itself
Two decisions in it are better than the guard you mirrored: Keeping Dropping Documenting the residual — a file whose import table maps zero project packages has no penalty signal — instead of implying the class is closed, is the right way to leave it. The measurement I want, and it is about this PR plus #1913These two are in the same stack and both narrow Go call resolution, and I do not think either PR's numbers capture what they do together. Walk one call through both:
So a call that was a confident edge before the stack becomes no edge at all after it — and neither PR sees that on its own. #1913 measures the false edges it removes; this one measures the weak edges it removes. Neither measures the calls that arrive at step 4 only because of step 2. I have asked #1913 for a Go CALLS census before/after with a strategy histogram. What I want here is the same measurement taken on the stack as a whole, not slice by slice: Go edge count and strategy histogram on The per-language census with its dumped per-edge sets is the instrument, and the Go leg is the baseline. None of this is a doubt about the direction. Status
Cleared. Approved on shape, pending the stack-level numbers. |
DeusData
added
bug
f76971f to
651ae3c
Compare
|
Rebased onto current main. Only conflict was test insertion-anchor drift in tests/test_pipeline.c (both merges added tests at the same anchor); every source file auto-merged. Full scripts/test.sh (ASan+UBSan) is green on the rebased stack head (#1936, which contains this commit). |
|
Stack-level census is up — full table and per-edge migration matrix posted on #1913 (they share one instrument run, so the numbers live in one place). The short version for the interaction you asked about: The step-2→step-4 cascade is small. Across the #1913 leg, The compounding loss the numbers did surface was elsewhere: a third QN site (go_lsp's enclosing-function QN) that #1913 had not moved, which cost method-body callers their LSP caller join (568 Whole-stack net: 21018 → 13815 Go CALLS, of which −6403 is this PR's weak-edge removal and −1019 is #1936's substring-hint removal; receiver-blind strategy share drops 43% → 10.5%. Stack rebased and force-pushed, full scripts/test.sh green on the head. |
|
Apologies — I assessed this some time ago and never posted the assessment. There is no comment and no review on this PR, so as far as you could tell it had been ignored. That is my error, and here is the review it should have had. Approved on shapeThis is the correct shape for a receiver-precision fix in this project, and it matches every constraint we hold guards to:
Keeping And the Your measurement answers what I was going to ask for: these strategies accounted for 36% of all CALLS edges on the measured repo, and a 14-line stdlib-only helper had 3 out of 3 false outbound edges. That is a census, not an anecdote. Your two reds are ours
Those two setup failures appear in every instance of this red, on three different sections across unrelated changes, which points at one shared setup fault rather than a daemon regression your diff could have caused. Nothing to change. I will merge once that guard is settled or the run comes back clean — and again, sorry for the silence. |
A Go selector call x.foo() whose receiver the Go LSP cannot type falls through to the generic registry resolver, which binds it by bare short name to an arbitrary same-named project symbol. Stdlib calls are the worst case: f.Close() on an *os.File gets a CALLS edge to whatever project Close wins candidate ranking (measured on a real Go repo: confidence 0.11, 15 candidates; suffix_match + unique_name were 36% of all CALLS edges, and one 14-line stdlib-only function got 3 out of 3 false outbound edges). Extend the TS/JS receiver-aware guard (DeusData#592/DeusData#606) to Go: - extract_calls.c: flag Go call_expression with a selector_expression callee as is_method, mirroring the TS/JS member_expression flag. - registry.c: add cbm_go_suppress_weak_method_match. Unlike the TS/JS drop-list, field_type_hint is KEPT (Go struct fields carry declared types, so the hint is receiver-aware — lrp_go_s8_field_type_hint), and unique_name is dropped only when its confidence carries the import-unreachability penalty (the stdlib-hijack shape); an unpenalized lone candidate inside the caller's import closure never enters the field-type-hint upgrade and must survive. - pass_calls.c / pass_parallel.c: feed the Go gate next to the TS/JS one; the drop still defers to the emit path so service/route/HTTP edges stay main-identical. Reproduce-first: pipeline_go_receiver_suppresses_weak_method_edge is RED without the extractor flag (the f.Close -> project Close edge exists) and GREEN with it; typed same-package calls, bare local calls and import-qualified cross-package calls still resolve. The old extraction contract test used Go as the flag-exempt language — Python takes that role, and extract_go_selector_call_flags_is_method pins the new behavior. Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
651ae3c to
50ee111
Compare
|
Rebased onto current main (8972ea6, post-memory-core) per the decision on #1913. The only source conflict was additive: main gained the Python local-binding guard ( |
|
Heads-up: I cancelled this PR's CI run on purpose a minute ago. It was sharing the hosted-runner pool with tonight's v0.11.0 release build, and the release needed the slots. Nothing about the PR is wrong. I will re-run the full CI as soon as the release is out, so any red you see right now is the cancellation, not your change. Thanks for your patience, and for the steady stream of Go fixes. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @ilyabrykau-orca, and thank you for holding this stack steady through the long wait. As promised, we've merged current main into this branch from our side. The only conflict was tests/test_pipeline.c, and both sets of tests are kept. If you have local work, please pull before your next push.
Here is the review, on the merge result:
- Tests: pipeline, extraction, go_lsp, lsp_resolution_probe, registry and parallel all pass (960), as do the 49 other suites with Go fixtures (3,199).
lint-ciand the memory-core ratchet are clean. - Gating: strictly Go (
lang == CBM_LANG_GOat both sites). LSP-resolved calls return before the guard, so typed receivers still bind. - Real code: on kubernetes
staging/src/k8s.io/apiserver(1,143 Go files), Go→Go CALLS drop from 24,482 to 16,377 with zero edges gained. The removals I hand-checked were all false:klog.V(4).Infofbound to a projectklogWrapper.Infof, a func-typed field, and an interface call bound to one concrete type.
One ask before we merge:
The Go gate in src/pipeline/pass_parallel.c isn't bound by any test. The pipeline test uses 4 files, so it runs the sequential path, and neutering the parallel-path hunk leaves every suite green. Real repositories take the parallel path. Could you add a parallel twin padded past MIN_FILES_FOR_PARALLEL, in the shape of pipeline_python_receiver_parallel_suppresses_weak_method_edges in tests/test_pipeline.c?
Two small nits, optional:
- The block comment at
tests/test_extraction.caround line 5696 still says a Go method call never setsis_method. registry.cre-types part ofweak_short_name_strategy's drop-list. Building on the shared list would keepweak_call_guards_share_one_drop_listmeaningful for Go too.
Thanks again. This removes a whole class of false edges, and the stack is close.
Brings the branch up to date with main (989 commits since the 2026-09-28 merge). Four conflicts, all additive on both sides: - pipeline.h / registry.c: main added cbm_suppress_weak_import_bound_call (DeusData#2127) at the same anchor as cbm_go_suppress_weak_method_match; both kept. - pass_calls.c / pass_parallel.c: drop_plain_call now chains the Go selector guard and main's Python import-binding guard, in the same order in both resolvers. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Review follow-up for DeusData#1907 (asks from the 2026-09-28 review): - pipeline_go_receiver_parallel_suppresses_weak_method_edge: parallel twin of pipeline_go_receiver_suppresses_weak_method_edge. The guard is wired into pass_parallel.c as well as pass_calls.c, but the four-file fixture only runs the sequential resolver, so the parallel hunk was bound by no test. The fixture is shared (write_go_receiver_fixture) and padded with 52 Go files past MIN_FILES_FOR_PARALLEL, with CBM_WORKERS=4, in the shape of the Python parallel twin. - registry.c: cbm_go_suppress_weak_method_match builds on the shared weak_short_name_strategy list and narrows it in two places only (field_type_hint kept, unique_name only when penalized). Behaviour is unchanged; weak_call_guards_share_one_drop_list now binds the Go guard to the shared list too. - test_extraction.c: the flag-exempt comment names the current flag set (Perl, Python, TS/JS/TSX and Go) and Rust as the exempt language. Refs DeusData#1906 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The parallel resolver re-picks a multi-candidate registry match through try_field_type_hint. That helper capitalizes the receiver VARIABLE name and accepts any candidate whose QN contains it as a substring: `t.Run` on a *testing.T became `T` and bound a project `TBench.Run`. The sequential resolver has no such step. The Go selector guard treated field_type_hint as receiver-aware and kept it, so the same weak pick it drops in sequential mode survived in parallel mode under a new name. On caddy + prometheus, parallel and sequential CALLS diverged by 1,684 (this branch) and 5,560 (with receiver-qualified method QNs, which put type names into more QNs). At base the gap is 84. SCIP precision of Go field_type_hint is 0.016 at base. Go no longer takes the hint. The guard keeps field_type_hint on the shared weak list, and the fixture of both twins now pins the `t.Run` case. A typed field call (lrp_go_s8, `s.repo.Find()`) resolves through the Go LSP and is unaffected. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @ilyabrykau-orca, for this guard and for keeping the stack steady through a long wait. To save you another round trip, we finished the review asks on your branch ourselves. Your commit is unchanged; ours sit on top, signed off:
How we measured the stackEach of these was indexed on 13 Go repos with the parallel resolver, and on caddy + prometheus with the sequential one as well:
Every repo was indexed twice per binary, and both runs were identical for every edge type. Precision and recall come from scip-go 0.2.7 as an independent oracle. Precision is counted only on the 6 repos where scip-go also records external references (on the other 7 it records none, so a call bound to the wrong project function would look correct there). Recall covers all 13.
Per repo,
The honest part is the recall trade. Overall recall goes from 0.925 to 0.873. The largest drops are vault (0.906 -> 0.792, −11.4 points) and terraform (−7.4 points). Most of what the guard removes was wrong, but not all of it. The true calls it loses are method calls on values whose receiver type the Go LSP does not infer: field chains ( One caveat on reading those numbers: #1913 changes what the oracle can match. Receiver-qualified QNs let scip-go map 20,912 more caller/callee pairs (237,806 vs 216,894), so recall only compares within one QN scheme. Winning those calls back is on us, not on you. Receiver typing in the Go LSP (field chains, method-return chains, One more change: the parallel resolver's field-type hint for GoRunning both resolvers side by side showed that the stack made them disagree. Here is CALLS from the parallel / sequential resolver:
The cause was ours, not yours. In the review asks we kept Receiver-qualified QNs (#1913) put type names into many more QNs, which made the hint fire far more often. The typed field call in The fix (
Both pipeline twins now assert that
No Go Thank you again. Your guard removes a whole class of false edges: precision on the oracle-checked repos goes from 0.632 to 0.900 with #1907 alone. Your census instrument on #1913 is what made the stack judgeable. |
…hod-receiver-qn Brings the branch up to date with the updated DeusData#1907 head, which carries main 9f85f10 and the DeusData#1907 review follow-up. One conflict in tests/test_pipeline.c, at the shared anchor: keep this branch's pipeline_go_method_caller_keeps_lsp_join and the shared write_go_receiver_fixture plus its sequential and parallel probes. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com> # Conflicts: # tests/test_registry.c
Brings in main 658639e (DeusData#2685, Rust generic-impl call scope). One conflict in tests/test_extraction.c: main's new F-R4 test and its helper sit right before the flag-exempt test, whose comment this branch rewrote (Go now sets is_method, so the guarded language is Rust). Both are kept: main's block, then this branch's comment. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Merged current |
This branch has not been deployed
2 participants
What does this PR do?
Fixes #1906.
Extends the TS/JS receiver-aware weak-match guard (#592/#606) to Go: a selector call
x.foo()whose receiver the Go LSP cannot type must not be bound by a receiver-blind short-name strategy to an arbitrary same-named project symbol (f.Close()on an*os.File→ a projectClose; on the repo measured in #1906 these strategies were 36% of all CALLS edges, and one 14-line stdlib-only helper had 3/3 false outbound edges).Mirrors how TS/JS joined the Perl guard (#476):
internal/cbm/extract_calls.c— flag Gocall_expressionwith aselector_expressioncallee asis_method, same block as the TS/JSmember_expressionflag (Go has nothis/superanalog to exempt).src/pipeline/registry.c+pipeline.h— newcbm_go_suppress_weak_method_match(is_go, is_method, strategy, confidence). Dropssuffix_match/fuzzyalways; dropsunique_nameonly when its confidence carries the import-unreachability penalty (<CONF_UNIQUE_NAME) — the stdlib-hijack shape.field_type_hintis deliberately kept: Go struct fields carry declared types, so the hint is receiver-aware there (lrp_go_s8_field_type_hintstays GREEN).src/pipeline/pass_calls.c/pass_parallel.c— feed the Go gate next to the TS/JS one. The drop defers to the emit path exactly like TS/JS, so service/route/HTTP/CONFIG edges stay main-identical.same_module,import_map,qualified_suffix,callee_suffixand alllsp_*strategies are untouched: typed receivers, same-package calls, and import-qualified calls resolve exactly as before.pipeline_go_receiver_suppresses_weak_method_edge— RED without the extractor flag (FAIL: ASSERT(!(cross_file_call_exists(s, project, "FileLen", "Close")))), GREEN with it. Asserts the stdlibf.Close()edge is gone while the typed same-packages.Close(), a bare local call, and an import-qualified cross-package call keep resolving. The fixture carries ago.mod— import reachability (theunique_namepenalty) depends on it, like every real Go repo.extract_go_selector_call_flags_is_methodpins the extractor flag; the old flag-exempt contract test keeps its guard using Python as the exempt language.go_suppress_drops_weak_selector_matches/go_suppress_keeps_typed_and_import_aware_matchesunit-pin the drop-list, the 0.75-vs-0.375unique_namesplit, and thelsp_*keeps.Perf/memory: one
ts_node_child_by_field_name+ twostrcmpper Go call site at extraction, one short-circuited predicate per resolved Go call, zero allocations, no new passes.Known residual (documented in #1906, out of scope here): a Go file whose import table maps zero project packages has no penalty signal, so a lone same-named project symbol can still capture a stdlib method call there;
same_modulecan still mis-bind interface-typed calls inside one package. Both need receiver typing, not name heuristics.Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
scripts/test.sh— full leg, ASan+UBSan, "All tests passed")git clang-format --diff HEADclean on changed lines; clang-tidy/cppcheck via CI)