From 85b09cc66d72b1ee2122c96e760b8ad52818aeab Mon Sep 17 00:00:00 2001 From: Noah S-C Date: Tue, 19 Dec 2023 14:46:33 +0000 Subject: [PATCH] bazel: add sgexit lint to nogo (#59092) Closes https://github.com/sourcegraph/sourcegraph/issues/54847 ## Test plan Sample deliberate error: ``` $ bazel build //dev/sg INFO: Analyzed target //dev/sg:sg (0 packages loaded, 0 targets configured). INFO: Found 1 target... ERROR: /home/noah/Sourcegraph/sourcegraph/dev/sg/BUILD.bazel:128:10: GoCompilePkg dev/sg/sg.a failed: (Exit 1): builder failed: error executing command (from target //dev/sg:sg) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src dev/sg/analytics.go -src dev/sg/checks.go -src ... (remaining 245 arguments skipped) Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging compilepkg: nogo: errors found by nogo during build-time code analysis: dev/sg/sg_run.go:28:4: use of `signal.Notify` forbidden because "breaks dev/sg/internal/interrupt" (forbidigo) Aspect @rules_rust//rust/private:clippy.bzl%rust_clippy_aspect of //dev/sg:sg up-to-date (nothing to build) INFO: Elapsed time: 52.557s, Critical Path: 26.79s INFO: 1435 processes: 2 internal, 1433 linux-sandbox. FAILED: Build did NOT complete successfully ``` --- dev/linters/forbidigo/BUILD.bazel | 1 + dev/linters/forbidigo/forbidigo.go | 78 ++++++++++++++++++------------ dev/linters/go.mod | 2 +- dev/sg/linters/linters.go | 2 +- 4 files changed, 51 insertions(+), 32 deletions(-) diff --git a/dev/linters/forbidigo/BUILD.bazel b/dev/linters/forbidigo/BUILD.bazel index 4eca244d915..28906154b55 100644 --- a/dev/linters/forbidigo/BUILD.bazel +++ b/dev/linters/forbidigo/BUILD.bazel @@ -9,6 +9,7 @@ go_library( "//dev/linters/nolint", "@com_github_ashanbrown_forbidigo//forbidigo:go_default_library", #keep "@com_github_ashanbrown_forbidigo//pkg/analyzer:go_default_library", #keep + "@com_github_gobwas_glob//:glob", "@org_golang_x_tools//go/analysis", ], ) diff --git a/dev/linters/forbidigo/forbidigo.go b/dev/linters/forbidigo/forbidigo.go index b834d7dd0e3..8ff06307b65 100644 --- a/dev/linters/forbidigo/forbidigo.go +++ b/dev/linters/forbidigo/forbidigo.go @@ -3,9 +3,11 @@ package forbidigo import ( "fmt" "go/ast" + "strings" "github.com/ashanbrown/forbidigo/forbidigo" "github.com/ashanbrown/forbidigo/pkg/analyzer" + "github.com/gobwas/glob" "golang.org/x/tools/go/analysis" "github.com/sourcegraph/sourcegraph/dev/linters/nolint" @@ -14,11 +16,23 @@ import ( // Analyzer is the analyzer nogo should use var Analyzer = nolint.Wrap(analyzer.NewAnalyzer()) -// defaultPatterns the patterns forbigigo should ban if they match. +// patterns the patterns forbigigo should ban if they match. // JSON based off the pattern struct: // https://sourcegraph.com/github.com/ashanbrown/forbidigo@57bf5a72a4f5c3c28dce5a8aebb37a9be36bece7/-/blob/forbidigo/patterns.go?L13-29 -var defaultPatterns = []string{ - fmt.Sprintf(`{"p":"^fmt\\.Errorf$", "msg": "%s should be used instead"}`, "`errors.Newf`"), +var patterns = []struct { + Pattern string + ExcludeFile func(path string) bool +}{ + { + Pattern: fmt.Sprintf(`{"p": "^fmt\\.Errorf$", "msg": "%s should be used instead"}`, "`errors.Newf`"), + }, + { + Pattern: `{"p": "^(os\\.Exit|signal\\.Notify|logger\\.Fatal|log\\.Fatal)$", "msg": "it breaks dev/sg/internal/interrupt"}`, + ExcludeFile: func(path string) bool { + match := glob.MustCompile("**dev/sg/**.go", '/') + return !match.Match(path) || (strings.Contains(path, "dev/sg/interrupt") || strings.HasSuffix(path, "_test.go") || strings.HasSuffix(path, "dev/sg/linters/go_checks.go")) + }, + }, } var config = struct { @@ -39,34 +53,38 @@ func init() { // runAnalysis is copied from forbigigo and slightly modified func runAnalysis(pass *analysis.Pass) (interface{}, error) { - linter, err := forbidigo.NewLinter(defaultPatterns, - forbidigo.OptionIgnorePermitDirectives(config.IgnorePermitDirective), - forbidigo.OptionExcludeGodocExamples(config.ExcludeGodocExamples), - forbidigo.OptionAnalyzeTypes(config.AnalyzeTypes), - ) - if err != nil { - return nil, err - } - nodes := make([]ast.Node, 0, len(pass.Files)) - for _, f := range pass.Files { - nodes = append(nodes, f) - } - runConfig := forbidigo.RunConfig{Fset: pass.Fset} - if config.AnalyzeTypes { - runConfig.TypesInfo = pass.TypesInfo - } - issues, err := linter.RunWithConfig(runConfig, nodes...) - if err != nil { - return nil, err - } - - for _, i := range issues { - diag := analysis.Diagnostic{ - Pos: i.Pos(), - Message: i.Details(), - Category: "restriction", + for _, group := range patterns { + linter, err := forbidigo.NewLinter([]string{group.Pattern}, + forbidigo.OptionIgnorePermitDirectives(config.IgnorePermitDirective), + forbidigo.OptionExcludeGodocExamples(config.ExcludeGodocExamples), + forbidigo.OptionAnalyzeTypes(config.AnalyzeTypes), + ) + if err != nil { + return nil, err + } + + nodes := make([]ast.Node, 0, len(pass.Files)) + for _, f := range pass.Files { + if group.ExcludeFile != nil && group.ExcludeFile(pass.Fset.Position(f.Package).Filename) { + continue + } + nodes = append(nodes, f) + } + + runConfig := forbidigo.RunConfig{Fset: pass.Fset, TypesInfo: pass.TypesInfo} + issues, err := linter.RunWithConfig(runConfig, nodes...) + if err != nil { + return nil, err + } + + for _, i := range issues { + diag := analysis.Diagnostic{ + Pos: i.Pos(), + Message: i.Details(), + Category: "restriction", + } + pass.Report(diag) } - pass.Report(diag) } return nil, nil } diff --git a/dev/linters/go.mod b/dev/linters/go.mod index 60600a592af..7ef526798cf 100644 --- a/dev/linters/go.mod +++ b/dev/linters/go.mod @@ -7,6 +7,7 @@ require ( github.com/OpenPeeDeeP/depguard/v2 v2.0.1 github.com/ashanbrown/forbidigo v1.5.1 github.com/go-critic/go-critic v0.6.7 + github.com/gobwas/glob v0.2.3 github.com/gordonklaus/ineffassign v0.0.0-20230107090616-13ace0543b28 github.com/grafana/regexp v0.0.0-20221123153739-15dc172cd2db github.com/kyoh86/exportloopref v0.1.11 @@ -24,7 +25,6 @@ require ( github.com/cockroachdb/redact v1.1.5 // indirect github.com/getsentry/sentry-go v0.25.0 // indirect github.com/go-toolsmith/astfmt v1.1.0 // indirect - github.com/gobwas/glob v0.2.3 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/google/go-cmp v0.6.0 // indirect github.com/gostaticanalysis/analysisutil v0.7.1 // indirect diff --git a/dev/sg/linters/linters.go b/dev/sg/linters/linters.go index 50cc067aa61..1ace95c29c3 100644 --- a/dev/sg/linters/linters.go +++ b/dev/sg/linters/linters.go @@ -43,7 +43,7 @@ var Targets = []Target{ lintLoggingLibraries(), onlyLocal(lintTracingLibraries()), goModGuards(), - lintSGExit(), + onlyLocal(lintSGExit()), }, }, {