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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/builtin.go

@@ -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
-}

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
+			// <directory>/... 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
-		// <directory>/... 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
 }

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
-		}
+		}()
 	})
 }
 

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 != "" {