lint: Detect missing TestMain in _test packages

With 3adbbfb5a (lint: Add linter for testhelper.Run, 2023-08-24) we
added a linter to detect if a package did not contain a `TestMain`
function. This was limited to checking non-test packages as each
analysis pass is self-contained and does not allow us to
cross-reference another package.

To allow us to reliably detect when a primary package does not have
tests, but its `_test` package does, start exporting a
`testhelperFact` that indicates if a package has a `TestMain` function.
When analyzing a `_test` package, we can import the fact of its related
package and prevent false positives.

6 files changed, 85 insertions, 13 deletions

diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox.go
new file mode 100644
index 000000000..42818a757
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox.go
@@ -0,0 +1,5 @@
+package testhelper_run_blackbox
+
+func Foo() string {
+	return "bar"
+}
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox_test.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox_test.go
new file mode 100644
index 000000000..d1cb4dd1c
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox_test.go
@@ -0,0 +1,12 @@
+package testhelper_run_blackbox_test // want "no TestMain in package testhelper_run_blackbox_test"
+
+import (
+	"testhelper_run_blackbox"
+	"testing"
+)
+
+func TestFoo(t *testing.T) {
+	if testhelper_run_blackbox.Foo() != "bar" {
+		t.FailNow()
+	}
+}
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go
index 94eb334f0..932b0cf75 100644
--- a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go
@@ -1,4 +1,4 @@
-package testhelper_run_no_exec_testmain
+package testhelper_run_no_exec_testmain // want package:"package has TestMain"
 
 import (
 	"testing"
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go
index 28afec330..688ec1a36 100644
--- a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go
@@ -1,4 +1,4 @@
-package testhelper_run_not_testhelper
+package testhelper_run_not_testhelper // want package:"package has TestMain"
 
 import "testing"
 
diff --git a/tools/golangci-lint/gitaly/testhelper_run.go b/tools/golangci-lint/gitaly/testhelper_run.go
index aa9226c02..116ab4162 100644
--- a/tools/golangci-lint/gitaly/testhelper_run.go
+++ b/tools/golangci-lint/gitaly/testhelper_run.go
@@ -17,6 +17,17 @@ type testhelperRunAnalyzerSettings struct {
 	IncludedFunctions []string `mapstructure:"included-functions"`
 }
 
+// testmainFact is used to report if a package has defined a `TestMain` function.
+type testmainFact struct {
+	HasTestMain bool
+}
+
+// AFact is used to satisfy the `Fact` interface.
+func (*testmainFact) AFact() {}
+
+// String returns the message expected by tests when a testmainFact is exported.
+func (*testmainFact) String() string { return "package has TestMain" }
+
 var toolPrefixPattern = regexp.MustCompile(`^gitlab.com/gitlab-org/gitaly(/v\d{2})?/tools`)
 
 // newTesthelperRunAnalyzer returns an analyzer to detect if a package that has tests does
@@ -25,23 +36,17 @@ var toolPrefixPattern = regexp.MustCompile(`^gitlab.com/gitlab-org/gitaly(/v\d{2
 // https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md?ref_type=heads#common-setup
 func newTesthelperRunAnalyzer(settings *testhelperRunAnalyzerSettings) *analysis.Analyzer {
 	return &analysis.Analyzer{
-		Name: testhelperRunAnalyzerName,
-		Doc:  `TestMain must be present and call testhelper.Run()`,
-		Run:  runTesthelperRunAnalyzer(settings.IncludedFunctions),
+		Name:      testhelperRunAnalyzerName,
+		Doc:       `TestMain must be present and call testhelper.Run()`,
+		Run:       runTesthelperRunAnalyzer(settings.IncludedFunctions),
+		FactTypes: []analysis.Fact{&testmainFact{}},
 	}
 }
 
 func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{}, error) {
 	return func(pass *analysis.Pass) (interface{}, error) {
 		var hasTestMain, hasTests bool
-
-		// Blackbox test packages ending with `_test` are considered to be
-		// part of the primary package for compilation, but are scanned in a
-		// separate pass by the analyzer. The primary and test packages cannot
-		// both define `TestMain`. Only require `TestMain` in the primary package.
-		if strings.HasSuffix(pass.Pkg.Name(), "_test") {
-			return nil, nil
-		}
+		var fact testmainFact
 
 		// Don't lint tools, they can't import `testhelper`.
 		if toolPrefixPattern.MatchString(pass.Pkg.Path()) {
@@ -53,12 +58,23 @@ func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{},
 				break
 			}
 
+			// Blackbox test packages ending with `_test` are considered to be
+			// part of the primary package for compilation, but are scanned in a
+			// separate pass by the analyzer. The primary and test packages cannot
+			// both define `TestMain`.
+			if isTestPkg(pass) && primaryPkgHasTestMain(pass, file) {
+				// Primary package has already defined `TestMain`, no need to check
+				// `_test` package.
+				break
+			}
+
 			ast.Inspect(file, func(node ast.Node) bool {
 				if decl, ok := node.(*ast.FuncDecl); ok {
 					declName := decl.Name.Name
 
 					if declName == "TestMain" {
 						hasTestMain = true
+						fact.HasTestMain = true
 
 						analyzeTestMain(pass, decl, rules)
 						analyzeFilename(pass, file, decl)
@@ -94,6 +110,9 @@ func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{},
 			})
 		}
 
+		if hasTestMain {
+			pass.ExportPackageFact(&fact)
+		}
 		return nil, nil
 	}
 }
@@ -134,3 +153,38 @@ func analyzeTestMain(pass *analysis.Pass, decl *ast.FuncDecl, rules []string) {
 		})
 	}
 }
+
+func primaryPkgHasTestMain(pass *analysis.Pass, file *ast.File) bool {
+	var primaryPkg *types.Package
+
+	ast.Inspect(file, func(node ast.Node) bool {
+		if spec, ok := node.(*ast.ImportSpec); ok {
+			obj, ok := pass.TypesInfo.Implicits[spec]
+			if !ok {
+				obj = pass.TypesInfo.Defs[spec.Name]
+			}
+			importedPkg := obj.(*types.PkgName).Imported()
+
+			if importedPkg.Path() == strings.TrimSuffix(pass.Pkg.Path(), "_test") {
+				primaryPkg = importedPkg
+			}
+		}
+
+		return true
+	})
+
+	if primaryPkg == nil {
+		return false
+	}
+
+	var primaryFact testmainFact
+	if !pass.ImportPackageFact(primaryPkg, &primaryFact) {
+		return false
+	}
+
+	return primaryFact.HasTestMain
+}
+
+func isTestPkg(pass *analysis.Pass) bool {
+	return strings.HasSuffix(pass.Pkg.Name(), "_test")
+}
diff --git a/tools/golangci-lint/gitaly/testhelper_run_test.go b/tools/golangci-lint/gitaly/testhelper_run_test.go
index e557d5287..7b13d5aa2 100644
--- a/tools/golangci-lint/gitaly/testhelper_run_test.go
+++ b/tools/golangci-lint/gitaly/testhelper_run_test.go
@@ -26,5 +26,6 @@ func TestTesthelperRun(t *testing.T) {
 		"testhelper_run_no_testmain",
 		"testhelper_run_no_exec_testmain",
 		"testhelper_run_not_testhelper",
+		"testhelper_run_blackbox",
 	)
 }