From c93f28baaae07c168b9e3eacc138484da397ec60 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Tue, 2 Jul 2019 18:35:35 -0400 Subject: [PATCH] internal/lsp: check no errs for assignability rename The satisfy package has a precondition for Finder.Find that requires that the package has no type errors. If this is a check that we would perform, give an error and do not rename. Fixes golang/go#32882 Change-Id: Id44b451bf86ff883fd78a6306f2b2565ad3bdeb9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/184857 Run-TryBot: Suzy Mueller TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/lsp_test.go | 10 +- internal/lsp/source/rename_check.go | 13 + internal/lsp/source/source_test.go | 10 +- .../lsp/testdata/rename/a/random.go.golden | 272 +++++++++--------- .../lsp/testdata/rename/bad/bad.go.golden | 2 + internal/lsp/testdata/rename/bad/bad.go.in | 8 + internal/lsp/tests/tests.go | 2 +- 7 files changed, 176 insertions(+), 141 deletions(-) create mode 100644 internal/lsp/testdata/rename/bad/bad.go.golden create mode 100644 internal/lsp/testdata/rename/bad/bad.go.in diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 17d845c5..05329ee6 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -510,6 +510,8 @@ func (r *runner) Reference(t *testing.T, data tests.References) { func (r *runner) Rename(t *testing.T, data tests.Renames) { ctx := context.Background() for spn, newText := range data { + tag := fmt.Sprintf("%s-rename", newText) + uri := spn.URI() filename := uri.Filename() sm, err := r.mapper(uri) @@ -529,7 +531,12 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { NewName: newText, }) if err != nil { - t.Error(err) + renamed := string(r.data.Golden(tag, filename, func() ([]byte, error) { + return []byte(err.Error()), nil + })) + if err.Error() != renamed { + t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v\n", newText, renamed, err) + } continue } @@ -556,7 +563,6 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { got := applyEdits(string(m.Content), sedits) - tag := fmt.Sprintf("%s-rename", newText) gorenamed := string(r.data.Golden(tag, filename, func() ([]byte, error) { return []byte(got), nil })) diff --git a/internal/lsp/source/rename_check.go b/internal/lsp/source/rename_check.go index 0b602f47..8d2ac92c 100644 --- a/internal/lsp/source/rename_check.go +++ b/internal/lsp/source/rename_check.go @@ -789,6 +789,19 @@ func (r *renamer) satisfy() map[satisfy.Constraint]bool { // Compute on demand: it's expensive. var f satisfy.Finder for _, pkg := range r.packages { + // From satisfy.Finder documentation: + // + // The package must be free of type errors, and + // info.{Defs,Uses,Selections,Types} must have been populated by the + // type-checker. + // + // Only proceed if all packages have no errors. + if errs := pkg.GetErrors(); len(errs) > 0 { + r.errorf(token.NoPos, // we don't have a position for this error. + "renaming %q to %q not possible because %q has errors", + r.from, r.to, pkg.PkgPath()) + return nil + } f.Find(pkg.GetTypesInfo(), pkg.GetSyntax()) } r.satisfyConstraints = f.Result diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index d3102509..4eb95de9 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -479,6 +479,8 @@ func (r *runner) Reference(t *testing.T, data tests.References) { func (r *runner) Rename(t *testing.T, data tests.Renames) { ctx := context.Background() for spn, newText := range data { + tag := fmt.Sprintf("%s-rename", newText) + f, err := r.view.GetFile(ctx, spn.URI()) if err != nil { t.Fatalf("failed for %v: %v", spn, err) @@ -492,7 +494,12 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { } changes, err := ident.Rename(context.Background(), newText) if err != nil { - t.Error(err) + renamed := string(r.data.Golden(tag, spn.URI().Filename(), func() ([]byte, error) { + return []byte(err.Error()), nil + })) + if err.Error() != renamed { + t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v\n", newText, renamed, err) + } continue } @@ -513,7 +520,6 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { } got := applyEdits(string(data), edits) - tag := fmt.Sprintf("%s-rename", newText) gorenamed := string(r.data.Golden(tag, spn.URI().Filename(), func() ([]byte, error) { return []byte(got), nil })) diff --git a/internal/lsp/testdata/rename/a/random.go.golden b/internal/lsp/testdata/rename/a/random.go.golden index 427b7d2d..0ad12fe7 100644 --- a/internal/lsp/testdata/rename/a/random.go.golden +++ b/internal/lsp/testdata/rename/a/random.go.golden @@ -42,6 +42,138 @@ func sw() { } } +-- fmt2-rename -- +package a + +import ( + lg "log" + "fmt" + fmt2 "fmt" +) + +func Random() int { + y := 6 + 7 + return y +} + +func Random2(y int) int { //@rename("y", "z") + return y +} + +type Pos struct { + x, y int +} + +func (p *Pos) Sum() int { + return p.x + p.y //@rename("x", "myX") +} + +func _() { + var p Pos //@rename("p", "pos") + _ = p.Sum() //@rename("Sum", "GetSum") +} + +func sw() { + var x interface{} + + switch y := x.(type) { //@rename("y", "y0") + case int: + fmt.Printf("%d", y) //@rename("y", "y1"),rename("fmt", "format") + case string: + lg.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") + default: + fmt2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") + } +} + +-- format-rename -- +package a + +import ( + lg "log" + format "fmt" + f2 "fmt" +) + +func Random() int { + y := 6 + 7 + return y +} + +func Random2(y int) int { //@rename("y", "z") + return y +} + +type Pos struct { + x, y int +} + +func (p *Pos) Sum() int { + return p.x + p.y //@rename("x", "myX") +} + +func _() { + var p Pos //@rename("p", "pos") + _ = p.Sum() //@rename("Sum", "GetSum") +} + +func sw() { + var x interface{} + + switch y := x.(type) { //@rename("y", "y0") + case int: + format.Printf("%d", y) //@rename("y", "y1"),rename("fmt", "format") + case string: + lg.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") + default: + f2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") + } +} + +-- log-rename -- +package a + +import ( + "log" + "fmt" + f2 "fmt" +) + +func Random() int { + y := 6 + 7 + return y +} + +func Random2(y int) int { //@rename("y", "z") + return y +} + +type Pos struct { + x, y int +} + +func (p *Pos) Sum() int { + return p.x + p.y //@rename("x", "myX") +} + +func _() { + var p Pos //@rename("p", "pos") + _ = p.Sum() //@rename("Sum", "GetSum") +} + +func sw() { + var x interface{} + + switch y := x.(type) { //@rename("y", "y0") + case int: + fmt.Printf("%d", y) //@rename("y", "y1"),rename("fmt", "format") + case string: + log.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") + default: + f2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") + } +} + -- myX-rename -- package a @@ -130,50 +262,6 @@ func sw() { } } --- z-rename -- -package a - -import ( - lg "log" - "fmt" - f2 "fmt" -) - -func Random() int { - y := 6 + 7 - return y -} - -func Random2(z int) int { //@rename("y", "z") - return z -} - -type Pos struct { - x, y int -} - -func (p *Pos) Sum() int { - return p.x + p.y //@rename("x", "myX") -} - -func _() { - var p Pos //@rename("p", "pos") - _ = p.Sum() //@rename("Sum", "GetSum") -} - -func sw() { - var x interface{} - - switch y := x.(type) { //@rename("y", "y0") - case int: - fmt.Printf("%d", y) //@rename("y", "y1"),rename("fmt", "format") - case string: - lg.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") - default: - f2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") - } -} - -- y0-rename -- package a @@ -350,55 +438,11 @@ func sw() { } } --- format-rename -- +-- z-rename -- package a import ( lg "log" - format "fmt" - f2 "fmt" -) - -func Random() int { - y := 6 + 7 - return y -} - -func Random2(y int) int { //@rename("y", "z") - return y -} - -type Pos struct { - x, y int -} - -func (p *Pos) Sum() int { - return p.x + p.y //@rename("x", "myX") -} - -func _() { - var p Pos //@rename("p", "pos") - _ = p.Sum() //@rename("Sum", "GetSum") -} - -func sw() { - var x interface{} - - switch y := x.(type) { //@rename("y", "y0") - case int: - format.Printf("%d", y) //@rename("y", "y1"),rename("fmt", "format") - case string: - lg.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") - default: - f2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") - } -} - --- log-rename -- -package a - -import ( - "log" "fmt" f2 "fmt" ) @@ -408,52 +452,8 @@ func Random() int { return y } -func Random2(y int) int { //@rename("y", "z") - return y -} - -type Pos struct { - x, y int -} - -func (p *Pos) Sum() int { - return p.x + p.y //@rename("x", "myX") -} - -func _() { - var p Pos //@rename("p", "pos") - _ = p.Sum() //@rename("Sum", "GetSum") -} - -func sw() { - var x interface{} - - switch y := x.(type) { //@rename("y", "y0") - case int: - fmt.Printf("%d", y) //@rename("y", "y1"),rename("fmt", "format") - case string: - log.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") - default: - f2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") - } -} - --- fmt2-rename -- -package a - -import ( - lg "log" - "fmt" - fmt2 "fmt" -) - -func Random() int { - y := 6 + 7 - return y -} - -func Random2(y int) int { //@rename("y", "z") - return y +func Random2(z int) int { //@rename("y", "z") + return z } type Pos struct { @@ -478,7 +478,7 @@ func sw() { case string: lg.Printf("%s", y) //@rename("y", "y2"),rename("lg","log") default: - fmt2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") + f2.Printf("%v", y) //@rename("y", "y3"),rename("f2","fmt2") } } diff --git a/internal/lsp/testdata/rename/bad/bad.go.golden b/internal/lsp/testdata/rename/bad/bad.go.golden new file mode 100644 index 00000000..7f458139 --- /dev/null +++ b/internal/lsp/testdata/rename/bad/bad.go.golden @@ -0,0 +1,2 @@ +-- rFunc-rename -- +renaming "sFunc" to "rFunc" not possible because "golang.org/x/tools/internal/lsp/rename/bad" has errors diff --git a/internal/lsp/testdata/rename/bad/bad.go.in b/internal/lsp/testdata/rename/bad/bad.go.in new file mode 100644 index 00000000..56dbee74 --- /dev/null +++ b/internal/lsp/testdata/rename/bad/bad.go.in @@ -0,0 +1,8 @@ +package bad + +type myStruct struct { +} + +func (s *myStruct) sFunc() bool { //@rename("sFunc", "rFunc") + return s.Bad +} diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 65a29f39..aa5fd607 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -34,7 +34,7 @@ const ( ExpectedTypeDefinitionsCount = 2 ExpectedHighlightsCount = 2 ExpectedReferencesCount = 4 - ExpectedRenamesCount = 11 + ExpectedRenamesCount = 12 ExpectedSymbolsCount = 1 ExpectedSignaturesCount = 21 ExpectedLinksCount = 2