internal/lsp/source: recursively search for qualified objects

When searching for references or renaming, we start from all packages
containing the current position. But as reported in golang/go#47564,
this fails if we're renaming an object in another package; we need to
start the search from the package containing the object definition.

This CL finds the missing packages by recursively searching all
locations we encounter. For now, this will cause us to consider the
object location, and may also help us behave correctly with respect to
build constraint variants in the future.

While at it, update the regtests to support renaming. This bug could
be exercised with marker tests, but it's good to have a regtest for
renaming anyway.

Fixes golang/go#47564

Change-Id: I5517e2aeaaa744fcc6b6b96ffbb0b2625b498ed5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340472
Trust: Robert Findley <rfindley@google.com>
Trust: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

gopls/internal/regtest/misc/rename_test.go

@@ -0,0 +1,58 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package misc
+
+import (
+	"strings"
+	"testing"
+
+	. "golang.org/x/tools/internal/lsp/regtest"
+)
+
+// Test for golang/go#47564.
+func TestRenameInTestVariant(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- stringutil/stringutil.go --
+package stringutil
+
+func Identity(s string) string {
+	return s
+}
+-- stringutil/stringutil_test.go --
+package stringutil
+
+func TestIdentity(t *testing.T) {
+	if got := Identity("foo"); got != "foo" {
+		t.Errorf("bad")
+	}
+}
+-- main.go --
+package main
+
+import (
+	"fmt"
+
+	"mod.com/stringutil"
+)
+
+func main() {
+	fmt.Println(stringutil.Identity("hello world"))
+}
+`
+
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("main.go")
+		pos := env.RegexpSearch("main.go", `stringutil\.(Identity)`)
+		env.Rename("main.go", pos, "Identityx")
+		text := env.Editor.BufferText("stringutil/stringutil_test.go")
+		if !strings.Contains(text, "Identityx") {
+			t.Errorf("stringutil/stringutil_test.go: missing expected token `Identityx` after rename:\n%s", text)
+		}
+	})
+}

internal/lsp/fake/client.go

@@ -7,7 +7,6 @@ package fake
 import (
 	"context"
 	"fmt"
-	"os"
 
 	"golang.org/x/tools/internal/lsp/protocol"
 )
@@ -121,19 +120,7 @@ func (c *Client) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceE
 		return &protocol.ApplyWorkspaceEditResponse{FailureReason: "Edit.Changes is unsupported"}, nil
 	}
 	for _, change := range params.Edit.DocumentChanges {
-		path := c.editor.sandbox.Workdir.URIToPath(change.TextDocument.URI)
-		edits := convertEdits(change.Edits)
-		if !c.editor.HasBuffer(path) {
-			err := c.editor.OpenFile(ctx, path)
-			if os.IsNotExist(err) {
-				c.editor.CreateBuffer(ctx, path, "")
-				err = nil
-			}
-			if err != nil {
-				return nil, err
-			}
-		}
-		if err := c.editor.EditBuffer(ctx, path, edits); err != nil {
+		if err := c.editor.applyProtocolEdit(ctx, change); err != nil {
 			return nil, err
 		}
 	}

internal/lsp/fake/editor.go

@@ -1095,6 +1095,49 @@ func (e *Editor) References(ctx context.Context, path string, pos Pos) ([]protoc
 	return locations, nil
 }
 
+func (e *Editor) Rename(ctx context.Context, path string, pos Pos, newName string) error {
+	if e.Server == nil {
+		return nil
+	}
+	params := &protocol.RenameParams{
+		TextDocument: e.textDocumentIdentifier(path),
+		Position:     pos.ToProtocolPosition(),
+		NewName:      newName,
+	}
+	wsEdits, err := e.Server.Rename(ctx, params)
+	if err != nil {
+		return err
+	}
+	for _, change := range wsEdits.DocumentChanges {
+		if err := e.applyProtocolEdit(ctx, change); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (e *Editor) applyProtocolEdit(ctx context.Context, change protocol.TextDocumentEdit) error {
+	path := e.sandbox.Workdir.URIToPath(change.TextDocument.URI)
+	if ver := int32(e.BufferVersion(path)); ver != change.TextDocument.Version {
+		return fmt.Errorf("buffer versions for %q do not match: have %d, editing %d", path, ver, change.TextDocument.Version)
+	}
+	if !e.HasBuffer(path) {
+		err := e.OpenFile(ctx, path)
+		if os.IsNotExist(err) {
+			// TODO: it's unclear if this is correct. Here we create the buffer (with
+			// version 1), then apply edits. Perhaps we should apply the edits before
+			// sending the didOpen notification.
+			e.CreateBuffer(ctx, path, "")
+			err = nil
+		}
+		if err != nil {
+			return err
+		}
+	}
+	fakeEdits := convertEdits(change.Edits)
+	return e.EditBuffer(ctx, path, fakeEdits)
+}
+
 // CodeAction executes a codeAction request on the server.
 func (e *Editor) CodeAction(ctx context.Context, path string, rng *protocol.Range, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) {
 	if e.Server == nil {

internal/lsp/regtest/wrappers.go

@@ -369,6 +369,13 @@ func (e *Env) References(path string, pos fake.Pos) []protocol.Location {
 	return locations
 }
 
+func (e *Env) Rename(path string, pos fake.Pos, newName string) {
+	e.T.Helper()
+	if err := e.Editor.Rename(e.Ctx, path, pos, newName); err != nil {
+		e.T.Fatal(err)
+	}
+}
+
 // Completion executes a completion request on the server.
 func (e *Env) Completion(path string, pos fake.Pos) *protocol.CompletionList {
 	e.T.Helper()

internal/lsp/source/implementation.go

@@ -15,6 +15,7 @@ import (
 
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/lsp/protocol"
+	"golang.org/x/tools/internal/span"
 	"golang.org/x/xerrors"
 )
 
@@ -65,7 +66,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.
 		fset  = s.FileSet()
 	)
 
-	qos, err := qualifiedObjsAtProtocolPos(ctx, s, f, pp)
+	qos, err := qualifiedObjsAtProtocolPos(ctx, s, f.URI(), pp)
 	if err != nil {
 		return nil, err
 	}
@@ -213,19 +214,72 @@ var (
 // referenced at the given position. An object will be returned for
 // every package that the file belongs to, in every typechecking mode
 // applicable.
-func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle, pp protocol.Position) ([]qualifiedObject, error) {
-	pkgs, err := s.PackagesForFile(ctx, fh.URI(), TypecheckAll)
+func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, uri span.URI, pp protocol.Position) ([]qualifiedObject, error) {
+	pkgs, err := s.PackagesForFile(ctx, uri, TypecheckAll)
 	if err != nil {
 		return nil, err
 	}
-	// Check all the packages that the file belongs to.
+	if len(pkgs) == 0 {
+		return nil, errNoObjectFound
+	}
+	pkg := pkgs[0]
+	var offset int
+	pgf, err := pkg.File(uri)
+	if err != nil {
+		return nil, err
+	}
+	spn, err := pgf.Mapper.PointSpan(pp)
+	if err != nil {
+		return nil, err
+	}
+	rng, err := spn.Range(pgf.Mapper.Converter)
+	if err != nil {
+		return nil, err
+	}
+	offset = pgf.Tok.Offset(rng.Start)
+	return qualifiedObjsAtLocation(ctx, s, objSearchKey{uri, offset}, map[objSearchKey]bool{})
+}
+
+type objSearchKey struct {
+	uri    span.URI
+	offset int
+}
+
+// qualifiedObjsAtLocation finds all objects referenced at offset in uri, across
+// all packages in the snapshot.
+func qualifiedObjsAtLocation(ctx context.Context, s Snapshot, key objSearchKey, seen map[objSearchKey]bool) ([]qualifiedObject, error) {
+	if seen[key] {
+		return nil, nil
+	}
+	seen[key] = true
+
+	// We search for referenced objects starting with all packages containing the
+	// current location, and then repeating the search for every distinct object
+	// location discovered.
+	//
+	// In the common case, there should be at most one additional location to
+	// consider: the definition of the object referenced by the location. But we
+	// try to be comprehensive in case we ever support variations on build
+	// constraints.
+
+	pkgs, err := s.PackagesForFile(ctx, key.uri, TypecheckAll)
+	if err != nil {
+		return nil, err
+	}
+
+	// report objects in the order we encounter them. This ensures that the first
+	// result is at the cursor...
 	var qualifiedObjs []qualifiedObject
+	// ...but avoid duplicates.
+	seenObjs := map[types.Object]bool{}
+
 	for _, searchpkg := range pkgs {
-		astFile, pos, err := getASTFile(searchpkg, fh, pp)
+		pgf, err := searchpkg.File(key.uri)
 		if err != nil {
 			return nil, err
 		}
-		path := pathEnclosingObjNode(astFile, pos)
+		pos := pgf.Tok.Pos(key.offset)
+		path := pathEnclosingObjNode(pgf.File, pos)
 		if path == nil {
 			continue
 		}
@@ -279,6 +333,41 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle,
 				sourcePkg: searchpkg,
 				node:      path[0],
 			})
+			seenObjs[obj] = true
+
+			// If the qualified object is in another file (or more likely, another
+			// package), it's possible that there is another copy of it in a package
+			// that we haven't searched, e.g. a test variant. See golang/go#47564.
+			//
+			// In order to be sure we've considered all packages, call
+			// qualifiedObjsAtLocation recursively for all locations we encounter. We
+			// could probably be more precise here, only continuing the search if obj
+			// is in another package, but this should be good enough to find all
+			// uses.
+
+			pos := obj.Pos()
+			var uri span.URI
+			offset := -1
+			for _, pgf := range pkg.CompiledGoFiles() {
+				if pgf.Tok.Base() <= int(pos) && int(pos) <= pgf.Tok.Base()+pgf.Tok.Size() {
+					offset = pgf.Tok.Offset(pos)
+					uri = pgf.URI
+				}
+			}
+			if offset >= 0 {
+				otherObjs, err := qualifiedObjsAtLocation(ctx, s, objSearchKey{uri, offset}, seen)
+				if err != nil {
+					return nil, err
+				}
+				for _, other := range otherObjs {
+					if !seenObjs[other.obj] {
+						qualifiedObjs = append(qualifiedObjs, other)
+						seenObjs[other.obj] = true
+					}
+				}
+			} else {
+				return nil, fmt.Errorf("missing file for position of %q in %q", obj.Name(), obj.Pkg().Name())
+			}
 		}
 	}
 	// Return an error if no objects were found since callers will assume that
@@ -289,22 +378,6 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle,
 	return qualifiedObjs, nil
 }
 
-func getASTFile(pkg Package, f FileHandle, pos protocol.Position) (*ast.File, token.Pos, error) {
-	pgf, err := pkg.File(f.URI())
-	if err != nil {
-		return nil, 0, err
-	}
-	spn, err := pgf.Mapper.PointSpan(pos)
-	if err != nil {
-		return nil, 0, err
-	}
-	rng, err := spn.Range(pgf.Mapper.Converter)
-	if err != nil {
-		return nil, 0, err
-	}
-	return pgf.File, rng.Start, nil
-}
-
 // pathEnclosingObjNode returns the AST path to the object-defining
 // node associated with pos. "Object-defining" means either an
 // *ast.Ident mapped directly to a types.Object or an ast.Node mapped

internal/lsp/source/references.go

@@ -33,7 +33,7 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit
 	ctx, done := event.Start(ctx, "source.References")
 	defer done()
 
-	qualifiedObjs, err := qualifiedObjsAtProtocolPos(ctx, s, f, pp)
+	qualifiedObjs, err := qualifiedObjsAtProtocolPos(ctx, s, f.URI(), pp)
 	// Don't return references for builtin types.
 	if errors.Is(err, errBuiltin) {
 		return nil, nil

internal/lsp/source/rename.go

@@ -51,7 +51,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot
 	ctx, done := event.Start(ctx, "source.PrepareRename")
 	defer done()
 
-	qos, err := qualifiedObjsAtProtocolPos(ctx, snapshot, f, pp)
+	qos, err := qualifiedObjsAtProtocolPos(ctx, snapshot, f.URI(), pp)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -94,7 +94,7 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position,
 	ctx, done := event.Start(ctx, "source.Rename")
 	defer done()
 
-	qos, err := qualifiedObjsAtProtocolPos(ctx, s, f, pp)
+	qos, err := qualifiedObjsAtProtocolPos(ctx, s, f.URI(), pp)
 	if err != nil {
 		return nil, err
 	}