fix(extract): receiver-qualify Go method QNs - #1913
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. |
c70025f to
12d50c0
Compare
|
Rebased onto current main (stacked on the rebased #1907). No semantic changes; full test leg green. Post-fix field numbers for this change are in the #1906 census comment (method-node recovery incl. generic receivers: #1906 (comment)). |
|
Reviewed the last commit. The bug is real and badly worth fixing, the shape of the fix is right, and there is one measurement I want before it lands. The problem is as bad as you say. Receiver-qualifying is the right cure, matching the Go interface-member and C++ out-of-line paths already in The measurement I wantYou describe this as a side effect, in one line:
I think that line is doing more work than it looks. So You measured the false edges removed. I would like the other half measured too, and there is already an instrument for it — the per-language CALLS census with its dumped per-edge sets. The Go leg of that is the right baseline. Concretely, what I am asking for is:
If the answer is "the LSP type-dispatch path already covers them, so the demotions are few and the removals are many", that settles it and the PR merges on the strength of the numbers rather than on the argument. If a meaningful number of true edges demote to import-distance guessing, that is worth knowing before this is in The Two smaller thingsThis shifts a baseline other PRs are being judged against. There is an open cluster of call-resolution PRs that we have decided to judge against one shared census precisely because whichever lands first makes the rest unreadable on their own numbers. A change to Go method QNs moves that baseline for the Go leg. Not an objection — just something I would rather sequence deliberately than discover afterwards. Your reindex note is right and needs to travel further than the PR. A QN-shape change means existing graphs pick up the new shape only on rebuild, and that belongs in the release notes, not only in #1909. I will make sure it gets there. Status
Thanks for the measurements you did bring — "20 methods → 2 nodes, 19 structs linked to one shared node" is what turns this from a plausible tidy-up into an obvious defect. |
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>
12d50c0 to
a6b1a69
Compare
|
Rebased onto current main as part of the 1907→1913→1915→1936 stack; no source conflicts, test anchors re-anchored. Full scripts/test.sh green on the stack head. |
A Go method's QN was the flat package form (proj.pkg.method) — the receiver was ignored, so every same-name method in a package collided on one QN and the graph upsert kept exactly one node. Measured on a real Go repo: 20 Process/Name methods across 15 files kept 2 nodes; 9 different Task() methods fused into one chimera node carrying all nine bodies' call edges; 19 structs pointed DEFINES_METHOD at a single shared method node; a _test.go mock Close outranked the production Close in the dedupe tie-break. The upsert's own comment calls kind-disambiguated QNs 'the real cure'. Qualify the QN with the receiver type (proj.pkg.Recv.method), the same shape as Go interface members and the C++ out-of-line method path right below it in extract_func_def: - extract_defs.c: def.qualified_name = parent_class + name whenever the receiver type resolves; go_receiver_type_name becomes the shared cbm_go_receiver_type_name (exported via helpers.h) so both sides of the contract use one formula. - extract_unified.c (compute_func_qn): mirror branch for method_declaration, so method-body calls keep exact source attribution instead of degrading to File-node fallback (calls_find_source). - Consumers already agree: pxc_build_lsp_def passes the def QN and parent_class (receiver_type) verbatim into the Go LSP registries, and check_go_class_implements explicitly supports class-qualified method QNs (its path (b)). Side effect: resolve_same_module's exact module.name hash no longer matches concrete methods, which kills the conf-0.9 false edges where an interface-typed call bound to an unrelated same-package method. Fixes DeusData#1909 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
a6b1a69 to
dbba757
Compare
|
Here is the measurement — and it caught something real, so thank you for insisting on it. The census (orca-runtime-sensor, Go→Go CALLS, isolated one-shot indexes)
Per-edge migration of #1907's 929 What the first run of this census caughtMy first measurement showed Amended into this PR (it is the same one-formula claim): One upstream test adopted the new shape: On the baseline-shift point: agreed — these numbers are from the stack in its merge order (#1907 → #1913 → #1915 → #1936), so whichever leg lands first, the others' columns above stay the readable reference. Full scripts/test.sh green on the amended stack head; branch force-pushed with the rebase. |
What does this PR do?
Fixes #1909. Stacked on #1907 (shares the Go test scaffolding in
tests/test_extraction.c; the first commit here is #1907's — review only the last commit).Go concrete methods carried a flat package QN (
proj.pkg.method, receiver ignored), so every same-name method in a package collided on one QN and the graph upsert kept exactly one node — on the repo measured in #1909: 20Process/Namemethods → 2 nodes, 9Task()methods fused into one chimera node carrying all nine bodies' call edges, 19 structsDEFINES_METHOD-linked to a single shared node, and a_test.gomock outranking the production method in the dedupe tie-break. The upsert's own comment calls kind-disambiguated QNs "the real cure".The change — same shape as Go interface members and the C++ out-of-line method path directly below it in
extract_func_def:internal/cbm/extract_defs.c— when the receiver type resolves,def.qualified_name = parent_class + "." + name(proj.pkg.Storage.Close);go_receiver_type_namebecomes the sharedcbm_go_receiver_type_name(declared inhelpers.h, following thecbm_cpp_out_of_line_parent_classprecedent) so both sides of the contract use one formula.internal/cbm/extract_unified.c(compute_func_qn) — mirror branch for Gomethod_declaration, so method-body calls keep exact source attribution instead of degrading to thecalls_find_sourceFile-node fallback.pxc_build_lsp_defpasses the def QN andparent_class(→receiver_type) verbatim into the Go LSP registries, solsp_type_dispatch/lsp_embed_dispatch/ interface-satisfaction emissions follow the new QN automatically (alllrp_go_s*probes stay GREEN, untouched).check_go_class_implementsexplicitly supports class-qualified method QNs (its path (b) reconstructs<ClassQN>.<method>; its comment describes the flat QN as the anomaly).Side effect (also #1909):
resolve_same_module's exactmodule.namehash no longer matches concrete methods, which removes the confidence-0.9 false edges where an interface-typed call (pipeline.Process(event)) bound to an unrelated same-package method.Tests:
extract_go_method_receiver_qualified_qn— two same-name methods on different receivers get distinct, receiver-qualified QNs with matchingparent_class; free functions keep the flat QN.extract_go_no_filename_in_module_qnupdated: the method expectation becomesproj.myapp.db.Conn.Query(its actual contract — no filename segment in the QN — still asserted).scripts/test.shleg green (ASan+UBSan, "All tests passed"), including alllsp_resolution_probeGo scenarios and the fix(pipeline): suppress weak short-name matches for Go selector calls #1907 pipeline test, with zero probe changes.Note for maintainers: this changes extracted QNs for Go methods, so existing graphs need a reindex to pick up the new shape (flagged in #1909 per CONTRIBUTING's indexing-change rule).
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 --diffclean on changed lines; clang-tidy/cppcheck via CI)