internal/lsp/cache: use GetHandle not Bind in astCacheData

This change replaces Bind (generational lifetime) with GetHandle
(reference counting) for the cache of buildASTCache calls, as
Bind is deprecated.

Also:
- add missing commentary, particularly on the question of why
  this cache is needed at all.
- remove unused field astCacheData.err
- simplify SignatureHelp to avoid unnecessary use of Declaration.
- minor simplifications to logic surrounding FindPackageFromPos
  and PosTo{Decl,Field}.

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

internal/lsp/cache/graph.go

@@ -24,8 +24,8 @@ type metadataGraph struct {
 	// importedBy maps package IDs to the list of packages that import them.
 	importedBy map[PackageID][]PackageID
 
-	// ids maps file URIs to package IDs. A single file may belong to multiple
-	// packages due to tests packages.
+	// ids maps file URIs to package IDs, sorted by (!valid, cli, packageID).
+	// A single file may belong to multiple packages due to tests packages.
 	ids map[span.URI][]PackageID
 }
 
@@ -89,21 +89,21 @@ func (g *metadataGraph) build() {
 	// 4: an invalid command-line-arguments package
 	for uri, ids := range g.ids {
 		sort.Slice(ids, func(i, j int) bool {
-			// Sort valid packages first.
+			// 1. valid packages appear earlier.
 			validi := g.metadata[ids[i]].Valid
 			validj := g.metadata[ids[j]].Valid
 			if validi != validj {
 				return validi
 			}
 
+			// 2. command-line-args packages appear later.
 			cli := source.IsCommandLineArguments(string(ids[i]))
 			clj := source.IsCommandLineArguments(string(ids[j]))
-			if cli && !clj {
-				return false
-			}
-			if !cli && clj {
-				return true
+			if cli != clj {
+				return clj
 			}
+
+			// 3. packages appear in name order.
 			return ids[i] < ids[j]
 		})
 

internal/lsp/cache/parse.go

@@ -128,19 +128,37 @@ func (s *snapshot) astCacheData(ctx context.Context, spkg source.Package, pos to
 	if err != nil {
 		return nil, err
 	}
-	astHandle := s.generation.Bind(astCacheKey{pkgHandle.key, pgf.URI}, func(ctx context.Context, arg memoize.Arg) interface{} {
+
+	// TODO(adonovan): opt: is it necessary to cache this operation?
+	//
+	// I expect the main benefit of CL 221021, which introduced it,
+	// was the replacement of PathEnclosingInterval, whose
+	// traversal is allocation-intensive, by buildASTCache.
+	//
+	// When run on the largest file in k8s, buildASTCache took
+	// ~6ms, but I expect most of that cost could be eliminated by
+	// using a stripped-down version of PathEnclosingInterval that
+	// cares only about syntax trees and not tokens. A stateless
+	// utility function that is cheap enough to call for each Pos
+	// would be a nice simplification.
+	//
+	// (The basic approach would be to use ast.Inspect, compare
+	// each node with the search Pos, and bail out as soon
+	// as a match is found. The pre-order hook would return false
+	// to avoid descending into any tree whose End is before
+	// the search Pos.)
+	//
+	// A representative benchmark would help.
+	astHandle, release := s.generation.GetHandle(astCacheKey{pkgHandle.key, pgf.URI}, func(ctx context.Context, arg memoize.Arg) interface{} {
 		return buildASTCache(pgf)
 	})
+	defer release()
 
 	d, err := astHandle.Get(ctx, s.generation, s)
 	if err != nil {
 		return nil, err
 	}
-	data := d.(*astCacheData)
-	if data.err != nil {
-		return nil, data.err
-	}
-	return data, nil
+	return d.(*astCacheData), nil
 }
 
 func (s *snapshot) PosToDecl(ctx context.Context, spkg source.Package, pos token.Pos) (ast.Decl, error) {
@@ -159,10 +177,15 @@ func (s *snapshot) PosToField(ctx context.Context, spkg source.Package, pos toke
 	return data.posToField[pos], nil
 }
 
+// An astCacheData maps object positions to syntax nodes for a single Go file.
 type astCacheData struct {
-	err error
+	// Maps the position of each name declared by a func/var/const/type
+	// Decl to the Decl node.  Also maps the name and type of each field
+	// (broadly defined) to its innermost enclosing Decl.
+	posToDecl map[token.Pos]ast.Decl
 
-	posToDecl  map[token.Pos]ast.Decl
+	// Maps the position of the Name and Type of each field
+	// (broadly defined) to the Field node.
 	posToField map[token.Pos]*ast.Field
 }
 

internal/lsp/source/signature_help.go

@@ -102,16 +102,7 @@ FindCall:
 		if err != nil {
 			return nil, 0, err
 		}
-		rng, err := objToMappedRange(snapshot, pkg, obj)
-		if err != nil {
-			return nil, 0, err
-		}
-		decl := Declaration{
-			obj:  obj,
-			node: node,
-		}
-		decl.MappedRange = append(decl.MappedRange, rng)
-		d, err := FindHoverContext(ctx, snapshot, pkg, decl.obj, decl.node, nil)
+		d, err := FindHoverContext(ctx, snapshot, pkg, obj, node, nil)
 		if err != nil {
 			return nil, 0, err
 		}

internal/lsp/source/types_format.go

@@ -259,10 +259,11 @@ func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj *
 		return types.TypeString(obj.Type(), qf)
 	}
 
-	expr, err := varType(ctx, snapshot, pkg, obj)
-	if err != nil {
+	field, err := snapshot.PosToField(ctx, pkg, obj.Pos())
+	if err != nil || field == nil {
 		return types.TypeString(obj.Type(), qf)
 	}
+	expr := field.Type
 
 	// If the given expr refers to a type parameter, then use the
 	// object's Type instead of the type parameter declaration. This helps
@@ -286,18 +287,6 @@ func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj *
 	return fmted
 }
 
-// varType returns the type expression for a *types.Var.
-func varType(ctx context.Context, snapshot Snapshot, pkg Package, obj *types.Var) (ast.Expr, error) {
-	field, err := snapshot.PosToField(ctx, pkg, obj.Pos())
-	if err != nil {
-		return nil, err
-	}
-	if field == nil {
-		return nil, fmt.Errorf("no declaration for object %s", obj.Name())
-	}
-	return field.Type, nil
-}
-
 // qualifyExpr applies the "pkgName." prefix to any *ast.Ident in the expr.
 func qualifyExpr(expr ast.Expr, srcpkg, pkg Package, clonedInfo map[token.Pos]*types.PkgName, qf types.Qualifier) ast.Expr {
 	ast.Inspect(expr, func(n ast.Node) bool {

internal/lsp/source/util.go

@@ -311,15 +311,15 @@ func FindPackageFromPos(ctx context.Context, snapshot Snapshot, pos token.Pos) (
 	for _, pkg := range pkgs {
 		parsed, err := pkg.File(uri)
 		if err != nil {
+			// TODO(adonovan): should this be a bug.Report or log.Fatal?
+			// The logic in Identifier seems to think so.
+			// Should it be a postcondition of PackagesForFile?
+			// And perhaps PackagesForFile should return the PGFs too.
 			return nil, err
 		}
-		if parsed == nil {
-			continue
+		if parsed != nil && parsed.Tok.Base() == tok.Base() {
+			return pkg, nil
 		}
-		if parsed.Tok.Base() != tok.Base() {
-			continue
-		}
-		return pkg, nil
 	}
 	return nil, fmt.Errorf("no package for given file position")
 }

internal/lsp/source/view.go

@@ -83,11 +83,15 @@ type Snapshot interface {
 	// to quickly find corresponding *ast.Field node given a *types.Var.
 	// We must refer to the AST to render type aliases properly when
 	// formatting signatures and other types.
+	// May return (nil, nil) if the file didn't declare an object at that position.
+	// TODO(adonovan): seems like a bug?
 	PosToField(ctx context.Context, pkg Package, pos token.Pos) (*ast.Field, error)
 
 	// PosToDecl maps certain objects' positions to their surrounding
 	// ast.Decl. This mapping is used when building the documentation
 	// string for the objects.
+	// May return (nil, nil) if the file didn't declare an object at that position.
+	// TODO(adonovan): seems like a bug?
 	PosToDecl(ctx context.Context, pkg Package, pos token.Pos) (ast.Decl, error)
 
 	// DiagnosePackage returns basic diagnostics, including list, parse, and type errors
@@ -147,8 +151,8 @@ type Snapshot interface {
 	// IsBuiltin reports whether uri is part of the builtin package.
 	IsBuiltin(ctx context.Context, uri span.URI) bool
 
-	// PackagesForFile returns the packages that this file belongs to, checked
-	// in mode.
+	// PackagesForFile returns an unordered list of packages that contain
+	// the file denoted by uri, type checked in the specified mode.
 	PackagesForFile(ctx context.Context, uri span.URI, mode TypecheckMode, includeTestVariants bool) ([]Package, error)
 
 	// PackageForFile returns a single package that this file belongs to,