From c2bea79de95bcea1bae400b32a80de2dfe68a9b4 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 15 Jan 2021 15:46:22 -0500 Subject: [PATCH] internal/lsp/source: make it an error to rename embedded fields Field embedding links two objects (a TypeName and a Var) by name, requiring special handling during renaming. In CL 282932, renaming of types was made to propagate to uses of their embeddings. However, no such propagation in the reverse direction was added, meaning that renaming an embedded field would not rename the corresponding type, and code could still be left in a non-compiling state. It should be an invariant that renaming does not change program behavior. To enforce with field embeddings this we'd need to also rename the corresponding type, but this seems problematic. If I'm hovering over the field selector x.T, and rename T, it is surprising that this would end up renaming a type. For lack of a better solution, make it an error to rename embedded fields, but try to provide a helpful error message. Also handle the blank identifier, for which renaming was giving a message to "please file a bug". Marker tests are added for the new errors in rename, but not for prepareRename. The prepareRename tests were not set up for asserting on errors -- perhaps that would be a good project for a later CL where we clean up errors. Fixes golang/go#43616 Change-Id: I66c2dd5e531dd102431d1edd443d553687d9ca7e Reviewed-on: https://go-review.googlesource.com/c/tools/+/284312 Run-TryBot: Robert Findley Trust: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- internal/lsp/rename.go | 6 ++- internal/lsp/source/rename.go | 38 +++++++++++++++---- internal/lsp/source/source_test.go | 2 +- internal/lsp/testdata/good/good1.go | 1 - .../rename/issue43616/issue43616.go.golden | 10 +++-- .../rename/issue43616/issue43616.go.in | 6 +-- internal/lsp/testdata/summary.txt.golden | 2 +- 7 files changed, 46 insertions(+), 19 deletions(-) diff --git a/internal/lsp/rename.go b/internal/lsp/rename.go index cef0638ff5..5f27d23d1b 100644 --- a/internal/lsp/rename.go +++ b/internal/lsp/rename.go @@ -43,9 +43,11 @@ func (s *Server) prepareRename(ctx context.Context, params *protocol.PrepareRena } // 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, snapshot, fh, params.Position) + item, usererr, err := source.PrepareRename(ctx, snapshot, fh, params.Position) if err != nil { - return nil, nil // ignore errors + // Return usererr here rather than err, to avoid cluttering the UI with + // internal error details. + return nil, usererr } // TODO(suzmue): return ident.Name as the placeholder text. return &item.Range, nil diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index fdf3f63ec7..da7faf8f7d 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -42,22 +42,30 @@ type PrepareItem struct { Text string } -func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp protocol.Position) (*PrepareItem, error) { +// PrepareRename searches for a valid renaming at position pp. +// +// The returned usererr is intended to be displayed to the user to explain why +// the prepare fails. Probably we could eliminate the redundancy in returning +// two errors, but for now this is done defensively. +func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp protocol.Position) (_ *PrepareItem, usererr, err error) { ctx, done := event.Start(ctx, "source.PrepareRename") defer done() qos, err := qualifiedObjsAtProtocolPos(ctx, snapshot, f, pp) if err != nil { - return nil, err + return nil, nil, err } node, obj, pkg := qos[0].node, qos[0].obj, qos[0].sourcePkg + if err := checkRenamable(obj); err != nil { + return nil, err, err + } mr, err := posToMappedRange(snapshot, pkg, node.Pos(), node.End()) if err != nil { - return nil, err + return nil, nil, err } rng, err := mr.Range() if err != nil { - return nil, err + return nil, nil, err } if _, isImport := node.(*ast.ImportSpec); isImport { // We're not really renaming the import path. @@ -66,10 +74,22 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot return &PrepareItem{ Range: rng, Text: obj.Name(), - }, nil + }, nil, nil } -// Rename returns a map of TextEdits for each file modified when renaming a given identifier within a package. +// checkRenamable verifies if an obj may be renamed. +func checkRenamable(obj types.Object) error { + if v, ok := obj.(*types.Var); ok && v.Embedded() { + return errors.New("can't rename embedded fields: rename the type directly or name the field") + } + if obj.Name() == "_" { + return errors.New("can't rename \"_\"") + } + return nil +} + +// Rename returns a map of TextEdits for each file modified when renaming a +// given identifier within a package. func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, newName string) (map[span.URI][]protocol.TextEdit, error) { ctx, done := event.Start(ctx, "source.Rename") defer done() @@ -79,9 +99,11 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, return nil, err } - obj := qos[0].obj - pkg := qos[0].pkg + obj, pkg := qos[0].obj, qos[0].pkg + if err := checkRenamable(obj); err != nil { + return nil, err + } if obj.Name() == newName { return nil, errors.Errorf("old and new names are the same: %s", newName) } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index bc670db5f5..5ad2ebcbc2 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -820,7 +820,7 @@ func (r *runner) PrepareRename(t *testing.T, src span.Span, want *source.Prepare if err != nil { t.Fatal(err) } - item, err := source.PrepareRename(r.ctx, r.snapshot, fh, srcRng.Start) + item, _, err := source.PrepareRename(r.ctx, r.snapshot, fh, srcRng.Start) 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/testdata/good/good1.go b/internal/lsp/testdata/good/good1.go index bdccaed636..c4664a7e5d 100644 --- a/internal/lsp/testdata/good/good1.go +++ b/internal/lsp/testdata/good/good1.go @@ -1,7 +1,6 @@ package good //@diag("package", "no_diagnostics", "", "error") import ( - _ "go/ast" //@prepare("go/ast", "_", "_") "golang.org/x/tools/internal/lsp/types" //@item(types_import, "types", "\"golang.org/x/tools/internal/lsp/types\"", "package") ) diff --git a/internal/lsp/testdata/rename/issue43616/issue43616.go.golden b/internal/lsp/testdata/rename/issue43616/issue43616.go.golden index 367d52d342..34d03ba7aa 100644 --- a/internal/lsp/testdata/rename/issue43616/issue43616.go.golden +++ b/internal/lsp/testdata/rename/issue43616/issue43616.go.golden @@ -1,9 +1,13 @@ -- bar-rename -- package issue43616 -type bar int //@rename("foo","bar") +type bar int //@rename("foo","bar"),prepare("oo","foo","foo") -var x struct{ bar } +var x struct{ bar } //@rename("foo","baz") -var _ = x.bar +var _ = x.bar //@rename("foo","quux") +-- baz-rename -- +can't rename embedded fields: rename the type directly or name the field +-- quux-rename -- +can't rename embedded fields: rename the type directly or name the field diff --git a/internal/lsp/testdata/rename/issue43616/issue43616.go.in b/internal/lsp/testdata/rename/issue43616/issue43616.go.in index 3686914c98..aaad531b73 100644 --- a/internal/lsp/testdata/rename/issue43616/issue43616.go.in +++ b/internal/lsp/testdata/rename/issue43616/issue43616.go.in @@ -1,7 +1,7 @@ package issue43616 -type foo int //@rename("foo","bar") +type foo int //@rename("foo","bar"),prepare("oo","foo","foo") -var x struct{ foo } +var x struct{ foo } //@rename("foo","baz") -var _ = x.foo +var _ = x.foo //@rename("foo","quux") diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index 5482a03c70..37bb48a98d 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -19,7 +19,7 @@ DefinitionsCount = 64 TypeDefinitionsCount = 2 HighlightsCount = 69 ReferencesCount = 25 -RenamesCount = 31 +RenamesCount = 33 PrepareRenamesCount = 7 SymbolsCount = 5 WorkspaceSymbolsCount = 20