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