Skip to content

Commit

Permalink
nogo: Allow not running analyzers for packages.
Browse files Browse the repository at this point in the history
Currently, nogo is run unconditionally for all go code. The exclude/include escape hatches only ignore diagnostic errors from running analyzers, however, the analyzers are still run.

This is problematic because some external code like AWS's EC2 go client are huge and take a lot of memory to compile. This causes our build to OOM. It would be nice to have a mechanism to not run an analyzer at all.

This would allow builds to run must faster since you aren't linting external code that you don't care to lint.

This PR adds two new fields to the nogo config: "exclude_pkg" and
"only_pkg". These semantics are the same as "only_file" and
"exclude_file"

Fixes bazel-contrib#3838
  • Loading branch information
DolceTriade committed Jan 28, 2024
1 parent 30099a6 commit db96b15
Show file tree
Hide file tree
Showing 3 changed files with 159 additions and 4 deletions.
40 changes: 38 additions & 2 deletions go/tools/builders/generate_nogo_main.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,26 @@ var configs = map[string]config{
{{- end}}
},
{{- end}}
{{- if $config.OnlyPkgs}}
onlyPkgs: []*regexp.Regexp{
{{- range $path, $comment := $config.OnlyPkgs}}
{{- if $comment}}
// {{$comment}}
{{end -}}
{{printf "regexp.MustCompile(%q)" $path}},
{{- end}}
},
{{- end -}}
{{- if $config.ExcludePkgs}}
excludePkgs: []*regexp.Regexp{
{{- range $path, $comment := $config.ExcludePkgs}}
{{- if $comment}}
// {{$comment}}
{{end -}}
{{printf "regexp.MustCompile(%q)" $path}},
{{- end}}
},
{{- end}}
},
{{- end}}
}
Expand Down Expand Up @@ -125,7 +145,8 @@ func genNogoMain(args []string) error {
for _, path := range analyzerImportPaths {
imports = append(imports, Import{
Path: path,
Name: "analyzer" + strconv.Itoa(suffix)})
Name: "analyzer" + strconv.Itoa(suffix),
})
if suffix == math.MaxInt32 {
return fmt.Errorf("cannot generate more than %d analyzers", suffix)
}
Expand All @@ -140,7 +161,7 @@ func genNogoMain(args []string) error {
Configs: config,
}
for _, c := range config {
if len(c.OnlyFiles) > 0 || len(c.ExcludeFiles) > 0 {
if len(c.OnlyFiles) > 0 || len(c.ExcludeFiles) > 0 || len(c.OnlyPkgs) > 0 || len(c.ExcludePkgs) > 0 {
data.NeedRegexp = true
break
}
Expand Down Expand Up @@ -176,11 +197,24 @@ func buildConfig(path string) (Configs, error) {
return Configs{}, fmt.Errorf("invalid pattern for analysis %q: %v", name, err)
}
}
for pattern := range config.OnlyPkgs {
if _, err := regexp.Compile(pattern); err != nil {
return Configs{}, fmt.Errorf("invalid pattern for analysis %q: %v", name, err)
}
}
for pattern := range config.ExcludePkgs {
if _, err := regexp.Compile(pattern); err != nil {
return Configs{}, fmt.Errorf("invalid pattern for analysis %q: %v", name, err)
}
}

configs[name] = Config{
// Description is currently unused.
OnlyFiles: config.OnlyFiles,
ExcludeFiles: config.ExcludeFiles,
AnalyzerFlags: config.AnalyzerFlags,
OnlyPkgs: config.OnlyPkgs,
ExcludePkgs: config.ExcludePkgs,
}
}
return configs, nil
Expand All @@ -193,4 +227,6 @@ type Config struct {
OnlyFiles map[string]string `json:"only_files"`
ExcludeFiles map[string]string `json:"exclude_files"`
AnalyzerFlags map[string]string `json:"analyzer_flags"`
OnlyPkgs map[string]string `json:"only_pkgs"`
ExcludePkgs map[string]string `json:"exclude_pkgs"`
}
52 changes: 51 additions & 1 deletion go/tools/builders/nogo_main.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ var typesSizes = types.SizesFor("gc", os.Getenv("GOARCH"))
func main() {
log.SetFlags(0) // no timestamp
log.SetPrefix("nogo: ")
log.SetOutput(os.Stderr)
if err := run(os.Args[1:]); err != nil {
log.Fatal(err)
}
Expand Down Expand Up @@ -189,7 +190,48 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil
}
}
}
roots = append(roots, visit(a))
var currentConfig config
// Use the base config if it exists.
if baseConfig, ok := configs[nogoBaseConfigName]; ok {
currentConfig = baseConfig
}
// Overwrite the config with the desired config. Any unset fields
// in the config will default to the base config.
if actionConfig, ok := configs[a.Name]; ok {
if actionConfig.onlyPkgs != nil {
currentConfig.onlyPkgs = actionConfig.onlyPkgs
}
if actionConfig.excludePkgs != nil {
currentConfig.excludePkgs = actionConfig.excludePkgs
}
}

include := true
if len(currentConfig.onlyPkgs) > 0 {
// Run this analyzer for *only* matching packages.
include = false
for _, pattern := range currentConfig.onlyPkgs {
if pattern.MatchString(packagePath) {
include = true
break
}
}
}
if include {
for _, pattern := range currentConfig.excludePkgs {
if pattern.MatchString(packagePath) {
include = false
break
}
}
}
if include {
roots = append(roots, visit(a))
}
}

if len(roots) == 0 {
return "", nil, nil
}

// Load the package, including AST, types, and facts.
Expand Down Expand Up @@ -548,6 +590,14 @@ type config struct {
// to Analyzer.Flags. Note that no leading '-' should be present in a flag
// name
analyzerFlags map[string]string

// onlyPkgs is a list of regular expressions that match packages an analyzer
// will emit run for. When empty, the analyzer will run on all packages.
onlyPkgs []*regexp.Regexp

// excludePkgs is a list of regular expressions that match packages that an
// analyzer will not run on.
excludePkgs []*regexp.Regexp
}

// importer is an implementation of go/types.Importer that imports type
Expand Down
71 changes: 70 additions & 1 deletion tests/core/nogo/custom/custom_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ nogo(
":foofuncname",
":importfmt",
":visibility",
":exitanalyzer",
],
# config = "",
visibility = ["//visibility:public"],
Expand All @@ -60,6 +61,14 @@ go_library(
visibility = ["//visibility:public"],
)
go_library(
name = "exitanalyzer",
srcs = ["exitanalyzer.go"],
importpath = "exitanalyzer",
deps = ["@org_golang_x_tools//go/analysis"],
visibility = ["//visibility:public"],
)
go_library(
name = "visibility",
srcs = ["visibility.go"],
Expand Down Expand Up @@ -280,6 +289,35 @@ func run(pass *analysis.Pass) (interface{}, error) {
return nil, nil
}
-- exitanalyzer.go --
// exitanalyzer exits uncleanly when run
package exitanalyzer
import (
"os"
"flag"
"golang.org/x/tools/go/analysis"
)
const doc = "exitanalyzer exits uncleanly when run."
var fs = *flag.NewFlagSet("exitanalyzer", flag.ContinueOnError)
var exit = fs.Bool("exit", false, "exit uncleanly")
var Analyzer = &analysis.Analyzer{
Name: "exitanalyzer",
Run: run,
Doc: doc,
Flags: fs,
}
func run(pass *analysis.Pass) (interface{}, error) {
if *exit {
os.Exit(-1)
}
return nil, nil
}
-- config.json --
{
"importfmt": {
Expand Down Expand Up @@ -315,6 +353,27 @@ func run(pass *analysis.Pass) (interface{}, error) {
}
}
-- exclude_config.json --
{
"exitanalyzer": {
"analyzer_flags": {
"exit": "true"
},
"exclude_pkgs": {
".*": "don't run at all"
}
}
}
-- exit_config.json --
{
"exitanalyzer": {
"analyzer_flags": {
"exit": "true"
}
}
}
-- has_errors.go --
package haserrors
Expand Down Expand Up @@ -457,6 +516,16 @@ func Test(t *testing.T) {
// note the cross platform regex :)
`.*[\\/]cgo[\\/]examplepkg[\\/]pure_src_with_err_calling_native.go:.*function must not be named Foo \(foofuncname\)`,
},
}, {
desc: "do_not_run_analyzer",
config: "exclude_config.json",
target: "//:no_errors",
wantSuccess: true,
}, {
desc: "run_and_exit",
config: "exit_config.json",
target: "//:no_errors",
wantSuccess: false,
}, {
desc: "no_errors",
target: "//:no_errors",
Expand All @@ -479,7 +548,7 @@ func Test(t *testing.T) {
if err := cmd.Run(); err == nil && !test.wantSuccess {
t.Fatal("unexpected success")
} else if err != nil && test.wantSuccess {
t.Fatalf("unexpected error: %v", err)
t.Fatalf("unexpected error: %v: %v", err, stderr.String())
}

for _, pattern := range test.includes {
Expand Down

0 comments on commit db96b15

Please sign in to comment.