From 39fdd541e6713605ef4dbc6fc9cd9157f70ca528 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Fri, 24 Jul 2020 17:41:50 -0400 Subject: [PATCH] internal/lsp: move builtin package to Snapshot The builtin package was the one special case where we parsed Go outside the context of a Snapshot. Move it up. Change-Id: I1f4bb536adb40019e0ea9c5c89f38b15737abb8c Reviewed-on: https://go-review.googlesource.com/c/tools/+/245057 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/check.go | 2 +- internal/lsp/cache/load.go | 2 +- internal/lsp/cache/parse.go | 22 +++--- internal/lsp/cache/snapshot.go | 56 ++++++++++++++++ internal/lsp/cache/view.go | 82 +---------------------- internal/lsp/source/completion_builtin.go | 8 ++- internal/lsp/source/completion_format.go | 2 +- internal/lsp/source/identifier.go | 10 ++- internal/lsp/source/signature_help.go | 6 +- internal/lsp/source/types_format.go | 10 +-- internal/lsp/source/view.go | 12 ++-- 11 files changed, 99 insertions(+), 113 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 723924abc3..ec997481b8 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -288,7 +288,7 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source return } pgh := snapshot.view.session.cache.parseGoHandle(ctx, fh, mode) - pgf, fixed, err := snapshot.view.parseGo(ctx, pgh) + pgf, fixed, err := snapshot.parseGo(ctx, pgh) if err != nil { actualErrors[i] = err return diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 6d9c2e159e..c0eb5d01b3 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -138,7 +138,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { } // Special case for the builtin package, as it has no dependencies. if pkg.PkgPath == "builtin" { - if err := s.view.buildBuiltinPackage(ctx, pkg.GoFiles); err != nil { + if err := s.buildBuiltinPackage(ctx, pkg.GoFiles); err != nil { return err } continue diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index af5f4e3f49..0bb1519eb0 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -57,13 +57,13 @@ func (c *Cache) parseGoHandle(ctx context.Context, fh source.FileHandle, mode so mode: mode, } parseHandle := c.store.Bind(key, func(ctx context.Context, arg memoize.Arg) interface{} { - view := arg.(*View) - return parseGo(ctx, view.session.cache.fset, fh, mode) + snapshot := arg.(*snapshot) + return parseGo(ctx, snapshot.view.session.cache.fset, fh, mode) }) astHandle := c.store.Bind(astCacheKey(key), func(ctx context.Context, arg memoize.Arg) interface{} { - view := arg.(*View) - return buildASTCache(ctx, view, parseHandle) + snapshot := arg.(*snapshot) + return buildASTCache(ctx, snapshot, parseHandle) }) return &parseGoHandle{ @@ -88,12 +88,12 @@ func (pgh *parseGoHandle) Mode() source.ParseMode { func (s *snapshot) ParseGo(ctx context.Context, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, error) { pgh := s.view.session.cache.parseGoHandle(ctx, fh, mode) - pgf, _, err := s.view.parseGo(ctx, pgh) + pgf, _, err := s.parseGo(ctx, pgh) return pgf, err } -func (v *View) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.ParsedGoFile, bool, error) { - d, err := pgh.handle.Get(ctx, v) +func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.ParsedGoFile, bool, error) { + d, err := pgh.handle.Get(ctx, s) if err != nil { return nil, false, err } @@ -108,7 +108,7 @@ func (s *snapshot) PosToDecl(ctx context.Context, pgf *source.ParsedGoFile) (map } pgh := s.view.session.cache.parseGoHandle(ctx, fh, pgf.Mode) - d, err := pgh.astCacheHandle.Get(ctx, s.view) + d, err := pgh.astCacheHandle.Get(ctx, s) if err != nil { return nil, err } @@ -128,7 +128,7 @@ func (s *snapshot) PosToField(ctx context.Context, pgf *source.ParsedGoFile) (ma } pgh := s.view.session.cache.parseGoHandle(ctx, fh, pgf.Mode) - d, err := pgh.astCacheHandle.Get(ctx, s.view) + d, err := pgh.astCacheHandle.Get(ctx, s) if err != nil || d == nil { return nil, err } @@ -151,7 +151,7 @@ type astCacheData struct { // buildASTCache builds caches to aid in quickly going from the typed // world to the syntactic world. -func buildASTCache(ctx context.Context, view *View, parseHandle *memoize.Handle) *astCacheData { +func buildASTCache(ctx context.Context, snapshot *snapshot, parseHandle *memoize.Handle) *astCacheData { var ( // path contains all ancestors, including n. path []ast.Node @@ -159,7 +159,7 @@ func buildASTCache(ctx context.Context, view *View, parseHandle *memoize.Handle) decls []ast.Decl ) - v, err := parseHandle.Get(ctx, view) + v, err := parseHandle.Get(ctx, snapshot) if err != nil { return &astCacheData{err: err} } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index ef3a7da8b8..5538a60118 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -28,6 +28,7 @@ import ( "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/typesinternal" + errors "golang.org/x/xerrors" ) type snapshot struct { @@ -38,6 +39,9 @@ type snapshot struct { active sync.WaitGroup + // builtin pins the AST and package for builtin.go in memory. + builtin *builtinPackageHandle + // mu guards all of the maps in the snapshot. mu sync.Mutex @@ -751,6 +755,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi result := &snapshot{ id: s.id + 1, view: s.view, + builtin: s.builtin, ids: make(map[span.URI][]packageID), importedBy: make(map[packageID][]packageID), metadata: make(map[packageID]*metadata), @@ -985,3 +990,54 @@ func (s *snapshot) shouldInvalidateMetadata(ctx context.Context, originalFH, cur } return false } + +func (s *snapshot) BuiltinPackage(ctx context.Context) (*source.BuiltinPackage, error) { + s.view.awaitInitialized(ctx) + + if s.builtin == nil { + return nil, errors.Errorf("no builtin package for view %s", s.view.name) + } + d, err := s.builtin.handle.Get(ctx, s) + if err != nil { + return nil, err + } + data := d.(*builtinPackageData) + return data.parsed, data.err +} + +func (s *snapshot) buildBuiltinPackage(ctx context.Context, goFiles []string) error { + if len(goFiles) != 1 { + return errors.Errorf("only expected 1 file, got %v", len(goFiles)) + } + uri := span.URIFromPath(goFiles[0]) + + // Get the FileHandle through the cache to avoid adding it to the snapshot + // and to get the file content from disk. + fh, err := s.view.session.cache.getFile(ctx, uri) + if err != nil { + return err + } + h := s.view.session.cache.store.Bind(fh.Identity(), func(ctx context.Context, arg memoize.Arg) interface{} { + snapshot := arg.(*snapshot) + + pgh := snapshot.view.session.cache.parseGoHandle(ctx, fh, source.ParseFull) + pgf, _, err := snapshot.parseGo(ctx, pgh) + if err != nil { + return &builtinPackageData{err: err} + } + pkg, err := ast.NewPackage(snapshot.view.session.cache.fset, map[string]*ast.File{ + pgf.URI.Filename(): pgf.File, + }, nil, nil) + if err != nil { + return &builtinPackageData{err: err} + } + return &builtinPackageData{ + parsed: &source.BuiltinPackage{ + ParsedFile: pgf, + Package: pkg, + }, + } + }) + s.builtin = &builtinPackageHandle{handle: h} + return nil +} diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index f32a3b2051..14672011d5 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -9,7 +9,6 @@ import ( "context" "encoding/json" "fmt" - "go/ast" "io" "io/ioutil" "os" @@ -33,8 +32,6 @@ import ( ) type View struct { - memoize.Arg // allow as a memoize.Function arg - session *Session id string @@ -117,9 +114,6 @@ type View struct { initializeOnce *sync.Once initializedErr error - // builtin pins the AST and package for builtin.go in memory. - builtin *builtinPackageHandle - // True if the view is either in GOPATH, a module, or some other // non go command build system. hasValidBuildConfiguration bool @@ -150,17 +144,8 @@ type builtinPackageHandle struct { type builtinPackageData struct { memoize.NoCopy - pkg *ast.Package - pgf *source.ParsedGoFile - err error -} - -func (d *builtinPackageData) Package() *ast.Package { - return d.pkg -} - -func (d *builtinPackageData) ParsedFile() *source.ParsedGoFile { - return d.pgf + parsed *source.BuiltinPackage + err error } // fileBase holds the common functionality for all files. @@ -299,69 +284,6 @@ func (v *View) Rebuild(ctx context.Context) (source.Snapshot, func(), error) { return snapshot, release, nil } -func (v *View) BuiltinPackage(ctx context.Context) (source.BuiltinPackage, error) { - v.awaitInitialized(ctx) - - if v.builtin == nil { - return nil, errors.Errorf("no builtin package for view %s", v.name) - } - data, err := v.builtin.handle.Get(ctx, v) - if err != nil { - return nil, err - } - if data == nil { - return nil, errors.Errorf("unexpected nil builtin package") - } - d, ok := data.(*builtinPackageData) - if !ok { - return nil, errors.Errorf("unexpected type %T", data) - } - if d.err != nil { - return nil, d.err - } - if d.pkg == nil || d.pkg.Scope == nil { - return nil, errors.Errorf("no builtin package") - } - return d, nil -} - -func (v *View) buildBuiltinPackage(ctx context.Context, goFiles []string) error { - if len(goFiles) != 1 { - return errors.Errorf("only expected 1 file, got %v", len(goFiles)) - } - uri := span.URIFromPath(goFiles[0]) - - // Get the FileHandle through the cache to avoid adding it to the snapshot - // and to get the file content from disk. - fh, err := v.session.cache.getFile(ctx, uri) - if err != nil { - return err - } - h := v.session.cache.store.Bind(fh.Identity(), func(ctx context.Context, arg memoize.Arg) interface{} { - view := arg.(*View) - - pgh := view.session.cache.parseGoHandle(ctx, fh, source.ParseFull) - pgf, _, err := view.parseGo(ctx, pgh) - if err != nil { - return &builtinPackageData{err: err} - } - pkg, err := ast.NewPackage(view.session.cache.fset, map[string]*ast.File{ - pgf.URI.Filename(): pgf.File, - }, nil, nil) - if err != nil { - return &builtinPackageData{err: err} - } - return &builtinPackageData{ - pgf: pgf, - pkg: pkg, - } - }) - v.builtin = &builtinPackageHandle{ - handle: h, - } - return nil -} - func (v *View) WriteEnv(ctx context.Context, w io.Writer) error { v.optionsMu.Lock() env, buildFlags := v.envLocked() diff --git a/internal/lsp/source/completion_builtin.go b/internal/lsp/source/completion_builtin.go index e67efb0d34..d0772acba5 100644 --- a/internal/lsp/source/completion_builtin.go +++ b/internal/lsp/source/completion_builtin.go @@ -14,13 +14,17 @@ import ( // argument. It attempts to use the AST hints from builtin.go where // possible. func (c *completer) builtinArgKind(ctx context.Context, obj types.Object, call *ast.CallExpr) objKind { - builtin, err := c.snapshot.View().BuiltinPackage(ctx) + builtin, err := c.snapshot.BuiltinPackage(ctx) if err != nil { return 0 } exprIdx := exprAtPos(c.pos, call.Args) - decl, ok := builtin.Package().Scope.Lookup(obj.Name()).Decl.(*ast.FuncDecl) + builtinObj := builtin.Package.Scope.Lookup(obj.Name()) + if builtinObj == nil { + return 0 + } + decl, ok := builtinObj.Decl.(*ast.FuncDecl) if !ok || exprIdx >= len(decl.Type.Params.List) { return 0 } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index d454d8acc0..348e3beed7 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -246,7 +246,7 @@ func (c *completer) formatBuiltin(ctx context.Context, cand candidate) (Completi item.Kind = protocol.ConstantCompletion case *types.Builtin: item.Kind = protocol.FunctionCompletion - sig, err := newBuiltinSignature(ctx, c.snapshot.View(), obj.Name()) + sig, err := newBuiltinSignature(ctx, c.snapshot, obj.Name()) if err != nil { return CompletionItem{}, err } diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index d4f9591d47..77d8cc26fb 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -171,11 +171,15 @@ func findIdentifier(ctx context.Context, s Snapshot, pkg Package, file *ast.File // Handle builtins separately. if result.Declaration.obj.Parent() == types.Universe { - builtin, err := view.BuiltinPackage(ctx) + builtin, err := s.BuiltinPackage(ctx) if err != nil { return nil, err } - decl, ok := builtin.Package().Scope.Lookup(result.Name).Decl.(ast.Node) + builtinObj := builtin.Package.Scope.Lookup(result.Name) + if builtinObj == nil { + return nil, fmt.Errorf("no builtin object for %s", result.Name) + } + decl, ok := builtinObj.Decl.(ast.Node) if !ok { return nil, errors.Errorf("no declaration for %s", result.Name) } @@ -183,7 +187,7 @@ func findIdentifier(ctx context.Context, s Snapshot, pkg Package, file *ast.File // The builtin package isn't in the dependency graph, so the usual utilities // won't work here. - rng := newMappedRange(view.Session().Cache().FileSet(), builtin.ParsedFile().Mapper, decl.Pos(), decl.Pos()+token.Pos(len(result.Name))) + rng := newMappedRange(view.Session().Cache().FileSet(), builtin.ParsedFile.Mapper, decl.Pos(), decl.Pos()+token.Pos(len(result.Name))) result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng) return result, nil diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index c471d1e97f..f841bd5718 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -74,7 +74,7 @@ FindCall: // Handle builtin functions separately. if obj, ok := obj.(*types.Builtin); ok { - return builtinSignature(ctx, snapshot.View(), callExpr, obj.Name(), rng.Start) + return builtinSignature(ctx, snapshot, callExpr, obj.Name(), rng.Start) } // Get the type information for the function being called. @@ -132,8 +132,8 @@ FindCall: }, activeParam, nil } -func builtinSignature(ctx context.Context, view View, callExpr *ast.CallExpr, name string, pos token.Pos) (*protocol.SignatureInformation, int, error) { - sig, err := newBuiltinSignature(ctx, view, name) +func builtinSignature(ctx context.Context, snapshot Snapshot, callExpr *ast.CallExpr, name string, pos token.Pos) (*protocol.SignatureInformation, int, error) { + sig, err := newBuiltinSignature(ctx, snapshot, name) if err != nil { return nil, 0, err } diff --git a/internal/lsp/source/types_format.go b/internal/lsp/source/types_format.go index ffd916a2f8..bd7d82e331 100644 --- a/internal/lsp/source/types_format.go +++ b/internal/lsp/source/types_format.go @@ -74,12 +74,12 @@ func (s *signature) format() string { return b.String() } -func newBuiltinSignature(ctx context.Context, view View, name string) (*signature, error) { - builtin, err := view.BuiltinPackage(ctx) +func newBuiltinSignature(ctx context.Context, snapshot Snapshot, name string) (*signature, error) { + builtin, err := snapshot.BuiltinPackage(ctx) if err != nil { return nil, err } - obj := builtin.Package().Scope.Lookup(name) + obj := builtin.Package.Scope.Lookup(name) if obj == nil { return nil, fmt.Errorf("no builtin object for %s", name) } @@ -98,8 +98,8 @@ func newBuiltinSignature(ctx context.Context, view View, name string) (*signatur variadic = true } } - params, _ := formatFieldList(ctx, view, decl.Type.Params, variadic) - results, needResultParens := formatFieldList(ctx, view, decl.Type.Results, false) + params, _ := formatFieldList(ctx, snapshot.View(), decl.Type.Params, variadic) + results, needResultParens := formatFieldList(ctx, snapshot.View(), decl.Type.Results, false) return &signature{ doc: decl.Doc.Text(), name: name, diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 7893c1d769..eaabf3f5ea 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -86,6 +86,9 @@ type Snapshot interface { // ModTidy returns the results of `go mod tidy` for the snapshot's module. ModTidy(ctx context.Context) (*TidiedModule, error) + // BuiltinPackage returns information about the special builtin package. + BuiltinPackage(ctx context.Context) (*BuiltinPackage, error) + // PackagesForFile returns the packages that this file belongs to. PackagesForFile(ctx context.Context, uri span.URI) ([]Package, error) @@ -122,9 +125,6 @@ type View interface { // ModFile is the go.mod file at the root of this view. It may not exist. ModFile() span.URI - // BuiltinPackage returns the go/ast.Object for the given name in the builtin package. - BuiltinPackage(ctx context.Context) (BuiltinPackage, error) - // BackgroundContext returns a context used for all background processing // on behalf of this view. BackgroundContext() context.Context @@ -172,9 +172,9 @@ type View interface { WorkspaceDirectories(ctx context.Context) ([]string, error) } -type BuiltinPackage interface { - Package() *ast.Package - ParsedFile() *ParsedGoFile +type BuiltinPackage struct { + Package *ast.Package + ParsedFile *ParsedGoFile } // A ParsedGoFile contains the results of parsing a Go file.