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 <noreply@anthropic.com>
This commit is contained in:
Abhinav Pandey 2026-09-03 06:05:19 +05:30
parent 9a61dc0fc6
commit 2b26d17873
No known key found for this signature in database
3 changed files with 357 additions and 22 deletions

View file

@ -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<string, string> },
): 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<string, string>();
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<string, SiblingFile[]>();
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<string, { filePath: string; defs: SymbolDefinition[] }[]>();
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<ScopeId, Map<string, BindingRef[]>>;
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;

View file

@ -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');
});
});

View file

@ -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([]);
});
});