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>

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

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)
 	}

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)

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")
 )
 

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

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")

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