From 7b25e351ac0e848b1efa6625bde14603cb29e2b3 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Mon, 8 Jul 2019 18:53:01 -0700 Subject: [PATCH] internal/lsp: rename identifiers in test packages Support renaming of identifiers in test packages. The packages for all of the references must be checked and the changes need to be deduped, since both a package and its test package contain some of the same files. Fixes golang/go#32974 Change-Id: Ie51e19716faae77ce7e5254eeb3956faa42c2a09 Reviewed-on: https://go-review.googlesource.com/c/tools/+/185277 Run-TryBot: Suzy Mueller TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/lsp_test.go | 51 +++++++++++-------- internal/lsp/source/references.go | 27 ++++++---- internal/lsp/source/rename.go | 9 +++- internal/lsp/source/source_test.go | 44 ++++++++++------ .../lsp/testdata/rename/a/random.go.golden | 11 ++++ internal/lsp/testdata/rename/testy/testy.go | 6 +++ .../lsp/testdata/rename/testy/testy.go.golden | 9 ++++ .../lsp/testdata/rename/testy/testy_test.go | 8 +++ .../rename/testy/testy_test.go.golden | 30 +++++++++++ internal/lsp/tests/tests.go | 2 +- 10 files changed, 146 insertions(+), 51 deletions(-) create mode 100644 internal/lsp/testdata/rename/testy/testy.go create mode 100644 internal/lsp/testdata/rename/testy/testy.go.golden create mode 100644 internal/lsp/testdata/rename/testy/testy_test.go create mode 100644 internal/lsp/testdata/rename/testy/testy_test.go.golden diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 05329ee6..6c358ac0 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -10,6 +10,7 @@ import ( "fmt" "go/token" "os/exec" + "path/filepath" "sort" "strings" "testing" @@ -540,35 +541,41 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { continue } - _, m, err := getSourceFile(ctx, r.server.session.ViewOf(uri), uri) - if err != nil { - t.Error(err) + var res []string + for uri, edits := range *workspaceEdits.Changes { + spnURI := span.URI(uri) + _, m, err := getSourceFile(ctx, r.server.session.ViewOf(span.URI(spnURI)), spnURI) + if err != nil { + t.Error(err) + } + + sedits, err := FromProtocolEdits(m, edits) + if err != nil { + t.Error(err) + } + + filename := filepath.Base(m.URI.Filename()) + contents := applyEdits(string(m.Content), sedits) + res = append(res, fmt.Sprintf("%s:\n%s", filename, contents)) } - changes := *workspaceEdits.Changes - if len(changes) != 1 { // Renames must only affect a single file in these tests. - t.Errorf("rename failed for %s, edited %d files, wanted 1 file", newText, len(*workspaceEdits.Changes)) - continue + // Sort on filename + sort.Strings(res) + + var got string + for i, val := range res { + if i != 0 { + got += "\n" + } + got += val } - edits := changes[string(uri)] - if edits == nil { - t.Errorf("rename failed for %s, did not edit %s", newText, filename) - continue - } - sedits, err := FromProtocolEdits(m, edits) - if err != nil { - t.Error(err) - } - - got := applyEdits(string(m.Content), sedits) - - gorenamed := string(r.data.Golden(tag, filename, func() ([]byte, error) { + renamed := string(r.data.Golden(tag, filename, func() ([]byte, error) { return []byte(got), nil })) - if gorenamed != got { - t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v", newText, gorenamed, got) + if renamed != got { + t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v", newText, renamed, got) } } } diff --git a/internal/lsp/source/references.go b/internal/lsp/source/references.go index 5cbb4429..8fd0df36 100644 --- a/internal/lsp/source/references.go +++ b/internal/lsp/source/references.go @@ -20,6 +20,7 @@ type ReferenceInfo struct { Range span.Range ident *ast.Ident obj types.Object + pkg Package isDeclaration bool } @@ -34,17 +35,6 @@ func (i *IdentifierInfo) References(ctx context.Context) ([]*ReferenceInfo, erro if i.decl.obj == nil { return nil, fmt.Errorf("no references for an import spec") } - if i.decl.wasImplicit { - // The definition is implicit, so we must add it separately. - // This occurs when the variable is declared in a type switch statement - // or is an implicit package name. Both implicits are local to a file. - references = append(references, &ReferenceInfo{ - Name: i.decl.obj.Name(), - Range: i.decl.rng, - obj: i.decl.obj, - isDeclaration: true, - }) - } pkgs := i.File.GetPackages(ctx) for _, pkg := range pkgs { @@ -55,6 +45,19 @@ func (i *IdentifierInfo) References(ctx context.Context) ([]*ReferenceInfo, erro if info == nil { return nil, fmt.Errorf("package %s has no types info", pkg.PkgPath()) } + + if i.decl.wasImplicit { + // The definition is implicit, so we must add it separately. + // This occurs when the variable is declared in a type switch statement + // or is an implicit package name. Both implicits are local to a file. + references = append(references, &ReferenceInfo{ + Name: i.decl.obj.Name(), + Range: i.decl.rng, + obj: i.decl.obj, + pkg: pkg, + isDeclaration: true, + }) + } for ident, obj := range info.Defs { if obj == nil || !sameObj(obj, i.decl.obj) { continue @@ -65,6 +68,7 @@ func (i *IdentifierInfo) References(ctx context.Context) ([]*ReferenceInfo, erro Range: span.NewRange(i.File.FileSet(), ident.Pos(), ident.End()), ident: ident, obj: obj, + pkg: pkg, isDeclaration: true, }}, references...) } @@ -76,6 +80,7 @@ func (i *IdentifierInfo) References(ctx context.Context) ([]*ReferenceInfo, erro Name: ident.Name, Range: span.NewRange(i.File.FileSet(), ident.Pos(), ident.End()), ident: ident, + pkg: pkg, obj: obj, }) } diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index 0b72a1d3..eb5f8ba8 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -73,7 +73,9 @@ func (i *IdentifierInfo) Rename(ctx context.Context, newName string) (map[span.U to: newName, packages: make(map[*types.Package]Package), } - r.packages[i.pkg.GetTypes()] = i.pkg + for _, from := range refs { + r.packages[from.pkg.GetTypes()] = from.pkg + } // Check that the renaming of the identifier is ok. for _, from := range refs { @@ -89,6 +91,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, newName string) (map[span.U // Rename all references to the identifier. func (r *renamer) update() (map[span.URI][]TextEdit, error) { result := make(map[span.URI][]TextEdit) + seen := make(map[span.Span]bool) docRegexp, err := regexp.Compile(`\b` + r.from + `\b`) if err != nil { @@ -99,6 +102,10 @@ func (r *renamer) update() (map[span.URI][]TextEdit, error) { if err != nil { return nil, err } + if seen[refSpan] { + continue + } + seen[refSpan] = true // Renaming a types.PkgName may result in the addition or removal of an identifier, // so we deal with this separately. diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 4eb95de9..78d2e90f 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -9,6 +9,7 @@ import ( "context" "fmt" "os/exec" + "path/filepath" "sort" "strings" "testing" @@ -503,29 +504,40 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { continue } - if len(changes) != 1 { // Renames must only affect a single file in these tests. - t.Errorf("rename failed for %s, edited %d files, wanted 1 file", newText, len(changes)) - continue + var res []string + for editSpn, edits := range changes { + f, err := r.view.GetFile(ctx, editSpn) + if err != nil { + t.Fatalf("failed for %v: %v", spn, err) + } + + data, _, err := f.Handle(ctx).Read(ctx) + if err != nil { + t.Error(err) + continue + } + filename := filepath.Base(editSpn.Filename()) + contents := applyEdits(string(data), edits) + res = append(res, fmt.Sprintf("%s:\n%s", filename, contents)) } - edits := changes[spn.URI()] - if edits == nil { - t.Errorf("rename failed for %s, did not edit %s", newText, spn.URI()) - continue - } - data, _, err := f.Handle(ctx).Read(ctx) - if err != nil { - t.Error(err) - continue + // Sort on filename + sort.Strings(res) + + var got string + for i, val := range res { + if i != 0 { + got += "\n" + } + got += val } - got := applyEdits(string(data), edits) - gorenamed := string(r.data.Golden(tag, spn.URI().Filename(), func() ([]byte, error) { + renamed := string(r.data.Golden(tag, spn.URI().Filename(), func() ([]byte, error) { return []byte(got), nil })) - if gorenamed != got { - t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v", newText, gorenamed, got) + if renamed != got { + t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v", newText, renamed, got) } } } diff --git a/internal/lsp/testdata/rename/a/random.go.golden b/internal/lsp/testdata/rename/a/random.go.golden index 0ad12fe7..53bbc29d 100644 --- a/internal/lsp/testdata/rename/a/random.go.golden +++ b/internal/lsp/testdata/rename/a/random.go.golden @@ -1,4 +1,5 @@ -- GetSum-rename -- +random.go: package a import ( @@ -43,6 +44,7 @@ func sw() { } -- fmt2-rename -- +random.go: package a import ( @@ -87,6 +89,7 @@ func sw() { } -- format-rename -- +random.go: package a import ( @@ -131,6 +134,7 @@ func sw() { } -- log-rename -- +random.go: package a import ( @@ -175,6 +179,7 @@ func sw() { } -- myX-rename -- +random.go: package a import ( @@ -219,6 +224,7 @@ func sw() { } -- pos-rename -- +random.go: package a import ( @@ -263,6 +269,7 @@ func sw() { } -- y0-rename -- +random.go: package a import ( @@ -307,6 +314,7 @@ func sw() { } -- y1-rename -- +random.go: package a import ( @@ -351,6 +359,7 @@ func sw() { } -- y2-rename -- +random.go: package a import ( @@ -395,6 +404,7 @@ func sw() { } -- y3-rename -- +random.go: package a import ( @@ -439,6 +449,7 @@ func sw() { } -- z-rename -- +random.go: package a import ( diff --git a/internal/lsp/testdata/rename/testy/testy.go b/internal/lsp/testdata/rename/testy/testy.go new file mode 100644 index 00000000..e5876259 --- /dev/null +++ b/internal/lsp/testdata/rename/testy/testy.go @@ -0,0 +1,6 @@ +package testy + +type tt int //@rename("tt", "testyType") + +func a() { +} diff --git a/internal/lsp/testdata/rename/testy/testy.go.golden b/internal/lsp/testdata/rename/testy/testy.go.golden new file mode 100644 index 00000000..955761b7 --- /dev/null +++ b/internal/lsp/testdata/rename/testy/testy.go.golden @@ -0,0 +1,9 @@ +-- testyType-rename -- +testy.go: +package testy + +type testyType int //@rename("tt", "testyType") + +func a() { +} + diff --git a/internal/lsp/testdata/rename/testy/testy_test.go b/internal/lsp/testdata/rename/testy/testy_test.go new file mode 100644 index 00000000..3d86e845 --- /dev/null +++ b/internal/lsp/testdata/rename/testy/testy_test.go @@ -0,0 +1,8 @@ +package testy + +import "testing" + +func TestSomething(t *testing.T) { + var x int //@rename("x", "testyX") + a() //@rename("a", "b") +} diff --git a/internal/lsp/testdata/rename/testy/testy_test.go.golden b/internal/lsp/testdata/rename/testy/testy_test.go.golden new file mode 100644 index 00000000..2bad81e7 --- /dev/null +++ b/internal/lsp/testdata/rename/testy/testy_test.go.golden @@ -0,0 +1,30 @@ +-- b-rename -- +testy.go: +package testy + +type tt int //@rename("tt", "testyType") + +func b() { +} + +testy_test.go: +package testy + +import "testing" + +func TestSomething(t *testing.T) { + var x int //@rename("x", "testyX") + b() //@rename("a", "b") +} + +-- testyX-rename -- +testy_test.go: +package testy + +import "testing" + +func TestSomething(t *testing.T) { + var testyX int //@rename("x", "testyX") + a() //@rename("a", "b") +} + diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 030b2bde..a1589d82 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -34,7 +34,7 @@ const ( ExpectedTypeDefinitionsCount = 2 ExpectedHighlightsCount = 2 ExpectedReferencesCount = 5 - ExpectedRenamesCount = 13 + ExpectedRenamesCount = 16 ExpectedSymbolsCount = 1 ExpectedSignaturesCount = 21 ExpectedLinksCount = 2