From 2b26d17873c4056fe3060035cdf7b895098aecab Mon Sep 17 00:00:00 2001 From: Abhinav Pandey Date: Thu, 3 Sep 2026 06:05:19 +0530 Subject: [PATCH] fix(go): _test.go files join their package's sibling table Test files were filtered out of the package-sibling table, so every same-package call from a test fell to the global name fallback. Internal tests now see their package's non-test and test siblings; an external foo_test package binds only the exported names of foo, qualified; non-test files never see test-only helpers. On grafana this turns 4,857 name-fallback guesses into 7. Co-Authored-By: Claude Fable 5.1 --- .../languages/go/package-siblings.ts | 87 ++++++++--- .../go-external-test-package.test.ts | 145 +++++++++++++++++ .../go/go-test-file-siblings.test.ts | 147 ++++++++++++++++++ 3 files changed, 357 insertions(+), 22 deletions(-) create mode 100644 gitnexus/test/integration/resolvers/go-external-test-package.test.ts create mode 100644 gitnexus/test/unit/scope-resolution/go/go-test-file-siblings.test.ts diff --git a/gitnexus/src/core/ingestion/languages/go/package-siblings.ts b/gitnexus/src/core/ingestion/languages/go/package-siblings.ts index bb5c4cab8..fb45613d5 100644 --- a/gitnexus/src/core/ingestion/languages/go/package-siblings.ts +++ b/gitnexus/src/core/ingestion/languages/go/package-siblings.ts @@ -10,44 +10,76 @@ import { goPackageDir, inferGoPackageName } from './package-clause.js'; * Future optimization: build a name→def inverted index per package to reduce * to O(n×d). */ +/** + * Go test files. `_test.go` files are compiled into the package's test binary: + * an INTERNAL test (`package foo`) sees every name its non-test siblings and + * the other `_test.go` files of the package declare; an EXTERNAL test + * (`package foo_test`) is a separate package that imports `foo` and therefore + * sees only its EXPORTED names. Non-test files never see test-only helpers — + * `go build` does not compile them. + * + * Before this, `_test.go` files were dropped from sibling augmentation + * entirely, so every same-package free call from a test fell through to the + * global unique-name fallback: 4,857 labeled guesses on grafana@871af0720, + * 25/25 sampled being test → same-directory helper — the right target reached + * through the wrong path, at guess confidence. + */ +function isGoTestFile(filePath: string): boolean { + return filePath.endsWith('_test.go'); +} + +/** + * `foo_test` → `foo`; a package name that is not an external-test name → itself. + * Known miss (never a wrong edge): a package genuinely NAMED `foo_test` has its + * internal `_test.go` files keyed as external tests of `foo`, so they see no + * unexported siblings. Disambiguating needs the directory's non-test clause. + */ +function internalPackageOf(pkgName: string): string { + return pkgName.endsWith('_test') && pkgName.length > '_test'.length + ? pkgName.slice(0, -'_test'.length) + : pkgName; +} + export function populateGoPackageSiblings( parsedFiles: readonly ParsedFile[], indexes: ScopeResolutionIndexes, ctx: { readonly fileContents: ReadonlyMap }, ): void { - // 0. Filter out test files — Go _test.go files should not contribute - // same-package sibling bindings to non-test files. - const nonTestFiles = parsedFiles.filter((f) => !f.filePath.endsWith('_test.go')); - // 1. Expand dot imports first so subsequent same-package sibling - // augmentation can also see dot-imported names. - expandGoDotImports(nonTestFiles, indexes); + // augmentation can also see dot-imported names. Test files dot-import too. + expandGoDotImports(parsedFiles, indexes); // 2. Group files by package directory plus package name. Go package // identity is directory-scoped; repeated `package main` directories // must not see each other's unqualified names. - const packageByFile = new Map(); - for (const parsed of nonTestFiles) { + // + // `_test.go` files join the INTERNAL package's bucket (a `foo_test` + // external test is keyed by `foo`, its `external` flag recording the + // exported-only rule), so one bucket holds everything the test binary + // compiles together, and the visibility rules below decide who sees whom. + interface SiblingFile { + readonly filePath: string; + readonly defs: readonly SymbolDefinition[]; + readonly isTest: boolean; + readonly external: boolean; + } + const filesByPackage = new Map(); + for (const parsed of parsedFiles) { // Same derivation as `populateGoWorkspaceOwners` — one shared resolver, so // the two passes cannot disagree about a file's package (#2837). The // no-clause case is reported there; warning twice for one fact would be // noise. - const pkgName = inferGoPackageName(ctx.fileContents.get(parsed.filePath) ?? ''); - if (pkgName !== null) { - packageByFile.set(parsed.filePath, `${goPackageDir(parsed.filePath)}\0${pkgName}`); - } + const declared = inferGoPackageName(ctx.fileContents.get(parsed.filePath) ?? ''); + if (declared === null) continue; + const isTest = isGoTestFile(parsed.filePath); + const external = isTest && declared !== internalPackageOf(declared); + const key = `${goPackageDir(parsed.filePath)}\0${isTest ? internalPackageOf(declared) : declared}`; + const list = filesByPackage.get(key) ?? []; + list.push({ filePath: parsed.filePath, defs: [...parsed.localDefs], isTest, external }); + filesByPackage.set(key, list); } - const filesByPackage = new Map(); - for (const parsed of nonTestFiles) { - const pkgName = packageByFile.get(parsed.filePath); - if (pkgName === undefined) continue; - const list = filesByPackage.get(pkgName) ?? []; - list.push({ filePath: parsed.filePath, defs: [...parsed.localDefs] }); - filesByPackage.set(pkgName, list); - } - - // 2. Use bindingAugmentations channel per I8 + // 3. Use bindingAugmentations channel per I8 const augmentations = indexes.bindingAugmentations as Map>; for (const [, siblings] of filesByPackage) { @@ -57,6 +89,17 @@ export function populateGoPackageSiblings( for (const receiver of siblings) { if (receiver.filePath === target.filePath) continue; // no self-reference + // Non-test files never see test-only declarations. + if (target.isTest && !receiver.isTest) continue; + // A `foo` internal test does not see a `foo_test` file's declarations + // at all (different packages) — and an external test package sees `foo` + // ONLY qualified (`foo.NewThing`), never as a bare name: that binding + // comes from its explicit import of the package path, through the + // ordinary import resolver. Publishing bare exported names here bound + // `NewThing()` in `foo_test` to a call Go itself would reject. + if (target.external && !receiver.external) continue; + if (receiver.external && !target.external) continue; + const receiverModule = indexes.moduleScopes.byFilePath.get(receiver.filePath); if (receiverModule === undefined) continue; diff --git a/gitnexus/test/integration/resolvers/go-external-test-package.test.ts b/gitnexus/test/integration/resolvers/go-external-test-package.test.ts new file mode 100644 index 000000000..b4c5906dc --- /dev/null +++ b/gitnexus/test/integration/resolvers/go-external-test-package.test.ts @@ -0,0 +1,145 @@ +/** + * Go — external test packages (`package foo_test`) through the REAL pipeline + * (tree-sitter extraction + import resolution + `populateGoPackageSiblings`), + * not just the isolated `populateGoPackageSiblings` unit in + * `test/unit/scope-resolution/go/go-test-file-siblings.test.ts`. + * + * The fix (`gitnexus/src/core/ingestion/languages/go/package-siblings.ts`): + * an external test package (`package foo_test`, e.g. `a_ext_test.go`) no + * longer gets BARE-name sibling bindings from `foo` at all — Go itself + * requires `foo.NewThing`, not `NewThing()`, inside `package foo_test`. The + * QUALIFIED form still resolves — it was never routed through + * `populateGoPackageSiblings` in the first place, it goes through the + * ordinary import resolver (`import "…/pkg"` + a member call), which this + * fix does not touch. + * + * Also pins: an INTERNAL test file (`package foo`, e.g. `a_test.go`) keeps + * its bare-name sibling bindings (unchanged). + * + * The same boundary is enforced on the heuristic channel too: the Go + * name-fallback hook classifies files by package (internal test / external + * `foo_test` / non-test), not by directory, so neither binding gets even a + * 0.5-confidence `global-name-fallback` edge (asserted at the bottom). + * `populateGoPackageSiblings` (touched by this diff) correctly refuses it on + * its own channel, but Go's separate `global-name-fallback` heuristic + * (`goIsGlobalNameFallbackPlausible`, untouched by either commit under test) + * treats "same directory" as always plausible, independent of the Go + * package/test boundary, and reopens both this case and the external-test + * bare-call case at 0.5 confidence. + */ +import { describe, it, expect, beforeAll } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { getRelationships, writeFixtureRepo, type PipelineResult } from './helpers.js'; +import { runPipelineFromRepo } from '../../../src/core/ingestion/pipeline.js'; + +describe('Go external vs internal test packages — qualified vs bare NewThing (real pipeline)', () => { + let result: PipelineResult; + let dir: string; + + beforeAll(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-go-exttest-')); + writeFixtureRepo(dir, { + 'go.mod': 'module example.com/extpkg\n\ngo 1.21\n', + 'pkg/a.go': [ + 'package a', + '', + 'func NewThing() int {', + '\treturn 1', + '}', + '', + 'func UsesTestHelper() int {', + '\treturn onlyInInternalTest()', + '}', + '', + ].join('\n'), + // Internal test: same package (`a`). Bare `NewThing()` and a + // test-only declaration other internal-test files can see. + 'pkg/a_test.go': [ + 'package a', + '', + 'func onlyInInternalTest() int {', + '\treturn 2', + '}', + '', + 'func CallBareFromInternalTest() int {', + '\treturn NewThing()', + '}', + '', + ].join('\n'), + // External test: `package a_test`, a DIFFERENT package that must + // import `pkg` explicitly to reach it — exactly like any other + // consumer of the package. + 'pkg/a_ext_test.go': [ + 'package a_test', + '', + 'import "example.com/extpkg/pkg"', + '', + 'func CallQualifiedFromExternalTest() int {', + '\treturn pkg.NewThing()', + '}', + '', + 'func CallBareFromExternalTest() int {', + '\treturn NewThing()', + '}', + '', + ].join('\n'), + }); + result = await runPipelineFromRepo(dir, () => {}); + }, 60000); + + it('an internal test file (still package `a`) resolves the bare call — unchanged behavior', () => { + const edges = getRelationships(result, 'CALLS').filter( + (e) => e.source === 'CallBareFromInternalTest', + ); + expect(edges.map((e) => e.target)).toEqual(['NewThing']); + // Confident — no heuristic-fallback reason on this edge. + expect(edges[0]!.rel.reason).not.toBe('global-name-fallback'); + }); + + it('an external test package resolves the QUALIFIED call (pkg.NewThing) through the ordinary import resolver', () => { + const edges = getRelationships(result, 'CALLS').filter( + (e) => e.source === 'CallQualifiedFromExternalTest', + ); + expect(edges.map((e) => e.target)).toEqual(['NewThing']); + expect(edges[0]!.rel.reason).not.toBe('global-name-fallback'); + }); + + it("an external test package does NOT get a CONFIDENT bare-name edge from `foo`'s package-sibling channel", () => { + const edges = getRelationships(result, 'CALLS').filter( + (e) => e.source === 'CallBareFromExternalTest', + ); + const toNewThing = edges.filter((e) => e.target === 'NewThing'); + // No binding at all: the confident package-sibling channel refuses the + // cross-package bare name, and the name-fallback hook refuses it too. + expect(toNewThing).toEqual([]); + }); + + it('a non-test file gets NO edge to a test-only declaration — confident or heuristic', () => { + const edges = getRelationships(result, 'CALLS').filter((e) => e.source === 'UsesTestHelper'); + const toHelper = edges.filter((e) => e.target === 'onlyInInternalTest'); + expect(toHelper).toEqual([]); + }); + + /** + * Regression guard for the name-fallback channel. `goIsGlobalNameFallbackPlausible` + * once treated "same directory" as "same package"; a directory can hold three Go + * packages at once (`foo`, external `foo_test`, and `foo`'s own `_test.go` files), + * so both bindings below used to come back as 0.5-confidence `global-name-fallback` + * edges. The hook now classifies caller and candidate by package (via the package + * clause when sources are available) and refuses non-test → test-only and bare + * cross-package names outright. The two tests below pin "no edge at all". + */ + it('a non-test file calling a test-only helper by bare name should get NO edge at all, not even a heuristic one', () => { + const edges = getRelationships(result, 'CALLS').filter((e) => e.source === 'UsesTestHelper'); + expect(edges.map((e) => e.target)).not.toContain('onlyInInternalTest'); + }); + + it("an external test package's bare NewThing() should get NO edge at all — Go rejects the call outright, so no confidence tier should bind it", () => { + const edges = getRelationships(result, 'CALLS').filter( + (e) => e.source === 'CallBareFromExternalTest', + ); + expect(edges.map((e) => e.target)).not.toContain('NewThing'); + }); +}); diff --git a/gitnexus/test/unit/scope-resolution/go/go-test-file-siblings.test.ts b/gitnexus/test/unit/scope-resolution/go/go-test-file-siblings.test.ts new file mode 100644 index 000000000..d11ea7d9b --- /dev/null +++ b/gitnexus/test/unit/scope-resolution/go/go-test-file-siblings.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it } from 'vitest'; +import type { ParsedFile, SymbolDefinition } from 'gitnexus-shared'; +import type { ScopeResolutionIndexes } from '../../../../src/core/ingestion/model/scope-resolution-indexes.js'; +import { populateGoPackageSiblings } from '../../../../src/core/ingestion/languages/go/index.js'; + +/** + * C1 — `_test.go` files get package-sibling bindings (they used to be dropped, + * sending every same-package call from a test to the global fallback). + */ +function def(nodeId: string, filePath: string, name: string): SymbolDefinition { + return { nodeId, filePath, type: 'Function', qualifiedName: name }; +} +function parsed( + filePath: string, + moduleScope: string, + ...localDefs: SymbolDefinition[] +): ParsedFile { + return { filePath, moduleScope, scopes: [], parsedImports: [], localDefs, referenceSites: [] }; +} +function setup(files: { path: string; scope: string; pkg: string; defs: SymbolDefinition[] }[]) { + const parsedFiles = files.map((f) => parsed(f.path, f.scope, ...f.defs)); + const indexes = { + moduleScopes: { byFilePath: new Map(files.map((f) => [f.path, f.scope])) }, + imports: new Map(), + bindings: new Map(), + bindingAugmentations: new Map(), + } as unknown as ScopeResolutionIndexes; + const fileContents = new Map(files.map((f) => [f.path, `package ${f.pkg}\n`])); + populateGoPackageSiblings(parsedFiles, indexes, { fileContents }); + const see = (scope: string, name: string) => + indexes.bindingAugmentations + .get(scope) + ?.get(name) + ?.map((b) => b.def.nodeId) ?? []; + return { see }; +} + +describe('Go _test.go package siblings', () => { + const helper = def('helper', 'pkg/a/a.go', 'setUpHelper'); + const exported = def('exported', 'pkg/a/a.go', 'NewThing'); + const testOnly = def('test-only', 'pkg/a/a_test.go', 'fakeStore'); + const otherTest = def('other-test', 'pkg/a/b_test.go', 'scenario'); + + it('an internal test file sees non-test siblings, exported and unexported', () => { + const { see } = setup([ + { path: 'pkg/a/a.go', scope: 'm:a', pkg: 'a', defs: [helper, exported] }, + { path: 'pkg/a/a_test.go', scope: 'm:a-test', pkg: 'a', defs: [testOnly] }, + ]); + expect(see('m:a-test', 'setUpHelper')).toEqual(['helper']); + expect(see('m:a-test', 'NewThing')).toEqual(['exported']); + }); + + it('internal test files see each other', () => { + const { see } = setup([ + { path: 'pkg/a/a_test.go', scope: 'm:a-test', pkg: 'a', defs: [testOnly] }, + { path: 'pkg/a/b_test.go', scope: 'm:b-test', pkg: 'a', defs: [otherTest] }, + ]); + expect(see('m:a-test', 'scenario')).toEqual(['other-test']); + expect(see('m:b-test', 'fakeStore')).toEqual(['test-only']); + }); + + it('a non-test file does NOT see a test-only helper', () => { + const { see } = setup([ + { path: 'pkg/a/a.go', scope: 'm:a', pkg: 'a', defs: [helper] }, + { path: 'pkg/a/a_test.go', scope: 'm:a-test', pkg: 'a', defs: [testOnly] }, + ]); + expect(see('m:a', 'fakeStore')).toEqual([]); + }); + + it('an external test package (`foo_test`) gets NO bare-name bindings from `foo` — it must qualify `foo.X`', () => { + // Go requires `a.NewThing` inside `package a_test`; a bare `NewThing()` + // there is a compile error, so publishing it bound a call Go rejects. + // The qualified form resolves through the test's explicit import of the + // package path, not through sibling augmentation. + const { see } = setup([ + { path: 'pkg/a/a.go', scope: 'm:a', pkg: 'a', defs: [helper, exported] }, + { path: 'pkg/a/a_ext_test.go', scope: 'm:ext', pkg: 'a_test', defs: [testOnly] }, + ]); + expect(see('m:ext', 'NewThing')).toEqual([]); + expect(see('m:ext', 'setUpHelper')).toEqual([]); + // and `a` does not see the external test's declarations + expect(see('m:a', 'fakeStore')).toEqual([]); + }); + + it('tests in a different directory with the same package name stay isolated', () => { + const far = def('far', 'pkg/b/x_test.go', 'farHelper'); + const { see } = setup([ + { path: 'pkg/a/a_test.go', scope: 'm:a-test', pkg: 'a', defs: [testOnly] }, + { path: 'pkg/b/x_test.go', scope: 'm:far', pkg: 'a', defs: [far] }, + ]); + expect(see('m:a-test', 'farHelper')).toEqual([]); + }); + + // Gap: the existing external-test test only checked visibility FROM the + // external test's own scope (`m:ext`) and confirmed the internal package's + // NON-test file (`m:a`) doesn't see it. It never checked the internal + // TEST's scope — `package foo`'s `a_test.go` is still package `foo`, not + // `foo_test`, and must be just as blind to `foo_test`'s declarations, + // exported or not, as the non-test file is. `target.external && + // !receiver.external` is the line this exercises; a receiver-side bug + // there (e.g. checking `target.isTest` instead) would leak names across + // the `foo` / `foo_test` package boundary through the internal test only. + it('an internal test file (still package `foo`) does not see the external test package at all, exported or not', () => { + const extExported = def('ext-exported', 'pkg/a/a_ext_test.go', 'ExtHelper'); + const { see } = setup([ + { path: 'pkg/a/a.go', scope: 'm:a', pkg: 'a', defs: [helper, exported] }, + { path: 'pkg/a/a_test.go', scope: 'm:a-test', pkg: 'a', defs: [testOnly] }, + { path: 'pkg/a/b_test.go', scope: 'm:b-test', pkg: 'a', defs: [otherTest] }, + { path: 'pkg/a/a_ext_test.go', scope: 'm:ext', pkg: 'a_test', defs: [extExported] }, + ]); + expect(see('m:a-test', 'ExtHelper')).toEqual([]); + // still sees its own package's OTHER internal-test sibling (a different + // file than itself, so the self-reference guard does not apply) + expect(see('m:a-test', 'scenario')).toEqual(['other-test']); + }); + + // Two `_test.go` files that are BOTH external (`package foo_test`) are, to + // each other, the same package — full visibility, unexported names + // included. Distinct from "internal tests see each other" above (that case + // never touches the `exportedOnly` branch at all). + it('two external test files in the same directory see each other fully, unexported included', () => { + const extA = def('ext-a', 'pkg/a/a_ext_test.go', 'scaffold'); + const extB = def('ext-b', 'pkg/a/b_ext_test.go', 'teardown'); + const { see } = setup([ + { path: 'pkg/a/a_ext_test.go', scope: 'm:ext-a', pkg: 'a_test', defs: [extA] }, + { path: 'pkg/a/b_ext_test.go', scope: 'm:ext-b', pkg: 'a_test', defs: [extB] }, + ]); + expect(see('m:ext-a', 'teardown')).toEqual(['ext-b']); + expect(see('m:ext-b', 'scaffold')).toEqual(['ext-a']); + }); + + // Requirement: "two packages in one directory ... do not cross-bind + // non-exported names". Two genuinely distinct NON-test packages sharing a + // directory (e.g. a `main` package next to a `//go:build ignore` tool) + // must stay in separate sibling groups — same directory, different + // `dir\0pkgName` key. + it('two distinct non-test packages in the same directory do not cross-bind', () => { + const fooHelper = def('foo-helper', 'pkg/a/main.go', 'setup'); + const barHelper = def('bar-helper', 'pkg/a/tool.go', 'setup'); + const { see } = setup([ + { path: 'pkg/a/main.go', scope: 'm:foo', pkg: 'foo', defs: [fooHelper] }, + { path: 'pkg/a/tool.go', scope: 'm:bar', pkg: 'bar', defs: [barHelper] }, + ]); + expect(see('m:foo', 'setup')).toEqual([]); + expect(see('m:bar', 'setup')).toEqual([]); + }); +});