fix(pipeline): suppress weak short-name matches for Go selector calls - #1907
fix(pipeline): suppress weak short-name matches for Go selector calls#1907ilyabrykau-orca wants to merge 1 commit 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
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>
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. |
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)