From e33b02e766163a704f840c6decda2d0c5b15cb8f Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 12 Nov 2019 17:58:37 -0500 Subject: [PATCH] internal/lsp: use versioned URIs in rename and code actions This change adds support for returning versions along with file URIs, so that the client can know when to apply changes. The version is not yet propagated along to the internal/lsp/cache package, so this change will have no effect (VS Code ignores a version of 0 and still applies the changes). A few minor changes made in the rename code (to remove the view parameter). Some minor staticcheck fixes. Updates golang/go#35243 Change-Id: Icc26bd9d9e5703c699f555424b94034c97b01d6f Reviewed-on: https://go-review.googlesource.com/c/tools/+/206882 Run-TryBot: Rebecca Stambler Reviewed-by: Ian Cottrell --- internal/lsp/cache/external.go | 10 +-- internal/lsp/cache/session.go | 8 ++- internal/lsp/cache/view.go | 2 +- internal/lsp/cmd/imports.go | 10 ++- internal/lsp/cmd/rename.go | 29 ++++----- internal/lsp/cmd/suggested_fix.go | 9 ++- internal/lsp/code_action.go | 52 +++++++++------ internal/lsp/lsp_test.go | 91 ++++++++++++++------------- internal/lsp/rename.go | 24 ++++--- internal/lsp/source/diagnostics.go | 1 + internal/lsp/source/implementation.go | 2 +- internal/lsp/source/rename.go | 15 ++--- internal/lsp/source/source_test.go | 11 +++- internal/lsp/source/view.go | 16 +++-- 14 files changed, 162 insertions(+), 118 deletions(-) diff --git a/internal/lsp/cache/external.go b/internal/lsp/cache/external.go index a75ec078f4..2498bf6842 100644 --- a/internal/lsp/cache/external.go +++ b/internal/lsp/cache/external.go @@ -28,16 +28,16 @@ type nativeFileHandle struct { } func (fs *nativeFileSystem) GetFile(uri span.URI, kind source.FileKind) source.FileHandle { - version := "DOES NOT EXIST" + identifier := "DOES NOT EXIST" if fi, err := os.Stat(uri.Filename()); err == nil { - version = fi.ModTime().String() + identifier = fi.ModTime().String() } return &nativeFileHandle{ fs: fs, identity: source.FileIdentity{ - URI: uri, - Version: version, - Kind: kind, + URI: uri, + Identifier: identifier, + Kind: kind, }, } } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 2174df4e5a..e81ee9408f 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -46,6 +46,7 @@ type overlay struct { uri span.URI data []byte hash string + version float64 kind source.FileKind // sameContentOnDisk is true if a file has been saved on disk, @@ -424,9 +425,10 @@ func (o *overlay) FileSystem() source.FileSystem { func (o *overlay) Identity() source.FileIdentity { return source.FileIdentity{ - URI: o.uri, - Version: o.hash, - Kind: o.kind, + URI: o.uri, + Identifier: o.hash, + Version: o.version, + Kind: o.kind, } } func (o *overlay) Read(ctx context.Context) ([]byte, string, error) { diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 5bd8d10e16..03a10ac363 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -274,7 +274,7 @@ func (v *view) storeModFileVersions() { func (v *view) fileVersion(filename string, kind source.FileKind) string { uri := span.FileURI(filename) f := v.session.GetFile(uri, kind) - return f.Identity().Version + return f.Identity().Identifier } func (v *view) Shutdown(ctx context.Context) { diff --git a/internal/lsp/cmd/imports.go b/internal/lsp/cmd/imports.go index 19531df41f..2127e25ab8 100644 --- a/internal/lsp/cmd/imports.go +++ b/internal/lsp/cmd/imports.go @@ -70,15 +70,19 @@ func (t *imports) Run(ctx context.Context, args ...string) error { } var edits []protocol.TextEdit for _, a := range actions { - if a.Title == "Organize Imports" { - edits = (*a.Edit.Changes)[string(uri)] + if a.Title != "Organize Imports" { + continue + } + for _, c := range a.Edit.DocumentChanges { + if c.TextDocument.URI == string(uri) { + edits = append(edits, c.Edits...) + } } } sedits, err := source.FromProtocolEdits(file.mapper, edits) if err != nil { return errors.Errorf("%v: %v", edits, err) } - newContent := diff.ApplyEdits(string(file.mapper.Content), sedits) filename := file.uri.Filename() diff --git a/internal/lsp/cmd/rename.go b/internal/lsp/cmd/rename.go index 47e26beb32..98f7d69c1d 100644 --- a/internal/lsp/cmd/rename.go +++ b/internal/lsp/cmd/rename.go @@ -64,42 +64,39 @@ func (r *rename) Run(ctx context.Context, args ...string) error { if file.err != nil { return file.err } - loc, err := file.mapper.Location(from) if err != nil { return err } - p := protocol.RenameParams{ TextDocument: protocol.TextDocumentIdentifier{URI: loc.URI}, Position: loc.Range.Start, NewName: args[1], } - we, err := conn.Rename(ctx, &p) + edit, err := conn.Rename(ctx, &p) if err != nil { return err } - - // Make output order predictable - var keys []string - for u := range *we.Changes { - keys = append(keys, u) + var orderedURIs []string + edits := map[span.URI][]protocol.TextEdit{} + for _, c := range edit.DocumentChanges { + uri := span.URI(c.TextDocument.URI) + edits[uri] = append(edits[uri], c.Edits...) + orderedURIs = append(orderedURIs, c.TextDocument.URI) } - sort.Strings(keys) - changeCount := len(keys) + sort.Strings(orderedURIs) + changeCount := len(orderedURIs) - for _, u := range keys { - edits := (*we.Changes)[u] - uri := span.NewURI(u) + for _, u := range orderedURIs { + uri := span.URI(u) cmdFile := conn.AddFile(ctx, uri) filename := cmdFile.uri.Filename() // convert LSP-style edits to []diff.TextEdit cuz Spans are handy - renameEdits, err := source.FromProtocolEdits(cmdFile.mapper, edits) + renameEdits, err := source.FromProtocolEdits(cmdFile.mapper, edits[uri]) if err != nil { return errors.Errorf("%v: %v", edits, err) } - newContent := diff.ApplyEdits(string(cmdFile.mapper.Content), renameEdits) switch { @@ -114,7 +111,7 @@ func (r *rename) Run(ctx context.Context, args ...string) error { diffs := diff.ToUnified(filename+".orig", filename, string(cmdFile.mapper.Content), renameEdits) fmt.Print(diffs) default: - if len(keys) > 1 { + if len(orderedURIs) > 1 { fmt.Printf("%s:\n", filepath.Base(filename)) } fmt.Print(string(newContent)) diff --git a/internal/lsp/cmd/suggested_fix.go b/internal/lsp/cmd/suggested_fix.go index 25ee8b62ba..15c6bc9983 100644 --- a/internal/lsp/cmd/suggested_fix.go +++ b/internal/lsp/cmd/suggested_fix.go @@ -88,8 +88,13 @@ func (s *suggestedfix) Run(ctx context.Context, args ...string) error { } var edits []protocol.TextEdit for _, a := range actions { - if a.IsPreferred || s.All { - edits = (*a.Edit.Changes)[string(uri)] + if !a.IsPreferred && !s.All { + continue + } + for _, c := range a.Edit.DocumentChanges { + if c.TextDocument.URI == string(uri) { + edits = append(edits, c.Edits...) + } } } diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 0d3966146b..cf66505228 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -28,12 +28,12 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara } snapshot := view.Snapshot() + fh := snapshot.Handle(ctx, f) // Determine the supported actions for this file kind. - fileKind := snapshot.Handle(ctx, f).Identity().Kind - supportedCodeActions, ok := view.Options().SupportedCodeActions[fileKind] + supportedCodeActions, ok := view.Options().SupportedCodeActions[fh.Identity().Kind] if !ok { - return nil, fmt.Errorf("no supported code actions for %v file kind", fileKind) + return nil, fmt.Errorf("no supported code actions for %v file kind", fh.Identity().Kind) } // The Only field of the context specifies which code actions the client wants. @@ -52,7 +52,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara } var codeActions []protocol.CodeAction - switch fileKind { + switch fh.Identity().Kind { case source.Mod: if !wanted[protocol.SourceOrganizeImports] { return nil, nil @@ -92,9 +92,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara Title: importFixTitle(importFix.Fix), Kind: protocol.QuickFix, Edit: &protocol.WorkspaceEdit{ - Changes: &map[string][]protocol.TextEdit{ - string(uri): importFix.Edits, - }, + DocumentChanges: documentChanges(fh, importFix.Edits), }, Diagnostics: fixDiagnostics, }) @@ -107,9 +105,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara Title: "Organize Imports", Kind: protocol.SourceOrganizeImports, Edit: &protocol.WorkspaceEdit{ - Changes: &map[string][]protocol.TextEdit{ - string(uri): edits, - }, + DocumentChanges: documentChanges(fh, edits), }, }) } @@ -223,19 +219,37 @@ func quickFixes(ctx context.Context, s source.Snapshot, f source.File, diagnosti continue } for _, fix := range srcErr.SuggestedFixes { - edits := make(map[string][]protocol.TextEdit) - for uri, e := range fix.Edits { - edits[protocol.NewURI(uri)] = e - } - codeActions = append(codeActions, protocol.CodeAction{ + action := protocol.CodeAction{ Title: fix.Title, Kind: protocol.QuickFix, Diagnostics: []protocol.Diagnostic{diag}, - Edit: &protocol.WorkspaceEdit{ - Changes: &edits, - }, - }) + Edit: &protocol.WorkspaceEdit{}, + } + for uri, edits := range fix.Edits { + f, err := s.View().GetFile(ctx, uri) + if err != nil { + log.Error(ctx, "no file", err, telemetry.URI.Of(uri)) + continue + } + fh := s.Handle(ctx, f) + action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, documentChanges(fh, edits)...) + } + codeActions = append(codeActions, action) } } return codeActions, nil } + +func documentChanges(fh source.FileHandle, edits []protocol.TextEdit) []protocol.TextDocumentEdit { + return []protocol.TextDocumentEdit{ + { + TextDocument: protocol.VersionedTextDocumentIdentifier{ + Version: fh.Identity().Version, + TextDocumentIdentifier: protocol.TextDocumentIdentifier{ + URI: protocol.NewURI(fh.Identity().URI), + }, + }, + Edits: edits, + }, + } +} diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 8e2615b570..f20b967de8 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -310,17 +310,14 @@ func (r *runner) Import(t *testing.T, spn span.Span) { if err != nil { t.Fatal(err) } - var edits []protocol.TextEdit - for _, a := range actions { - if a.Title == "Organize Imports" { - edits = (*a.Edit.Changes)[string(uri)] + got := string(m.Content) + if len(actions) > 0 { + res, err := applyWorkspaceEdits(r, actions[0].Edit) + if err != nil { + t.Fatal(err) } + got = res[uri] } - sedits, err := source.FromProtocolEdits(m, edits) - if err != nil { - t.Error(err) - } - got := diff.ApplyEdits(string(m.Content), sedits) if goimported != got { t.Errorf("import failed for %s, expected:\n%v\ngot:\n%v", filename, goimported, got) } @@ -348,24 +345,17 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span) { }, }) if err != nil { - t.Error(err) - return + t.Fatal(err) } - m, err := r.data.Mapper(f.URI()) + // TODO: This test should probably be able to handle multiple code actions. + if len(actions) > 1 { + t.Fatal("expected only 1 code action") + } + res, err := applyWorkspaceEdits(r, actions[0].Edit) if err != nil { t.Fatal(err) } - var edits []protocol.TextEdit - for _, a := range actions { - if a.Title == "Remove" { - edits = (*a.Edit.Changes)[string(uri)] - } - } - sedits, err := source.FromProtocolEdits(m, edits) - if err != nil { - t.Error(err) - } - got := diff.ApplyEdits(string(m.Content), sedits) + got := res[uri] fixed := string(r.data.Golden("suggestedfix", filename, func() ([]byte, error) { return []byte(got), nil })) @@ -563,7 +553,7 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) { t.Fatalf("failed for %v: %v", spn, err) } - workspaceEdits, err := r.server.Rename(r.ctx, &protocol.RenameParams{ + wedit, err := r.server.Rename(r.ctx, &protocol.RenameParams{ TextDocument: protocol.TextDocumentIdentifier{ URI: protocol.NewURI(uri), }, @@ -579,33 +569,26 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) { } return } - - var res []string - for uri, edits := range *workspaceEdits.Changes { - m, err := r.data.Mapper(span.URI(uri)) - if err != nil { - t.Fatal(err) - } - sedits, err := source.FromProtocolEdits(m, edits) - if err != nil { - t.Error(err) - } - filename := filepath.Base(m.URI.Filename()) - contents := applyEdits(string(m.Content), sedits) - if len(*workspaceEdits.Changes) > 1 { - contents = fmt.Sprintf("%s:\n%s", filename, contents) - } - res = append(res, contents) + res, err := applyWorkspaceEdits(r, wedit) + if err != nil { + t.Fatal(err) } - - // Sort on filename - sort.Strings(res) + var orderedURIs []string + for uri := range res { + orderedURIs = append(orderedURIs, string(uri)) + } + sort.Strings(orderedURIs) var got string - for i, val := range res { + for i := 0; i < len(res); i++ { if i != 0 { got += "\n" } + uri := span.URI(orderedURIs[i]) + if len(res) > 1 { + got += filepath.Base(uri.Filename()) + ":\n" + } + val := res[uri] got += val } @@ -650,6 +633,24 @@ func (r *runner) PrepareRename(t *testing.T, src span.Span, want *source.Prepare } } +func applyWorkspaceEdits(r *runner, wedit *protocol.WorkspaceEdit) (map[span.URI]string, error) { + res := map[span.URI]string{} + for _, docEdits := range wedit.DocumentChanges { + uri := span.URI(docEdits.TextDocument.URI) + m, err := r.data.Mapper(uri) + if err != nil { + return nil, err + } + res[uri] = string(m.Content) + sedits, err := source.FromProtocolEdits(m, docEdits.Edits) + if err != nil { + return nil, err + } + res[uri] = applyEdits(res[uri], sedits) + } + return res, nil +} + func applyEdits(contents string, edits []diff.TextEdit) string { res := contents diff --git a/internal/lsp/rename.go b/internal/lsp/rename.go index 9ce36729a1..9664b5c449 100644 --- a/internal/lsp/rename.go +++ b/internal/lsp/rename.go @@ -23,16 +23,22 @@ func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*pr if err != nil { return nil, err } - edits, err := ident.Rename(ctx, view, params.NewName) + edits, err := ident.Rename(ctx, params.NewName) if err != nil { return nil, err } - changes := make(map[string][]protocol.TextEdit) + var docChanges []protocol.TextDocumentEdit for uri, e := range edits { - changes[protocol.NewURI(uri)] = e + f, err := view.GetFile(ctx, uri) + if err != nil { + return nil, err + } + fh := ident.Snapshot.Handle(ctx, f) + docChanges = append(docChanges, documentChanges(fh, e)...) } - - return &protocol.WorkspaceEdit{Changes: &changes}, nil + return &protocol.WorkspaceEdit{ + DocumentChanges: docChanges, + }, nil } func (s *Server) prepareRename(ctx context.Context, params *protocol.PrepareRenameParams) (*protocol.Range, error) { @@ -42,11 +48,15 @@ func (s *Server) prepareRename(ctx context.Context, params *protocol.PrepareRena if err != nil { return nil, err } + ident, err := source.Identifier(ctx, view, f, params.Position) + if err != nil { + return nil, nil // ignore errors + } // Do not return errors here, as it adds clutter. // Returning a nil result means there is not a valid rename. - item, err := source.PrepareRename(ctx, view, f, params.Position) + item, err := ident.PrepareRename(ctx) if err != nil { - return nil, nil + return nil, nil // ignore errors } // TODO(suzmue): return ident.Name as the placeholder text. return &item.Range, nil diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index fe4d38a438..e85c62319f 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -109,6 +109,7 @@ type diagnosticSet struct { func diagnostics(ctx context.Context, view View, pkg Package, reports map[span.URI][]Diagnostic) bool { ctx, done := trace.StartSpan(ctx, "source.diagnostics", telemetry.Package.Of(pkg.ID())) + _ = ctx // circumvent SA4006 defer done() diagSets := make(map[span.URI]*diagnosticSet) diff --git a/internal/lsp/source/implementation.go b/internal/lsp/source/implementation.go index 2793333a6a..f82f646acc 100644 --- a/internal/lsp/source/implementation.go +++ b/internal/lsp/source/implementation.go @@ -83,7 +83,7 @@ func Implementation(ctx context.Context, view View, f File, position protocol.Po } } if containingFile == nil { - return nil, fmt.Errorf("Failed to find file %q in package %v", file.Name(), pkg.PkgPath()) + return nil, fmt.Errorf("failed to find file %q in package %v", file.Name(), pkg.PkgPath()) } uri := containingFile.Identity().URI diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index d6b50f829a..1717c66c75 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -41,15 +41,10 @@ type PrepareItem struct { Text string } -func PrepareRename(ctx context.Context, view View, f File, pos protocol.Position) (*PrepareItem, error) { +func (i *IdentifierInfo) PrepareRename(ctx context.Context) (*PrepareItem, error) { ctx, done := trace.StartSpan(ctx, "source.PrepareRename") defer done() - i, err := Identifier(ctx, view, f, pos) - if err != nil { - return nil, err - } - // TODO(rstambler): We should handle this in a better way. // If the object declaration is nil, assume it is an import spec. if i.Declaration.obj == nil { @@ -77,7 +72,7 @@ func PrepareRename(ctx context.Context, view View, f File, pos protocol.Position } // 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][]protocol.TextEdit, error) { +func (i *IdentifierInfo) Rename(ctx context.Context, newName string) (map[span.URI][]protocol.TextEdit, error) { ctx, done := trace.StartSpan(ctx, "source.Rename") defer done() @@ -90,7 +85,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) if err != nil { return nil, err } - return ident.Rename(ctx, view, newName) + return ident.Rename(ctx, newName) } if i.Name == newName { return nil, errors.Errorf("old and new names are the same: %s", newName) @@ -117,7 +112,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) r := renamer{ ctx: ctx, - fset: view.Session().Cache().FileSet(), + fset: i.Snapshot.View().Session().Cache().FileSet(), refs: refs, objsToUpdate: make(map[types.Object]bool), from: i.Name, @@ -147,7 +142,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) for uri, edits := range changes { // These edits should really be associated with FileHandles for maximal correctness. // For now, this is good enough. - f, err := view.GetFile(ctx, uri) + f, err := i.Snapshot.View().GetFile(ctx, uri) if err != nil { return nil, err } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 8e550f6c30..4281635c77 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -663,7 +663,7 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) { t.Error(err) return } - changes, err := ident.Rename(r.ctx, r.view, newText) + changes, err := ident.Rename(r.ctx, newText) if err != nil { renamed := string(r.data.Golden(tag, spn.URI().Filename(), func() ([]byte, error) { return []byte(err.Error()), nil @@ -747,7 +747,14 @@ func (r *runner) PrepareRename(t *testing.T, src span.Span, want *source.Prepare t.Fatal(err) } // Find the identifier at the position. - item, err := source.PrepareRename(ctx, r.view, f, srcRng.Start) + ident, err := source.Identifier(ctx, r.view, f, srcRng.Start) + if err != nil { + if want.Text != "" { // expected an ident. + t.Errorf("prepare rename failed for %v: got error: %v", src, err) + } + return + } + item, err := ident.PrepareRename(ctx) if err != nil { if want.Text != "" { // expected an ident. t.Errorf("prepare rename failed for %v: got error: %v", src, err) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 842e5e2446..568151afe7 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -20,13 +20,21 @@ import ( // FileIdentity uniquely identifies a file at a version from a FileSystem. type FileIdentity struct { - URI span.URI - Version string - Kind FileKind + URI span.URI + + // Version is the version of the file, as specified by the client. + Version float64 + + // Identifier represents a unique identifier for the file. + // It could be a file's modification time or its SHA1 hash if it is not on disk. + Identifier string + + // Kind is the file's kind. + Kind FileKind } func (identity FileIdentity) String() string { - return fmt.Sprintf("%s%s%s", identity.URI, identity.Version, identity.Kind) + return fmt.Sprintf("%s%f%s%s", identity.URI, identity.Version, identity.Identifier, identity.Kind) } // FileHandle represents a handle to a specific version of a single file from