internal/lsp: don't use ast.NewPackage to build builtin

ast.NewPackage mutates the input files, making it difficult to avoid
races with our caching model. I had avoided a race resulting from cache
handle cancellation, just to run into another race in multi-session
servers.

But there's no reason to use ast.NewPackage when we only have a single
file. We can just interrogate the file scope wherever needed.

Fixes golang/go#45868

Change-Id: I521475b51ee3b1c3e408916affecafbc629b0191
Reviewed-on: https://go-review.googlesource.com/c/tools/+/315629
Trust: Robert Findley <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

Author: findleyr

internal/lsp/cache/load.go

@@ -148,9 +148,10 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 		}
 		// Special case for the builtin package, as it has no dependencies.
 		if pkg.PkgPath == "builtin" {
-			if err := s.buildBuiltinPackage(ctx, pkg.GoFiles); err != nil {
-				return err
+			if len(pkg.GoFiles) != 1 {
+				return errors.Errorf("only expected 1 file for builtin, got %v", len(pkg.GoFiles))
 			}
+			s.setBuiltin(pkg.GoFiles[0])
 			continue
 		}
 		// Skip test main packages.

internal/lsp/cache/snapshot.go

@@ -50,9 +50,6 @@ type snapshot struct {
 	// the cache generation that contains the data for this snapshot.
 	generation *memoize.Generation
 
-	// builtin pins the AST and package for builtin.go in memory.
-	builtin *builtinPackageData
-
 	// The snapshot's initialization state is controlled by the fields below.
 	//
 	// initializeOnce guards snapshot initialization. Each snapshot is
@@ -64,9 +61,12 @@ type snapshot struct {
 	// to avoid too many go/packages calls.
 	initializedErr *source.CriticalError
 
-	// mu guards all of the maps in the snapshot.
+	// mu guards all of the maps in the snapshot, as well as the builtin URI.
 	mu sync.Mutex
 
+	// builtin pins the AST and package for builtin.go in memory.
+	builtin span.URI
+
 	// ids maps file URIs to package IDs.
 	// It may be invalidated on calls to go/packages.
 	ids map[span.URI][]packageID
@@ -1745,60 +1745,37 @@ func extractMagicComments(f *ast.File) []string {
 	return results
 }
 
-func (s *snapshot) BuiltinPackage(ctx context.Context) (*source.BuiltinPackage, error) {
+func (s *snapshot) BuiltinFile(ctx context.Context) (*source.ParsedGoFile, error) {
 	s.AwaitInitialized(ctx)
 
-	if s.builtin == nil {
+	s.mu.Lock()
+	builtin := s.builtin
+	s.mu.Unlock()
+
+	if builtin == "" {
 		return nil, errors.Errorf("no builtin package for view %s", s.view.name)
 	}
-	return s.builtin.parsed, s.builtin.err
-}
 
-func (s *snapshot) IsBuiltin(ctx context.Context, uri span.URI) bool {
-	builtin, err := s.BuiltinPackage(ctx)
+	fh, err := s.GetFile(ctx, builtin)
 	if err != nil {
-		event.Error(ctx, "checking for builtin", err)
-		return false
+		return nil, err
 	}
+	return s.ParseGo(ctx, fh, source.ParseFull)
+}
+
+func (s *snapshot) IsBuiltin(ctx context.Context, uri span.URI) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
 	// We should always get the builtin URI in a canonical form, so use simple
 	// string comparison here. span.CompareURI is too expensive.
-	return builtin.ParsedFile.URI == uri
+	return uri == s.builtin
 }
 
-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
-	}
-	s.builtin = buildBuiltinPackage(ctx, fh, s)
-	return nil
-}
+func (s *snapshot) setBuiltin(path string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
 
-func buildBuiltinPackage(ctx context.Context, fh *fileHandle, snapshot *snapshot) *builtinPackageData {
-	pgh := snapshot.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 = span.URIFromPath(path)
 }
 
 // BuildGoplsMod generates a go.mod file for all modules in the workspace. It

internal/lsp/cache/view.go

@@ -151,11 +151,6 @@ type builtinPackageHandle struct {
 	handle *memoize.Handle
 }
 
-type builtinPackageData struct {
-	parsed *source.BuiltinPackage
-	err    error
-}
-
 // fileBase holds the common functionality for all files.
 // It is intended to be embedded in the file implementations
 type fileBase struct {

internal/lsp/source/completion/builtin.go

@@ -14,13 +14,13 @@ 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.BuiltinPackage(ctx)
+	builtin, err := c.snapshot.BuiltinFile(ctx)
 	if err != nil {
 		return 0
 	}
 	exprIdx := exprAtPos(c.pos, call.Args)
 
-	builtinObj := builtin.Package.Scope.Lookup(obj.Name())
+	builtinObj := builtin.File.Scope.Lookup(obj.Name())
 	if builtinObj == nil {
 		return 0
 	}

internal/lsp/source/identifier.go

@@ -179,11 +179,11 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a
 
 	// Handle builtins separately.
 	if result.Declaration.obj.Parent() == types.Universe {
-		builtin, err := snapshot.BuiltinPackage(ctx)
+		builtin, err := snapshot.BuiltinFile(ctx)
 		if err != nil {
 			return nil, err
 		}
-		builtinObj := builtin.Package.Scope.Lookup(result.Name)
+		builtinObj := builtin.File.Scope.Lookup(result.Name)
 		if builtinObj == nil {
 			return nil, fmt.Errorf("no builtin object for %s", result.Name)
 		}
@@ -195,7 +195,7 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a
 
 		// The builtin package isn't in the dependency graph, so the usual
 		// utilities won't work here.
-		rng := NewMappedRange(snapshot.FileSet(), builtin.ParsedFile.Mapper, decl.Pos(), decl.Pos()+token.Pos(len(result.Name)))
+		rng := NewMappedRange(snapshot.FileSet(), builtin.Mapper, decl.Pos(), decl.Pos()+token.Pos(len(result.Name)))
 		result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng)
 		return result, nil
 	}
@@ -204,14 +204,14 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a
 	// that this is the builtin Error.
 	if obj := result.Declaration.obj; obj.Parent() == nil && obj.Pkg() == nil && obj.Name() == "Error" {
 		if _, ok := obj.Type().(*types.Signature); ok {
-			builtin, err := snapshot.BuiltinPackage(ctx)
+			builtin, err := snapshot.BuiltinFile(ctx)
 			if err != nil {
 				return nil, err
 			}
 			// Look up "error" and then navigate to its only method.
 			// The Error method does not appear in the builtin package's scope.log.Pri
 			const errorName = "error"
-			builtinObj := builtin.Package.Scope.Lookup(errorName)
+			builtinObj := builtin.File.Scope.Lookup(errorName)
 			if builtinObj == nil {
 				return nil, fmt.Errorf("no builtin object for %s", errorName)
 			}
@@ -236,7 +236,7 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a
 			}
 			name := method.Names[0].Name
 			result.Declaration.node = method
-			rng := NewMappedRange(snapshot.FileSet(), builtin.ParsedFile.Mapper, method.Pos(), method.Pos()+token.Pos(len(name)))
+			rng := NewMappedRange(snapshot.FileSet(), builtin.Mapper, method.Pos(), method.Pos()+token.Pos(len(name)))
 			result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng)
 			return result, nil
 		}

internal/lsp/source/types_format.go

@@ -81,11 +81,11 @@ func (s *signature) Params() []string {
 // NewBuiltinSignature returns signature for the builtin object with a given
 // name, if a builtin object with the name exists.
 func NewBuiltinSignature(ctx context.Context, s Snapshot, name string) (*signature, error) {
-	builtin, err := s.BuiltinPackage(ctx)
+	builtin, err := s.BuiltinFile(ctx)
 	if err != nil {
 		return nil, err
 	}
-	obj := builtin.Package.Scope.Lookup(name)
+	obj := builtin.File.Scope.Lookup(name)
 	if obj == nil {
 		return nil, fmt.Errorf("no builtin object for %s", name)
 	}

internal/lsp/source/view.go

@@ -125,8 +125,8 @@ type Snapshot interface {
 	// GoModForFile returns the URI of the go.mod file for the given URI.
 	GoModForFile(uri span.URI) span.URI
 
-	// BuiltinPackage returns information about the special builtin package.
-	BuiltinPackage(ctx context.Context) (*BuiltinPackage, error)
+	// BuiltinFile returns information about the special builtin package.
+	BuiltinFile(ctx context.Context) (*ParsedGoFile, error)
 
 	// IsBuiltin reports whether uri is part of the builtin package.
 	IsBuiltin(ctx context.Context, uri span.URI) bool
@@ -259,11 +259,6 @@ type FileSource interface {
 	GetFile(ctx context.Context, uri span.URI) (FileHandle, error)
 }
 
-type BuiltinPackage struct {
-	Package    *ast.Package
-	ParsedFile *ParsedGoFile
-}
-
 // A ParsedGoFile contains the results of parsing a Go file.
 type ParsedGoFile struct {
 	URI  span.URI