From 5e2e09d7033ea83325238becf47818c7e54dd013 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 19:48:12 +0200 Subject: [PATCH] Only print type-checking error once in nogo (#4083) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** Previously, it was printed once by analyzer. **Which issues(s) does this PR fix?** Fixes #4080 **Other notes for review** --- go/tools/builders/nogo_main.go | 13 ++++++++--- tests/core/nogo/custom/custom_test.go | 31 +++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index e9aff5ef5..8a3871d3f 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -372,9 +372,7 @@ func (act *action) execOnce() { act.pass = pass var err error - if act.pkg.illTyped && !pass.Analyzer.RunDespiteErrors { - err = fmt.Errorf("analysis skipped due to type-checking error: %v", act.pkg.typeCheckError) - } else { + if !act.pkg.illTyped || pass.Analyzer.RunDespiteErrors { act.result, err = pass.Analyzer.Run(pass) if err == nil { if got, want := reflect.TypeOf(act.result), pass.Analyzer.ResultType; got != want { @@ -471,7 +469,13 @@ func checkAnalysisResults(actions []*action, pkg *goPackage) string { if cwd == "" || err != nil { errs = append(errs, fmt.Errorf("nogo failed to get CWD: %w", err)) } + numSkipped := 0 for _, act := range actions { + if act.pkg.illTyped && !act.a.RunDespiteErrors { + // Don't report type-checking errors once per analyzer. + numSkipped++ + continue + } if act.err != nil { // Analyzer failed. errs = append(errs, fmt.Errorf("analyzer %q failed: %v", act.a.Name, act.err)) @@ -543,6 +547,9 @@ func checkAnalysisResults(actions []*action, pkg *goPackage) string { } } } + if numSkipped > 0 { + errs = append(errs, fmt.Errorf("%d analyzers skipped due to type-checking error: %v", numSkipped, pkg.typeCheckError)) + } if len(diagnostics) == 0 && len(errs) == 0 { return "" } diff --git a/tests/core/nogo/custom/custom_test.go b/tests/core/nogo/custom/custom_test.go index b23a20f00..3eb220d1a 100644 --- a/tests/core/nogo/custom/custom_test.go +++ b/tests/core/nogo/custom/custom_test.go @@ -31,7 +31,7 @@ func TestMain(m *testing.M) { Nogo: "@//:nogo", Main: ` -- BUILD.bazel -- -load("@io_bazel_rules_go//go:def.bzl", "go_library", "nogo") +load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "nogo") nogo( name = "nogo", @@ -124,6 +124,12 @@ go_library( importpath = "dep", ) +go_binary( + name = "type_check_fail", + srcs = ["type_check_fail.go"], + pure = "on", +) + -- foofuncname.go -- // foofuncname checks for functions named "Foo". // It has the same package name as another check to test the checks with @@ -434,6 +440,16 @@ func Foo() bool { // This should fail foofuncname return Bar() } +-- type_check_fail.go -- +package type_check_fail + +import ( + "strings" +) + +func Foo() bool { + return strings.Split("a,b,c", ",") // This fails type-checking +} `, }) } @@ -443,6 +459,7 @@ func Test(t *testing.T) { desc, config, target string wantSuccess bool includes, excludes []string + bazelArgs []string }{ { desc: "default_config", @@ -507,6 +524,16 @@ func Test(t *testing.T) { // note the cross platform regex :) `examplepkg[\\/]pure_src_with_err_calling_native.go:.*function must not be named Foo \(foofuncname\)`, }, + }, { + desc: "type_check_fail", + config: "config.json", + target: "//:type_check_fail", + wantSuccess: false, + includes: []string{ + "4 analyzers skipped due to type-checking error: type_check_fail.go:8:10:", + }, + // Ensure that nogo runs even though compilation fails + bazelArgs: []string{"--keep_going"}, }, { desc: "no_errors", target: "//:no_errors", @@ -527,7 +554,7 @@ func Test(t *testing.T) { defer replaceInFile("BUILD.bazel", customConfig, origConfig) } - cmd := bazel_testing.BazelCmd("build", test.target) + cmd := bazel_testing.BazelCmd(append([]string{"build", test.target}, test.bazelArgs...)...) stderr := &bytes.Buffer{} cmd.Stderr = stderr if err := cmd.Run(); err == nil && !test.wantSuccess {