From fddb0d54e992eac271a4e852def1c5091c9ad1fb Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 18 Jan 2022 13:51:12 -0500 Subject: [PATCH] internal/lsp/analysis/noresultvalues: update for CL 379116 error message change CL 379116 changes the format of the error this checker is looking for. Update it to handle both the old and new form. Change-Id: I91668e4fcbe203a9028d07b780fd17e9758fc838 Reviewed-on: https://go-review.googlesource.com/c/tools/+/379174 Trust: Russ Cox Run-TryBot: Russ Cox gopls-CI: kokoro Reviewed-by: Robert Findley TryBot-Result: Gopher Robot --- go/analysis/analysistest/analysistest.go | 18 +++++++++++------- go/analysis/analysistest/analysistest_test.go | 12 ++++++------ gopls/doc/analyzers.md | 5 +++-- .../analysis/noresultvalues/noresultvalues.go | 9 ++++++--- .../noresultvalues/testdata/src/a/a.go | 4 ++-- .../noresultvalues/testdata/src/a/a.go.golden | 4 ++-- .../testdata/src/typeparams/a.go | 2 +- .../testdata/src/typeparams/a.go.golden | 2 +- internal/lsp/source/api_json.go | 4 ++-- 9 files changed, 34 insertions(+), 26 deletions(-) diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index aa27c1b9df..df79a4419b 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -284,7 +284,7 @@ func Run(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Res testenv.NeedsGoPackages(t) } - pkgs, err := loadPackages(dir, patterns...) + pkgs, err := loadPackages(a, dir, patterns...) if err != nil { t.Errorf("loading %s: %v", patterns, err) return nil @@ -308,7 +308,7 @@ type Result = checker.TestAnalyzerResult // dependencies) from dir, which is the root of a GOPATH-style project // tree. It returns an error if any package had an error, or the pattern // matched no packages. -func loadPackages(dir string, patterns ...string) ([]*packages.Package, error) { +func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*packages.Package, error) { // packages.Load loads the real standard library, not a minimal // fake version, which would be more efficient, especially if we // have many small tests that import, say, net/http. @@ -327,9 +327,13 @@ func loadPackages(dir string, patterns ...string) ([]*packages.Package, error) { return nil, err } - // Print errors but do not stop: - // some Analyzers may be disposed to RunDespiteErrors. - packages.PrintErrors(pkgs) + // Do NOT print errors if the analyzer will continue running. + // It is incredibly confusing for tests to be printing to stderr + // willy-nilly instead of their test logs, especially when the + // errors are expected and are going to be fixed. + if !a.RunDespiteErrors { + packages.PrintErrors(pkgs) + } if len(pkgs) == 0 { return nil, fmt.Errorf("no packages matched %s", patterns) @@ -441,7 +445,7 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis want[k] = expects return } - unmatched = append(unmatched, fmt.Sprintf("%q", exp.rx)) + unmatched = append(unmatched, fmt.Sprintf("%#q", exp.rx)) } } if unmatched == nil { @@ -505,7 +509,7 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis var surplus []string for key, expects := range want { for _, exp := range expects { - err := fmt.Sprintf("%s:%d: no %s was reported matching %q", key.file, key.line, exp.kind, exp.rx) + err := fmt.Sprintf("%s:%d: no %s was reported matching %#q", key.file, key.line, exp.kind, exp.rx) surplus = append(surplus, err) } } diff --git a/go/analysis/analysistest/analysistest_test.go b/go/analysis/analysistest/analysistest_test.go index cb9bdd2fd1..8c7ff7363c 100644 --- a/go/analysis/analysistest/analysistest_test.go +++ b/go/analysis/analysistest/analysistest_test.go @@ -134,19 +134,19 @@ func println(...interface{}) { println_TEST_() } // want println:"found" "call o `a/b.go:6: in 'want' comment: got String after foo, want ':'`, `a/b.go:7: in 'want' comment: got EOF, want regular expression`, `a/b.go:8: in 'want' comment: invalid char escape`, - `a/b.go:11:9: diagnostic "call of println(...)" does not match pattern "wrong expectation text"`, + "a/b.go:11:9: diagnostic \"call of println(...)\" does not match pattern `wrong expectation text`", `a/b.go:14:9: unexpected diagnostic: call of println(...)`, - `a/b.go:11: no diagnostic was reported matching "wrong expectation text"`, - `a/b.go:17: no diagnostic was reported matching "unsatisfied expectation"`, + "a/b.go:11: no diagnostic was reported matching `wrong expectation text`", + "a/b.go:17: no diagnostic was reported matching `unsatisfied expectation`", // duplicate copies of each message from the test package (see issue #40574) `a/b.go:5: in 'want' comment: unexpected ":"`, `a/b.go:6: in 'want' comment: got String after foo, want ':'`, `a/b.go:7: in 'want' comment: got EOF, want regular expression`, `a/b.go:8: in 'want' comment: invalid char escape`, - `a/b.go:11:9: diagnostic "call of println(...)" does not match pattern "wrong expectation text"`, + "a/b.go:11:9: diagnostic \"call of println(...)\" does not match pattern `wrong expectation text`", `a/b.go:14:9: unexpected diagnostic: call of println(...)`, - `a/b.go:11: no diagnostic was reported matching "wrong expectation text"`, - `a/b.go:17: no diagnostic was reported matching "unsatisfied expectation"`, + "a/b.go:11: no diagnostic was reported matching `wrong expectation text`", + "a/b.go:17: no diagnostic was reported matching `unsatisfied expectation`", } if !reflect.DeepEqual(got, want) { t.Errorf("got:\n%s\nwant:\n%s", diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index ddb8ffc376..e79f01a1ce 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -612,10 +612,11 @@ will turn into ## **noresultvalues** -suggested fixes for "no result values expected" +suggested fixes for unexpected return values This checker provides suggested fixes for type errors of the -type "no result values expected". For example: +type "no result values expected" or "too many return values". +For example: func z() { return nil } will turn into func z() { return } diff --git a/internal/lsp/analysis/noresultvalues/noresultvalues.go b/internal/lsp/analysis/noresultvalues/noresultvalues.go index 0e6b26f8bd..b9f21f3135 100644 --- a/internal/lsp/analysis/noresultvalues/noresultvalues.go +++ b/internal/lsp/analysis/noresultvalues/noresultvalues.go @@ -10,6 +10,7 @@ import ( "bytes" "go/ast" "go/format" + "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -17,10 +18,11 @@ import ( "golang.org/x/tools/internal/analysisinternal" ) -const Doc = `suggested fixes for "no result values expected" +const Doc = `suggested fixes for unexpected return values This checker provides suggested fixes for type errors of the -type "no result values expected". For example: +type "no result values expected" or "too many return values". +For example: func z() { return nil } will turn into func z() { return } @@ -83,5 +85,6 @@ func run(pass *analysis.Pass) (interface{}, error) { } func FixesError(msg string) bool { - return msg == "no result values expected" + return msg == "no result values expected" || + strings.HasPrefix(msg, "too many return values") && strings.Contains(msg, "want ()") } diff --git a/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go b/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go index 30265a42f2..3daa7f7c76 100644 --- a/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go +++ b/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go @@ -4,6 +4,6 @@ package noresultvalues -func x() { return nil } // want "no result values expected" +func x() { return nil } // want `no result values expected|too many return values` -func y() { return nil, "hello" } // want "no result values expected" +func y() { return nil, "hello" } // want `no result values expected|too many return values` diff --git a/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go.golden b/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go.golden index 6b29cefa36..5e93aa4135 100644 --- a/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go.golden +++ b/internal/lsp/analysis/noresultvalues/testdata/src/a/a.go.golden @@ -4,6 +4,6 @@ package noresultvalues -func x() { return } // want "no result values expected" +func x() { return } // want `no result values expected|too many return values` -func y() { return } // want "no result values expected" +func y() { return } // want `no result values expected|too many return values` diff --git a/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go b/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go index d1b2980103..f8aa43665c 100644 --- a/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go +++ b/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go @@ -2,5 +2,5 @@ package noresult func hello[T any]() { var z T - return z // want "no result values expected" + return z // want `no result values expected|too many return values` } diff --git a/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go.golden b/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go.golden index c5f56a9274..963e3f4e1a 100644 --- a/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go.golden +++ b/internal/lsp/analysis/noresultvalues/testdata/src/typeparams/a.go.golden @@ -2,5 +2,5 @@ package noresult func hello[T any]() { var z T - return // want "no result values expected" + return // want `no result values expected|too many return values` } diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go index 50c55199ec..8249fb471b 100755 --- a/internal/lsp/source/api_json.go +++ b/internal/lsp/source/api_json.go @@ -420,7 +420,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "\"noresultvalues\"", - Doc: "suggested fixes for \"no result values expected\"\n\nThis checker provides suggested fixes for type errors of the\ntype \"no result values expected\". For example:\n\tfunc z() { return nil }\nwill turn into\n\tfunc z() { return }\n", + Doc: "suggested fixes for unexpected return values\n\nThis checker provides suggested fixes for type errors of the\ntype \"no result values expected\" or \"too many return values\".\nFor example:\n\tfunc z() { return nil }\nwill turn into\n\tfunc z() { return }\n", Default: "true", }, { @@ -932,7 +932,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "noresultvalues", - Doc: "suggested fixes for \"no result values expected\"\n\nThis checker provides suggested fixes for type errors of the\ntype \"no result values expected\". For example:\n\tfunc z() { return nil }\nwill turn into\n\tfunc z() { return }\n", + Doc: "suggested fixes for unexpected return values\n\nThis checker provides suggested fixes for type errors of the\ntype \"no result values expected\" or \"too many return values\".\nFor example:\n\tfunc z() { return nil }\nwill turn into\n\tfunc z() { return }\n", Default: true, }, {