From f04f2c82d09b716585c8205f5cd482d1eca2aa3d Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 15 Jan 2020 00:22:42 -0500 Subject: [PATCH] internal/lsp/cache: refactor initialization for builtins This change combines the two packages.Load calls that happen on view creation. Builtins can be loaded along with the rest of the workspace. To avoid race conditions, create a builtinPackageHandle type for builtins and use it to create the data. Updates golang/go#36531 Change-Id: I7aa342c463a0b7718e1ad5fee507622310d8443b Reviewed-on: https://go-review.googlesource.com/c/tools/+/214877 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/builtin.go | 82 ------------------------- internal/lsp/cache/load.go | 57 +++++++++-------- internal/lsp/cache/view.go | 111 +++++++++++++++++++++++++++------- internal/lsp/mod/mod_test.go | 2 +- 4 files changed, 124 insertions(+), 128 deletions(-) delete mode 100644 internal/lsp/cache/builtin.go diff --git a/internal/lsp/cache/builtin.go b/internal/lsp/cache/builtin.go deleted file mode 100644 index 42a1a065bc..0000000000 --- a/internal/lsp/cache/builtin.go +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright 2019 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 cache - -import ( - "context" - "go/ast" - "strings" - - "golang.org/x/tools/go/packages" - "golang.org/x/tools/internal/lsp/source" - "golang.org/x/tools/internal/span" - errors "golang.org/x/xerrors" -) - -type builtinPkg struct { - pkg *ast.Package - files []source.ParseGoHandle -} - -func (v *view) LookupBuiltin(name string) (*ast.Object, error) { - if v.builtin == nil || v.builtin.pkg == nil || v.builtin.pkg.Scope == nil { - return nil, errors.Errorf("no builtin package") - } - astObj := v.builtin.pkg.Scope.Lookup(name) - if astObj == nil { - return nil, errors.Errorf("no builtin object for %s", name) - } - return astObj, nil -} - -func (b *builtinPkg) CompiledGoFiles() []source.ParseGoHandle { - return b.files -} - -// buildBuiltinPkg builds the view's builtin package. -// It assumes that the view is not active yet, -// i.e. it has not been added to the session's list of views. -func (v *view) buildBuiltinPackage(ctx context.Context) error { - if v.builtin != nil { - return errors.Errorf("rebuilding builtin package") - } - cfg := v.Config(ctx) - pkgs, err := packages.Load(cfg, "builtin") - // If the error is related to a go.mod parse error, we want to continue loading. - if err != nil && strings.Contains(err.Error(), ".mod:") { - return nil - } - if err != nil { - return err - } - if len(pkgs) != 1 { - return errors.Errorf("expected 1 (got %v) packages for builtin", len(pkgs)) - } - files := make(map[string]*ast.File) - var pghs []source.ParseGoHandle - for _, filename := range pkgs[0].GoFiles { - fh := v.session.GetFile(span.FileURI(filename)) - pgh := v.session.cache.ParseGoHandle(fh, source.ParseFull) - pghs = append(pghs, pgh) - file, _, _, err := pgh.Parse(ctx) - if err != nil { - return err - } - files[filename] = file - - v.ignoredURIsMu.Lock() - v.ignoredURIs[span.NewURI(filename)] = struct{}{} - v.ignoredURIsMu.Unlock() - } - pkg, err := ast.NewPackage(cfg.Fset, files, nil, nil) - if err != nil { - return err - } - v.builtin = &builtinPkg{ - files: pghs, - pkg: pkg, - } - return nil -} diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 92ca3eb2af..19fa7c0e62 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -35,29 +35,30 @@ type metadata struct { config *packages.Config } -func (s *snapshot) load(ctx context.Context, scope interface{}) ([]*metadata, error) { +func (s *snapshot) load(ctx context.Context, scopes ...interface{}) ([]*metadata, error) { var query []string - switch scope := scope.(type) { - case []packagePath: - for _, p := range scope { - query = append(query, string(p)) + for _, scope := range scopes { + switch scope := scope.(type) { + case []packagePath: + for _, p := range scope { + query = append(query, string(p)) + } + case packagePath: + query = append(query, string(scope)) + case fileURI: + query = append(query, fmt.Sprintf("file=%s", span.URI(scope).Filename())) + case directoryURI: + filename := span.URI(scope).Filename() + q := fmt.Sprintf("%s/...", filename) + // Simplify the query if it will be run in the requested directory. + // This ensures compatibility with Go 1.12 that doesn't allow + // /... in GOPATH mode. + if s.view.folder.Filename() == filename { + q = "./..." + } + query = append(query, q) } - case packagePath: - query = append(query, string(scope)) - case fileURI: - query = append(query, fmt.Sprintf("file=%s", span.URI(scope).Filename())) - case directoryURI: - filename := span.URI(scope).Filename() - q := fmt.Sprintf("%s/...", filename) - // Simplify the query if it will be run in the requested directory. - // This ensures compatibility with Go 1.12 that doesn't allow - // /... in GOPATH mode. - if s.view.folder.Filename() == filename { - q = "./..." - } - query = append(query, q) } - ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.Query.Of(query)) defer done() @@ -78,7 +79,7 @@ func (s *snapshot) load(ctx context.Context, scope interface{}) ([]*metadata, er } return nil, err } - return s.updateMetadata(ctx, scope, pkgs, cfg) + return s.updateMetadata(ctx, scopes, pkgs, cfg) } // shouldLoad reparses a file's package and import declarations to @@ -127,10 +128,18 @@ func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, current return false } -func (s *snapshot) updateMetadata(ctx context.Context, scope interface{}, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) { +func (s *snapshot) updateMetadata(ctx context.Context, scopes []interface{}, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) { var results []*metadata for _, pkg := range pkgs { - if _, isDir := scope.(directoryURI); !isDir || s.view.Options().VerboseOutput { + // Don't log output for full workspace packages.Loads. + var containsDir bool + for _, scope := range scopes { + if _, ok := scope.(directoryURI); ok { + containsDir = true + break + } + } + if !containsDir || s.view.Options().VerboseOutput { log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles)) } // golang/go#36292: Ignore packages with no sources and no errors. @@ -154,7 +163,7 @@ func (s *snapshot) updateMetadata(ctx context.Context, scope interface{}, pkgs [ s.clearAndRebuildImportGraph() if len(results) == 0 { - return nil, errors.Errorf("no metadata for %s", scope) + return nil, errors.Errorf("no metadata for %s", scopes) } return results, nil } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 7915643976..b6a529c88f 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -18,11 +18,11 @@ import ( "sync" "time" - "golang.org/x/sync/errgroup" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/debug" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/memoize" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/log" "golang.org/x/tools/internal/telemetry/tag" @@ -97,8 +97,20 @@ type view struct { initialized chan struct{} initializationError error - // builtin is used to resolve builtin types. - builtin *builtinPkg + // builtin pins the AST and package for builtin.go in memory. + builtin *builtinPackageHandle +} + +type builtinPackageHandle struct { + handle *memoize.Handle + file source.ParseGoHandle +} + +type builtinPackageData struct { + memoize.NoCopy + + pkg *ast.Package + err error } // fileBase holds the common functionality for all files. @@ -169,6 +181,60 @@ func (v *view) SetOptions(ctx context.Context, options source.Options) (source.V return newView, err } +func (v *view) LookupBuiltin(name string) (*ast.Object, error) { + data := v.builtin.handle.Get(context.Background()) + 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") + } + astObj := d.pkg.Scope.Lookup(name) + if astObj == nil { + return nil, errors.Errorf("no builtin object for %s", name) + } + return astObj, nil +} + +func (v *view) buildBuiltinPackage(ctx context.Context, m *metadata) error { + if len(m.goFiles) != 1 { + return errors.Errorf("only expected 1 file, got %v", len(m.goFiles)) + } + uri := m.goFiles[0] + v.addIgnoredFile(uri) // to avoid showing diagnostics for builtin.go + + // Get the FileHandle through the session to avoid adding it to the snapshot. + pgh := v.session.cache.ParseGoHandle(v.session.GetFile(uri), source.ParseFull) + fset := v.session.cache.fset + h := v.session.cache.store.Bind(pgh.File().Identity(), func(ctx context.Context) interface{} { + data := &builtinPackageData{} + file, _, _, err := pgh.Parse(ctx) + if err != nil { + data.err = err + return data + } + data.pkg, data.err = ast.NewPackage(fset, map[string]*ast.File{ + pgh.File().Identity().URI.Filename(): file, + }, nil, nil) + if err != nil { + return err + } + return data + }) + v.builtin = &builtinPackageHandle{ + handle: h, + file: pgh, + } + return nil +} + // Config returns the configuration used for the view's interaction with the // go/packages API. It is shared across all views. func (v *view) Config(ctx context.Context) *packages.Config { @@ -445,6 +511,13 @@ func (v *view) Ignore(uri span.URI) bool { return ok } +func (v *view) addIgnoredFile(uri span.URI) { + v.ignoredURIsMu.Lock() + defer v.ignoredURIsMu.Unlock() + + v.ignoredURIs[uri] = struct{}{} +} + func (v *view) BackgroundContext() context.Context { v.mu.Lock() defer v.mu.Unlock() @@ -467,35 +540,31 @@ func (v *view) initialize(ctx context.Context, s *snapshot) { v.initializeOnce.Do(func() { defer close(v.initialized) - g, ctx := errgroup.WithContext(ctx) - - // Load all of the packages in the workspace. - g.Go(func() error { + v.initializationError = func() error { // Do not cancel the call to go/packages.Load for the entire workspace. - meta, err := s.load(ctx, directoryURI(v.folder)) + meta, err := s.load(ctx, directoryURI(v.folder), packagePath("builtin")) if err != nil { return err } - // A test variant of a package can only be loaded directly by loading - // the non-test variant with -test. Track the import path of the non-test variant. + // Keep track of the workspace packages. for _, m := range meta { + // Make sure to handle the builtin package separately + // Don't set it as a workspace package. + if m.pkgPath == "builtin" { + if err := s.view.buildBuiltinPackage(ctx, m); err != nil { + return err + } + continue + } + // A test variant of a package can only be loaded directly by loading + // the non-test variant with -test. Track the import path of the non-test variant. s.setWorkspacePackage(m.id, m.pkgPath) if _, err := s.packageHandle(ctx, m.id); err != nil { return err } } return nil - }) - - // Build the builtin package on initialization. - g.Go(func() error { - return v.buildBuiltinPackage(ctx) - }) - - // Wait for all initialization tasks to complete. - if err := g.Wait(); err != nil { - v.initializationError = err - } + }() }) } diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go index a72054617e..05c6169464 100644 --- a/internal/lsp/mod/mod_test.go +++ b/internal/lsp/mod/mod_test.go @@ -155,7 +155,7 @@ func TestDiagnostics(t *testing.T) { t.Fatal(err) } if len(reports) != 1 { - t.Errorf("expected 1 fileHandle, got %d", len(reports)) + t.Errorf("expected 1 diagnostic, got %d", len(reports)) } for fh, got := range reports { if diff := tests.DiffDiagnostics(fh.URI, tt.want, got); diff != "" {