From 28c1392e354b4ffa5db5a3295ca1b3c5a226996f Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 26 Apr 2021 09:50:10 -0400 Subject: [PATCH] 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 Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Heschi Kreinick --- .../regtest/diagnostics/builtin_test.go | 38 +++++++++++ internal/lsp/cache/snapshot.go | 63 ++++++++++--------- internal/lsp/diagnostics.go | 10 +++ internal/lsp/fake/editor.go | 2 +- internal/lsp/regtest/wrappers.go | 2 +- internal/lsp/source/view.go | 3 + 6 files changed, 85 insertions(+), 33 deletions(-) create mode 100644 gopls/internal/regtest/diagnostics/builtin_test.go diff --git a/gopls/internal/regtest/diagnostics/builtin_test.go b/gopls/internal/regtest/diagnostics/builtin_test.go new file mode 100644 index 0000000000..775e7ec0b1 --- /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 3215b7ea29..7c8dfae447 100644 --- a/internal/lsp/cache/snapshot.go +++ b/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) { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index f93e525c10..f892910c26 100644 --- a/internal/lsp/diagnostics.go +++ b/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 diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index f10ea44022..cceebe3472 100644 --- a/internal/lsp/fake/editor.go +++ b/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 diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go index 20388490eb..0137218366 100644 --- a/internal/lsp/regtest/wrappers.go +++ b/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) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 412866c0dc..2595823c0a 100644 --- a/internal/lsp/source/view.go +++ b/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)