gopls/internal/lsp/source: clarify qualifiedObject

Clarify documentation around qualifiedObject and
its field invariants.
(Initially I tried to delete it but it's not so easy.)

Also, hoist KnownPackages et al outside of loop over
qualifiedIdentifiers in implementations() to avoid unnecessary
recomputation.

Change-Id: Idf087634b918a2277eabf8bbab2fdf49a8fc946c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/431839
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>

gopls/internal/lsp/source/implementation.go

@@ -13,9 +13,9 @@ import (
 	"go/types"
 	"sort"
 
-	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/safetoken"
+	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/span"
 )
 
@@ -59,23 +59,48 @@ var ErrNotAType = errors.New("not a type name or method")
 
 // implementations returns the concrete implementations of the specified
 // interface, or the interfaces implemented by the specified concrete type.
+// It populates only the definition-related fields of qualifiedObject.
+// (Arguably it should return a smaller data type.)
 func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position) ([]qualifiedObject, error) {
+	// Find all named types, even local types
+	// (which can have methods due to promotion).
 	var (
-		impls []qualifiedObject
-		seen  = make(map[token.Position]bool)
-		fset  = s.FileSet()
+		allNamed []*types.Named
+		pkgs     = make(map[*types.Package]Package)
 	)
+	knownPkgs, err := s.KnownPackages(ctx)
+	if err != nil {
+		return nil, err
+	}
+	for _, pkg := range knownPkgs {
+		pkgs[pkg.GetTypes()] = pkg
+		for _, obj := range pkg.GetTypesInfo().Defs {
+			obj, ok := obj.(*types.TypeName)
+			// We ignore aliases 'type M = N' to avoid duplicate reporting
+			// of the Named type N.
+			if !ok || obj.IsAlias() {
+				continue
+			}
+			if named, ok := obj.Type().(*types.Named); ok {
+				allNamed = append(allNamed, named)
+			}
+		}
+	}
 
 	qos, err := qualifiedObjsAtProtocolPos(ctx, s, f.URI(), pp)
 	if err != nil {
 		return nil, err
 	}
+	var (
+		impls []qualifiedObject
+		seen  = make(map[token.Position]bool)
+	)
 	for _, qo := range qos {
+		// Ascertain the query identifier (type or method).
 		var (
 			queryType   types.Type
 			queryMethod *types.Func
 		)
-
 		switch obj := qo.obj.(type) {
 		case *types.Func:
 			queryMethod = obj
@@ -94,32 +119,6 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.
 			return nil, nil
 		}
 
-		// Find all named types, even local types (which can have methods
-		// due to promotion).
-		var (
-			allNamed []*types.Named
-			pkgs     = make(map[*types.Package]Package)
-		)
-		knownPkgs, err := s.KnownPackages(ctx)
-		if err != nil {
-			return nil, err
-		}
-		for _, pkg := range knownPkgs {
-			pkgs[pkg.GetTypes()] = pkg
-			info := pkg.GetTypesInfo()
-			for _, obj := range info.Defs {
-				obj, ok := obj.(*types.TypeName)
-				// We ignore aliases 'type M = N' to avoid duplicate reporting
-				// of the Named type N.
-				if !ok || obj.IsAlias() {
-					continue
-				}
-				if named, ok := obj.Type().(*types.Named); ok {
-					allNamed = append(allNamed, named)
-				}
-			}
-		}
-
 		// Find all the named types that match our query.
 		for _, named := range allNamed {
 			var (
@@ -146,7 +145,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.
 				candObj = sel.Obj()
 			}
 
-			pos := fset.Position(candObj.Pos())
+			pos := s.FileSet().Position(candObj.Pos())
 			if candObj == queryMethod || seen[pos] {
 				continue
 			}
@@ -155,7 +154,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.
 
 			impls = append(impls, qualifiedObject{
 				obj: candObj,
-				pkg: pkgs[candObj.Pkg()],
+				pkg: pkgs[candObj.Pkg()], // may be nil (e.g. error)
 			})
 		}
 	}
@@ -192,17 +191,16 @@ func ensurePointer(T types.Type) types.Type {
 	return T
 }
 
+// A qualifiedObject is the result of resolving a reference from an
+// identifier to an object.
 type qualifiedObject struct {
-	obj types.Object
+	// definition
+	obj types.Object // the referenced object
+	pkg Package      // the Package that defines the object (nil => universe)
 
-	// pkg is the Package that contains obj's definition.
-	pkg Package
-
-	// node is the *ast.Ident or *ast.ImportSpec we followed to find obj, if any.
-	node ast.Node
-
-	// sourcePkg is the Package that contains node, if any.
-	sourcePkg Package
+	// reference (optional)
+	node      ast.Node // the reference (*ast.Ident or *ast.ImportSpec) to the object
+	sourcePkg Package  // the Package containing node
 }
 
 var (

gopls/internal/lsp/source/references.go

@@ -15,9 +15,9 @@ import (
 	"sort"
 	"strconv"
 
-	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/bug"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/internal/bug"
+	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/span"
 )
 
@@ -126,6 +126,10 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit
 }
 
 // references is a helper function to avoid recomputing qualifiedObjsAtProtocolPos.
+// The first element of qos is considered to be the declaration;
+// if isDeclaration, the first result is an extra item for it.
+// Only the definition-related fields of qualifiedObject are used.
+// (Arguably it should accept a smaller data type.)
 func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, includeDeclaration, includeInterfaceRefs, includeEmbeddedRefs bool) ([]*ReferenceInfo, error) {
 	var (
 		references []*ReferenceInfo
@@ -134,8 +138,10 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i
 
 	pos := qos[0].obj.Pos()
 	if pos == token.NoPos {
-		return nil, fmt.Errorf("no position for %s", qos[0].obj)
+		return nil, fmt.Errorf("no position for %s", qos[0].obj) // e.g. error.Error
 	}
+	// Inv: qos[0].pkg != nil, since Pos is valid.
+	// Inv: qos[*].pkg != nil, since all qos are logically the same declaration.
 	filename := snapshot.FileSet().Position(pos).Filename
 	pgf, err := qos[0].pkg.File(span.URIFromPath(filename))
 	if err != nil {
@@ -220,6 +226,8 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i
 	// happened to have a String method.
 	_, isType := declIdent.Declaration.obj.(*types.TypeName)
 	if includeInterfaceRefs && !isType {
+		// TODO(adonovan): opt: don't go back into the position domain:
+		// we have complete type information already.
 		declRange, err := declIdent.Range()
 		if err != nil {
 			return nil, err
@@ -256,6 +264,8 @@ func interfaceReferences(ctx context.Context, s Snapshot, f FileHandle, pp proto
 		return nil, err
 	}
 
+	// Make a separate call to references() for each element
+	// since it treats the first qualifiedObject as a definition.
 	var refs []*ReferenceInfo
 	for _, impl := range implementations {
 		implRefs, err := references(ctx, s, []qualifiedObject{impl}, false, false, false)

gopls/internal/lsp/source/rename.go

@@ -17,9 +17,9 @@ import (
 	"strings"
 
 	"golang.org/x/tools/go/types/typeutil"
-	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/internal/diff"
+	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/span"
 	"golang.org/x/tools/refactor/satisfy"
 )