fix(extract): drop cgo's import "C" pseudo-package - #1931
ilyabrykau-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. |
A Go import path names a package - never a function, method, field or another language's build target. Strategy 1 resolves every correct Go import (module path -> the package's Folder node); when it misses the import is external and the correct result is NO edge. Two name-guess fallbacks violated that: - Strategy 3's language-agnostic symbol-name fallback matched the path's last segment against any project definition of the same simple name and returned the lexicographically smallest survivor: import "os/exec" bound a test harness's exec() method (52 edges on the measured repo), two imports bound a Function extracted from a Makefile. 89 of 2295 Go IMPORTS edges (3.9%) were false; 27 of those are the import "C" case (DeusData#1926/DeusData#1931), the remaining 62 this. - Strategy 1b's sibling-file resolution admits symbol labels through import_targetable_label, re-creating the same bug one directory closer: with Strategy 3 gated, the field census still bound one os/exec import to a same-package exec() method. Gate both on the importing file's language through a new pure predicate, cbm_import_symbol_fallback_allowed(): false for Go, true for everything else - member-importing languages (Python, Java, Rust use crate::ops::helper) legitimately need the symbol fallback, and build/markup grammars (SCSS partials, Meson subdir, Pony use) the sibling one. Strategies 1, 2 and 4 are untouched. Reproduce-first test ei_go_import_never_binds_symbol asserts the IMPORTS relation EXACTLY (an internal package import stays; neither the cross-package nor the same-package decoy may be bound): RED count=2 expected==1 before each gate, GREEN after. Adds ei_edge_count_is, the exact-count sibling of ei_edge_present, since a floor cannot catch a fabricated extra edge. Field-validated on a real ~1150-file Go+C repo: Go IMPORTS by target label went Folder 2206 / Method 85 / Function 4 on main to Folder 2206 / Method 0 / Function 0 with the fix; all 2206 correct package-Folder edges survive. Fixes DeusData#1934 Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
|
Heads-up: this went red-to-merge because #1937 ( Checked with Re-anchor your test and its |
`import "C"` is not an import: "C" is reserved by go/build and no package with that path can exist — it only tells the toolchain to compile the preceding comment as C. cbm emitted it as an ordinary import, so the import resolver fell through to its symbol-name fallback (cbm_pipeline_resolve_import_node Strategy 3, pass_pkgmap.c), matched the literal name "C" against every project node called C, and picked the lexicographically smallest one. Measured on a real Go+C repo: all 27 files carrying `import "C"` pointed their IMPORTS edge at the same unrelated `C` member of a test helper — 27 false edges, and 27 files whose import map named a package that does not exist. Skip the spec in parse_go_import_spec. Node counts are unchanged and the file's real imports are untouched; only the false edge disappears (a 3-file fixture goes 22 edges -> 21, 14 nodes -> 14; the real repo goes 27 -> 0 such edges). Reproduce-first: go_cgo_pseudo_import_dropped is RED without the extractor change (the "C" import is still in imports[]) and GREEN with it, while still asserting the file's real `fmt` import survives. Signed-off-by: Ilya Brykau <ilya.brykau@orca.security>
8336851 to
5cb90f6
Compare
|
Rebased onto current main — as your merge-tree note predicted, the only conflict was test insertion-anchor drift (tests/test_extraction.c), re-anchored. Full scripts/test.sh on the rebased head: 7782 passed, 1 failed — the failure is test_daemon_runtime's marker_published assert, a load-timing flake from running three suites concurrently on this machine; it passes solo (47/47) and is unreachable from this diff (extract_imports only). |
|
This is green and clean, and I was about to merge it — then checked the mechanism and stopped. I think the headline symptom may already be fixed, and I would rather ask than land it on a premise that has moved. The questionYour description routes the bug through Strategy 3:
But bool cbm_import_symbol_fallback_allowed(CBMLanguage lang) {
return lang != CBM_LANG_GO;
}introduced by And If that reading is right, the 27 false Was the 27-file measurement taken before Why I still think it has valueEven with the fallback disabled,
That effect is untouched by The guard itself is right regardless. What I needPlease confirm whether the 27-edge measurement predates Not blocking on anything else: 34 checks green, |
|
Confirmed: the 27-edge measurement predates Re-measured today on current
So the false edges are already gone on main, exactly as the gate predicts, and this PR removes none. I have rewritten the description to lead with the import-map effect: the extractor still pushes The RED/GREEN evidence is unaffected — the reproduce-first test asserts at extraction level ( |
Fixes #1926
Problem
import "C"is not an import."C"is reserved bygo/build; no package with thatimport path can exist. The clause only tells the Go toolchain to compile the
immediately preceding comment block as C.
The Go import parser treats it as an ordinary import spec, so every cgo file's
import list — and therefore the per-file import map that call resolution
consults — carries a package that cannot exist. Dropping it at extraction is the
fix: Go-specific knowledge belongs in the Go import parser rather than in the
language-agnostic resolver.
History: the false-edge symptom this PR originally led with
When #1926 was filed (2026-08-30), the pseudo-import also produced false
IMPORTSedges: it fell through Strategies 1 and 2 and landed in the Strategy 3 symbol-name
fallback (
src/pipeline/pass_pkgmap.c), which bound all 27 cgo files of themeasured Go+C repository to the same unrelated
Cmember of a test helper.That measurement predates
7d76b88f("fix(pipeline): never bind a Go import toa symbol", merged 2026-08-31 via #1938), which gates Strategy 3 off for Go entirely.
Re-measured on current
main(8972ea69) against the same repository — which nowcarries 54
import "C"files:IMPORTSedges with targetC(by node name orby
local_name/moduleproperty): 0. The GoIMPORTStarget histogram is100%
Folder(2,303 edges). The false edges are gone exactly as the Strategy-3gating predicts, so this PR no longer removes any edge; its effect is confined to
the extraction layer.
Change
internal/cbm/extract_imports.c,parse_go_import_spec()— skip the spec when theimport path is exactly
"C", before the import is pushed. The guard cannot lose areal import:
go/buildreserves the path, so no resolvable package can ever bespelled
"C".Effect on current main
name a package that cannot exist — so import-closure reasoning (e.g. the
penalty logic in
unique_nameresolution) never consults a phantom package.IMPORTScount with targetCis 0 both before and after this change on current main; the pre-fix(pipeline): never bind a Go import to a symbol #1938 history
above is what the original description measured.
Test
tests/test_extraction.c—go_cgo_pseudo_import_dropped.Reproduce-first (extraction-level, so it stays RED-capable regardless of the
resolver gating): RED without the extractor change —
GREEN with it —
319 passed.The test also asserts the same file's real
fmtimport survives, so the fix cannotregress into dropping the whole import block.
Checks
scripts/test.sh --suites extraction— 319 passed, 0 failed.clang-formatclean on both edited hunks.scripts/lint.shcould not complete locally:clang-tidyandcppcheckare notinstalled on this machine. Relying on CI for those two legs.