diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 60f9bedd0508..9bb60358961f 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -2026,6 +2026,26 @@ func TestLint(t *testing.T) { "redact.Sprintf", }, ",") + nakedGoroutineExceptions := `(` + strings.Join([]string{ + `pkg/.*_test.go`, + `pkg/workload/`, + `pkg/cli/systembench/`, + `pkg/cmd/roachprod/`, + `pkg/cmd/roachtest/`, + `pkg/cmd/roachprod-stress/`, + `pkg/cmd/urlcheck/`, + `pkg/acceptance/`, + `pkg/cli/syncbench/`, + `pkg/workload/`, + `pkg/cmd/cmp-protocol/`, + `pkg/cmd/cr2pg/`, + `pkg/cmd/smithtest/`, + `pkg/cmd/reduce/`, + `pkg/cmd/zerosum/`, + `pkg/cmd/allocsim/`, + `pkg/testutils/`, + }, ")|(") + `)` + filters := []stream.Filter{ // Ignore generated files. stream.GrepNot(`pkg/.*\.pb\.go:`), @@ -2074,6 +2094,11 @@ func TestLint(t *testing.T) { stream.GrepNot(`pkg/cmd/roachtest/log\.go:.*format argument is not a constant expression`), // We purposefully produce nil dereferences in this file to test crash conditions stream.GrepNot(`pkg/util/log/logcrash/crash_reporting_test\.go:.*nil dereference in type assertion`), + // Spawning naked goroutines is ok when it's not as part of the main CRDB + // binary. This is for now - if we use #58164 to introduce more aggressive + // pooling, etc, then test code needs to adhere as well. + stream.GrepNot(nakedGoroutineExceptions + `:.*Use of go keyword not allowed`), + stream.GrepNot(nakedGoroutineExceptions + `:.*Illegal call to Group\.Go\(\)`), } const vetTool = "roachvet" diff --git a/pkg/testutils/lint/passes/forbiddenmethod/BUILD.bazel b/pkg/testutils/lint/passes/forbiddenmethod/BUILD.bazel index 9521801f7788..a665457c4d91 100644 --- a/pkg/testutils/lint/passes/forbiddenmethod/BUILD.bazel +++ b/pkg/testutils/lint/passes/forbiddenmethod/BUILD.bazel @@ -11,6 +11,7 @@ go_library( srcs = [ "analyzers.go", "forbiddenmethod.go", + "naked_go.go", ], importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod", visibility = ["//visibility:public"], @@ -25,7 +26,11 @@ go_library( go_test( name = "forbiddenmethod_test", size = "small", - srcs = ["descriptormarshal_test.go"], + srcs = [ + "descriptormarshal_test.go", + "main_test.go", + "naked_go_test.go", + ], data = [ ":testdata", "@go_sdk//:files", diff --git a/pkg/testutils/lint/passes/forbiddenmethod/analyzers.go b/pkg/testutils/lint/passes/forbiddenmethod/analyzers.go index 7fef0b5e0e3f..efa2b6c686ea 100644 --- a/pkg/testutils/lint/passes/forbiddenmethod/analyzers.go +++ b/pkg/testutils/lint/passes/forbiddenmethod/analyzers.go @@ -43,6 +43,15 @@ var grpcStatusWithDetailsOptions = Options{ Hint: "does not work with gogoproto-generated Protobufs.", } +var errGroupGo = Options{ + PassName: "errgroupgo", + Doc: `prevent calls to (*errgroup.Group).Go`, + Package: "golang.org/x/sync/errgroup", + Type: "Group", + Method: "Go", + Hint: "Use (*ctxgroup.Group).Go instead", +} + // DescriptorMarshalAnalyzer checks for correct unmarshaling of descpb // descriptors by disallowing calls to (descpb.Descriptor).GetTable() et al. var DescriptorMarshalAnalyzer = Analyzer(descriptorMarshalOptions) @@ -52,6 +61,10 @@ var DescriptorMarshalAnalyzer = Analyzer(descriptorMarshalOptions) // Errant calls to Close() disrupt the connection for all users. var GRPCClientConnCloseAnalyzer = Analyzer(grpcClientConnCloseOptions) +// ErrGroupGoAnalyzer checks for calls to (*errgroup.Group).Go. We should use +// (*ctxgroup.Group).Go instead. +var ErrGroupGoAnalyzer = Analyzer(errGroupGo) + // GRPCStatusWithDetailsAnalyzer checks that we don't use Status.WithDetails(), // since it does not support Protobufs generated by gogoproto. This is // because it uses an Any field to store details, with a reference to the @@ -69,4 +82,12 @@ var Analyzers = []*analysis.Analyzer{ DescriptorMarshalAnalyzer, GRPCClientConnCloseAnalyzer, GRPCStatusWithDetailsAnalyzer, + // TODO(tbg): fix the lint infractions this finds and + // enable. + // + // ErrGroupGoAnalyzer, + // NakedGoAnalyzer, } + +var _ = ErrGroupGoAnalyzer +var _ = NakedGoAnalyzer diff --git a/pkg/testutils/lint/passes/forbiddenmethod/descriptormarshal_test.go b/pkg/testutils/lint/passes/forbiddenmethod/descriptormarshal_test.go index 7dae7686f824..5988254bbab6 100644 --- a/pkg/testutils/lint/passes/forbiddenmethod/descriptormarshal_test.go +++ b/pkg/testutils/lint/passes/forbiddenmethod/descriptormarshal_test.go @@ -13,20 +13,13 @@ package forbiddenmethod_test import ( "testing" - "github.com/cockroachdb/cockroach/pkg/build/bazel" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "golang.org/x/tools/go/analysis/analysistest" ) -func init() { - if bazel.BuiltWithBazel() { - bazel.SetGoEnv() - } -} - -func Test(t *testing.T) { +func TestForbiddenMethod(t *testing.T) { skip.UnderStress(t) testdata := testutils.TestDataPath(t) analysistest.TestData = func() string { return testdata } diff --git a/pkg/testutils/lint/passes/forbiddenmethod/main_test.go b/pkg/testutils/lint/passes/forbiddenmethod/main_test.go new file mode 100644 index 000000000000..59538e6ad03e --- /dev/null +++ b/pkg/testutils/lint/passes/forbiddenmethod/main_test.go @@ -0,0 +1,19 @@ +// Copyright 2019 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package forbiddenmethod_test + +import "github.com/cockroachdb/cockroach/pkg/build/bazel" + +func init() { + if bazel.BuiltWithBazel() { + bazel.SetGoEnv() + } +} diff --git a/pkg/testutils/lint/passes/forbiddenmethod/naked_go.go b/pkg/testutils/lint/passes/forbiddenmethod/naked_go.go new file mode 100644 index 000000000000..6dac73fe3d59 --- /dev/null +++ b/pkg/testutils/lint/passes/forbiddenmethod/naked_go.go @@ -0,0 +1,92 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package forbiddenmethod + +import ( + "fmt" + "go/ast" + "strings" + + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/passesutil" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +const nakedGoPassName = "nakedgo" + +// NakedGoAnalyzer prevents use of the `go` keyword. +var NakedGoAnalyzer = &analysis.Analyzer{ + Name: nakedGoPassName, + Doc: "Prevents direct use of the 'go' keyword. Goroutines should be launched through Stopper.", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: func(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + inspect.Preorder([]ast.Node{ + (*ast.GoStmt)(nil), + }, func(n ast.Node) { + node := n.(*ast.GoStmt) + + const debug = false + + // NB: we're not using passesutil.HasNoLintComment because it + // has false negatives (i.e. comments apply to infractions that + // they were clearly not intended for). + // + // The approach here is inspired by `analysistest.check` - the + // nolint comment has to be on the same line as the *end* of the + // `*GoStmt`. + f := passesutil.FindContainingFile(pass, n) + cm := ast.NewCommentMap(pass.Fset, node, f.Comments) + var cc *ast.Comment + for _, cg := range cm[n] { + for _, c := range cg.List { + if c.Pos() < node.Go { + // The current comment is "before" the `go` invocation, so it's + // not relevant for silencing the lint. + continue + } + if cc == nil || cc.Pos() > node.Go { + // This comment is after, but closer to the `go` invocation than + // previous candidate. + cc = c + if debug { + fmt.Printf("closest comment now %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text) + } + } + } + } + if cc != nil && strings.Contains(cc.Text, "nolint:"+nakedGoPassName) { + if debug { + fmt.Printf("GoStmt at: %d-%d\n", n.Pos(), n.End()) + fmt.Printf("GoStmt.Go at: %d\n", node.Go) + fmt.Printf("GoStmt.Call at: %d-%d\n", node.Call.Pos(), node.Call.End()) + } + + goPos := pass.Fset.Position(node.End()) + cmPos := pass.Fset.Position(cc.Pos()) + + if goPos.Line == cmPos.Line { + if debug { + fmt.Printf("suppressing lint because of %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text) + } + return + } + } + + pass.Report(analysis.Diagnostic{ + Pos: n.Pos(), + Message: "Use of go keyword not allowed, use a Stopper instead", + }) + }) + return nil, nil + }, +} diff --git a/pkg/testutils/lint/passes/forbiddenmethod/naked_go_test.go b/pkg/testutils/lint/passes/forbiddenmethod/naked_go_test.go new file mode 100644 index 000000000000..cacbc0fac719 --- /dev/null +++ b/pkg/testutils/lint/passes/forbiddenmethod/naked_go_test.go @@ -0,0 +1,25 @@ +// Copyright 2019 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package forbiddenmethod_test + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod" + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestNakedGo(t *testing.T) { + testdata := testutils.TestDataPath(t) + analysistest.TestData = func() string { return testdata } + analysistest.Run(t, testdata, forbiddenmethod.NakedGoAnalyzer, "nakedgotest") +} diff --git a/pkg/testutils/lint/passes/forbiddenmethod/testdata/src/nakedgotest/foo.go b/pkg/testutils/lint/passes/forbiddenmethod/testdata/src/nakedgotest/foo.go new file mode 100644 index 000000000000..548e1f01a196 --- /dev/null +++ b/pkg/testutils/lint/passes/forbiddenmethod/testdata/src/nakedgotest/foo.go @@ -0,0 +1,48 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package nakedgotest + +func toot() { + // The nolint comment below should have no effect. + // For some reason though it shows up in the CommentMap + // for the *ast.GoStmt, though. No idea why. + + //nolint:nakedgo should not help anyone +} + +func A() { + //nolint: I'm a noop + go func() {}() // want `Use of go keyword not allowed, use a Stopper instead` + go toot() // want `Use of go keyword not allowed, use a Stopper instead` + go /* nolint: nakedgo not helping */ toot() // want `Use of go keyword not allowed, use a Stopper instead` + /* nolint: nakedgo nope */ go toot() // want `Use of go keyword not allowed, use a Stopper instead` + //nolint:nakedgo nope, this one neither + + go func() {}() // want `Use of go keyword not allowed, use a Stopper instead` + + go func() {}() //nolint:nakedgo + + go toot() //nolint:nakedgo + + go func() { /* want `Use of go keyword not allowed, use a Stopper instead` */ //nolint:nakedgo + _ = 0 + }() + + go func() { // want `Use of go keyword not allowed, use a Stopper instead` + _ = 0 //nolint:nakedgo + }() + + // Finally, doing it right! + + go func() { + _ = 0 + }() //nolint:nakedgo +}