From db96b153e1c363814d5a4423f1285a25c3611548 Mon Sep 17 00:00:00 2001 From: Harsh Modi Date: Sat, 20 Jan 2024 12:07:48 +0000 Subject: [PATCH] nogo: Allow not running analyzers for packages. 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 #3838 --- go/tools/builders/generate_nogo_main.go | 40 +++++++++++++- go/tools/builders/nogo_main.go | 52 +++++++++++++++++- tests/core/nogo/custom/custom_test.go | 71 ++++++++++++++++++++++++- 3 files changed, 159 insertions(+), 4 deletions(-) diff --git a/go/tools/builders/generate_nogo_main.go b/go/tools/builders/generate_nogo_main.go index 872b9b0a6..4db6d810f 100644 --- a/go/tools/builders/generate_nogo_main.go +++ b/go/tools/builders/generate_nogo_main.go @@ -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}} } @@ -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) } @@ -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 } @@ -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 @@ -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"` } diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index 23acdef0e..1902a392c 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -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) } @@ -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. @@ -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 diff --git a/tests/core/nogo/custom/custom_test.go b/tests/core/nogo/custom/custom_test.go index 276244311..b3ae45f15 100644 --- a/tests/core/nogo/custom/custom_test.go +++ b/tests/core/nogo/custom/custom_test.go @@ -39,6 +39,7 @@ nogo( ":foofuncname", ":importfmt", ":visibility", + ":exitanalyzer", ], # config = "", visibility = ["//visibility:public"], @@ -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"], @@ -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": { @@ -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 @@ -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", @@ -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 {