gopls/internal/lsp/cache: pre-compute load diagnostics
Process go/packages errors immediately after loading, to eliminate a
dependency on syntax packages.
This simplifies the cache.Package type, which is now just a simple join
of metadata and type-checked syntax.
For golang/go#57987
Change-Id: I3d120fb4cb5687df2a376f8d8617e23e16ddaa92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/468776
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index c6e6f74..edefa43 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -636,12 +636,12 @@
// These may be attached to import declarations in the transitive source files
// of pkg, or to 'requires' declarations in the package's go.mod file.
//
-// TODO(rfindley): move this to errors.go
-func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsErrors []*packagesinternal.PackageError) ([]*source.Diagnostic, error) {
+// TODO(rfindley): move this to load.go
+func depsErrors(ctx context.Context, m *source.Metadata, meta *metadataGraph, fs source.FileSource, workspacePackages map[PackageID]PackagePath) ([]*source.Diagnostic, error) {
// Select packages that can't be found, and were imported in non-workspace packages.
// Workspace packages already show their own errors.
var relevantErrors []*packagesinternal.PackageError
- for _, depsError := range depsErrors {
+ for _, depsError := range m.DepsErrors {
// Up to Go 1.15, the missing package was included in the stack, which
// was presumably a bug. We want the next one up.
directImporterIdx := len(depsError.ImportStack) - 1
@@ -650,7 +650,7 @@
}
directImporter := depsError.ImportStack[directImporterIdx]
- if s.isWorkspacePackage(PackageID(directImporter)) {
+ if _, ok := workspacePackages[PackageID(directImporter)]; ok {
continue
}
relevantErrors = append(relevantErrors, depsError)
@@ -661,21 +661,31 @@
return nil, nil
}
+ // Subsequent checks require Go files.
+ if len(m.CompiledGoFiles) == 0 {
+ return nil, nil
+ }
+
// Build an index of all imports in the package.
type fileImport struct {
cgf *source.ParsedGoFile
imp *ast.ImportSpec
}
allImports := map[string][]fileImport{}
- for _, cgf := range pkg.compiledGoFiles {
+ for _, uri := range m.CompiledGoFiles {
+ pgf, err := parseGoURI(ctx, fs, uri, source.ParseHeader)
+ if err != nil {
+ return nil, err
+ }
+ fset := source.SingletonFileSet(pgf.Tok)
// TODO(adonovan): modify Imports() to accept a single token.File (cgf.Tok).
- for _, group := range astutil.Imports(pkg.fset, cgf.File) {
+ for _, group := range astutil.Imports(fset, pgf.File) {
for _, imp := range group {
if imp.Path == nil {
continue
}
path := strings.Trim(imp.Path.Value, `"`)
- allImports[path] = append(allImports[path], fileImport{cgf, imp})
+ allImports[path] = append(allImports[path], fileImport{pgf, imp})
}
}
}
@@ -686,7 +696,7 @@
for _, depErr := range relevantErrors {
for i := len(depErr.ImportStack) - 1; i >= 0; i-- {
item := depErr.ImportStack[i]
- if s.isWorkspacePackage(PackageID(item)) {
+ if _, ok := workspacePackages[PackageID(item)]; ok {
break
}
@@ -695,7 +705,7 @@
if err != nil {
return nil, err
}
- fixes, err := goGetQuickFixes(s, imp.cgf.URI, item)
+ fixes, err := goGetQuickFixes(m.Module != nil, imp.cgf.URI, item)
if err != nil {
return nil, err
}
@@ -711,18 +721,11 @@
}
}
- if len(pkg.compiledGoFiles) == 0 {
- return errors, nil
- }
- mod := s.GoModForFile(pkg.compiledGoFiles[0].URI)
- if mod == "" {
- return errors, nil
- }
- fh, err := s.GetFile(ctx, mod)
+ modFile, err := nearestModFile(ctx, m.CompiledGoFiles[0], fs)
if err != nil {
return nil, err
}
- pm, err := s.ParseMod(ctx, fh)
+ pm, err := parseModURI(ctx, fs, modFile)
if err != nil {
return nil, err
}
@@ -732,7 +735,7 @@
for _, depErr := range relevantErrors {
for i := len(depErr.ImportStack) - 1; i >= 0; i-- {
item := depErr.ImportStack[i]
- m := s.Metadata(PackageID(item))
+ m := meta.metadata[PackageID(item)]
if m == nil || m.Module == nil {
continue
}
@@ -745,7 +748,7 @@
if err != nil {
return nil, err
}
- fixes, err := goGetQuickFixes(s, pm.URI, item)
+ fixes, err := goGetQuickFixes(true, pm.URI, item)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/debug.go b/gopls/internal/lsp/cache/debug.go
index fd82aff..9d9de63 100644
--- a/gopls/internal/lsp/cache/debug.go
+++ b/gopls/internal/lsp/cache/debug.go
@@ -30,6 +30,8 @@
// If debugEnabled is true, dumpWorkspace prints a summary of workspace
// packages to stderr. If debugEnabled is false, it is a no-op.
+//
+// TODO(rfindley): this has served its purpose. Delete.
func (s *snapshot) dumpWorkspace(context string) {
if !debugEnabled {
return
diff --git a/gopls/internal/lsp/cache/errors.go b/gopls/internal/lsp/cache/errors.go
index 9585cf4..2d21705 100644
--- a/gopls/internal/lsp/cache/errors.go
+++ b/gopls/internal/lsp/cache/errors.go
@@ -9,8 +9,10 @@
// source.Diagnostic form, and suggesting quick fixes.
import (
+ "context"
"fmt"
"go/scanner"
+ "go/token"
"go/types"
"log"
"regexp"
@@ -28,20 +30,26 @@
"golang.org/x/tools/internal/typesinternal"
)
-func goPackagesErrorDiagnostics(e packages.Error, pkg *syntaxPackage, fromDir string) (diags []*source.Diagnostic, rerr error) {
- if diag, ok := parseGoListImportCycleError(e, pkg); ok {
+// goPackagesErrorDiagnostics translates the given go/packages Error into a
+// diagnostic, using the provided metadata and filesource.
+//
+// The slice of diagnostics may be empty.
+func goPackagesErrorDiagnostics(ctx context.Context, e packages.Error, m *source.Metadata, fs source.FileSource) ([]*source.Diagnostic, error) {
+ if diag, err := parseGoListImportCycleError(ctx, e, m, fs); err != nil {
+ return nil, err
+ } else if diag != nil {
return []*source.Diagnostic{diag}, nil
}
var spn span.Span
if e.Pos == "" {
- spn = parseGoListError(e.Msg, fromDir)
+ spn = parseGoListError(e.Msg, m.LoadDir)
// We may not have been able to parse a valid span. Apply the errors to all files.
- if _, err := spanToRange(pkg, spn); err != nil {
+ if _, err := spanToRange(ctx, fs, spn); err != nil {
var diags []*source.Diagnostic
- for _, pgf := range pkg.compiledGoFiles {
+ for _, uri := range m.CompiledGoFiles {
diags = append(diags, &source.Diagnostic{
- URI: pgf.URI,
+ URI: uri,
Severity: protocol.SeverityError,
Source: source.ListError,
Message: e.Msg,
@@ -50,7 +58,7 @@
return diags, nil
}
} else {
- spn = span.ParseInDir(e.Pos, fromDir)
+ spn = span.ParseInDir(e.Pos, m.LoadDir)
}
// TODO(rfindley): in some cases the go command outputs invalid spans, for
@@ -64,7 +72,7 @@
// likely because *token.File lacks information about newline termination.
//
// We could do better here by handling that case.
- rng, err := spanToRange(pkg, spn)
+ rng, err := spanToRange(ctx, fs, spn)
if err != nil {
return nil, err
}
@@ -136,7 +144,7 @@
}
if match := importErrorRe.FindStringSubmatch(e.primary.Msg); match != nil {
- diag.SuggestedFixes, err = goGetQuickFixes(snapshot, loc.URI.SpanURI(), match[1])
+ diag.SuggestedFixes, err = goGetQuickFixes(snapshot.moduleMode(), loc.URI.SpanURI(), match[1])
if err != nil {
return nil, err
}
@@ -150,9 +158,8 @@
return []*source.Diagnostic{diag}, nil
}
-func goGetQuickFixes(snapshot *snapshot, uri span.URI, pkg string) ([]source.SuggestedFix, error) {
- // Go get only supports module mode for now.
- if snapshot.workspaceMode()&moduleMode == 0 {
+func goGetQuickFixes(moduleMode bool, uri span.URI, pkg string) ([]source.SuggestedFix, error) {
+ if !moduleMode {
return nil, nil
}
title := fmt.Sprintf("go get package %v", pkg)
@@ -312,14 +319,20 @@
return ecode, loc, err
}
-// spanToRange converts a span.Span to a protocol.Range,
-// assuming that the span belongs to the package whose diagnostics are being computed.
-func spanToRange(pkg *syntaxPackage, spn span.Span) (protocol.Range, error) {
- pgf, err := pkg.File(spn.URI())
+// spanToRange converts a span.Span to a protocol.Range, by mapping content
+// contained in the provided FileSource.
+func spanToRange(ctx context.Context, fs source.FileSource, spn span.Span) (protocol.Range, error) {
+ uri := spn.URI()
+ fh, err := fs.GetFile(ctx, uri)
if err != nil {
return protocol.Range{}, err
}
- return pgf.Mapper.SpanRange(spn)
+ content, err := fh.Read()
+ if err != nil {
+ return protocol.Range{}, err
+ }
+ mapper := protocol.NewMapper(uri, content)
+ return mapper.SpanRange(spn)
}
// parseGoListError attempts to parse a standard `go list` error message
@@ -338,28 +351,36 @@
return span.ParseInDir(input[:msgIndex], wd)
}
-func parseGoListImportCycleError(e packages.Error, pkg *syntaxPackage) (*source.Diagnostic, bool) {
+// parseGoListImportCycleError attempts to parse the given go/packages error as
+// an import cycle, returning a diagnostic if successful.
+//
+// If the error is not detected as an import cycle error, it returns nil, nil.
+func parseGoListImportCycleError(ctx context.Context, e packages.Error, m *source.Metadata, fs source.FileSource) (*source.Diagnostic, error) {
re := regexp.MustCompile(`(.*): import stack: \[(.+)\]`)
matches := re.FindStringSubmatch(strings.TrimSpace(e.Msg))
if len(matches) < 3 {
- return nil, false
+ return nil, nil
}
msg := matches[1]
importList := strings.Split(matches[2], " ")
// Since the error is relative to the current package. The import that is causing
// the import cycle error is the second one in the list.
if len(importList) < 2 {
- return nil, false
+ return nil, nil
}
// Imports have quotation marks around them.
circImp := strconv.Quote(importList[1])
- for _, pgf := range pkg.compiledGoFiles {
+ for _, uri := range m.CompiledGoFiles {
+ pgf, err := parseGoURI(ctx, fs, uri, source.ParseHeader)
+ if err != nil {
+ return nil, err
+ }
// Search file imports for the import that is causing the import cycle.
for _, imp := range pgf.File.Imports {
if imp.Path.Value == circImp {
rng, err := pgf.NodeMappedRange(imp)
if err != nil {
- return nil, false
+ return nil, nil
}
return &source.Diagnostic{
@@ -368,9 +389,35 @@
Severity: protocol.SeverityError,
Source: source.ListError,
Message: msg,
- }, true
+ }, nil
}
}
}
- return nil, false
+ return nil, nil
+}
+
+// parseGoURI is a helper to parse the Go file at the given URI from the file
+// source fs. The resulting syntax and token.File belong to an ephemeral,
+// encapsulated FileSet, so this file stands only on its own: it's not suitable
+// to use in a list of file of a package, for example.
+//
+// It returns an error if the file could not be read.
+func parseGoURI(ctx context.Context, fs source.FileSource, uri span.URI, mode source.ParseMode) (*source.ParsedGoFile, error) {
+ fh, err := fs.GetFile(ctx, uri)
+ if err != nil {
+ return nil, err
+ }
+ return parseGoImpl(ctx, token.NewFileSet(), fh, source.ParseHeader)
+}
+
+// parseModURI is a helper to parse the Mod file at the given URI from the file
+// source fs.
+//
+// It returns an error if the file could not be read.
+func parseModURI(ctx context.Context, fs source.FileSource, uri span.URI) (*source.ParsedModule, error) {
+ fh, err := fs.GetFile(ctx, uri)
+ if err != nil {
+ return nil, err
+ }
+ return parseModImpl(ctx, fh)
}
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 98a8fec..22af16c 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -229,10 +229,20 @@
event.Log(ctx, fmt.Sprintf("%s: updating metadata for %d packages", eventName, len(updates)))
- s.meta = s.meta.Clone(updates)
- s.resetIsActivePackageLocked()
+ // Before mutating the snapshot, ensure that we compute load diagnostics
+ // successfully. This could fail if the context is cancelled, and we don't
+ // want to leave the snapshot metadata in a partial state.
+ meta := s.meta.Clone(updates)
+ workspacePackages := computeWorkspacePackagesLocked(s, meta)
+ for _, update := range updates {
+ if err := computeLoadDiagnostics(ctx, update, meta, lockedSnapshot{s}, workspacePackages); err != nil {
+ return err
+ }
+ }
+ s.meta = meta
+ s.workspacePackages = workspacePackages
- s.workspacePackages = computeWorkspacePackagesLocked(s, s.meta)
+ s.resetIsActivePackageLocked()
s.dumpWorkspace("load")
s.mu.Unlock()
@@ -549,6 +559,46 @@
m.DepsByImpPath = depsByImpPath
m.DepsByPkgPath = depsByPkgPath
+ // m.Diagnostics is set later in the loading pass, using
+ // computeLoadDiagnostics.
+
+ return nil
+}
+
+// computeLoadDiagnostics computes and sets m.Diagnostics for the given metadata m.
+//
+// It should only be called during metadata construction in snapshot.load.
+func computeLoadDiagnostics(ctx context.Context, m *source.Metadata, meta *metadataGraph, fs source.FileSource, workspacePackages map[PackageID]PackagePath) error {
+ for _, packagesErr := range m.Errors {
+ // Filter out parse errors from go list. We'll get them when we
+ // actually parse, and buggy overlay support may generate spurious
+ // errors. (See TestNewModule_Issue38207.)
+ if strings.Contains(packagesErr.Msg, "expected '") {
+ continue
+ }
+ pkgDiags, err := goPackagesErrorDiagnostics(ctx, packagesErr, m, fs)
+ if err != nil {
+ // There are certain cases where the go command returns invalid
+ // positions, so we cannot panic or even bug.Reportf here.
+ event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(string(m.ID)))
+ continue
+ }
+ m.Diagnostics = append(m.Diagnostics, pkgDiags...)
+ }
+
+ // TODO(rfindley): this is buggy: an insignificant change to a modfile
+ // (or an unsaved modfile) could affect the position of deps errors,
+ // without invalidating the package.
+ depsDiags, err := depsErrors(ctx, m, meta, fs, workspacePackages)
+ if err != nil {
+ if ctx.Err() == nil {
+ // TODO(rfindley): consider making this a bug.Reportf. depsErrors should
+ // not normally fail.
+ event.Error(ctx, "unable to compute deps errors", err, tag.Package.Of(string(m.ID)))
+ }
+ return nil
+ }
+ m.Diagnostics = append(m.Diagnostics, depsDiags...)
return nil
}
diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go
index 8c45948..76060ef 100644
--- a/gopls/internal/lsp/cache/pkg.go
+++ b/gopls/internal/lsp/cache/pkg.go
@@ -11,7 +11,6 @@
"go/scanner"
"go/token"
"go/types"
- "strings"
"golang.org/x/tools/go/types/objectpath"
"golang.org/x/tools/gopls/internal/lsp/protocol"
@@ -19,8 +18,6 @@
"golang.org/x/tools/gopls/internal/lsp/source/methodsets"
"golang.org/x/tools/gopls/internal/lsp/source/xrefs"
"golang.org/x/tools/gopls/internal/span"
- "golang.org/x/tools/internal/event"
- "golang.org/x/tools/internal/event/tag"
"golang.org/x/tools/internal/memoize"
)
@@ -39,54 +36,8 @@
// loadDiagnostics, because the value of the snapshot.packages map is just the
// package handle. Fix this.
type Package struct {
- m *source.Metadata
- pkg *syntaxPackage
- loadDiagnostics *memoize.Promise // post-processed errors from loading
-}
-
-func newPackage(m *source.Metadata, pkg *syntaxPackage) *Package {
- p := &Package{
- m: m,
- pkg: pkg,
- }
- if len(m.Errors) > 0 || len(m.DepsErrors) > 0 {
- p.loadDiagnostics = memoize.NewPromise(fmt.Sprintf("loadDiagnostics(%s)", m.ID), func(ctx context.Context, arg interface{}) interface{} {
- s := arg.(*snapshot)
- var diags []*source.Diagnostic
- for _, packagesErr := range p.m.Errors {
- // Filter out parse errors from go list. We'll get them when we
- // actually parse, and buggy overlay support may generate spurious
- // errors. (See TestNewModule_Issue38207.)
- if strings.Contains(packagesErr.Msg, "expected '") {
- continue
- }
- pkgDiags, err := goPackagesErrorDiagnostics(packagesErr, p.pkg, p.m.LoadDir)
- if err != nil {
- // There are certain cases where the go command returns invalid
- // positions, so we cannot panic or even bug.Reportf here.
- event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(string(p.m.ID)))
- continue
- }
- diags = append(diags, pkgDiags...)
- }
-
- // TODO(rfindley): this is buggy: an insignificant change to a modfile
- // (or an unsaved modfile) could affect the position of deps errors,
- // without invalidating the package.
- depsDiags, err := s.depsErrors(ctx, p.pkg, p.m.DepsErrors)
- if err != nil {
- if ctx.Err() == nil {
- // TODO(rfindley): consider making this a bug.Reportf. depsErrors should
- // not normally fail.
- event.Error(ctx, "unable to compute deps errors", err, tag.Package.Of(string(p.m.ID)))
- }
- return nil
- }
- diags = append(diags, depsDiags...)
- return diags
- })
- }
- return p
+ m *source.Metadata
+ pkg *syntaxPackage
}
// syntaxPackage contains parse trees and type information for a package.
@@ -235,21 +186,14 @@
func (p *Package) DiagnosticsForFile(ctx context.Context, s source.Snapshot, uri span.URI) ([]*source.Diagnostic, error) {
var diags []*source.Diagnostic
- for _, diag := range p.pkg.diagnostics {
+ for _, diag := range p.m.Diagnostics {
if diag.URI == uri {
diags = append(diags, diag)
}
}
-
- if p.loadDiagnostics != nil {
- res, err := p.loadDiagnostics.Get(ctx, s)
- if err != nil {
- return nil, err
- }
- for _, diag := range res.([]*source.Diagnostic) {
- if diag.URI == uri {
- diags = append(diags, diag)
- }
+ for _, diag := range p.pkg.diagnostics {
+ if diag.URI == uri {
+ diags = append(diags, diag)
}
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 8463b3d..dd18018 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -669,7 +669,7 @@
}
continue
}
- pkgs[i] = newPackage(ph.m, p)
+ pkgs[i] = &Package{ph.m, p}
}
return pkgs, firstErr
@@ -1144,6 +1144,20 @@
return match
}
+// nearestModFile finds the nearest go.mod file contained in the directory
+// containing uri, or a parent of that directory.
+//
+// The given uri must be a file, not a directory.
+func nearestModFile(ctx context.Context, uri span.URI, fs source.FileSource) (span.URI, error) {
+ // TODO(rfindley)
+ dir := filepath.Dir(uri.Filename())
+ mod, err := findRootPattern(ctx, dir, "go.mod", fs)
+ if err != nil {
+ return "", err
+ }
+ return span.URIFromPath(mod), nil
+}
+
func (s *snapshot) Metadata(id PackageID) *source.Metadata {
s.mu.Lock()
defer s.mu.Unlock()
@@ -1216,20 +1230,27 @@
// GetFile succeeds even if the file does not exist. A non-nil error return
// indicates some type of internal error, for example if ctx is cancelled.
func (s *snapshot) GetFile(ctx context.Context, uri span.URI) (source.FileHandle, error) {
- s.view.markKnown(uri)
-
s.mu.Lock()
defer s.mu.Unlock()
- if fh, ok := s.files.Get(uri); ok {
+ return lockedSnapshot{s}.GetFile(ctx, uri)
+}
+
+// A lockedSnapshot implements the source.FileSource interface while holding
+// the lock for the wrapped snapshot.
+type lockedSnapshot struct{ wrapped *snapshot }
+
+func (s lockedSnapshot) GetFile(ctx context.Context, uri span.URI) (source.FileHandle, error) {
+ s.wrapped.view.markKnown(uri)
+ if fh, ok := s.wrapped.files.Get(uri); ok {
return fh, nil
}
- fh, err := s.view.fs.GetFile(ctx, uri) // read the file
+ fh, err := s.wrapped.view.fs.GetFile(ctx, uri) // read the file
if err != nil {
return nil, err
}
- s.files.Set(uri, fh)
+ s.wrapped.files.Set(uri, fh)
return fh, nil
}
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index cc3ee5d..172a4fb 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -468,7 +468,8 @@
DepsByPkgPath map[PackagePath]PackageID // values are unique and non-empty
Module *packages.Module
DepsErrors []*packagesinternal.PackageError
- LoadDir string // directory from which go/packages was run
+ Diagnostics []*Diagnostic // processed diagnostics from 'go list'
+ LoadDir string // directory from which go/packages was run
}
func (m *Metadata) String() string { return string(m.ID) }
