Skip to content

Commit 099496d

Browse files
authored
Emit test_ref facts for Go so test-only symbols aren't reported dead (#89)
A production function whose only caller is its own _test.go was reported as high-confidence dead code. Two gates caused it, and fixing either alone is a no-op: the walker collects an ignored file for reference-only extraction only when it matches a test glob, and runTestRefExtractors skips extractors that don't implement plugin.TestRefExtractor. - config.Default().TestGlobs gains "**/*_test.go". Go's bare suffix is safe: the toolchain defines any *_test.go as a test file, so no production file can collide with it the way _test.rb did (v97). - goextractor implements ExtractTestRefs, resolving targets with the production resolvers (flattenSelector/collectLocalTypes/resolveChain). A reference from a test is therefore spelled exactly as one from production, and inherits goBuiltins filtering — a package-level min shadowing the builtin is still not credited, from either side. Copying Ruby's identifier-level walker would have rescued unrelated symbols by bare name, trading false positives for false negatives. - Deliberately no OwnsFile: plugin.FileOwner is what opts an extractor into the incremental cache, and adding it would move .go files out of the shared partition that keys every other extractor. ExtractTestRefs filters internally. cacheVersion v100 + cachecov entry. go_sample gains a_test.go (in-package) and a_ext_test.go (package a_test) to pin both Go test idioms; the latter calls a dedicated Gamma rather than Alpha, whose dependent count TestE2E_ImpactAnalysis asserts — a test_ref counts as a graph dependent, which is a separate pre-existing defect in god-class/hotspots.
1 parent 5f20213 commit 099496d

10 files changed

Lines changed: 468 additions & 2 deletions

File tree

internal/cachecov/coverage_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ var versionCoverage = map[int][]string{
131131
97: {"TestMatchAnyGlob_MidPatternDoublestar", "TestGolden"}, // Directory-scoped Ruby test globs: production *_ab_test.rb no longer deleted from the graph
132132
98: {"TestKtComplexity_ScalingLoopDepth_ConstantRangeDiscounted", "TestKtComplexity_ScalingLoopDepth_VariableRangeNotDiscounted", "TestKtComplexity_ScalingLoopDepth_ConstantIteratorReceiverDiscounted", "TestKtComplexity_ScalingLoopDepth_VariableIteratorReceiverNotDiscounted", "TestKtComplexity_ScalingLoopDepth_ConstantOuterScalingInner", "TestKtComplexity_ScalingLoopDepth_ConstantInnerScalingOuter", "TestKtComplexity_ScalingLoopDepth_InfiniteLoopDiscounted", "TestKtComplexity_ScalingLoopDepth_ConditionalWhileNotDiscounted", "TestKtComplexity_ScalingLoopDepth_AbsentWithoutLoops", "TestKtComplexity_LoopCountAndCyclomaticUnchangedByBounding", "TestGolden"}, // Kotlin bounded-loop discounting: scaling_loop_depth joins the Go/Python/TS convention
133133
99: {"TestKtComplexity_CallsInScalingLoop_ConstantExcluded", "TestKtComplexity_CallsInScalingLoop_InfiniteLoopCallsRetained", "TestKtComplexity_CallsInScalingLoop_ScalingRetained", "TestKtComplexity_CallsInScalingLoop_MixedLoops", "TestKtComplexity_CallsInScalingLoop_AbsentWithoutLoopCalls", "TestExtract_CallsInScalingLoop_InfiniteLoopCallsRetained", "TestExtract_CallsInScalingLoop_PresentButEmptyWhenAllBounded", "TestExtract_CallsInScalingLoop_AbsentWithoutLoopCalls", "TestPyComplexity_CallsInScalingLoop_InfiniteLoopCallsRetained", "TestPyComplexity_CallsInScalingLoop_PresentButEmptyWhenAllBounded", "TestPyComplexity_CallsInScalingLoop_AbsentWithoutLoopCalls", "TestTsComplexity_CallsInScalingLoop_InfiniteLoopCallsRetained", "TestTsComplexity_CallsInScalingLoop_PresentButEmptyWhenAllBounded", "TestTsComplexity_CallsInScalingLoop_AbsentWithoutLoopCalls", "TestGolden"}, // calls_in_scaling_loop = repeated (not merely scaling) loops, emitted even when empty
134+
100: {"TestExtractTestRefs_InPackageCallResolvesToProductionSymbol", "TestExtractTestRefs_ExternalTestPackageResolvesThroughImport", "TestExtractTestRefs_SkipsBuiltinsAndEmitsNoSymbols", "TestExtractTestRefs_IgnoresFilesItDoesNotOwn", "TestExtractTestRefs_ReferenceFreeFileYieldsNoFact", "TestDefaultTestGlobsCoverGoAndStayIgnored", "TestGolden"}, // Go test_ref facts: both gates, so a function called only from its _test.go is no longer high-confidence dead
134135
}
135136

136137
func TestCacheVersionCoverage(t *testing.T) {

internal/config/config.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,12 @@ func Default() *Config {
102102
// include test symbols — but the engine collects them separately for
103103
// reference-only extraction so the dead-code detector can see that a
104104
// production symbol is exercised by a test and not mis-report it as dead.
105-
TestGlobs: []string{"**/spec/**/*_spec.rb", "**/test/**/*_test.rb"},
105+
// A glob here without an extractor implementing plugin.TestRefExtractor is a
106+
// no-op (engine.runTestRefExtractors skips non-implementers), so extend this
107+
// list only alongside the matching extractor. Go's bare suffix is correct:
108+
// the toolchain defines any *_test.go as a test file, so — unlike Ruby's
109+
// _test.rb (v97) — no production file can collide with it.
110+
TestGlobs: []string{"**/*_test.go", "**/spec/**/*_spec.rb", "**/test/**/*_test.rb"},
106111
Extractors: []string{"cpp", "go", "grpc", "java", "kotlin", "openapi", "php", "python", "typescript", "swift", "ruby"},
107112
Explainers: []string{"cycles", "layers", "crossrepo", "coverage", "unused-routes", "god-class", "hotspots", "dependency-depth", "exported-surface", "complexity-outliers"},
108113
Renderers: []string{"llm_context"},

internal/config/config_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,31 @@ func TestDefaultIgnoresPythonEnvs(t *testing.T) {
4242
}
4343
}
4444
}
45+
46+
// TestDefaultTestGlobsCoverGoAndStayIgnored pins both halves of the test-ref
47+
// contract for Go (GAP-GO-06, v100).
48+
//
49+
// A test file's references survive only if it is BOTH ignored for normal indexing
50+
// AND matched by a TestGlob — engine.walkRepo collects an ignored file for
51+
// reference-only extraction only when matchesTestGlob says so. Adding a glob to
52+
// one list and not the other silently drops the file (ignored, never recovered)
53+
// or indexes test symbols as production code. config.go states the invariant in a
54+
// comment; this asserts it.
55+
//
56+
// Go's bare-suffix form is correct and must stay: unlike Ruby — where
57+
// "**/*_test.rb" swallowed a production job named ..._ab_test.rb and had to become
58+
// directory-scoped (v97) — the Go toolchain DEFINES any *_test.go as a test file,
59+
// so no production file can collide.
60+
func TestDefaultTestGlobsCoverGoAndStayIgnored(t *testing.T) {
61+
cfg := Default()
62+
63+
if !contains(cfg.TestGlobs, "**/*_test.go") {
64+
t.Errorf("Default().TestGlobs missing %q — Go test files are ignored but never recovered", "**/*_test.go")
65+
}
66+
67+
for _, g := range cfg.TestGlobs {
68+
if !contains(cfg.Ignore, g) {
69+
t.Errorf("TestGlob %q is not in Default().Ignore; a test glob that is not ignored indexes test symbols as production code", g)
70+
}
71+
}
72+
}

internal/engine/cache.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,18 @@ import (
285285
// calls_in_loop. The second bug masked the first: fixing only the emptiness would have
286286
// deleted true-positive N+1 findings on infinite loops. Cached Go/Python/TypeScript/Kotlin
287287
// snapshots must re-extract.
288-
const cacheVersion = "v99"
288+
// v100: the Go extractor implements plugin.TestRefExtractor and config.Default().TestGlobs
289+
// gained "**/*_test.go", so a production function whose only caller is its own _test.go is
290+
// no longer reported as high-confidence dead code. Both gates were needed: the glob alone
291+
// is a no-op (runTestRefExtractors skips non-implementers), and the interface alone never
292+
// sees the files (walkRepo collects an ignored file only when it matches a test glob). Go
293+
// was the sharpest case of GAP-XL-02 — _test.go is ignored under BOTH shipped configs and a
294+
// plain function's orphan tier is `high`. Test refs are resolved with the production
295+
// resolvers (flattenSelector/collectLocalTypes/resolveChain), so a reference from a test is
296+
// spelled exactly as one from production and inherits goBuiltins filtering: a package-level
297+
// `min` shadowing the Go 1.21 builtin is still not credited, from either side. The file set
298+
// reaching the extractors changes, so cached snapshots must re-extract.
299+
const cacheVersion = "v100"
289300

290301
// extractorCache holds per-extractor facts keyed by a content hash of the files
291302
// the extractor depends on. It is loaded from disk at the start of a snapshot and

internal/engine/testdata/golden/go_sample.facts.jsonl

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,12 @@
77
{"kind":"module","name":"pkg/b","file":"pkg/b","repo":"go_sample","props":{"language":"go","package":"b"}}
88
{"kind":"symbol","name":"..main","file":"main.go","line":10,"repo":"go_sample","props":{"cyclomatic":1,"exported":false,"language":"go","symbol_kind":"function"},"relations":[{"kind":"calls","target":"pkg/a.Alpha"},{"kind":"calls","target":"pkg/b.Beta"},{"kind":"declares","target":"."}]}
99
{"kind":"symbol","name":"pkg/a.Alpha","file":"pkg/a/a.go","line":7,"repo":"go_sample","props":{"cyclomatic":1,"exported":true,"language":"go","symbol_kind":"function"},"relations":[{"kind":"calls","target":"pkg/b.Beta"},{"kind":"declares","target":"pkg/a"}]}
10+
{"kind":"symbol","name":"pkg/a.Gamma","file":"pkg/a/a.go","line":43,"repo":"go_sample","props":{"cyclomatic":1,"exported":true,"language":"go","symbol_kind":"function"},"relations":[{"kind":"declares","target":"pkg/a"}]}
1011
{"kind":"symbol","name":"pkg/a.GetPath","file":"pkg/a/a.go","line":15,"repo":"go_sample","props":{"calls_in_loop":["pkg/a.getByID"],"calls_in_scaling_loop":["pkg/a.getByID"],"cyclomatic":2,"exported":true,"language":"go","loop_count":1,"loop_depth":1,"scaling_loop_depth":0,"symbol_kind":"function"},"relations":[{"kind":"calls","target":"pkg/a.getByID"},{"kind":"declares","target":"pkg/a"}]}
1112
{"kind":"symbol","name":"pkg/a.Seed","file":"pkg/a/a.go","line":24,"repo":"go_sample","props":{"calls_in_loop":["pkg/a.setup"],"calls_in_scaling_loop":[],"cyclomatic":2,"exported":true,"language":"go","loop_count":1,"loop_depth":1,"scaling_loop_depth":0,"symbol_kind":"function"},"relations":[{"kind":"calls","target":"pkg/a.setup"},{"kind":"declares","target":"pkg/a"}]}
1213
{"kind":"symbol","name":"pkg/a.getByID","file":"pkg/a/a.go","line":30,"repo":"go_sample","props":{"cyclomatic":1,"exported":false,"language":"go","symbol_kind":"function"},"relations":[{"kind":"declares","target":"pkg/a"}]}
14+
{"kind":"symbol","name":"pkg/a.helper","file":"pkg/a/a.go","line":37,"repo":"go_sample","props":{"cyclomatic":1,"exported":false,"language":"go","symbol_kind":"function"},"relations":[{"kind":"declares","target":"pkg/a"}]}
1315
{"kind":"symbol","name":"pkg/a.setup","file":"pkg/a/a.go","line":31,"repo":"go_sample","props":{"cyclomatic":1,"exported":false,"language":"go","symbol_kind":"function"},"relations":[{"kind":"declares","target":"pkg/a"}]}
1416
{"kind":"symbol","name":"pkg/b.Beta","file":"pkg/b/b.go","line":6,"repo":"go_sample","props":{"cyclomatic":1,"exported":true,"language":"go","symbol_kind":"function"},"relations":[{"kind":"calls","target":"pkg/a.Alpha"},{"kind":"declares","target":"pkg/b"}]}
17+
{"kind":"test_ref","name":"pkg/a/a_ext_test.go","file":"pkg/a/a_ext_test.go","repo":"go_sample","props":{"language":"go"},"relations":[{"kind":"calls","target":"pkg/a.Gamma"},{"kind":"calls","target":"t.Log"}]}
18+
{"kind":"test_ref","name":"pkg/a/a_test.go","file":"pkg/a/a_test.go","repo":"go_sample","props":{"language":"go"},"relations":[{"kind":"calls","target":"pkg/a.helper"},{"kind":"calls","target":"t.Fatalf"}]}

internal/engine/testdata/repos/go_sample/pkg/a/a.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,15 @@ func Seed() {
2929

3030
func getByID(id int) {}
3131
func setup(c string) {}
32+
33+
// helper has no production caller. Its only reference is from a_test.go, which is
34+
// ignored for indexing and recovered as a test_ref fact — so the graph must still
35+
// carry an incoming edge for it. Before v100 no such fact existed for Go and the
36+
// symbol looked dead. Pins GAP-GO-06 for the in-package test idiom. (v100)
37+
func helper(n int) int { return n * 2 }
38+
39+
// Gamma is the same case for the OTHER Go test idiom: its only reference is from
40+
// a_ext_test.go (`package a_test`), which reaches it through an import alias rather
41+
// than unqualified. Deliberately not Alpha — Alpha's dependent count is asserted by
42+
// TestE2E_ImpactAnalysis, and a test_ref counts as a dependent (see GAP-XL-15). (v100)
43+
func Gamma() {}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package a_test
2+
3+
import (
4+
"testing"
5+
6+
"example.com/gosample/pkg/a"
7+
)
8+
9+
// The external-test-package idiom: `package a_test` imports the package under test,
10+
// so the reference is qualified by an import alias and must resolve through
11+
// buildFileImports to the same canonical name ("pkg/a.Gamma"). (v100)
12+
func TestGammaFromExternalPackage(t *testing.T) {
13+
a.Gamma()
14+
t.Log("called")
15+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package a
2+
3+
import "testing"
4+
5+
// The in-package idiom: the production function is called unqualified, so the
6+
// test-ref pass must resolve it against the file's own package dir ("pkg/a.helper").
7+
// This is the shape behind golf's NewRateLimiter false positive. (v100)
8+
func TestHelperDoubles(t *testing.T) {
9+
if helper(2) != 4 {
10+
t.Fatalf("helper(2) != 4")
11+
}
12+
}
Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
package goextractor
2+
3+
import (
4+
"context"
5+
"go/ast"
6+
"go/parser"
7+
"go/token"
8+
"log"
9+
"os"
10+
"path/filepath"
11+
"strings"
12+
13+
"github.com/enola-labs/enola/internal/facts"
14+
"github.com/enola-labs/enola/internal/parallel"
15+
)
16+
17+
// isGoTestFile reports whether a repo-relative path is a Go test file. The Go
18+
// toolchain defines the suffix, so this needs no directory scoping — a production
19+
// file cannot legally be named *_test.go and still compile into the package.
20+
func isGoTestFile(relFile string) bool { return strings.HasSuffix(relFile, "_test.go") }
21+
22+
// ExtractTestRefs implements plugin.TestRefExtractor. It parses *_test.go files for
23+
// the SOLE purpose of capturing their outbound references into production code,
24+
// emitting one facts.KindTestRef fact per file that carries only RelCalls edges —
25+
// no symbols. Test functions therefore never become dead-code candidates, and no
26+
// symbol/module/route explainer is affected, while the dead-code detector can see
27+
// that a production function is exercised by a test and not mis-report it as dead.
28+
//
29+
// The engine hands every TestGlob match to every TestRefExtractor whose repo it
30+
// detected, scoped by plugin.FileOwner when the extractor implements it.
31+
// GoExtractor deliberately does not: FileOwner is what opts an extractor into the
32+
// incremental cache (see its doc comment), and implementing it here would both
33+
// enable Go caching and pull .go files out of computeExtractorKeys' shared
34+
// partition, changing the shared hash that keys EVERY other extractor. So the
35+
// filter lives here instead — Ruby's ExtractTestRefs filters internally too.
36+
func (e *GoExtractor) ExtractTestRefs(ctx context.Context, repoPath string, files []string) ([]facts.Fact, error) {
37+
var goFiles []string
38+
for _, relFile := range files {
39+
if isGoTestFile(relFile) {
40+
goFiles = append(goFiles, relFile)
41+
}
42+
}
43+
if len(goFiles) == 0 {
44+
return nil, nil
45+
}
46+
47+
modulePath := readModulePath(repoPath)
48+
perFile := parallel.MapFiles(ctx, goFiles, func(relFile string) []facts.Fact {
49+
src, err := os.ReadFile(filepath.Join(repoPath, relFile))
50+
if err != nil {
51+
log.Printf("[go-extractor] error reading test file %s: %v", relFile, err)
52+
return nil
53+
}
54+
return refsFromGoTest(src, relFile, modulePath)
55+
})
56+
57+
var out []facts.Fact
58+
for _, ff := range perFile {
59+
out = append(out, ff...)
60+
}
61+
return out, nil
62+
}
63+
64+
// refsFromGoTest parses one Go test file and returns a single reference-only fact
65+
// carrying the production symbols it calls, or nil when it references nothing.
66+
//
67+
// Call targets are resolved with the PRODUCTION resolvers (flattenSelector,
68+
// collectLocalTypes, resolveChain), so a reference from a test is spelled exactly
69+
// as the same reference from production code would be, and the dead-code detector
70+
// needs no special case. That also inherits goBuiltins filtering, so len/make/min
71+
// never become phantom targets.
72+
//
73+
// Only call expressions yield targets — matching analyzeBody, which likewise
74+
// ignores composite literals. A type constructed only as `Foo{}` from a test is
75+
// therefore still reported dead, but so is one constructed only that way from
76+
// production code: that blind spot is pre-existing and not specific to tests.
77+
func refsFromGoTest(src []byte, relFile, modulePath string) []facts.Fact {
78+
fset := token.NewFileSet()
79+
f, err := parser.ParseFile(fset, relFile, src, parser.SkipObjectResolution)
80+
if err != nil {
81+
log.Printf("[go-extractor] error parsing test file %s: %v", relFile, err)
82+
return nil
83+
}
84+
85+
base := resolveCtx{
86+
pkgDir: filepath.Dir(relFile),
87+
modulePath: modulePath,
88+
// pkgNames is deliberately nil. It exists to recover a declared package name
89+
// that differs from its directory base ("go-auth" → package auth), which
90+
// needs a view of every parsed package — and this pass sees only test files.
91+
// Worse, a test file's own package name carries a _test suffix
92+
// (`package svc_test`), so feeding these in would alias the import under
93+
// test as "svc_test" and break the very idiom this exists to resolve.
94+
// buildFileImports then falls back to the import path's base, exactly as the
95+
// production pass does for any package it did not parse.
96+
imports: buildFileImports(f, modulePath, nil),
97+
}
98+
99+
seen := make(map[string]bool)
100+
var rels []facts.Relation
101+
add := func(target string) {
102+
if target == "" || seen[target] {
103+
return
104+
}
105+
seen[target] = true
106+
rels = append(rels, facts.Relation{Kind: facts.RelCalls, Target: target})
107+
}
108+
collectCalls := func(n ast.Node, ctx resolveCtx) {
109+
ast.Inspect(n, func(node ast.Node) bool {
110+
if call, ok := node.(*ast.CallExpr); ok {
111+
if chain := flattenSelector(call.Fun); chain != nil {
112+
add(resolveChain(chain, ctx))
113+
}
114+
}
115+
return true
116+
})
117+
}
118+
119+
for _, decl := range f.Decls {
120+
switch d := decl.(type) {
121+
case *ast.FuncDecl:
122+
if d.Body == nil {
123+
continue
124+
}
125+
ctx := base
126+
if d.Recv != nil && len(d.Recv.List) > 0 {
127+
field := d.Recv.List[0]
128+
ctx.recvType = typeExprToString(field.Type)
129+
if len(field.Names) > 0 {
130+
ctx.recvVar = field.Names[0].Name
131+
}
132+
}
133+
ctx.localTypes = collectLocalTypes(d.Body, ctx)
134+
collectCalls(d.Body, ctx)
135+
case *ast.GenDecl:
136+
// File-scope initializers (`var _ = Register(handler)`) reference
137+
// production code with no enclosing function to attribute them to.
138+
for _, spec := range d.Specs {
139+
vs, ok := spec.(*ast.ValueSpec)
140+
if !ok {
141+
continue
142+
}
143+
for _, v := range vs.Values {
144+
collectCalls(v, base)
145+
}
146+
}
147+
}
148+
}
149+
150+
if len(rels) == 0 {
151+
return nil
152+
}
153+
return []facts.Fact{{
154+
Kind: facts.KindTestRef,
155+
Name: relFile,
156+
File: relFile,
157+
Props: map[string]any{"language": "go"},
158+
Relations: rels,
159+
}}
160+
}

0 commit comments

Comments
 (0)