Skip to content

Commit 7743555

Browse files
herkolateganherkolategan
committed
testutils: add roachtest analyzer
Previously, there was no linter that would detect use of the `go` keyword in roachtest. All roachtest goroutines must be launched through the `Task` interface provided by the roachtest framework. This change reuses a linter from the `lint passes forbiddenmethod` package to detect use of the `go` keyword in roachtest. The roachtest go analyzer only operates on the roachtest package, with a filter hardcoded in the analyzer. Thus we can still add excludes in the nogo config to allow goroutines in other packages. Epic: None Release note: None
1 parent 18e2dc8 commit 7743555

File tree

6 files changed

+114
-56
lines changed

6 files changed

+114
-56
lines changed

‎BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,5 +208,6 @@ nogo(
208208
"//pkg/testutils/lint/passes/returnerrcheck",
209209
"//pkg/testutils/lint/passes/shadow",
210210
"//pkg/testutils/lint/passes/unconvert",
211+
"//pkg/testutils/lint/passes/roachtest",
211212
],
212213
)

‎build/bazelutil/nogo_config.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@
1515
"pkg/.*_generated\\.go$": "generated code"
1616
}
1717
},
18+
"roachtestgo": {
19+
"exclude_files": {
20+
"pkg/cmd/roachtest/roachtestutil/task": "task manager implements goroutine management",
21+
"pkg/cmd/roachtest/roachtestutil/mixedversion": "part of the roachtest framework",
22+
"pkg/cmd/roachtest/operations": "operations do not run under the managed roachtest framework",
23+
"pkg/cmd/roachtest/[^/]+\\.go$": "roachtest framework",
24+
"_test\\.go$": "tests"
25+
}
26+
},
1827
"deepequalerrors": {
1928
"exclude_files": {
2029
"external/": "exclude all third-party code for all analyzers",

‎pkg/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2454,6 +2454,7 @@ GO_TARGETS = [
24542454
"//pkg/testutils/lint/passes/returncheck:returncheck",
24552455
"//pkg/testutils/lint/passes/returnerrcheck:returnerrcheck",
24562456
"//pkg/testutils/lint/passes/returnerrcheck:returnerrcheck_test",
2457+
"//pkg/testutils/lint/passes/roachtest:roachtest",
24572458
"//pkg/testutils/lint/passes/shadow:shadow",
24582459
"//pkg/testutils/lint/passes/staticcheck:staticcheck",
24592460
"//pkg/testutils/lint/passes/unconvert:unconvert",

‎pkg/testutils/lint/passes/forbiddenmethod/naked_go.go‎

Lines changed: 68 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -16,72 +16,84 @@ import (
1616
"golang.org/x/tools/go/ast/inspector"
1717
)
1818

19-
const nakedGoPassName = "nakedgo"
20-
2119
// NakedGoAnalyzer prevents use of the `go` keyword.
22-
var NakedGoAnalyzer = &analysis.Analyzer{
23-
Name: nakedGoPassName,
24-
Doc: "Prevents direct use of the 'go' keyword. Goroutines should be launched through Stopper.",
25-
Requires: []*analysis.Analyzer{inspect.Analyzer},
26-
Run: func(pass *analysis.Pass) (interface{}, error) {
27-
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
28-
inspect.Preorder([]ast.Node{
29-
(*ast.GoStmt)(nil),
30-
}, func(n ast.Node) {
31-
node := n.(*ast.GoStmt)
20+
var NakedGoAnalyzer = NewNakedGoAnalyzer(
21+
"nakedgo",
22+
"Use of go keyword not allowed, use a Stopper instead",
23+
"Prevents direct use of the 'go' keyword. Goroutines should be launched through Stopper.",
24+
nil,
25+
)
3226

33-
const debug = false
27+
type FilterFunc func(pass *analysis.Pass) bool
3428

35-
// NB: we're not using passesutil.HasNoLintComment because it
36-
// has false negatives (i.e. comments apply to infractions that
37-
// they were clearly not intended for).
38-
//
39-
// The approach here is inspired by `analysistest.check` - the
40-
// nolint comment has to be on the same line as the *end* of the
41-
// `*GoStmt`.
42-
f := passesutil.FindContainingFile(pass, n)
43-
cm := ast.NewCommentMap(pass.Fset, node, f.Comments)
44-
var cc *ast.Comment
45-
for _, cg := range cm[n] {
46-
for _, c := range cg.List {
47-
if c.Pos() < node.Go {
48-
// The current comment is "before" the `go` invocation, so it's
49-
// not relevant for silencing the lint.
50-
continue
51-
}
52-
if cc == nil || cc.Pos() > node.Go {
53-
// This comment is after, but closer to the `go` invocation than
54-
// previous candidate.
55-
cc = c
56-
if debug {
57-
fmt.Printf("closest comment now %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
29+
func NewNakedGoAnalyzer(name, message, doc string, filter FilterFunc) *analysis.Analyzer {
30+
return &analysis.Analyzer{
31+
Name: name,
32+
Doc: doc,
33+
Requires: []*analysis.Analyzer{inspect.Analyzer},
34+
Run: func(pass *analysis.Pass) (interface{}, error) {
35+
if filter != nil && !filter(pass) {
36+
return nil, nil
37+
}
38+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
39+
inspect.Preorder([]ast.Node{
40+
(*ast.GoStmt)(nil),
41+
}, func(n ast.Node) {
42+
node := n.(*ast.GoStmt)
43+
44+
const debug = false
45+
46+
// NB: we're not using passesutil.HasNoLintComment because it
47+
// has false negatives (i.e. comments apply to infractions that
48+
// they were clearly not intended for).
49+
//
50+
// The approach here is inspired by `analysistest.check` - the
51+
// nolint comment has to be on the same line as the *end* of the
52+
// `*GoStmt`.
53+
f := passesutil.FindContainingFile(pass, n)
54+
cm := ast.NewCommentMap(pass.Fset, node, f.Comments)
55+
var cc *ast.Comment
56+
for _, cg := range cm[n] {
57+
for _, c := range cg.List {
58+
if c.Pos() < node.Go {
59+
// The current comment is "before" the `go` invocation, so it's
60+
// not relevant for silencing the lint.
61+
continue
62+
}
63+
if cc == nil || cc.Pos() > node.Go {
64+
// This comment is after, but closer to the `go` invocation than
65+
// previous candidate.
66+
cc = c
67+
if debug {
68+
fmt.Printf("closest comment now %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
69+
}
5870
}
5971
}
6072
}
61-
}
62-
if cc != nil && strings.Contains(cc.Text, "nolint:"+nakedGoPassName) {
63-
if debug {
64-
fmt.Printf("GoStmt at: %d-%d\n", n.Pos(), n.End())
65-
fmt.Printf("GoStmt.Go at: %d\n", node.Go)
66-
fmt.Printf("GoStmt.Call at: %d-%d\n", node.Call.Pos(), node.Call.End())
67-
}
73+
if cc != nil && strings.Contains(cc.Text, "nolint:"+name) {
74+
if debug {
75+
fmt.Printf("GoStmt at: %d-%d\n", n.Pos(), n.End())
76+
fmt.Printf("GoStmt.Go at: %d\n", node.Go)
77+
fmt.Printf("GoStmt.Call at: %d-%d\n", node.Call.Pos(), node.Call.End())
78+
}
6879

69-
goPos := pass.Fset.Position(node.End())
70-
cmPos := pass.Fset.Position(cc.Pos())
80+
goPos := pass.Fset.Position(node.End())
81+
cmPos := pass.Fset.Position(cc.Pos())
7182

72-
if goPos.Line == cmPos.Line {
73-
if debug {
74-
fmt.Printf("suppressing lint because of %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
83+
if goPos.Line == cmPos.Line {
84+
if debug {
85+
fmt.Printf("suppressing lint because of %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
86+
}
87+
return
7588
}
76-
return
7789
}
78-
}
7990

80-
pass.Report(analysis.Diagnostic{
81-
Pos: n.Pos(),
82-
Message: "Use of go keyword not allowed, use a Stopper instead",
91+
pass.Report(analysis.Diagnostic{
92+
Pos: n.Pos(),
93+
Message: message,
94+
})
8395
})
84-
})
85-
return nil, nil
86-
},
96+
return nil, nil
97+
},
98+
}
8799
}

‎pkg/testutils/lint/passes/roachtest/BUILD.bazel‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
load("@io_bazel_rules_go//go:def.bzl", "go_library")
2+
3+
go_library(
4+
name = "roachtest",
5+
srcs = ["naked_go.go"],
6+
importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/roachtest",
7+
visibility = ["//visibility:public"],
8+
deps = [
9+
"//pkg/testutils/lint/passes/forbiddenmethod",
10+
"@org_golang_x_tools//go/analysis",
11+
],
12+
)

‎pkg/testutils/lint/passes/roachtest/naked_go.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright 2025 The Cockroach Authors.
2+
//
3+
// Use of this software is governed by the CockroachDB Software License
4+
// included in the /LICENSE file.
5+
6+
package roachtest
7+
8+
import (
9+
"strings"
10+
11+
"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod"
12+
"golang.org/x/tools/go/analysis"
13+
)
14+
15+
// RoachtestGoAnalyzer prevents use of the `go` keyword in roachtest.
16+
var Analyzer = forbiddenmethod.NewNakedGoAnalyzer(
17+
"roachtestgo",
18+
"Use of go keyword not allowed in roachtest, use the Task interface instead",
19+
"Prevents direct use of the 'go' keyword in roachtest. Goroutines should be launched through the Task interface supplied by the roachtest framework.",
20+
func(pass *analysis.Pass) bool {
21+
return strings.Contains(pass.Pkg.Path(), "cmd/roachtest")
22+
},
23+
)

0 commit comments

Comments
 (0)