From bbf8380961d57b5bb9347781a1718de28a09f6ae Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 3 Nov 2023 11:44:46 -0400 Subject: [PATCH] gopls/internal/regtest/marker: use golden diffs for suggested fixes Following the model of codeactionerr, use diffs to reduce the verbosity and redundancy of golden content, for the suggestedfix marker. Also, since all suggested fixes should be of kind 'quickfix', remove the unnecessary kind parameter. Finally, clean up some stale comments. For golang/go#54845 Change-Id: I2eb08e4415dcff4acba604bf97b16c0a82c0a658 Reviewed-on: https://go-review.googlesource.com/c/tools/+/539656 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/lsp/regtest/marker.go | 18 ++----- .../marker/testdata/diagnostics/typeerr.txt | 15 +++--- .../marker/testdata/stubmethods/basic.txt | 23 ++++----- .../testdata/stubmethods/issue61693.txt | 27 ++++------ .../testdata/stubmethods/issue61830.txt | 30 ++++------- .../testdata/suggestedfix/self_assignment.txt | 19 +++---- .../testdata/suggestedfix/undeclared.txt | 50 ++++++++----------- .../testdata/suggestedfix/unusedrequire.txt | 11 ++-- .../suggestedfix/unusedrequire_gowork.txt | 22 +++++--- 9 files changed, 92 insertions(+), 123 deletions(-) diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go index d07c6d070e3..eea6e15d283 100644 --- a/gopls/internal/lsp/regtest/marker.go +++ b/gopls/internal/lsp/regtest/marker.go @@ -249,9 +249,6 @@ var update = flag.Bool("update", false, "if set, update test data during marker // to have exactly one associated code action of the specified kind. // This action is executed for its editing effects on the source files. // Like rename, the golden directory contains the expected transformed files. -// TODO(rfindley): we probably only need 'suggestedfix' for quick-fixes. All -// other actions should use codeaction markers. In that case, we can remove -// the 'kind' parameter. // // - rank(location, ...completionItem): executes a textDocument/completion // request at the given location, and verifies that each expected @@ -387,15 +384,11 @@ var update = flag.Bool("update", false, "if set, update test data during marker // // Existing marker tests (in ../testdata) to port: // - CallHierarchy -// - Completions -// - CompletionSnippets -// - CaseSensitiveCompletions -// - RankCompletions // - SemanticTokens -// - FunctionExtractions +// - SuggestedFixes // - MethodExtractions -// - Renames // - InlayHints +// - Renames // - SelectionRanges func RunMarkerTests(t *testing.T, dir string) { // The marker tests must be able to run go/packages.Load. @@ -407,7 +400,6 @@ func RunMarkerTests(t *testing.T, dir string) { } // Opt: use a shared cache. - // TODO(rfindley): opt: use a memoize store with no eviction. cache := cache.New(nil) for _, test := range tests { @@ -2035,7 +2027,7 @@ func (mark marker) consumeExtraNotes(name string, f func(marker)) { // kind, golden) marker. It acts like @diag(location, regexp), to set // the expectation of a diagnostic, but then it applies the first code // action of the specified kind suggested by the matched diagnostic. -func suggestedfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, actionKind string, golden *Golden) { +func suggestedfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, golden *Golden) { loc.Range.End = loc.Range.Start // diagnostics ignore end position. // Find and remove the matching diagnostic. diag, ok := removeDiagnostic(mark, loc, re) @@ -2045,14 +2037,14 @@ func suggestedfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, a } // Apply the fix it suggests. - changed, err := codeAction(mark.run.env, loc.URI, diag.Range, actionKind, &diag) + changed, err := codeAction(mark.run.env, loc.URI, diag.Range, "quickfix", &diag) if err != nil { mark.errorf("suggestedfix failed: %v. (Use @suggestedfixerr for expected errors.)", err) return } // Check the file state. - checkChangedFiles(mark, changed, golden) + checkDiffs(mark, changed, golden) } // codeAction executes a textDocument/codeAction request for the specified diff --git a/gopls/internal/regtest/marker/testdata/diagnostics/typeerr.txt b/gopls/internal/regtest/marker/testdata/diagnostics/typeerr.txt index 345c48e420a..c14b9d734ba 100644 --- a/gopls/internal/regtest/marker/testdata/diagnostics/typeerr.txt +++ b/gopls/internal/regtest/marker/testdata/diagnostics/typeerr.txt @@ -19,15 +19,12 @@ package a func f(x int) { append("") //@diag(re`""`, re"a slice") - x := 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", "quickfix", fix) + x := 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", fix) } -- @fix/typeerr.go -- -package a - -func f(x int) { - append("") //@diag(re`""`, re"a slice") - - x = 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", "quickfix", fix) -} - +--- before ++++ after +@@ -6 +6 @@ +- x := 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", fix) ++ x = 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", fix) diff --git a/gopls/internal/regtest/marker/testdata/stubmethods/basic.txt b/gopls/internal/regtest/marker/testdata/stubmethods/basic.txt index 253ecd79cda..9a651288306 100644 --- a/gopls/internal/regtest/marker/testdata/stubmethods/basic.txt +++ b/gopls/internal/regtest/marker/testdata/stubmethods/basic.txt @@ -9,16 +9,15 @@ package a type C int -var _ error = C(0) //@suggestedfix(re"C.0.", re"missing method Error", "quickfix", stub) - +var _ error = C(0) //@suggestedfix(re"C.0.", re"missing method Error", stub) -- @stub/a/a.go -- -package a - -type C int - -// Error implements error. -func (C) Error() string { - panic("unimplemented") -} - -var _ error = C(0) //@suggestedfix(re"C.0.", re"missing method Error", "quickfix", stub) +--- before ++++ after +@@ -3 +3,6 @@ +-type C int ++type C int ++ ++// Error implements error. ++func (C) Error() string { ++ panic("unimplemented") ++} diff --git a/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt b/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt index 8dda66293e9..f767b656b42 100644 --- a/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt +++ b/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt @@ -15,21 +15,16 @@ func F(err ...error) {} func _() { var x error - F(x, C(0)) //@suggestedfix(re"C.0.", re"missing method Error", "quickfix", stub) + F(x, C(0)) //@suggestedfix(re"C.0.", re"missing method Error", stub) } -- @stub/main.go -- -package main - -type C int - -// Error implements error. -func (C) Error() string { - panic("unimplemented") -} - -func F(err ...error) {} - -func _() { - var x error - F(x, C(0)) //@suggestedfix(re"C.0.", re"missing method Error", "quickfix", stub) -} +--- before ++++ after +@@ -3 +3,6 @@ +-type C int ++type C int ++ ++// Error implements error. ++func (C) Error() string { ++ panic("unimplemented") ++} diff --git a/gopls/internal/regtest/marker/testdata/stubmethods/issue61830.txt b/gopls/internal/regtest/marker/testdata/stubmethods/issue61830.txt index 43633557d89..3e6fab1bb00 100644 --- a/gopls/internal/regtest/marker/testdata/stubmethods/issue61830.txt +++ b/gopls/internal/regtest/marker/testdata/stubmethods/issue61830.txt @@ -14,23 +14,15 @@ type I interface { type A struct{} -var _ I = &A{} //@suggestedfix(re"&A..", re"missing method M", "quickfix", stub) +var _ I = &A{} //@suggestedfix(re"&A..", re"missing method M", stub) -- @stub/p.go -- -package p - -import "io" - -type B struct{} - -type I interface { - M(io.Reader, B) -} - -type A struct{} - -// M implements I. -func (*A) M(io.Reader, B) { - panic("unimplemented") -} - -var _ I = &A{} //@suggestedfix(re"&A..", re"missing method M", "quickfix", stub) +--- before ++++ after +@@ -11 +11,6 @@ +-type A struct{} ++type A struct{} ++ ++// M implements I. ++func (*A) M(io.Reader, B) { ++ panic("unimplemented") ++} diff --git a/gopls/internal/regtest/marker/testdata/suggestedfix/self_assignment.txt b/gopls/internal/regtest/marker/testdata/suggestedfix/self_assignment.txt index 241a80a99c2..1003ef21ffa 100644 --- a/gopls/internal/regtest/marker/testdata/suggestedfix/self_assignment.txt +++ b/gopls/internal/regtest/marker/testdata/suggestedfix/self_assignment.txt @@ -9,20 +9,13 @@ import ( func goodbye() { s := "hiiiiiii" - s = s //@suggestedfix("s = s", re"self-assignment", "quickfix", fix) + s = s //@suggestedfix("s = s", re"self-assignment", fix) log.Print(s) } -- @fix/a.go -- -package suggestedfix - -import ( - "log" -) - -func goodbye() { - s := "hiiiiiii" - //@suggestedfix("s = s", re"self-assignment", "quickfix", fix) - log.Print(s) -} - +--- before ++++ after +@@ -9 +9 @@ +- s = s //@suggestedfix("s = s", re"self-assignment", fix) ++ //@suggestedfix("s = s", re"self-assignment", fix) diff --git a/gopls/internal/regtest/marker/testdata/suggestedfix/undeclared.txt b/gopls/internal/regtest/marker/testdata/suggestedfix/undeclared.txt index 6dc27eefd85..e2c15675b98 100644 --- a/gopls/internal/regtest/marker/testdata/suggestedfix/undeclared.txt +++ b/gopls/internal/regtest/marker/testdata/suggestedfix/undeclared.txt @@ -9,54 +9,46 @@ go 1.12 package p func a() { - z, _ := 1+y, 11 //@suggestedfix("y", re"(undeclared name|undefined): y", "quickfix", a) + z, _ := 1+y, 11 //@suggestedfix("y", re"(undeclared name|undefined): y", a) _ = z } -- @a/a.go -- -package p - -func a() { - y := - z, _ := 1+y, 11 //@suggestedfix("y", re"(undeclared name|undefined): y", "quickfix", a) - _ = z -} - +--- before ++++ after +@@ -3 +3,2 @@ +-func a() { ++func a() { ++ y := -- b.go -- package p func b() { if 100 < 90 { - } else if 100 > n+2 { //@suggestedfix("n", re"(undeclared name|undefined): n", "quickfix", b) + } else if 100 > n+2 { //@suggestedfix("n", re"(undeclared name|undefined): n", b) } } -- @b/b.go -- -package p - -func b() { - n := - if 100 < 90 { - } else if 100 > n+2 { //@suggestedfix("n", re"(undeclared name|undefined): n", "quickfix", b) - } -} - +--- before ++++ after +@@ -3 +3,2 @@ +-func b() { ++func b() { ++ n := -- c.go -- package p func c() { - for i < 200 { //@suggestedfix("i", re"(undeclared name|undefined): i", "quickfix", c) + for i < 200 { //@suggestedfix("i", re"(undeclared name|undefined): i", c) } r() //@diag("r", re"(undeclared name|undefined): r") } -- @c/c.go -- -package p - -func c() { - i := - for i < 200 { //@suggestedfix("i", re"(undeclared name|undefined): i", "quickfix", c) - } - r() //@diag("r", re"(undeclared name|undefined): r") -} - +--- before ++++ after +@@ -3 +3,2 @@ +-func c() { ++func c() { ++ i := diff --git a/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire.txt b/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire.txt index 6317b73f067..c9f6eee5c3a 100644 --- a/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire.txt +++ b/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire.txt @@ -13,12 +13,15 @@ module mod.com go 1.14 -require example.com v1.0.0 //@suggestedfix("require", re"not used", "quickfix", a) +require example.com v1.0.0 //@suggestedfix("require", re"not used", a) -- @a/a/go.mod -- -module mod.com - -go 1.14 +--- before ++++ after +@@ -4,3 +4 @@ +- +-require example.com v1.0.0 //@suggestedfix("require", re"not used", a) +- -- a/main.go -- package main func main() {} diff --git a/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire_gowork.txt b/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire_gowork.txt index 8a090d7fa48..35ed16c8d9d 100644 --- a/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire_gowork.txt +++ b/gopls/internal/regtest/marker/testdata/suggestedfix/unusedrequire_gowork.txt @@ -23,12 +23,15 @@ module mod.com/a go 1.14 -require example.com v1.0.0 //@suggestedfix("require", re"not used", "quickfix", a) +require example.com v1.0.0 //@suggestedfix("require", re"not used", a) -- @a/a/go.mod -- -module mod.com/a - -go 1.14 +--- before ++++ after +@@ -4,3 +4 @@ +- +-require example.com v1.0.0 //@suggestedfix("require", re"not used", a) +- -- a/main.go -- package main func main() {} @@ -38,12 +41,15 @@ module mod.com/b go 1.14 -require example.com v1.0.0 //@suggestedfix("require", re"not used", "quickfix", b) +require example.com v1.0.0 //@suggestedfix("require", re"not used", b) -- @b/b/go.mod -- -module mod.com/b - -go 1.14 +--- before ++++ after +@@ -4,3 +4 @@ +- +-require example.com v1.0.0 //@suggestedfix("require", re"not used", b) +- -- b/main.go -- package main func main() {}