From 7d76b88f1e206ac18fc61e82900bcd3f1e313e65 Mon Sep 17 00:00:00 2001 From: Ilya Brykau Date: Sun, 30 Aug 2026 14:15:09 +0200 Subject: [PATCH] fix(pipeline): never bind a Go import to a symbol 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 (#1926/#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 #1934 Signed-off-by: Ilya Brykau --- src/pipeline/pass_pkgmap.c | 38 +++++++++++++++++++++++++--- src/pipeline/pipeline_internal.h | 8 ++++++ tests/test_edge_imports.c | 43 ++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/pipeline/pass_pkgmap.c b/src/pipeline/pass_pkgmap.c index a44864659..3c4a1d338 100644 --- a/src/pipeline/pass_pkgmap.c +++ b/src/pipeline/pass_pkgmap.c @@ -1539,6 +1539,21 @@ static bool import_targetable_label(const char *label) { return false; } +/* #1934: whether the name-guess import fallbacks — Strategy 1b (sibling file, + * whose label filter admits symbols) and Strategy 3 (symbol name) — may run + * for imports from this language. A Go import path names a package — never a + * function, method + * or field — and every correct Go import resolves in Strategy 1 (module path + * → the package's Folder node); when that misses the import is external and + * the correct result is NO edge. The fallback instead bound the last path + * segment to an arbitrary same-named project symbol (`import "os/exec"` → a + * test harness's exec() method, two imports → a Makefile target). Languages + * whose import genuinely can name a member (Python `from m import f`, Java + * `import com.example.Foo`, Rust `use crate::ops::helper`) keep it. */ +bool cbm_import_symbol_fallback_allowed(CBMLanguage lang) { + return lang != CBM_LANG_GO; +} + static const char *path_leaf(const char *path) { const char *leaf = path; for (const char *p = path; p && *p; p++) { @@ -1817,10 +1832,24 @@ const cbm_gbuf_node_t *cbm_pipeline_resolve_import_node(const cbm_pipeline_ctx_t return target; } + /* Name-guess fallbacks below (Strategy 1b sibling-file, Strategy 3 + * symbol-name) are gated per importing-file language — see + * cbm_import_symbol_fallback_allowed (#1934). */ + const char *src_base = source_rel ? source_rel : ""; + for (const char *pb = src_base; *pb; pb++) { + if (*pb == '/' || *pb == '\\') { + src_base = pb + SKIP_ONE; + } + } + const bool symbol_fallback_allowed = + cbm_import_symbol_fallback_allowed(cbm_language_for_filename(src_base)); + /* Strategy 1b: sibling-file resolution for build/markup grammars whose * import string is a sibling filename or directory (SCSS partials, Just/ - * BitBake/func includes, Meson subdir, Pony use). */ - { + * BitBake/func includes, Meson subdir, Pony use). Its label filter admits + * symbols too, so for Go it re-creates the Strategy-3 bug one directory + * closer (`os/exec` → a same-package exec() method) — gated the same. */ + if (symbol_fallback_allowed) { const cbm_gbuf_node_t *sib = resolve_sibling_file(ctx, source_rel, source_file_qn, imp->module_path); if (sib) { @@ -1898,7 +1927,8 @@ const cbm_gbuf_node_t *cbm_pipeline_resolve_import_node(const cbm_pipeline_ctx_t /* Strategy 3: symbol-name fallback. Derive a representative imported * symbol (handling alias / glob / grouped forms) and match it against an * in-graph definition of the same simple name in another file - * (Rust `helper`, Java `Util`, Kotlin grouped, ...). */ + * (Rust `helper`, Java `Util`, Kotlin grouped, ...). + * Gated per importing-file language, like Strategy 1b above (#1934). */ char symbuf[256]; /* Prefer the clean candidate from the module path; the local_name may be an * alias (Rust `as h`, Kotlin `as U`) that names no real symbol. */ @@ -1962,7 +1992,7 @@ const cbm_gbuf_node_t *cbm_pipeline_resolve_import_node(const cbm_pipeline_ctx_t *dot = '\0'; end = dot; } - for (int ci = 0; ci < ncands; ci++) { + for (int ci = 0; symbol_fallback_allowed && ci < ncands; ci++) { const cbm_gbuf_node_t **hits = NULL; int n = 0; if (cbm_gbuf_find_by_name(ctx->gbuf, cands[ci], &hits, &n) == 0 && hits) { diff --git a/src/pipeline/pipeline_internal.h b/src/pipeline/pipeline_internal.h index e686ac3b7..45b0baf6b 100644 --- a/src/pipeline/pipeline_internal.h +++ b/src/pipeline/pipeline_internal.h @@ -234,6 +234,14 @@ static inline int cbm_pipeline_check_cancel(const cbm_pipeline_ctx_t *ctx) { /* ── Testable helpers ────────────────────────────────────────────── */ +/* #1934: whether the import resolver's name-guess fallbacks — Strategy 1b + * (sibling file; its label filter admits symbols) and Strategy 3 (symbol + * name) — may run for imports from this language. False for Go: an import + * path names a package, never a symbol, so a Strategy-1 miss means the import + * is external and the correct result is no edge. Pure; exercised through + * ei_go_import_never_binds_symbol. */ +bool cbm_import_symbol_fallback_allowed(CBMLanguage lang); + /* Check if a file path is worth tracking for git history analysis. */ bool cbm_is_trackable_file(const char *path); diff --git a/tests/test_edge_imports.c b/tests/test_edge_imports.c index 8f34e95a7..bf99e8d42 100644 --- a/tests/test_edge_imports.c +++ b/tests/test_edge_imports.c @@ -241,6 +241,20 @@ static void ei_cleanup(EILangProj *lp, cbm_store_t *store) { /* Index `files`, check IMPORTS count >= `floor`. Dumps a diagnostic on * failure so failures are self-diagnosable without re-running manually. */ +/* Exact-count variant of ei_edge_present: a fabricated EXTRA edge must fail + * the probe, so a floor is not enough (#1932's negative-assertion gap). */ +static int ei_edge_count_is(const EILangFile *files, int nfiles, const char *edge_type, + int expected) { + EILangProj lp; + cbm_store_t *store = ei_index_files(&lp, files, nfiles); + int got = store ? cbm_store_count_edges_by_type(store, lp.project, edge_type) : -1; + if (got != expected) { + fprintf(stderr, " [%s] FAIL count=%d expected==%d\n", edge_type, got, expected); + } + ei_cleanup(&lp, store); + return got == expected; +} + static int ei_edge_present(const EILangFile *files, int nfiles, const char *edge_type, int floor) { EILangProj lp; cbm_store_t *store = ei_index_files(&lp, files, nfiles); @@ -513,6 +527,34 @@ TEST(ei_go_two_consumers_same_package) { PASS(); } +TEST(ei_go_import_never_binds_symbol) { + /* #1934: a Go import path names a package, never a symbol. `os/exec` is + * external (not in the graph), so the ONLY correct outcome is no edge — + * but Strategy 3's symbol-name fallback matched the path's last segment + * against any project definition named `exec` and bound the import to a + * test harness's method. Exact count: the internal `util` import (edge 1, + * via Strategy 1 → the package Folder) must be the whole IMPORTS + * relation; the fallback edge onto harness.exec (reproduce-first RED: + * count 2) must not exist. */ + static const EILangFile f[] = { + {"go.mod", "module example.com/fxi\n\ngo 1.22\n"}, + {"util/util.go", "package util\n\nfunc Tag() string { return \"t\" }\n"}, + {"helper/harness.go", "package helper\n\ntype harness struct{ n int }\n\n" + "func (h *harness) exec(cmd string) error { return nil }\n"}, + /* Same-package decoy: the field-measured survivor bound os/exec to a + * method in the IMPORTER'S OWN package (Strategy 1b's sibling-file + * resolution accepts symbol labels too), so the fixture needs the + * collision both cross-package and same-package. */ + {"app/aux.go", "package app\n\ntype runner struct{ n int }\n\n" + "func (r *runner) exec(cmd string) error { return nil }\n"}, + {"app/run.go", "package app\n\nimport (\n\t\"os/exec\"\n\n" + "\t\"example.com/fxi/util\"\n)\n\n" + "func Run() error {\n\t_ = util.Tag()\n" + "\treturn exec.Command(\"true\").Run()\n}\n"}}; + ASSERT_TRUE(ei_edge_count_is(f, 5, "IMPORTS", 1)); + PASS(); +} + /* C++: header include should resolve to the header file node, not the same-stem * source node. Also exercises angle-bracket include resolution. */ TEST(ei_cpp_header_include_targets_header_file) { @@ -1037,6 +1079,7 @@ SUITE(edge_imports) { RUN_TEST(ei_go_subpackage_import); RUN_TEST(ei_go_blank_import); RUN_TEST(ei_go_two_consumers_same_package); + RUN_TEST(ei_go_import_never_binds_symbol); RUN_TEST(ei_cpp_header_include_targets_header_file); /* ── RED REPRODUCTIONS — Rust (expected to FAIL until pipeline fixed) ── */