Skip to content

fix(extract): give each Go init() its own QN - #1915

Open
ilyabrykau-orca wants to merge 3 commits into
DeusData:mainfrom
ilyabrykau-orca:fix/go-init-qn
Open

fix(extract): give each Go init() its own QN#1915
ilyabrykau-orca wants to merge 3 commits into
DeusData:mainfrom
ilyabrykau-orca:fix/go-init-qn

Conversation

@ilyabrykau-orca

Copy link
Copy Markdown

What does this PR do?

Fixes #1910. Stacked on #1913 (which stacks on #1907) — same extraction area and shared test scaffolding; review only the last commit.

Go allows any number of init() functions per package — even several in one file — and all of them run at start-up. On the flat QN (proj.pkg.init) they all collided and the graph upsert kept one node per package, silently dropping the rest (measured in #1910: 21 decoder-registration inits in one package → 1 node; 29 in the repo → 7 nodes).

The change, following the #495 Rust cfg-twin pattern (and the #1266 GoogleTest name-derivation precedent):

  1. internal/cbm/extract_defs.c — suffix the QN with the file basename and line: proj.pkg.init#a.go:L5. Calling init explicitly is illegal in Go, so nothing ever joins on the plain QN — no resolution path is affected.
  2. internal/cbm/extract_unified.c (compute_func_qn) — mirror the exact formula, so init-body calls keep their source attribution instead of degrading to the calls_find_source File-node fallback.

Tests (reproduce-first):

  • pipeline_go_multi_init_nodes_survive — two files, one package, one init() each: RED without the def-side change (ic == 1, expected 2), GREEN with it.
  • extract_go_multiple_init_disambiguated — two inits in ONE file get distinct suffixed QNs (the line component disambiguates within a file).

Full scripts/test.sh leg green (ASan+UBSan, "All tests passed"); git clang-format --diff clean on changed lines.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (scripts/test.sh — full leg, ASan+UBSan, "All tests passed")
  • Lint passes (git clang-format --diff clean on changed lines; clang-tidy/cppcheck via CI)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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.

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>
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>
Go allows any number of init() functions per package — even several in
one file — and all of them run at start-up. On the flat QN
(proj.pkg.init) they all collided and the graph upsert kept ONE node
per package, silently dropping the rest: measured on a real Go repo, an
event-source package registering each decoder in its own file's init()
collapsed 21 init functions into one node (29 in source, 7 in the
graph), erasing the whole registration pattern.

Disambiguate with the DeusData#495 cfg-twin pattern: fold the file basename
and line into the QN (proj.pkg.init#a.go:L5). Calling init explicitly
is illegal in Go, so nothing ever joins on the plain QN; the
call-scope side (compute_func_qn) mirrors the exact formula so
init-body calls keep their source attribution instead of degrading to
the calls_find_source File fallback.

Reproduce-first: pipeline_go_multi_init_nodes_survive is RED without
the def-side change (node count 1, expected 2) and GREEN with it;
extract_go_multiple_init_disambiguated pins two same-file inits to
distinct suffixed QNs.

Fixes DeusData#1910

Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
@ilyabrykau-orca

Copy link
Copy Markdown
Author

Rebased onto current main (stacked on the rebased #1913). No semantic changes; full test leg green. Post-fix init-node recovery (29/29) is in the #1906 census comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Go: multiple init() per package collapse onto one QN — bodies and registration edges silently dropped

1 participant