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>
diff --git a/gopls/internal/regtest/diagnostics/builtin_test.go b/gopls/internal/regtest/diagnostics/builtin_test.go
new file mode 100644
index 0000000..775e7ec
--- /dev/null
+++ b/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"),
+ ))
+ })
+}
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 3215b7e..7c8dfae 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -51,7 +51,7 @@
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 @@
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 @@
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 @@
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) {
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index f93e525..f892910 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -121,6 +121,7 @@
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 @@
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 @@
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
diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go
index f10ea44..cceebe3 100644
--- a/internal/lsp/fake/editor.go
+++ b/internal/lsp/fake/editor.go
@@ -693,7 +693,7 @@
}
// 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
diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go
index 2038849..0137218 100644
--- a/internal/lsp/regtest/wrappers.go
+++ b/internal/lsp/regtest/wrappers.go
@@ -156,7 +156,7 @@
}
// 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)
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 412866c..2595823 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -128,6 +128,9 @@
// 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)
