internal/lsp: don't call PackagesForFile on builtin.go

Calling PackagesForFile on builtin.go loads is at a
command-line-arguments package, which has many type checking errors.

Add a new snapshot method IsBuiltin, which is used to avoid calling
PackagesForFile on builtin.go when diagnosing changed files or checking
for orphaned files. There may be other places where this should be used,
but this functionality can't reasonably be pushed down, as
PackagesForFile should always return something.

This exacerbated an existing race to building the builtin, because
ast.NewPackage unfortunately mutates the ast.File. Fix this by just
building the builtin package directly when building the handle. It
should be very fast.

Fixes golang/go#44866

Change-Id: Ie6c07478493fa011e92e6966289c2fa822d87b35
Reviewed-on: https://go-review.googlesource.com/c/tools/+/314290
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

gopls/internal/regtest/diagnostics/builtin_test.go

@@ -0,0 +1,38 @@
+// Copyright 2021 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 diagnostics
+
+import (
+	"strings"
+	"testing"
+
+	. "golang.org/x/tools/internal/lsp/regtest"
+)
+
+func TestIssue44866(t *testing.T) {
+	src := `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- a.go --
+package a
+
+const (
+	c = iota
+)
+`
+	Run(t, src, func(t *testing.T, env *Env) {
+		env.OpenFile("a.go")
+		name, _ := env.GoToDefinition("a.go", env.RegexpSearch("a.go", "iota"))
+		if !strings.HasSuffix(name, "builtin.go") {
+			t.Fatalf("jumped to %q, want builtin.go", name)
+		}
+		env.Await(OnceMet(
+			env.DoneWithOpen(),
+			NoDiagnostics("builtin.go"),
+		))
+	})
+}

internal/lsp/cache/snapshot.go

@@ -51,7 +51,7 @@ type snapshot struct {
 	generation *memoize.Generation
 
 	// builtin pins the AST and package for builtin.go in memory.
-	builtin *builtinPackageHandle
+	builtin *builtinPackageData
 
 	// The snapshot's initialization state is controlled by the fields below.
 	//
@@ -1361,10 +1361,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		newGen.Inherit(s.workspaceDirHandle)
 	}
 
-	if s.builtin != nil {
-		newGen.Inherit(s.builtin.handle)
-	}
-
 	// Copy all of the FileHandles.
 	for k, v := range s.files {
 		result.files[k] = v
@@ -1755,12 +1751,18 @@ func (s *snapshot) BuiltinPackage(ctx context.Context) (*source.BuiltinPackage,
 	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.generation, s)
+	return s.builtin.parsed, s.builtin.err
+}
+
+func (s *snapshot) IsBuiltin(ctx context.Context, uri span.URI) bool {
+	builtin, err := s.BuiltinPackage(ctx)
 	if err != nil {
-		return nil, err
+		event.Error(ctx, "checking for builtin", err)
+		return false
 	}
-	data := d.(*builtinPackageData)
-	return data.parsed, data.err
+	// 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
 }
 
 func (s *snapshot) buildBuiltinPackage(ctx context.Context, goFiles []string) error {
@@ -1775,31 +1777,30 @@ func (s *snapshot) buildBuiltinPackage(ctx context.Context, goFiles []string) er
 	if err != nil {
 		return err
 	}
-	h := s.generation.Bind(fh.FileIdentity(), func(ctx context.Context, arg memoize.Arg) interface{} {
-		snapshot := arg.(*snapshot)
-
-		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,
-			},
-		}
-	}, nil)
-	s.builtin = &builtinPackageHandle{handle: h}
+	s.builtin = buildBuiltinPackage(ctx, fh, s)
 	return nil
 }
 
+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,
+		},
+	}
+}
+
 // BuildGoplsMod generates a go.mod file for all modules in the workspace. It
 // bypasses any existing gopls.mod.
 func BuildGoplsMod(ctx context.Context, root span.URI, s source.Snapshot) (*modfile.File, error) {

internal/lsp/diagnostics.go

@@ -121,6 +121,7 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.U
 func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snapshot, uris []span.URI, onDisk bool) {
 	ctx, done := event.Start(ctx, "Server.diagnoseChangedFiles", tag.Snapshot.Of(snapshot.ID()))
 	defer done()
+
 	packages := make(map[source.Package]struct{})
 	for _, uri := range uris {
 		// If the change is only on-disk and the file is not open, don't
@@ -133,6 +134,11 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps
 		if snapshot.FindFile(uri) == nil {
 			continue
 		}
+		// Don't call PackagesForFile for builtin.go, as it results in a
+		// command-line-arguments load.
+		if snapshot.IsBuiltin(ctx, uri) {
+			continue
+		}
 		pkgs, err := snapshot.PackagesForFile(ctx, uri, source.TypecheckFull)
 		if err != nil {
 			// TODO (findleyr): we should probably do something with the error here,
@@ -393,6 +399,10 @@ func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snaps
 	if fh.Kind() != source.Go {
 		return nil
 	}
+	// builtin files won't have a package, but they are never orphaned.
+	if snapshot.IsBuiltin(ctx, fh.URI()) {
+		return nil
+	}
 	pkgs, err := snapshot.PackagesForFile(ctx, fh.URI(), source.TypecheckWorkspace)
 	if len(pkgs) > 0 || err == nil {
 		return nil

internal/lsp/fake/editor.go

@@ -693,7 +693,7 @@ func (e *Editor) setBufferContentLocked(ctx context.Context, path string, dirty
 }
 
 // GoToDefinition jumps to the definition of the symbol at the given position
-// in an open buffer.
+// in an open buffer. It returns the path and position of the resulting jump.
 func (e *Editor) GoToDefinition(ctx context.Context, path string, pos Pos) (string, Pos, error) {
 	if err := e.checkBufferPosition(path, pos); err != nil {
 		return "", Pos{}, err

internal/lsp/regtest/wrappers.go

@@ -156,7 +156,7 @@ func (e *Env) SaveBufferWithoutActions(name string) {
 }
 
 // GoToDefinition goes to definition in the editor, calling t.Fatal on any
-// error.
+// error. It returns the path and position of the resulting jump.
 func (e *Env) GoToDefinition(name string, pos fake.Pos) (string, fake.Pos) {
 	e.T.Helper()
 	n, p, err := e.Editor.GoToDefinition(e.Ctx, name, pos)

internal/lsp/source/view.go

@@ -128,6 +128,9 @@ type Snapshot interface {
 	// BuiltinPackage returns information about the special builtin package.
 	BuiltinPackage(ctx context.Context) (*BuiltinPackage, error)
 
+	// 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(ctx context.Context, uri span.URI, mode TypecheckMode) ([]Package, error)