From c9403068c1cd1b80a777f3ffce656fa17a45e51c Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Thu, 22 Aug 2019 13:31:03 -0400 Subject: [PATCH] internal/lsp: add prepare rename support Prepare rename gets the range of the identifier to rename. Returns an error when there is no identifier to rename. Change-Id: I5e5865bc9ff97e6a95ac4f0c48edddcfd0f9ed67 Reviewed-on: https://go-review.googlesource.com/c/tools/+/191170 Run-TryBot: Suzy Mueller TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cmd/cmd_test.go | 4 ++ internal/lsp/general.go | 10 ++++- internal/lsp/lsp_test.go | 44 ++++++++++++++++++++++ internal/lsp/rename.go | 32 ++++++++++++++++ internal/lsp/server.go | 5 ++- internal/lsp/source/identifier.go | 2 +- internal/lsp/source/rename.go | 57 ++++++++++++++++++++++++++--- internal/lsp/source/source_test.go | 47 ++++++++++++++++++++++++ internal/lsp/testdata/good/good0.go | 4 +- internal/lsp/testdata/good/good1.go | 10 +++-- internal/lsp/tests/tests.go | 28 ++++++++++++++ 11 files changed, 228 insertions(+), 15 deletions(-) diff --git a/internal/lsp/cmd/cmd_test.go b/internal/lsp/cmd/cmd_test.go index a86b39cecc..dfa214cb3a 100644 --- a/internal/lsp/cmd/cmd_test.go +++ b/internal/lsp/cmd/cmd_test.go @@ -60,6 +60,10 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { //TODO: add command line rename tests when it works } +func (r *runner) PrepareRename(t *testing.T, data tests.PrepareRenames) { + //TODO: add command line prepare rename tests when it works +} + func (r *runner) Symbol(t *testing.T, data tests.Symbols) { //TODO: add command line symbol tests when it works } diff --git a/internal/lsp/general.go b/internal/lsp/general.go index c14a7ab525..7ff0a51ef2 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -90,6 +90,14 @@ func (s *Server) initialize(ctx context.Context, params *protocol.InitializePara } else { codeActionProvider = true } + var renameOpts interface{} + if params.Capabilities.TextDocument.Rename.PrepareSupport { + renameOpts = &protocol.RenameOptions{ + PrepareProvider: true, + } + } else { + renameOpts = true + } return &protocol.InitializeResult{ Capabilities: protocol.ServerCapabilities{ CodeActionProvider: codeActionProvider, @@ -104,7 +112,7 @@ func (s *Server) initialize(ctx context.Context, params *protocol.InitializePara DocumentHighlightProvider: true, DocumentLinkProvider: &protocol.DocumentLinkOptions{}, ReferencesProvider: true, - RenameProvider: true, + RenameProvider: renameOpts, SignatureHelpProvider: &protocol.SignatureHelpOptions{ TriggerCharacters: []string{"(", ","}, }, diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index b5f166ceff..0356b88a25 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -644,6 +644,50 @@ func (r *runner) Rename(t *testing.T, data tests.Renames) { } } +func (r *runner) PrepareRename(t *testing.T, data tests.PrepareRenames) { + for src, want := range data { + + sm, err := r.mapper(src.URI()) + if err != nil { + t.Fatal(err) + } + loc, err := sm.Location(src) + if err != nil { + t.Fatalf("failed for %v: %v", src, err) + } + + params := &protocol.TextDocumentPositionParams{ + TextDocument: protocol.TextDocumentIdentifier{URI: loc.URI}, + Position: loc.Range.Start, + } + + rng, err := r.server.PrepareRename(context.Background(), params) + if err != nil { + t.Errorf("prepare rename failed for %v: got error: %v", src, err) + continue + } + if rng == nil { + if want.Text != "" { // expected an ident. + t.Errorf("prepare rename failed for %v: got nil", src) + } + continue + } + + wantSpn, err := want.Range.Span() + if err != nil { + t.Fatalf("failed for %v: %v", src, err) + } + got, err := sm.RangeSpan(*rng) + if err != nil { + t.Fatalf("failed for %v: %v", src, err) + } + + if got != wantSpn { + t.Errorf("prepare rename failed: incorrect range got %v want %v", got, wantSpn) + } + } +} + func applyEdits(contents string, edits []diff.TextEdit) string { res := contents diff --git a/internal/lsp/rename.go b/internal/lsp/rename.go index 40912413bb..ba3da3cc74 100644 --- a/internal/lsp/rename.go +++ b/internal/lsp/rename.go @@ -46,3 +46,35 @@ func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*pr return &protocol.WorkspaceEdit{Changes: &changes}, nil } + +func (s *Server) prepareRename(ctx context.Context, params *protocol.TextDocumentPositionParams) (*protocol.Range, error) { + uri := span.NewURI(params.TextDocument.URI) + view := s.session.ViewOf(uri) + f, err := getGoFile(ctx, view, uri) + if err != nil { + return nil, err + } + m, err := getMapper(ctx, f) + if err != nil { + return nil, err + } + + // Find the identifier at the position. + ident, err := source.PrepareRename(ctx, view, f, params.Position) + if err != nil { + // Do not return the errors here, as it adds clutter. + // Returning a nil result means there is not a valid rename. + return nil, nil + } + identSpn, err := ident.Range.Span() + if err != nil { + return nil, err + } + + identRng, err := m.Range(identSpn) + if err != nil { + return nil, err + } + // TODO(suzmue): return ident.Name as the placeholder text. + return &identRng, nil +} diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 2a722467a5..0baceaeb3b 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -268,8 +268,9 @@ func (s *Server) LogTraceNotification(context.Context, *protocol.LogTraceParams) return notImplemented("LogtraceNotification") } -func (s *Server) PrepareRename(context.Context, *protocol.TextDocumentPositionParams) (*protocol.Range, error) { - return nil, notImplemented("PrepareRename") +func (s *Server) PrepareRename(ctx context.Context, params *protocol.TextDocumentPositionParams) (*protocol.Range, error) { + // TODO(suzmue): support sending placeholder text. + return s.prepareRename(ctx, params) } func (s *Server) SetTraceNotification(context.Context, *protocol.SetTraceParams) error { diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index a062dcc6e1..69f58134ff 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -313,7 +313,7 @@ func importSpec(ctx context.Context, view View, f GoFile, fAST *ast.File, pkg Pa Name: importPath, pkg: pkg, } - if result.mappedRange, err = posToRange(ctx, view, imp.Pos(), imp.End()); err != nil { + if result.mappedRange, err = posToRange(ctx, view, imp.Path.Pos(), imp.Path.End()); err != nil { return nil, err } // Consider the "declaration" of an import spec to be the imported package. diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index 603fc84ea6..e3eb481565 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/go/types/typeutil" "golang.org/x/tools/internal/lsp/diff" + "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/trace" "golang.org/x/tools/refactor/satisfy" @@ -35,6 +36,39 @@ type renamer struct { changeMethods bool } +type PrepareItem struct { + Range span.Range + Text string +} + +func PrepareRename(ctx context.Context, view View, f GoFile, pos protocol.Position) (*PrepareItem, error) { + ctx, done := trace.StartSpan(ctx, "source.PrepareRename") + defer done() + + i, err := Identifier(ctx, view, f, pos) + if err != nil { + return nil, err + } + // If the object declaration is nil, assume it is an import spec. + if i.Declaration.obj == nil { + // Find the corresponding package name for this import spec + // and rename that instead. + i, err = i.getPkgName(ctx) + if err != nil { + return nil, err + } + } + + // Do not rename builtin identifiers. + if i.Declaration.obj.Parent() == types.Universe { + return nil, errors.Errorf("cannot rename builtin %q", i.Name) + } + return &PrepareItem{ + Range: i.spanRange, + Text: i.Name, + }, nil +} + // Rename returns a map of TextEdits for each file modified when renaming a given identifier within a package. func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) (map[span.URI][]diff.TextEdit, error) { ctx, done := trace.StartSpan(ctx, "source.Rename") @@ -53,7 +87,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) if i.Name == newName { return nil, errors.Errorf("old and new names are the same: %s", newName) } - if !isValidIdentifier(i.Name) { + if !isValidIdentifier(newName) { return nil, errors.Errorf("invalid identifier to rename: %q", i.Name) } // Do not rename builtin identifiers. @@ -112,18 +146,31 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) // getPkgName gets the pkg name associated with an identifer representing // the import path in an import spec. func (i *IdentifierInfo) getPkgName(ctx context.Context) (*IdentifierInfo, error) { - file := i.File.FileSet().File(i.mappedRange.spanRange.Start) - pkgLine := file.Line(i.mappedRange.spanRange.Start) + file, err := i.File.GetAST(ctx, ParseHeader) + if err != nil { + return nil, err + } + var namePos token.Pos + for _, spec := range file.Imports { + if spec.Path.Pos() == i.spanRange.Start { + namePos = spec.Pos() + break + } + } + if !namePos.IsValid() { + return nil, errors.Errorf("import spec not found for %q", i.Name) + } + // Look for the object defined at NamePos. for _, obj := range i.pkg.GetTypesInfo().Defs { pkgName, ok := obj.(*types.PkgName) - if ok && file.Line(pkgName.Pos()) == pkgLine { + if ok && pkgName.Pos() == namePos { return getPkgNameIdentifier(ctx, i, pkgName) } } for _, obj := range i.pkg.GetTypesInfo().Implicits { pkgName, ok := obj.(*types.PkgName) - if ok && file.Line(pkgName.Pos()) == pkgLine { + if ok && pkgName.Pos() == namePos { return getPkgNameIdentifier(ctx, i, pkgName) } } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 941ad0ddcb..bca619c2aa 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -634,6 +634,53 @@ func applyEdits(contents string, edits []diff.TextEdit) string { return res } +func (r *runner) PrepareRename(t *testing.T, data tests.PrepareRenames) { + ctx := context.Background() + for src, want := range data { + f, err := r.view.GetFile(ctx, src.URI()) + if err != nil { + t.Fatal(err) + } + srcRng, err := spanToRange(r.data, src) + if err != nil { + t.Fatal(err) + } + + // Find the identifier at the position. + item, err := source.PrepareRename(ctx, r.view, f.(source.GoFile), srcRng.Start) + if err != nil { + if want.Text != "" { // expected an ident. + t.Errorf("prepare rename failed for %v: got error: %v", src, err) + } + continue + } + if item == nil { + if want.Text != "" { + t.Errorf("prepare rename failed for %v: got nil", src) + } + continue + } + + if want.Text == "" && item != nil { + t.Errorf("prepare rename failed for %v: expected nil, got %v", src, item) + continue + } + gotSpn, err := item.Range.Span() + if err != nil { + t.Fatal(err) + } + + wantSpn, err := want.Range.Span() + if err != nil { + t.Fatal(err) + } + + if gotSpn != wantSpn { + t.Errorf("prepare rename failed: incorrect range got %v want %v", item.Range, want.Range) + } + } +} + func (r *runner) Symbol(t *testing.T, data tests.Symbols) { ctx := r.ctx for uri, expectedSymbols := range data { diff --git a/internal/lsp/testdata/good/good0.go b/internal/lsp/testdata/good/good0.go index 98f03b090c..85c46aa8c7 100644 --- a/internal/lsp/testdata/good/good0.go +++ b/internal/lsp/testdata/good/good0.go @@ -1,6 +1,6 @@ package good //@diag("package", "no_diagnostics", "") -func stuff() { //@item(good_stuff, "stuff", "func()", "func") +func stuff() { //@item(good_stuff, "stuff", "func()", "func"),prepare("stu", "stuff", "stuff") x := 5 - random2(x) + random2(x) //@prepare("dom", "random2", "random2") } diff --git a/internal/lsp/testdata/good/good1.go b/internal/lsp/testdata/good/good1.go index 184fbd26dd..306335f694 100644 --- a/internal/lsp/testdata/good/good1.go +++ b/internal/lsp/testdata/good/good1.go @@ -1,17 +1,19 @@ package good //@diag("package", "no_diagnostics", "") import ( - "golang.org/x/tools/internal/lsp/types" //@item(types_import, "types", "\"golang.org/x/tools/internal/lsp/types\"", "package") + _ "go/ast" //@prepare("go/ast", "_", "_") + "golang.org/x/tools/internal/lsp/types" //@item(types_import, "types", "\"golang.org/x/tools/internal/lsp/types\"", "package"),prepare("types","\"", "types") ) func random() int { //@item(good_random, "random", "func() int", "func") - y := 6 + 7 - return y + _ = "random() int" //@prepare("random", "", "") + y := 6 + 7 //@prepare("7", "", "") + return y //@prepare("return", "","") } func random2(y int) int { //@item(good_random2, "random2", "func(y int) int", "func"),item(good_y_param, "y", "int", "parameter") //@complete("", good_y_param, types_import, good_random, good_random2, good_stuff) - var b types.Bob = &types.X{} + var b types.Bob = &types.X{} //@prepare("ypes","types", "types") if _, ok := b.(*types.X); ok { //@complete("X", X_struct, Y_struct, Bob_interface) } diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 5f0846be02..788ee87ea2 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -40,6 +40,7 @@ const ( ExpectedHighlightsCount = 2 ExpectedReferencesCount = 5 ExpectedRenamesCount = 20 + ExpectedPrepareRenamesCount = 8 ExpectedSymbolsCount = 1 ExpectedSignaturesCount = 21 ExpectedLinksCount = 4 @@ -65,6 +66,7 @@ type Definitions map[span.Span]Definition type Highlights map[string][]span.Span type References map[span.Span][]span.Span type Renames map[span.Span]string +type PrepareRenames map[span.Span]*source.PrepareItem type Symbols map[span.URI][]source.Symbol type SymbolsChildren map[string][]source.Symbol type Signatures map[span.Span]*source.SignatureInformation @@ -84,6 +86,7 @@ type Data struct { Highlights Highlights References References Renames Renames + PrepareRenames PrepareRenames Symbols Symbols symbolsChildren SymbolsChildren Signatures Signatures @@ -105,6 +108,7 @@ type Tests interface { Highlight(*testing.T, Highlights) Reference(*testing.T, References) Rename(*testing.T, Renames) + PrepareRename(*testing.T, PrepareRenames) Symbol(*testing.T, Symbols) SignatureHelp(*testing.T, Signatures) Link(*testing.T, Links) @@ -151,6 +155,7 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data { Highlights: make(Highlights), References: make(References), Renames: make(Renames), + PrepareRenames: make(PrepareRenames), Symbols: make(Symbols), symbolsChildren: make(SymbolsChildren), Signatures: make(Signatures), @@ -235,6 +240,7 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data { "highlight": data.collectHighlights, "refs": data.collectReferences, "rename": data.collectRenames, + "prepare": data.collectPrepareRenames, "symbol": data.collectSymbols, "signature": data.collectSignatures, "snippet": data.collectCompletionSnippets, @@ -339,6 +345,15 @@ func Run(t *testing.T, tests Tests, data *Data) { tests.Rename(t, data.Renames) }) + t.Run("PrepareRenames", func(t *testing.T) { + t.Helper() + if len(data.PrepareRenames) != ExpectedPrepareRenamesCount { + t.Errorf("got %v prepare renames expected %v", len(data.PrepareRenames), ExpectedPrepareRenamesCount) + } + + tests.PrepareRename(t, data.PrepareRenames) + }) + t.Run("Symbols", func(t *testing.T) { t.Helper() if len(data.Symbols) != ExpectedSymbolsCount { @@ -613,6 +628,19 @@ func (data *Data) collectRenames(src span.Span, newText string) { data.Renames[src] = newText } +func (data *Data) collectPrepareRenames(src span.Span, rng span.Range, placeholder string) { + if int(rng.End-rng.Start) != len(placeholder) { + // If the length of the placeholder and the length of the range do not match, + // make the range just be the start. + rng = span.NewRange(rng.FileSet, rng.Start, rng.Start) + } + + data.PrepareRenames[src] = &source.PrepareItem{ + Range: rng, + Text: placeholder, + } +} + func (data *Data) collectSymbols(name string, spn span.Span, kind string, parentName string) { sym := source.Symbol{ Name: name,