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 <rfindley@google.com>
Trust: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/rename.go b/internal/lsp/rename.go
index cef0638..5f27d23 100644
--- a/internal/lsp/rename.go
+++ b/internal/lsp/rename.go
@@ -43,9 +43,11 @@
}
// 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 fdf3f63..da7faf8 100644
--- a/internal/lsp/source/rename.go
+++ b/internal/lsp/source/rename.go
@@ -42,22 +42,30 @@
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 @@
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 @@
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 bc670db..5ad2ebc 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -820,7 +820,7 @@
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 bdccaed..c4664a7 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 367d52d..34d03ba 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 3686914..aaad531 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 5482a03..37bb48a 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -19,7 +19,7 @@
TypeDefinitionsCount = 2
HighlightsCount = 69
ReferencesCount = 25
-RenamesCount = 31
+RenamesCount = 33
PrepareRenamesCount = 7
SymbolsCount = 5
WorkspaceSymbolsCount = 20
