diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go index 88c9f14719..c1beff8286 100644 --- a/internal/lsp/cache/graph.go +++ b/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] }) diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index c3eae2f764..712f26ad71 100644 --- a/internal/lsp/cache/parse.go +++ b/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 } diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 813f67e7b3..12e359008f 100644 --- a/internal/lsp/source/signature_help.go +++ b/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 } diff --git a/internal/lsp/source/types_format.go b/internal/lsp/source/types_format.go index 93344e0867..5e10a50900 100644 --- a/internal/lsp/source/types_format.go +++ b/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 { diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 1097038929..8d205ee6ce 100644 --- a/internal/lsp/source/util.go +++ b/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") } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 98d11517d8..c8656153bd 100644 --- a/internal/lsp/source/view.go +++ b/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,